Bug 161493

Created attachment 287633 [details] Rendering in Chrome

Created attachment 287634 [details] Rendering in Firefox

Upon further examination, it seems that Y offsets returned from HarfBuzz are eventually discarded when creating the GlyphBuffer. In HarfBuzzShaper::fillGlyphBufferFromHarfBuzzRun(), the Y offset is (incorrectly?) used to calculate the Y advance of the glyph. Also it seems that support for Y offsets in GlyphBuffer is enabled only on Windows. So probably what is needed here is to enable GlyphBuffer offsets also for Gtk, populate the from HarfBuzz runs, and finally using them in FontCascade::drawGlyphs(). This is all pure speculation from just reading the code.

By Gtk I meant Cairo.

Created attachment 287860 [details] Proof of concept patch This is a proof of concept patch that fixes the Y positioning issue for me. It seems just setting the offsets is all needed, probably other parts of the code will apply them when present. I haven’t tested what is the effect of setting the X offset as well (e.g. on kerning).

Comment on attachment 287860 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=287860&action=review > Source/WebCore/platform/graphics/GlyphBuffer.h:257 > Vector<FloatSize, 2048> m_offsets; By the way, I'm doing something similar here: https://bugs.webkit.org/show_bug.cgi?id=161119

Myles, we don't have any GTK+ reviewers who understand fonts, let alone complex layouts or Arabic fonts. I know you don't know much or anything about FreeType or Harbuzz or Cairo, but I think you're nevertheless the reviewer for this. Do you mind keeping an eye on this bug, and review Khaled's work when he thinks it's ready to land? (It's not obsoleted by your work in bug #161119, is it?)

(In reply to comment #7) > Myles, we don't have any GTK+ reviewers who understand fonts, let alone > complex layouts or Arabic fonts. I know you don't know much or anything > about FreeType or Harbuzz or Cairo, but I think you're nevertheless the > reviewer for this. Do you mind keeping an eye on this bug, and review > Khaled's work when he thinks it's ready to land? (It's not obsoleted by your > work in bug #161119, is it?) I feel honored 😊 Yeah, I'll do my best to review it. Luckily, I have machines which can run FreeType, HarfBuzz, & Cairo, so I'll spend some time getting to know those libraries in order to review this. The patch you linked to should be orthogonal to this one. Khaled: am I correct in assuming this bug only reproduces with specific fonts? Because of licensing issues, we can't include arbitrary fonts in our tests. I would be happy to work with you to make a test font which exhibits this bug.

Comment on attachment 287860 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=287860&action=review >> Source/WebCore/platform/graphics/GlyphBuffer.h:257 >> Vector<FloatSize, 2048> m_offsets; > > By the way, I'm doing something similar here: https://bugs.webkit.org/show_bug.cgi?id=161119 Whoops, this was the wrong link. I meant to link to https://trac.webkit.org/changeset/205396

The issue happens in any font that makes use of Y direction movement, and most Arabic fonts do that so it can be quite visible there bit it is not limited to them. I actually already built a test font that can be used to test this bug and I can provide it under whatever suitable license. I just need to figure out how to do the tests part then provide a finished patch.

GlyphBuffer is only used for painting. As such, we can save space if, instead of storing separate advances / origins in it, we collapse these into "painting advances" which are the offset from each glyph's paint position to the next. I considered renaming GlyphBuffer to something like PaintingGlyphBuffer but ultimately decided it wasn't worth the effort. Ultimately, we should remove the m_offsets member from GlyphBuffer in the Windows port. I plan on doing this at some point in the future. Rather than increasing the usage of this variable, I would suggest computing each glyph's paint position inside HarfBuzzShaper (thereby taking into account the offsets) and storing that in GlyphBuffer.

(In reply to comment #10) > The issue happens in any font that makes use of Y direction movement, and > most Arabic fonts do that so it can be quite visible there bit it is not > limited to them. > > I actually already built a test font that can be used to test this bug and I > can provide it under whatever suitable license. I just need to figure out > how to do the tests part then provide a finished patch. This is fantastic! Please let me know if you have any questions about our testing infrastructure. I would be glad to help out :D

Comment on attachment 287860 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=287860&action=review > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:580 > + glyphBuffer->add(glyphs[i], currentRun->fontData(), createGlyphBufferAdvance(glyphAdvanceX, glyphAdvanceY), currentCharacterIndex, &offset); It appears that FontCascade::drawGlyphBuffer() doesn't actually move any glyphs according to the offsets. How does this work?

> > Whoops, this was the wrong link. I meant to link to > https://trac.webkit.org/changeset/205396 This commit does the same thing that this patch does, except for Cocoa ports.

Comment on attachment 287860 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=287860&action=review >> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:580 >> + glyphBuffer->add(glyphs[i], currentRun->fontData(), createGlyphBufferAdvance(glyphAdvanceX, glyphAdvanceY), currentCharacterIndex, &offset); > > It appears that FontCascade::drawGlyphBuffer() doesn't actually move any glyphs according to the offsets. How does this work? There is a different add() function which takes a GlyphBufferAdvance, which should be 2-dimensional.

Comment on attachment 287860 [details] Proof of concept patch View in context: https://bugs.webkit.org/attachment.cgi?id=287860&action=review >>> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:580 >>> + glyphBuffer->add(glyphs[i], currentRun->fontData(), createGlyphBufferAdvance(glyphAdvanceX, glyphAdvanceY), currentCharacterIndex, &offset); >> >> It appears that FontCascade::drawGlyphBuffer() doesn't actually move any glyphs according to the offsets. How does this work? > > There is a different add() function which takes a GlyphBufferAdvance, which should be 2-dimensional. Hopefully, if you migrate to this other add() function, you can remove the entire add() function which takes the float width.

(In reply to comment #11) > GlyphBuffer is only used for painting. As such, we can save space if, > instead of storing separate advances / origins in it, we collapse these into > "painting advances" which are the offset from each glyph's paint position to > the next. > > I considered renaming GlyphBuffer to something like PaintingGlyphBuffer but > ultimately decided it wasn't worth the effort. > > Ultimately, we should remove the m_offsets member from GlyphBuffer in the > Windows port. I plan on doing this at some point in the future. Rather than > increasing the usage of this variable, I would suggest computing each > glyph's paint position inside HarfBuzzShaper (thereby taking into account > the offsets) and storing that in GlyphBuffer. I'm doing this at https://bugs.webkit.org/show_bug.cgi?id=161814

(In reply to comment #17) > (In reply to comment #11) > > GlyphBuffer is only used for painting. As such, we can save space if, > > instead of storing separate advances / origins in it, we collapse these into > > "painting advances" which are the offset from each glyph's paint position to > > the next. > > > > I considered renaming GlyphBuffer to something like PaintingGlyphBuffer but > > ultimately decided it wasn't worth the effort. > > > > Ultimately, we should remove the m_offsets member from GlyphBuffer in the > > Windows port. I plan on doing this at some point in the future. Rather than > > increasing the usage of this variable, I would suggest computing each > > glyph's paint position inside HarfBuzzShaper (thereby taking into account > > the offsets) and storing that in GlyphBuffer. > > I'm doing this at https://bugs.webkit.org/show_bug.cgi?id=161814 Whoops, looks like it's used in FontCGWin.cpp... not sure how I missed that...

Created attachment 289820 [details] Patch

Since advances and offsets are different things, if we need to get rid of the separate offsets we will need to store the absolute glyph position in the GlyphBuffer instead. Is this what you are suggesting?

Did you intend to request a review on this, Fred?

(In reply to comment #21) > Did you intend to request a review on this, Fred? No, I was just uploading Khaled's test into LayoutTests. However, the patch does not change the render tree so it should probably be verified with a reftest instead.

Created attachment 289849 [details] Patch

(In reply to comment #20) > Since advances and offsets are different things, if we need to get rid of > the separate offsets we will need to store the absolute glyph position in > the GlyphBuffer instead. Is this what you are suggesting? Not quite. Let me approach it from a different way: If you have base advances, along with offsets, you can find absolute glyph positions. Then, once you have absolute glyph positions, you can find glyph paint-time advances by subtracting successive pairs of absolute positions. I'm suggesting putting glyph paint-time advances into the GlyphBuffer.

Comment on attachment 289849 [details] Patch r- because the the approach I detailed earlier. Please let me know if I can offer any more assistance on this patch - you can find me on the Freenode IRC server with the username "litherum". I am very excited for this work! Thanks for helping us out with this! :D

(In reply to comment #24) > I'm suggesting putting glyph paint-time advances into the GlyphBuffer. But that wouldn’t help with the bug here, since the issue is related to Y offsets, not X offsets so storing the paint advances would gain me nothing as Y offsets would still be unhandled (unless it is vertical text of course, but that is not what I’m dealing with here).

(In reply to comment #26) > (In reply to comment #24) > > I'm suggesting putting glyph paint-time advances into the GlyphBuffer. > > But that wouldn’t help with the bug here, since the issue is related to Y > offsets, not X offsets so storing the paint advances would gain me nothing > as Y offsets would still be unhandled (unless it is vertical text of course, > but that is not what I’m dealing with here). GlyphBuffer's "m_advances" member variable is a Vector of GlyphBufferAdvances. It appears that on all ports, each GlyphBufferAdvance holds both an X component and a Y component.

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #24) > > > I'm suggesting putting glyph paint-time advances into the GlyphBuffer. > > > > But that wouldn’t help with the bug here, since the issue is related to Y > > offsets, not X offsets so storing the paint advances would gain me nothing > > as Y offsets would still be unhandled (unless it is vertical text of course, > > but that is not what I’m dealing with here). > > GlyphBuffer's "m_advances" member variable is a Vector of > GlyphBufferAdvances. It appears that on all ports, each GlyphBufferAdvance > holds both an X component and a Y component. I still don’t see how this can be used here, GlyphBufferAdvance stores width and height, I can see how width can be used, e.g. instead of doing: x = 0 y = 0 for glyph in buffer: draw(glyph, x + glyph.xoffset, y + glyph.yoffset) x += glyph.width We can resolve glyph widths to take into account x offsets and then do: x = 0 y = 0 for glyph in buffer: draw(glyph, x, y + glyph.yoffset) x += glyph.width But I don’t see how this works for y offsets since y (unlike x) is not incremented by previous glyph y advance, so whatever value we calculate for the y advance is not going to affect the y placement of the glyph, unless I’m missing something here.

> for glyph in buffer: > draw(glyph, x + glyph.xoffset, y + glyph.yoffset) In WebKit, glyphs are not drawn individually. Inside FontCascade::drawGlyphBuffer(), WebKit breaks up text into runs which all use the same font, and then calls the platform-specific implementation of FontCascade::drawGlyphs() on each run. As you can see in the Windows/Cocoa implementations of FontCascade::drawGlyphs() (inside FontCGWin.cpp), a position as well as a series of X and Y advances are given to the platform drawing routine. > y (unlike x) is not incremented by previous glyph y advance Inside FontCascade::drawGlyphBuffer(), you can see[1] that both X and Y advances are summed to produce the position of each subsequent run. Therefore, each X advance and Y advance are treated the same way - both contribute to the position of the next run. "nextY += glyphBuffer.advanceAt(nextGlyph).height();" The semantics of the GlyphBuffer are such that the X and Y components of a glyph's advance form a vector which represents the displacement of that glyph's position to the next. Therefore, both X and Y are incremented by the advance. Each individual platform may require a different format of input (meaning: your claim may be true for the input of a particular platform's drawing routine). However, since all ports of WebKit share GlyphBuffer, its own semantics must be consistent across all ports. The semantics of GlyphBuffer are such that its advances' X and Y components form a displacement vector. If a particular platform's drawing routine requires the input to be in a different form, then that platform-specific implementation of FontCascade::drawGlyphs() must do any work to convert WebKit's GlyphBuffer format to whatever format the platform requires. Fortunately, the Mac, iOS, and Windows code all share the same format as GlyphBuffer, so those ports perform minimal conversion inside their platform-specific code. The previous paragraph describes the semantics of GlyphBuffer today. Perhaps the current semantics of GlyphBuffer can be improved to a form which matches your expectations. However, such a discussion should be discussed in a separate bug since it is a separate issue. I hope this is helpful; please let me know if you have any confusion or any questions about this. Thanks for taking such an interest in this! I am very excited to see this problem fixed :D [1] https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/FontCascade.cpp#L1306

Currently, it looks like the Cairo implementation of FontCascade::drawGlyphs() disregards the Y component of the advances in the GlyphBuffer. FontCascade::drawGlyphs() will need to be changed to consider these Y components in order to fix this bug. In particular, the line "glyphs[i].y = point.y();" will ignore the glyph's y position and will simply force the glyph's position to be the position of the baseline. According to the Cairo documentation[1], it looks like the X and Y coordinates of the cairo_glyph_t structure (which is used in drawGlyphsToContext() by cairo_show_glyphs() to draw a run of glyphs) have a similar meaning. These seem to be a position for the glyph, which can be created by summing up all the Y advances in the GlyphBuffer. It seems that the modification to FontCascade::drawGlyphs() will be a one-line change. [1] https://www.cairographics.org/manual/cairo-text.html#cairo-glyph-t

Khaled, any interest in updating this patch?

(In reply to comment #31) > Khaled, any interest in updating this patch? Unfortunately I’m still not sure I follow the proposed changes and I don’t have much time to look into it anytime soon.

(In reply to comment #32) > (In reply to comment #31) > > Khaled, any interest in updating this patch? > > Unfortunately I’m still not sure I follow the proposed changes and I don’t > have much time to look into it anytime soon. I just looked into it and I actually want to make ComplexTextController work with Uniscribe and Harfbuzz. I will try to do the Uniscribe piece tomorrow. If this works, the Windows and Linux ports will get a lot of stuff for free, including vertical glyph origins.

Thanks a bunch Myles, much appreciated!

(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #31) > > > Khaled, any interest in updating this patch? > > > > Unfortunately I’m still not sure I follow the proposed changes and I don’t > > have much time to look into it anytime soon. > > I just looked into it and I actually want to make ComplexTextController work > with Uniscribe and Harfbuzz. I will try to do the Uniscribe piece tomorrow. > If this works, the Windows and Linux ports will get a lot of stuff for free, > including vertical glyph origins. Work in progress at https://bugs.webkit.org/show_bug.cgi?id=167566

Created attachment 334733 [details] Test patch Here is a different and simpler batch based on the changed to text layout since the first patch. It works most of the time except when the first glyph in the run is shifted vertically (e.g. the last character in the Arabic text is a combining mark), then the mark will be drawn at the baseline and the rest of the glyphs will be shifted relative to it (the patch has some debug code to draw a baseline to make spotting this easier). I’m not sure how to fix this. I tried to offset all the glyphs by the height of the first one, which fixes the case of the last character being a mark, but break when the before last character is a mark. I appreciate any suggestions how to move this forward.

I think the problem is that ComplexTextController adds glyph origins to their advance in a single advances. But in cairo we need origins and advances separately. We need to apply the current origin when drawing, and then advance. We are using the current point as origin, and then advancing the advance + origin. In these examples there's a y origin we are ignoring. So, we have: glyphs[i] = { glyphsData[i], xOffset, yOffset }; xOffset += advances[i].width(); this should be glyphs[i] = { glyphsData[i], xOffset + xOrigin, yOffset + yOrigin }; xOffset += advances[i].width(); but we don't have the origins in GlyphBuffer.

(In reply to Carlos Garcia Campos from comment #37) > I think the problem is that ComplexTextController adds glyph origins to > their advance in a single advances. But in cairo we need origins and > advances separately. We need to apply the current origin when drawing, and > then advance. We are using the current point as origin, and then advancing > the advance + origin. In these examples there's a y origin we are ignoring. > > So, we have: > > glyphs[i] = { glyphsData[i], xOffset, yOffset }; > xOffset += advances[i].width(); > > this should be > > glyphs[i] = { glyphsData[i], xOffset + xOrigin, yOffset + yOrigin }; > xOffset += advances[i].width(); > > but we don't have the origins in GlyphBuffer. Ok, now I understand how the paint advances work. The origins are taken into account in the advances, but we need to use the first origin as the initial advance of the run.

Created attachment 366166 [details] Patch

Created attachment 366167 [details] Patch for landing

Committed r243602: <https://trac.webkit.org/changeset/243602>