Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add drawWithMetrics method that clips SDF to fit browser-provided TextMetrics #24

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

ChrisLoer
Copy link
Contributor

Using the canvas TextMetrics capabilities gives TinySDF two benefits:

  • TextMetrics.width (available in browsers going back to IE9) gives all the information necessary to do text shaping for non-ideographic (variable-advance) glyphs.
  • When the bounding box calculations are available on newer browsers, we can trim whitespace out of the SDF, and then adjust the glyph metrics we return to keep the shaping from changing.

cc @mourner

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great overall, and much simpler than I anticipated 👍

One general thing I'm thinking about is that while this PR is implemented to make sure the change is backwards-compatible, we should revisit the API for a potential v2 (which would also include moving over to ES6). E.g. I'd imagine there should be no reason not to measure metrics if we can do so cheaply, and to return fixed-size data if we can trim it, so an ideal API would just be draw returning trimmed data with metrics (either width-based or complete) always.

index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@ChrisLoer ChrisLoer force-pushed the with_metrics branch 2 times, most recently from edbd52b to 7d4b379 Compare January 11, 2021 01:28
@ChrisLoer ChrisLoer requested a review from mourner January 11, 2021 01:28
@ChrisLoer
Copy link
Contributor Author

🤔 Hmm... I might want to hold off on merging this until I do more performance investigation. I profiled some page loads using this in Mapbox GL JS and Chrome. With metrics extraction turned on, about 30% more time was spent in TinySDF, despite rendering 8% fewer pixels (the pixel trimming wasn't dramatic because this test was using ideographs that were pretty close to the canvas size TinySDF already uses). If I flip the getMetrics flag to false (basically simulating how drawWithMetrics works in older browsers, where the width can still be extracted but actualBoundingBox[Left|Right|etc] doesn't exist), then there's no apparent performance hit. So my best guess is that actualBoundingBoxLeft and family are surprisingly slow, but I don't have a good explanation for why that would be.

@ChrisLoer
Copy link
Contributor Author

So my best guess is that actualBoundingBoxLeft and family are surprisingly slow, but I don't have a good explanation for why that would be.

This doesn't seem to be the case.

(1) Looking at Chromium code, it looks like all the work is done at the time the TextMetrics object is created.

(2) If I make all the same measurement calls, but then call getImageData on the full canvas, there's no noticeable performance hit.

So maybe there is some optimization that makes getImageData much faster when it's getting the whole canvas? I'll try getting the whole image out but only SDF-ifying a subset.

@mourner
Copy link
Member

mourner commented Jan 11, 2021

So maybe there is some optimization that makes getImageData much faster when it's getting the whole canvas? I'll try getting the whole image out but only SDF-ifying a subset.

Hmm, this sounds unlikely. What might be worth checking is whether there are any overflow errors on some of the glyphs, e.g. the x or y is negative or width + 2 * buffer extends past the size. Does the slowdown happen if you cap the four values passed to getImageData?

@ChrisLoer
Copy link
Contributor Author

What might be worth checking is whether there are any overflow errors on some of the glyphs, e.g. the x or y is negative or width + 2 * buffer extends past the size. Does the slowdown happen if you cap the four values passed to getImageData?

Bingo, good call! There were in fact overflows of up to four pixels, but they didn't cause noticeable visual artifacts because the browser just returns "transparent black" (e.g. alpha 0%) for the overflow pixels, and the overflow was basically at the edges of the halos.

Capping the values gets rid of the slowdown, although there's still no noticeable speed improvement (I think because the canvas size was already tightly fit to the CJK glyphs, in fact slightly too small). If I switch to generating latin glyphs (which have more space to trim), I see a small improvement in SDF rendering time (~6%, sample size of three, timing not super reliable so I'm not sure of the significance). I think it's probably still worth going after the optimization since it also decreases the size of the glyph atlas.

I tried measuring again adding an extra 5 pixel buffer to the canvas size (so for a 24 point font with 3px "buffer" argument, that comes out to a 35px square -- the same as what we used the whole time on the gl-native side). In that case, I saw a 30% improvement in SDF rendering time on a San Francisco (e.g. heavy on latin text) scene.

I think the way forward here is to:

(1) Increase the canvas size so we're not likely to have overflow. Correctness is the first priority, although the fact that we never noticed does seem to hint we can cut corners with halos. This will slow down anyone who's using the old draw interface or a browser that doesn't support bounding box metrics.

(2) Check the metrics and cap for overflow just in case (we don't know what font will load, some may have giant glyphs).

@ChrisLoer
Copy link
Contributor Author

OK, I've pushed a commit that makes those changes. The "5" constant feels icky, and I'm basically using it because it's enough for my current tests and it matches what gl-native was implicitly using. It might make sense to do it as percentage instead.

The overdraw inspector gives a nice visual impression of the difference between clipping/not-clipping the glyphs:

Screen Shot 2021-01-12 at 13 44 07
Screen Shot 2021-01-12 at 13 44 27

@mourner
Copy link
Member

mourner commented Jan 12, 2021

@ChrisLoer whoa, looking at the screenshots, looks like it's a lot less fragments to draw, so it's significant for GPU rendering performance. Just wondering, how does the overdraw compare to our server-supplied font glyphs — close to the first one?

Regarding performance, some potential micro-optimizations that may be worth checking based on a read through the code:

  • Calling getImageData only on the actual bbox glyph rather than the full width/height we use for the SDF, since it should be smaller, making both getImageData and preparing grids potentially faster.
  • Doing clearRect on the previously drawn pixels (when metrics is available) rather than the whole canvas.
  • Seeing whether we could avoid allocating a new array for the data, e.g. accepting an optional argument of an array to write to, if it's discarded after reading (might not come up directly in the profiler but it's stressing GC).

@ChrisLoer
Copy link
Contributor Author

Just wondering, how does the overdraw compare to our server-supplied font glyphs — close to the first one?

Yeah, server-supplied glyphs look close to the first one. To be clear, this screenshot is with (1) latin glyphs, and (2) the 35px square (to avoid overflow) as opposed to the 30px we were using before. So in the old case where we were doing just CJK glyphs with 30px square canvas, there was much less excess overdraw.

I'll experiment with those optimization ideas.

@ChrisLoer
Copy link
Contributor Author

Calling getImageData only on the actual bbox glyph rather than the full width/height we use for the SDF, since it should be smaller, making both getImageData and preparing grids potentially faster.

This looked like a pretty significant win, basically proportional to the ignored area for the getImageData call. prepareGrid cost didn't seem to change much (it still has to zero that area of the grid out).

The way this has evolved, the canvas has extra space for the "buffer" which isn't really necessary since we're not reading the buffer area out of the canvas anymore. However, we still need to worry about overflowing gridOuter and gridInner (which are based on the canvas size), so we need to keep the same logic around. I wouldn't expect reducing the canvas size to make much performance difference, but it might be worth figuring out how to refactor for clarity.

Doing clearRect on the previously drawn pixels (when metrics is available) rather than the whole canvas.

I couldn't measure a change after this, but it was trivial to do, so I did.

Seeing whether we could avoid allocating a new array for the data, e.g. accepting an optional argument of an array to write to, if it's discarded after reading (might not come up directly in the profiler but it's stressing GC).

It doesn't look like it's discarded after reading. It's used to create a long-lived AlphaImage which should create a view into the ArrayBuffer created with the new Uint8ClampedArray. So I think we'd just be moving around where the creation happened.

After these changes, I re-ran profiling of a scene in Japan (16.77/35.295792/139.579883) on the 2x_sdf gl-js branch. Four runs averaged 178ms generating glyphs, vs. 110ms at 1x resolution without any of the perf optimizations.

@ChrisLoer
Copy link
Contributor Author

The way this has evolved, the canvas has extra space for the "buffer" which isn't really necessary since we're not reading the buffer area out of the canvas anymore. However, we still need to worry about overflowing gridOuter and gridInner (which are based on the canvas size), so we need to keep the same logic around. I wouldn't expect reducing the canvas size to make much performance difference, but it might be worth figuring out how to refactor for clarity.

I realized that adding extra buffer space to account for large glyphs broke backwards compatibility (because the caller assumes the size is exactly fontSize + 2 * buffer), so I settled for keeping the canvas size the same, but allowing the grids to extend out by a further buffer pixels on either side (this space will only be used if metrics are on and they show some overflow).

Unfortunately, the complexity of this change has gone up a lot, so that TinySDF doubles in size with this PR. It's still pretty small, but I know the goal is "tiny"! 😞

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress! The code increase is mostly code comments + the new prepareGrids routine that's now a bit too verbose for my liking (being ~30 lines in place of 4). Perhaps it's worth looking into whether zeroing all values at once with typedArray.fill(value) is faster than doing so one at a time explicitly — while it zeroes more values, it does so with a native method at bulk.

@ChrisLoer
Copy link
Contributor Author

Perhaps it's worth looking into whether zeroing all values at once with typedArray.fill(value) is faster

Good suggestion: that removed 15 lines without appearing to slow things down (actually ~10% faster in my three test runs, but well within a standard deviation).

Unfortunately I had to add some complexity back in: when I did the getImageData optimization, I forgot that the old/no-metrics pathway was implicitly depending on reading pixels that overflowed into the "buffer" area of the canvas, so I had to restore that behavior.

I also expose the fontBoundingBoxAscent to the caller to allow GL JS to line up baselines.

Here are some magnified before/after screencaps of this PR being used on the 2x_sdf GL JS branch (the "serif" font is probably not a common choice, but kind of worked as a stress-test since it had more overflow and also rendered more poorly at low res):

Before
Screen Shot 2021-01-15 at 15 09 23
Screen Shot 2021-01-15 at 15 08 59

After
Screen Shot 2021-01-15 at 15 06 31
Screen Shot 2021-01-15 at 15 07 07

…extMetrics.

Don't read known whitespace from canvas.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants