Bug 90362 – [Chromium] Adding HarfBuzz-ng on Linux

• Component: Platform
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

I'm thinking about adding harfbuzz-ng support on Linux so that we can easily check shaping result and performance degradation. It will be behind a compile-time flag. I'll post a patch soon.

[Kenichi Ishibashi]

Created attachment 150373 [details]
WIP patch

[Kenichi Ishibashi]

Tony, Jungshik,

I'd like to ask your opinions. What do you think about adding harfbuzz-ng behind a compile-time flag? The current harfbuzz-ng can't pass all layout tests, and might have performance issue. However, I'd like to proceed this transition.

Note: WIP patch contains some bug fix in HarfBuzzShaper.cpp.

[Tony Chang]

A compile time flag is fine. Please break your patch into smaller pieces and don't mix refactoring with bug fixes.

[Behdad Esfahbod]

Would be nice to have this (or better yet, choose at runtime), so we can measure the performance differences and improve it with solid numbers in hand.  I'm working on optimizing the innermost loop which is the main slowdown spot.

[Kenichi Ishibashi]

Created attachment 150714 [details]
Patch

[Kenichi Ishibashi]

Hi Tony,

(In reply to comment #3)
> A compile time flag is fine. Please break your patch into smaller pieces and don't mix refactoring with bug fixes.

Thank you for the reply. I've uploaded a patch. The patch doesn't include bug fix. The patch is still large, but it's somewhat difficult to break into smaller pieces. Most part of the patch just add ifdefs and its logic is straightforward except for HarfBuzzFaceSkia.cpp. As for HarfBuzzFaceSkia.cpp, I think Behdad is the right person to review.

Behdad, could you do informal review for HarfBuzzFaceSkia.cpp?

[Behdad Esfahbod]

Comment on attachment 150714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150714&action=review

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:54
> +    return value * (1 << 16);

This should depend on some Skia macro.  If SkScalar is a float, this is correct.  If it's a fixed representation instead, we probably want to use it directly.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:57
> +static void SkiaGetGlyphWidthAndExtents(SkPaint* paint, hb_codepoint_t codepoint, hb_position_t* width, hb_glyph_extents_t* extents)

We probably need to add a caching layer to this later on.  Works for now.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:59
> +    ASSERT(codepoint <= 0xFFFF);

Don't assert, just return.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:60
> +    paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding);

Can't you set this once when creating the paint?

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:82
> +    paint->setTextEncoding(SkPaint::kUTF16_TextEncoding);

Same here.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:180
> +GlyphBufferAdvance HarfBuzzShaper::createGlyphBufferAdvance(float width, float height)

Humm, where is this used?

[Behdad Esfahbod]

Comment on attachment 150714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150714&action=review

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:173
> +    hb_font_set_ppem(font, size, size);

Perhaps we should set this only if size is integral.  This affects hinting, etc.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:174
> +    int scale = (1 << 16) * static_cast<int>(size);

Nope.  Why round?  Non-integral font sizes are perfectly valid.  Also, perhaps use SkiaScalarToHarfbuzzPosition() here?

[Kenichi Ishibashi]

Created attachment 150862 [details]
Patch V2

[Kenichi Ishibashi]

Comment on attachment 150714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150714&action=review

Thank you for review!

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:54
>> +    return value * (1 << 16);
> 
> This should depend on some Skia macro.  If SkScalar is a float, this is correct.  If it's a fixed representation instead, we probably want to use it directly.

Changed to use SkScalarToFixed().

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:57
>> +static void SkiaGetGlyphWidthAndExtents(SkPaint* paint, hb_codepoint_t codepoint, hb_position_t* width, hb_glyph_extents_t* extents)
> 
> We probably need to add a caching layer to this later on.  Works for now.

OK, I'll revisit if this is a hot spot.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:59
>> +    ASSERT(codepoint <= 0xFFFF);
> 
> Don't assert, just return.

Done.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:60
>> +    paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding);
> 
> Can't you set this once when creating the paint?

Not sure why, bot it doesn't work. It seems that we need to call setTextEncoding() for each Skia API call.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:82
>> +    paint->setTextEncoding(SkPaint::kUTF16_TextEncoding);
> 
> Same here.

Please see above comment.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:173
>> +    hb_font_set_ppem(font, size, size);
> 
> Perhaps we should set this only if size is integral.  This affects hinting, etc.

Hmm, how can we see whether size is integral? Not calling hb_font_set_ppem() seems doesn't work.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:174
>> +    int scale = (1 << 16) * static_cast<int>(size);
> 
> Nope.  Why round?  Non-integral font sizes are perfectly valid.  Also, perhaps use SkiaScalarToHarfbuzzPosition() here?

Ah, ComplexTextController (which uses old harfbuzz) passes scale as int and I just copied it. Changed to use SkiaScalarToHarfbuzzPosition().

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:180
>> +GlyphBufferAdvance HarfBuzzShaper::createGlyphBufferAdvance(float width, float height)
> 
> Humm, where is this used?

This is a part of HarfBuzzShaper class and used internally. This should be placed in HarfBuzzShaper.cpp, but GlyphBufferAdvance is a platform-dependent type and there is no common way to create it so we can't place this method in HarfBuzzShaper.cpp.

[Behdad Esfahbod]

(In reply to comment #10)

> >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:173
> >> +    hb_font_set_ppem(font, size, size);
> > 
> > Perhaps we should set this only if size is integral.  This affects hinting, etc.
> 
> Hmm, how can we see whether size is integral?

A lazy way is "floorf(size) == size".

> Not calling hb_font_set_ppem() seems doesn't work.

That sounds wrong.  "doesn't work" as in what?

[Kenichi Ishibashi]

Created attachment 150993 [details]
Patch V3

[Kenichi Ishibashi]

> > Not calling hb_font_set_ppem() seems doesn't work.
> 
> That sounds wrong.  "doesn't work" as in what?

I saw broken shaping result, but it turns out this is unrelated. Patch updated.

[Behdad Esfahbod]

Comment on attachment 150993 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=150993&action=review

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:72
> +        // Skia flips the y bearing so we need not put minus here.

The comments is out of sync with code.  That said, the extents are all unused in harfbuzz as of now.  They will be used in the future for fallback mark positioning.  So perhaps you can update the comment to say we're unsure whether a negation is needed, and adjust later when we're sure.

[Tony Chang]

Comment on attachment 150993 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=150993&action=review

r- because of running code in an ASSERT.

> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:227
> +    ASSERT(shaper.shape());

In a release build, will the call to shaper.shape() get compiled out?

> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:245
> +    ASSERT(shaper.shape());

Ditto.

> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:264
> +    ASSERT(shaper.shape());

Ditto.

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:139
> +#if USE(HARFBUZZ_NG)
> +    HarfBuzzFace* harfbuzzFace() const;
> +#else
>      HarfbuzzFace* harfbuzzFace() const;
> +#endif

The difference between HarfBuzzFace and HarfbuzzFace is too subtle.  It would be nice if we could make the names more distinct.  Maybe HarfbuzzNGFace?  This could be a follow up patch.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:150
> +    return hb_blob_create((const char*)buffer, tableSize,

Nit: const_cast<>

> Source/WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:55
> +    // FIXME: Implement.

Nit: Replace this with notImplemented()?

[Kenichi Ishibashi]

Created attachment 151358 [details]
Patch V4

[Kenichi Ishibashi]

Comment on attachment 150993 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=150993&action=review

>> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:227
>> +    ASSERT(shaper.shape());
> 
> In a release build, will the call to shaper.shape() get compiled out?

Thanks. Fixed.

>> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:139
>> +#endif
> 
> The difference between HarfBuzzFace and HarfbuzzFace is too subtle.  It would be nice if we could make the names more distinct.  Maybe HarfbuzzNGFace?  This could be a follow up patch.

I see. HarfBuzzShaper uses this interface, and HarfBuzzShaper is used in mac port so I'd like to do it in a follow up patch.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:72
>> +        // Skia flips the y bearing so we need not put minus here.
> 
> The comments is out of sync with code.  That said, the extents are all unused in harfbuzz as of now.  They will be used in the future for fallback mark positioning.  So perhaps you can update the comment to say we're unsure whether a negation is needed, and adjust later when we're sure.

Done. Thanks for the heads-up.

>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFaceSkia.cpp:150
>> +    return hb_blob_create((const char*)buffer, tableSize,
> 
> Nit: const_cast<>

Done.

>> Source/WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:55
>> +    // FIXME: Implement.
> 
> Nit: Replace this with notImplemented()?

Done.

[Tony Chang]

Comment on attachment 151358 [details]
Patch V4

View in context: https://bugs.webkit.org/attachment.cgi?id=151358&action=review

> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:188
> +    ASSERT(shaper.shape(&glyphBuffer));

This looks wrong.  Please fix before landing.

[Kenichi Ishibashi]

Comment on attachment 151358 [details]
Patch V4

View in context: https://bugs.webkit.org/attachment.cgi?id=151358&action=review

>> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:188
>> +    ASSERT(shaper.shape(&glyphBuffer));
> 
> This looks wrong.  Please fix before landing.

I should have checked carefully.. Thanks.

[Kenichi Ishibashi]

Created attachment 151549 [details]
Patch for landing

[Kenichi Ishibashi]

Created attachment 151550 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151550 [details]
Patch for landing

Clearing flags on attachment: 151550

Committed r122310: <http://trac.webkit.org/changeset/122310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.