Offset-only GlyphBuffer

Offset-only GlyphBuffer

Instead of storing glyph advances (only to later convert to offsets for the Skia draw calls), refactor GlyphBuffer to store offsets directly. This reduces the amount of buffer post-processing and decreases the API friction when calling into Skia.

GlyphBuffer offsets are relative to the text run origin and are either horizontal/x-only (generated by the simple shaper) or x/y interleaved (generated by the complex shaper). These correspond to the text positioning modes used by Skia.

Thanks to the new buffer format, the refactor also includes:

* zero offset processing when building text blobs (the offsets are essentially memcpy-ed)
* minimal processing (translate by destination point) for the legacy drawPosText path (we could also optimize this path, but the added complexity is probably not justified given the plans to completely switch to text blobs)
* SimpleShaper::adjustSpacing no longer needs to inject synthetic glyphs but simply adjusts the offsets for following glyps
* drawEmphasisMarks uses explicit offsets to glyph-midpoint instead of computing them based on advances

Other things to investigate:

* multi-run buffer builder (to support multi-run text blobs)
* stop storing space glyphs (simply adjust the following glyph offsets instead)

R=jbroman@chromium.org,eae@chromium.org,dominik.rottsches@intel.com
BUG=377845
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185129

[f(malita), 2014-10-29 02:03:13]

fmalita@chromium.org changed reviewers:
+ dominik.rottsches@intel.com, eae@chromium.org, jbroman@chromium.org

[f(malita), 2014-10-29 02:15:21]

8 tests require minimal rebaselining due to slightly different rounding (oddly,
no platform overlap):

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/28...
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/28...
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/34...
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/34...
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/34...

[jbroman, 2014-10-29 15:06:08]

Nice work. I'm totally unconcerned about the layout test changes as I think
they're the same as ones we see due to text blobs anyhow.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:120: if (runInfo.run.rtl()) {
Can you add a comment clarifying how this RTL code works, and why we need to do
this complicated thing to reverse it? I think it's non-obvious.

(On the other hand, thanks for removing the pointless comment "RTL" that was
there before.)

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:968: float midMarkOffset =
offsetToMiddleOfGlyph(markFontData, markGlyphData.glyph);
Can you clarify why we're subtracting midMarkOffset from the offsets? I realize
this is pre-existing logic, but I've spent awhile staring at it and I'm still
not sure I understand properly.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:55: return size() != m_offsets.size();
Any reason not to just use a bool to explicitly track this? It's not immediately
obvious why this works.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:72: ASSERT(index < size());
Do these assertions slow down debug builds much?

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:126: // 0             afterOffset totalWidth
This diagram is helpful, but I could use some words explaining what these
parameters are, too. Especially since this method is rather subtle.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:127:
ASSERT_WITH_SECURITY_IMPLICATION(!m_offsets.isEmpty());
Why is it illegal to reverse an empty glyph buffer?

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBufferTest.cpp (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBufferTest.cpp:158: TEST(GlyphBufferTest,
OffsetArrayWithOffset)
Perhaps "OffsetArrayWithNonZeroIndex" or something? We didn't have offsets when
I named this test, and this name sounds confusing (of course offset arrays would
have offsets, etc.).

[f(malita), 2014-10-29 16:48:30]

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:120: if (runInfo.run.rtl()) {
On 2014/10/29 15:06:07, jbroman wrote:
> Can you add a comment clarifying how this RTL code works, and why we need to
do
> this complicated thing to reverse it? I think it's non-obvious.
> 
> (On the other hand, thanks for removing the pointless comment "RTL" that was
> there before.)

Added a comment + more comprehensive explanation in GlyphBuffer::reverse().

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:968: float midMarkOffset =
offsetToMiddleOfGlyph(markFontData, markGlyphData.glyph);
On 2014/10/29 15:06:07, jbroman wrote:
> Can you clarify why we're subtracting midMarkOffset from the offsets? I
realize
> this is pre-existing logic, but I've spent awhile staring at it and I'm still
> not sure I understand properly.

Added some comments (we are essentially aligning the middle of the emphasis mark
to the middle of the target glyph, and the input offsets are already adjusted to
point to the latter).

I think we can do all this when building the input buffer (possibly even avoid
building a separate mark buffer altogether). Added a FIXME + will look at it as
a follow-up.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:55: return size() != m_offsets.size();
On 2014/10/29 15:06:07, jbroman wrote:
> Any reason not to just use a bool to explicitly track this? It's not
immediately
> obvious why this works.

add() is pretty hot if I'm not mistaken, and optimizing for it (avoiding any
unnecessary tracking work) seems desirable (I'm assuming hasVerticalOffsets is
not nearly as hot).

I haven't really measured the impact so maybe this is a case of premature
optimization, but if we can avoid it for now... added some comments, let me know
what you think.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:72: ASSERT(index < size());
On 2014/10/29 15:06:07, jbroman wrote:
> Do these assertions slow down debug builds much?

Nothing noticeable.

A couple of local fast/text layout test runs, user time:

[0m2.657s, 0m2.655s] (ToT) vs. [0m2.642s, 0m2.672s] (patched)

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:126: // 0             afterOffset totalWidth
On 2014/10/29 15:06:07, jbroman wrote:
> This diagram is helpful, but I could use some words explaining what these
> parameters are, too. Especially since this method is rather subtle.

Done.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:127:
ASSERT_WITH_SECURITY_IMPLICATION(!m_offsets.isEmpty());
On 2014/10/29 15:06:07, jbroman wrote:
> Why is it illegal to reverse an empty glyph buffer?

Ah, good catch: I assumed we check for an empty buffer at the call site, but
apparently not. Handling it here is the better choice - done.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBufferTest.cpp (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBufferTest.cpp:158: TEST(GlyphBufferTest,
OffsetArrayWithOffset)
On 2014/10/29 15:06:07, jbroman wrote:
> Perhaps "OffsetArrayWithNonZeroIndex" or something? We didn't have offsets
when
> I named this test, and this name sounds confusing (of course offset arrays
would
> have offsets, etc.).

Done.

[jbroman, 2014-10-29 19:05:37]

LGTM, but you might want to also get eae's opinion before landing.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:55: return size() != m_offsets.size();
On 2014/10/29 16:48:29, Florin Malita wrote:
> On 2014/10/29 15:06:07, jbroman wrote:
> > Any reason not to just use a bool to explicitly track this? It's not
> immediately
> > obvious why this works.
> 
> add() is pretty hot if I'm not mistaken, and optimizing for it (avoiding any
> unnecessary tracking work) seems desirable (I'm assuming hasVerticalOffsets is
> not nearly as hot).
> 
> I haven't really measured the impact so maybe this is a case of premature
> optimization, but if we can avoid it for now... added some comments, let me
know
> what you think.

Hmm, okay. I suspect the expense is relatively minimal (e.g. there's also
branches for each vector we append to in case we need to increase capacity), but
I don't feel strongly.

https://codereview.chromium.org/676523003/diff/170001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:72: ASSERT(index < size());
On 2014/10/29 16:48:29, Florin Malita wrote:
> On 2014/10/29 15:06:07, jbroman wrote:
> > Do these assertions slow down debug builds much?
> 
> Nothing noticeable.
> 
> A couple of local fast/text layout test runs, user time:
> 
> [0m2.657s, 0m2.655s] (ToT) vs. [0m2.642s, 0m2.672s] (patched)

Acknowledged.

https://codereview.chromium.org/676523003/diff/180001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBufferTest.cpp (right):

https://codereview.chromium.org/676523003/diff/180001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBufferTest.cpp:196: TEST(GlyphBufferTest, Reverse)
nit: ReverseForSimpleRTL, since you've renamed the method?

[f(malita), 2014-10-30 15:53:11]

Thanks.

Emil/Dominik - WDYT?

https://codereview.chromium.org/676523003/diff/180001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBufferTest.cpp (right):

https://codereview.chromium.org/676523003/diff/180001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBufferTest.cpp:196: TEST(GlyphBufferTest, Reverse)
On 2014/10/29 19:05:36, jbroman wrote:
> nit: ReverseForSimpleRTL, since you've renamed the method?

Done.

[eae, 2014-11-04 15:33:27]

This is a very nice and exciting change, thanks for taking it on!

https://codereview.chromium.org/676523003/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/676523003/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1153: crbug.com/377845
css1/box_properties/float_on_text_elements.html [ NeedsRebaseline ]
Why would any of these tests be affected by this change?

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:791: static void
drawVerticalGlyphs(GraphicsContext* gc, const SimpleFontData* font,
Dominik is working on changing our vertical text support, I'd like him to take a
look at this part of the change to make sure it doesn't conflict with or prevent
the work he is doing.

Also why is this called drawVetricalGlyphs and the horizontal one
paintGlyphsHorizontal? Could you rename one or the other please and ideally have
them have the same or similar signatures.

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:66: const float* offsets(unsigned from)
const
Using a float* to represent both the x and y offsets is a little confusing. I
understand why but please at least add a comment here and on line 159 explaining
the format.

[f(malita), 2014-11-04 21:52:23]

https://codereview.chromium.org/676523003/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/676523003/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:1153: crbug.com/377845
css1/box_properties/float_on_text_elements.html [ NeedsRebaseline ]
On 2014/11/04 15:33:26, eae wrote:
> Why would any of these tests be affected by this change?

The diffs are linked in the first message, and as far as I can tell they're just
rounding/precision snapping artifacts - due to the slightly different way we
accumulate advances.

(Interestingly, but not surprisingly, most of them appear to be the exact same
diffs we get with --enable-text-blobs).

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:791: static void
drawVerticalGlyphs(GraphicsContext* gc, const SimpleFontData* font,
On 2014/11/04 15:33:26, eae wrote:
> Dominik is working on changing our vertical text support, I'd like him to take
a
> look at this part of the change to make sure it doesn't conflict with or
prevent
> the work he is doing.

Ack. IIUC, he's going to make this whole think go away - so I think it's not a
conflicting direction, but he should confirm.


> Also why is this called drawVetricalGlyphs and the horizontal one
> paintGlyphsHorizontal? Could you rename one or the other please and ideally
have
> them have the same or similar signatures.

The current paint* methods take pre-processed Skia buffers while this operates
at the same logical level as drawGlyphs, so I felt compelled to differentiate :)

Renamed.

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/G...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/676523003/diff/200001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:66: const float* offsets(unsigned from)
const
On 2014/11/04 15:33:26, eae wrote:
> Using a float* to represent both the x and y offsets is a little confusing. I
> understand why but please at least add a comment here and on line 159
explaining
> the format.

Done.

[eae, 2014-11-05 19:24:30]

LGTM

[f(malita), 2014-11-11 15:27:59]

I'm tempted to land this in order to unblock related work and avoid further
bitrot (not sure when Dominik will be back). I did take a close look at the
opentype vertical cl (https://codereview.chromium.org/617103003/), and while
there's going to be some rebasing involved I don't see any fundamental issues.

Any objections?

[eae, 2014-11-11 16:09:07]

Go for it.

[f(malita), 2014-11-11 16:31:34]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-11-11 16:32:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676523003/240001

[commit-bot: I haz the power, 2014-11-11 16:34:29]

Committed patchset #14 (id:240001) as 185129

[behdad_google, 2014-11-11 18:18:01]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2014-11-11 18:18:02]

I'm not sure this is a good idea.

Currently our uses of GlyphBuffer only need the glyph positions.  But I think in
the long run we want to use advances for width measurements of selection, etc.

We sure can add them back again if needed, but this is a tricky piece of code,
as eae@ can definitely tell given his experience moving from relative glyph
offsets to the offsets and advances model two month ago...

[f(malita), 2014-11-11 20:53:04]

On 2014/11/11 18:18:02, behdad_google wrote:
> I'm not sure this is a good idea.
> 
> Currently our uses of GlyphBuffer only need the glyph positions.  But I think
in
> the long run we want to use advances for width measurements of selection, etc.

I guess my argument is:

* We only build glyph buffers for drawing, not for measuring.
* Regardless of whether or not we store advances, it makes sense to compute and
store the final offsets simply because that's what we ultimately need to pass to
Skia - why postpone it and force another pass if we have all the info upfront?
* Storing offsets opens the door for some interesting optimizations (we could
skip space glyphs, generate the emphasis marks buffer directly/during shaping,
coalesce multiple runs <- particularly interesting for text blobs)
* Since we no longer have a use for advances beyond shaping, why store them? It
would be trivial to add them later if needed (alongside final offsets), but I
would be interested to see what that use case looks like (and possibly question
whether GlyphBuffer is the right structure for the task).


> We sure can add them back again if needed, but this is a tricky piece of code,
> as eae@ can definitely tell given his experience moving from relative glyph
> offsets to the offsets and advances model two month ago...

I agree that the code is tricky, but I think this is actually removing some
complexity. At a high level we're just front-loading the final offset
computation at GB build time. The logic is equivalent (advance accumulator ->
offsets) and the only diffs should be related to rounding/precision.