#104 Support fonts with broad unicode coverage 23 hours ago

Comment by ~eliasnaur on ~eliasnaur/gio

On Mon Jul 6, 2020 at 16:54, ~tainted-bit wrote:

Rendering text is great for a demo but overkill for a test. It seems to me a test that inspects the result of Layout (and/or Shape) for a string containing fallback character(s) would suffice.

This will mostly work, but the tricky part is testing that the replacement glyph comes from the first font in the collection. This is not visible in the []text.Line returned by Layout because the glyph rune will be the same. Distinguishing the replacement characters will require at least a call to Shape and potentially examining the returned op.CallOp to identify the differences.

Alternatively, we could just not test that part of the code and leave it up to Collection to decide where replacement characters come from. After all, if all goes well we'll almost never see them anyway. :)

Thanks for the detailed description. I'll leave it to you how best to test :)

I'd like to keep any checked in TTF/OTC files tiny, < 1kb or even < 512 bytes like the other reference files in refs/. I'm ok with test relying on the Go fonts not changing, and I'm also ok with non-general OTC merging code (since it's only for the test).

I'm pretty sure that sizes like those are attainable with pyftsubset and potentially some other tools / manual massaging, but I won't know the final size for sure until I try. The plan is to have two TTFs that each contain two glyphs (replacement glyph and one identifiable symbol) and nothing else.

Great!

-- elias

#104 Support fonts with broad unicode coverage a day ago

Comment by ~eliasnaur on ~eliasnaur/gio

On Mon Jul 6, 2020 at 16:17, ~tainted-bit wrote:

If it's alright with you, I'd like to avoid the mutex for now, deferring optimization to when it has a bigger impact.

Alright. Perhaps a short comment should be left around the sfnt.Buffer declaration to discourage optimization before the time is right?

I'm not sure a comment is worth it. It would have to be duplicated on several sfnt.Buffer declarations or risk being overlooked. In any case we're not discouraging optimizations as much as we're avoiding race conditions.

I see your point. Let's keep the cool demo in a separate repository (yours?) and I'll link to that from the newsletter and such.

Sure. I'll make a repository for it somewhere and post the link here once the patch is merged.

Without an example, is there a way to add a test that doesn't need large dependencies? I'd like for the new feature not to bit-rot in the future.

That makes sense. My thought is to add internal/rendertest/shaper_test.go with TestCollectionAsFace to do some black-box testing. This would generate an OTC of two fonts, then render the following:

Rendering text is great for a demo but overkill for a test. It seems to me a test that inspects the result of Layout (and/or Shape) for a string containing fallback character(s) would suffice.

The main question is which fonts to use for the test, and how to make the OTC. Right now I'm leaning towards storing sample TTFs and the merged OTC as raw files in the repository (probably under internal/rendertest/refs/ ?). This would avoid the test becoming brittle if the Go fonts change (although admittedly that is unlikely, given that the last update was 3 years ago) and also avoid dependency on Roboto (which is not currently used elsewhere). It also avoids the need to use OTCMerge as a dependency or to include rudimentary OTC merging code (which is admittedly only a few lines if generality is not required).

I'd like to keep any checked in TTF/OTC files tiny, < 1kb or even < 512 bytes like the other reference files in refs/.

I'm ok with test relying on the Go fonts not changing, and I'm also ok with non-general OTC merging code (since it's only for the test).

Thanks Elias

#104 Support fonts with broad unicode coverage a day ago

Comment by ~eliasnaur on ~eliasnaur/gio

On Sat Jul 4, 2020 at 18:59, ~tainted-bit wrote:

Until benchmarks show a definite performance improvement, I prefer the simple approach of just not sharing buffers.

BenchmarkDrawUI in internal/rendertest is a good place to benchmark the approaches, since it calls Layout and Shape on an opentype.Font, but not concurrently (so it is similar to how a single window performs). The lock-vs-no-lock approach we're talking about here is going to be a tiny cost for most applications for the same reason that lock contention would be very low: text.faceCache will cache the layout and paths for most text drawing calls. Consequently the approach we take mainly affects GUIs that make poor use of the cache, such as a program that changes the text it draws very rapidly (e.g., a stopwatch GUI), or a GUI that is being resized by the user. To simulate this, I modified the drawText function in internal/rendertest/bench_test.go to bypass the cache so that it could test the performance of Layout and Shape:

txt.Text = textRows[x] + strconv.Itoa(rand.Int())

Here are the results of 3 runs with -test.benchtime=5s on my i7-6700K:

This patch (using sync.Mutex):

BenchmarkDrawUI-8          399      14885780 ns/op     5600746 B/op       1571 allocs/op
BenchmarkDrawUI-8          398      15011455 ns/op     5601157 B/op       1571 allocs/op
BenchmarkDrawUI-8          399      15005305 ns/op     5600769 B/op       1571 allocs/op

7bbe0da (not sharing sfnt.Buffer):

BenchmarkDrawUI-8          400      15011438 ns/op     5794994 B/op       1891 allocs/op
BenchmarkDrawUI-8          398      14971487 ns/op     5795127 B/op       1891 allocs/op
BenchmarkDrawUI-8          399      15030301 ns/op     5794782 B/op       1891 allocs/op

So in this degenerate case of constantly changing text, both approaches have roughly the same CPU time, but reusing sfnt.Buffer saves many allocs, as expected. The amount that it saves will be proportional to the number of uncached calls to text.Font. The difference between the approaches will be negligible for most GUIs (where the code is rarely called), but measurable for others. So the sync.Mutex approach has superior or equal performance, but slightly more code complexity. Personally I think that using a sync.Mutex and reusing the buffers is simple enough to justify this performance improvement, but I can see why you might disagree.

The number of allocations seems to rise by abuot 20%, and the CPU time and bytes allocated are largely unchanged. If it's alright with you, I'd like to avoid the mutex for now, deferring optimization to when it has a bigger impact.

I would love an example/fonts example showing off the new feature. Can you add it? I believe you can reuse example/hello.

I can certainly add an example for v3 of this patch. However, if it uses the actual Noto OTCs, that will add a 120 MB download as a dependency. For that reason I'm thinking of adding it as a nested module instead of directly in the gioui.org/example module, if that's okay with you.

I see your point. Let's keep the cool demo in a separate repository (yours?) and I'll link to that from the newsletter and such.

Without an example, is there a way to add a test that doesn't need large dependencies? I'd like for the new feature not to bit-rot in the future.

Thanks, Elias

#104 Support fonts with broad unicode coverage 3 days ago

Comment by ~eliasnaur on ~eliasnaur/gio

On Fri Jul 3, 2020 at 19:24, ~tainted-bit wrote:

Patch 11445 contains the revised code. I was initially confused about why it was okay for opentype.Font to use an sfnt.Buffer in multi-window settings when it was not okay for opentype.Collection to do the same. It turns out that it's not okay for either: there is an existing race condition in opentype.Font. It is triggered very rarely, because it only occurs when multiple windows are rendering strings using the same font simultaneously. Moreover, clobbering the sfnt.Buffer may not actually be noticeable depending on what the sfnt package does with it. In any case, this patch also fixes the existing race condition by adding a lock around buffers for both Font and Collection. The locks will have very low contention, so fast path mutex overhead is almost certainly worth the cost in order to keep buffers around to avoid GC pressure.

Until benchmarks show a definite performance improvement, I prefer the simple approach of just not sharing buffers. I fixed opentype.Font in

https://gioui.org/commit/7bbe0da

Thanks for noticing the race.

I've also packaged the Noto font family in public repositories under github.com/gonoto. This makes it easy to test this patch with a simple change to example/hello:

otc, _ := opentype.ParseCollection(notosans.OTC()) th := material.NewTheme([]text.FontFace{{Face: otc}})

I would love an example/fonts example showing off the new feature. Can you add it? I believe you can reuse example/hello.

-- elias

#144 Nondeterminism in rendertests with NVIDIA 3 days ago

Comment by ~eliasnaur on ~eliasnaur/gio

I lied; the issue was still reproducable. There was a subtle race condition between headless' call to MakeCurrent and ReleaseCurrent, fixed by https://gioui.org/commit/e2278b64c1.

#144 Nondeterminism in rendertests with NVIDIA 3 days ago

Comment by ~eliasnaur on ~eliasnaur/gio

It turns out that yes, the issue is independent of the GPU.

I can no longer reproduce the errors after the patchset ending in https://gioui.org/commit/30ad63283b. Let me know if you can.

REPORTED RESOLVED FIXED

#144 Nondeterminism in rendertests with NVIDIA 3 days ago

Comment by ~eliasnaur on ~eliasnaur/gio

Are the tests similarly brittle when run without NVIDIA?

#298 "Clone repo" results in bad request error 5 days ago

Comment by ~eliasnaur on ~sircmpwn/git.sr.ht

I still can. Maybe it's because I have a repository with that already?

#148 Add UI for selectively sending emails to oneself 5 days ago

Comment by ~eliasnaur on ~sircmpwn/lists.sr.ht

Thank you!

#125 Multimedia support 7 days ago

Comment by ~eliasnaur on ~eliasnaur/gio

Thank you. I'll wait for the entire patchset before reviewing so I have the whole picture. Are you able to take a stab at the Windows Direct3D backend for luminance textures? Unfortunately, you'll need a Windows installation to build HLSL versions of the shaders.

Please also add a YUV test to internal/rendertest.