Separate advance from offset in GlypBuffer

Separate advance from offset in GlypBuffer

Change GlyphBuffer to store advances and offsets independently. This
greatly simplifies shaping and drawing of complex text. As advances are
now a single float value per glyph it is made clearer that the simple
text path only handles horizontal advances (as opposed to vertical and
horizontal offsets in addition to horizontal advances).

Also gets rid of HarfBuzzShaper::adjustStartPoint as it is no longer
needed now that offsets and advances are separate.

Must land after 175253002.

TEST=Source/platform/fonts/GlyphBufferTest.cpp
BUG=418071
R=behdad@chromium.org,dominik.rottsches@intel.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182991

[eae, 2014-09-25 12:53:01]

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

[eae, 2014-09-25 15:56:47]

Ready for review, depends on 175253002 (or it won't compile on Mac).

[behdad_google, 2014-09-25 16:08:34]

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

[behdad_google, 2014-09-25 16:08:35]

lgtm

[f(malita), 2014-09-25 16:21:00]

fmalita@chromium.org changed reviewers:
+ jbroman@chromium.org

[f(malita), 2014-09-25 16:42:04]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2014-09-25 16:42:05]

I like the idea of storing offsets, and I think ultimately we'll want to do that
even for simple text (Skia consumes offsets, and we're performing the conversion
regardless).

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:113: Vector<FloatSize> m_offsets;
Why no inline capacity for m_offsets? Using stack-allocated buffers is much more
efficient than going through the heap allocator (as long as we don't overflow,
which is super unlikely given the current sizing).

We're halving the advance buffer size, so we should be able to reserve space for
1024 offsets without changing sizeof(GlyphBuffer).

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:344: const FloatSize* offsets =
glyphBuffer.offsets(start);
I think we still want advances here. We only support text blobs for simple text,
in which case offsets() is empty IIUC.

[jbroman, 2014-09-25 17:03:32]

I'm not an expert on the complex-shaping side of things, but I have a few
thoughts about GlyphBuffer.

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:48: void reserveOffsetsCapacity(size_t size)
{ m_offsets.reserveCapacity(size); }
This seems like an odd method to have. Why not just reserve capacity in all of
the vectors if we know how many glyphs there will be?

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:80: {
Could we do something to ensure callers don't wind up calling both this and the
other overload that takes an offset? That could result in bizarre situations
where the offsets don't line up properly.

I'm not sure if this should be an ASSERT(!hasOffsets()) here, or some sort of
sanity check after the glyph buffer is done. But if I just saw the interface to
this class I would have no idea that I'm supposed to use only one or the other.

Another alternative would be for there to be a GlyphBuffer and a subclass
ComplexGlyphBuffer (or similar), where the latter adds the offset-related code
and hides the wrong add method.

[jbroman, 2014-09-25 17:11:35]

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:98: m_offsets.reverse();
Are these offsets from the text origin, or from the previous character?

If the former, I don't think this logically reverses the glyph buffer. This
probably works because it so happens we only call reverse in the simple and
Mac-complex paths.

If the latter, then I don't understand how they differ from advances, and we
should probably leave a comment explaining it.

[eae, 2014-09-26 05:26:51]

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
File Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:48: void reserveOffsetsCapacity(size_t size)
{ m_offsets.reserveCapacity(size); }
On 2014/09/25 17:03:31, jbroman wrote:
> This seems like an odd method to have. Why not just reserve capacity in all of
> the vectors if we know how many glyphs there will be?

Good point, I'll rename it and change it to operate on all fields.

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:80: {
On 2014/09/25 17:03:32, jbroman wrote:
> Could we do something to ensure callers don't wind up calling both this and
the
> other overload that takes an offset? That could result in bizarre situations
> where the offsets don't line up properly.
> 
> I'm not sure if this should be an ASSERT(!hasOffsets()) here, or some sort of
> sanity check after the glyph buffer is done. But if I just saw the interface
to
> this class I would have no idea that I'm supposed to use only one or the
other.
> 
> Another alternative would be for there to be a GlyphBuffer and a subclass
> ComplexGlyphBuffer (or similar), where the latter adds the offset-related code
> and hides the wrong add method.

Yeah, I was thinking about that, I don't really want to slow things down by
making the add method virtual but it might be the better option.

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
Source/platform/fonts/GlyphBuffer.h:113: Vector<FloatSize> m_offsets;
On 2014/09/25 16:42:05, Florin Malita wrote:
> Why no inline capacity for m_offsets? Using stack-allocated buffers is much
more
> efficient than going through the heap allocator (as long as we don't overflow,
> which is super unlikely given the current sizing).
> 
> We're halving the advance buffer size, so we should be able to reserve space
for
> 1024 offsets without changing sizeof(GlyphBuffer).

As explained in the CL description the m_offsets field is only used for complex
text, always reserving size for it even for the simple text path where it is not
used seems even more inefficient. In the complex text case it is pre-allocated
to the right size before being used.

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/ha...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):

https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/ha...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:344: const FloatSize* offsets =
glyphBuffer.offsets(start);
On 2014/09/25 16:42:05, Florin Malita wrote:
> I think we still want advances here. We only support text blobs for simple
text,
> in which case offsets() is empty IIUC.

Right, thanks!

[Dominik Röttsches, 2014-09-26 11:58:53]

Great - looking forward to landing this. LGTM.

https://codereview.chromium.org/607483002/diff/100001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):

https://codereview.chromium.org/607483002/diff/100001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:170: // FIXME: text rendering
speed:
Do you wanna remove this obsolete comment while we're at it?

https://codereview.chromium.org/607483002/diff/100001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (left):

https://codereview.chromium.org/607483002/diff/100001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:574: FloatPoint
HarfBuzzShaper::adjustStartPoint(const FloatPoint& point)
*sigh of relief* My favorite part of this patch.

[eae, 2014-09-26 11:59:50]

Thanks!

https://codereview.chromium.org/607483002/diff/100001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):

https://codereview.chromium.org/607483002/diff/100001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:170: // FIXME: text rendering
speed:
On 2014/09/26 11:58:53, Dominik Röttsches wrote:
> Do you wanna remove this obsolete comment while we're at it?

Done.

[jbroman, 2014-09-26 13:02:11]

LGTM with nits

I can see that this is more rational for the complex path.

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

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:43: GlyphBuffer() { }
We can probably remove the default constructors since they're implicitly defined
this way. Any reason to explicitly declare them?

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:72: m_fontData.append(font);
I'm not sure how expensive adding
ASSERT(!hasOffsets());
here would be. It's possible that this could get called on a
GlyphBufferWithOffsets instance through the base pointer, and it would be nice
to catch that. But if it kills performance of debug builds too much, perhaps not
worth it.

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:98: class GlyphBufferWithOffsets: public
GlyphBuffer {
super-nit: space before :

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:102: virtual bool hasOffsets() const {
return true; }
"override", here and the other virtual methods? (We can apparently use the
lowercase one now!)

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/G...
Source/platform/fonts/GlyphBuffer.h:119: ASSERT_NOT_REACHED();
We should be able to catch this case at compile time. Consider hiding this
method.

Once it becomes permitted, I'd use "= delete" for this. In the meantime, making
it private should do the trick.

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (left):

https://codereview.chromium.org/607483002/diff/120001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:305:
ASSERT(!glyphBuffer.hasVerticalAdvances());
Mind putting the equivalent assertion back here?

ASSERT(!glyphBuffer.hasOffsets());

This path doesn't currently handle the complex case where there are offsets (and
it would merrily position such runs horizontally, which is wrong). It shouldn't
be reachable in such cases, but it would be nice to keep asserting that.

[eae, 2014-09-26 13:04:06]

Thanks jbroman, I'll make the suggested changes.

[f(malita), 2014-09-26 13:13:35]

lgtm

[Dominik Röttsches, 2014-09-30 14:00:55]

175253002 has landed in r182920.
https://src.chromium.org/viewvc/blink?view=revision&revision=182920
Mac build should succeed now.

[behdad_google, 2014-09-30 14:13:17]

On 2014/09/25 17:11:35, jbroman wrote:
>
https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
> File Source/platform/fonts/GlyphBuffer.h (right):
> 
>
https://codereview.chromium.org/607483002/diff/60001/Source/platform/fonts/Gl...
> Source/platform/fonts/GlyphBuffer.h:98: m_offsets.reverse();
> Are these offsets from the text origin, or from the previous character?

From current glyph's "logical origin".


> If the former, I don't think this logically reverses the glyph buffer. This
> probably works because it so happens we only call reverse in the simple and
> Mac-complex paths.

It's probably worth asserting that reverse() is only ever called from the simple
path.


> If the latter, then I don't understand how they differ from advances, and we
> should probably leave a comment explaining it.

[eae, 2014-09-30 14:53:09]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-30 14:54:25]

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

[commit-bot: I haz the power, 2014-09-30 15:02:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 15:02:56]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[eae, 2014-09-30 15:25:35]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-30 15:26:14]

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

[commit-bot: I haz the power, 2014-09-30 16:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 16:33:59]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[eae, 2014-09-30 18:38:28]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:39:12]

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

[commit-bot: I haz the power, 2014-09-30 18:46:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:46:40]

Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[eae, 2014-09-30 18:47:36]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:48:35]

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

[commit-bot: I haz the power, 2014-09-30 19:00:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:00:23]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[eae, 2014-09-30 19:01:34]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:02:28]

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

[eae, 2014-09-30 21:02:45]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-30 21:02:59]

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

[commit-bot: I haz the power, 2014-09-30 22:16:13]

Committed patchset #11 (id:200001) as 182991