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.
Created attachment 150373 [details] WIP patch
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.
A compile time flag is fine. Please break your patch into smaller pieces and don't mix refactoring with bug fixes.
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.
Created attachment 150714 [details] Patch
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?
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?
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?
Created attachment 150862 [details] Patch V2
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.
(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?
Created attachment 150993 [details] Patch V3
> > 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.
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.
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()?
Created attachment 151358 [details] Patch V4
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.
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.
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.
Created attachment 151549 [details] Patch for landing
Created attachment 151550 [details] Patch for landing
Comment on attachment 151550 [details] Patch for landing Clearing flags on attachment: 151550 Committed r122310: <http://trac.webkit.org/changeset/122310>
All reviewed patches have been landed. Closing bug.