Bug 59182 – [Chromium] Vertical positions are off for some Arabic glyphs on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Kenichi Ishibashi]

See http://crbug.com/76458 for details.

1. Install Scheherazade.ttf, which can be found at [1].
2. Visit [1] and compare the result of "Intalled font:" and "Reference graphic".

[1] http://www.w3.org/International/tests/tests-html-css/tests-webfonts/generate?test=5&format=html5

The rendering is slightly different.  The cause is that ComplexTextController doesn't take care of the vertical offsets of the shaping result. I'll send a patch soon.

[Kenichi Ishibashi]

Created attachment 90682 [details]
Patch V0

[Evan Martin]

Comment on attachment 90682 [details]
Patch V0

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

This patch looks perfect.  I only have very minor comments.

> LayoutTests/platform/chromium-linux/fast/text/arabic-vertical-offset.html:7
> +    src: url(data:font/opentype;base64,AAEAAAAQAQAABAAARkZUTVqB2JMAAAZoAAAAHEdERUYAOQAQAAAFuAAAACZHUE9TtHzC6QAABgAAAABmR1NVQkR2THUAAAXgAAAAIE9TLzJ8FWiUAAABiAAAAFZjbWFwB9EI6gAAAfQAAAFKY3Z0IAAhAnkAAANAAAAABGdhc3D//wADAAAFsAAAAAhnbHlmCNrWUwAAA1AAAACMaGVhZPax+w0AAAEMAAAANmhoZWEGOgOZAAABRAAAACRobXR4DQQAwwAAAeAAAAASbG9jYQBiAJoAAANEAAAADG1heHAASQA5AAABaAAAACBuYW1ldi8ayAAAA9wAAAGVcG9zdCbbGCcAAAV0AAAAOgABAAAAAQAAriJ80l8PPPUAiwPoAAAAAMnW2/EAAAAAydbb8QAh/6wDRAKaAAAACAACAAAAAAAAAAEAAAKa/6wAWgPoAAAAAANEAAEAAAAAAAAAAAAAAAAAAAAEAAEAAAAFAAgAAgAAAAAAAgAAAAEAAQAAAEAALgAAAAAAAQPoAfQABQAAAooCvAAAAIwCigK8AAAB4AAxAQIAAAIABgkAAAAAAAAAACAAAAAAAAAAAAAAAAAAUGZFZABABgYG4QMg/zgAWgKaAFQAAAABAAAAAAAAA+gAIQAAAAAD6AAAA+gAogFMAAAAAAADAAAAAwAAABwAAQAAAAAARAADAAEAAAAcAAQAKAAAAAYABAABAAIGBgbh//8AAAYGBuH///n9+SMAAQAAAAAAAAAAAQYAAAEAAAAAAAAAAQIAAAACAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAhAnkAAAAqACoAKgA4AEYAAgAhAAABKgKaAAMABwAusQEALzyyBwQA7TKxBgXcPLIDAgDtMgCxAwAvPLIFBADtMrIHBgH8PLIBAgDtMjMRIREnMxEjIQEJ6MfHApr9ZiECWAAAAQCiAPsDRAFRAAMAABMhFSGiAqL9XgFRVgAAAAEBTP+sAkwAqAADAAAlIRUhAUwBAP8AqPwAAAAAAA4ArgABAAAAAAAAAAAAAgABAAAAAAABAAUADwABAAAAAAACAAYAIwABAAAAAAADACEAbgABAAAAAAAEAAUAnAABAAAAAAAFABAAxAABAAAAAAAGAAUA4QADAAEECQAAAAAAAAADAAEECQABAAoAAwADAAEECQACAAwAFQADAAEECQADAEIAKgADAAEECQAEAAoAkAADAAEECQAFACAAogADAAEECQAGAAoA1QAAAAB0AGUAcwB0ADEAAHRlc3QxAABNAGUAZABpAHUAbQAATWVkaXVtAABGAG8AbgB0AEYAbwByAGcAZQAgADIALgAwACAAOgAgAHQAZQBzAHQAMQAgADoAIAAyADIALQA0AC0AMgAwADEAMQAARm9udEZvcmdlIDIuMCA6IHRlc3QxIDogMjItNC0yMDExAAB0AGUAcwB0ADEAAHRlc3QxAABWAGUAcgBzAGkAbwBuACAAMAAwADEALgAwADAAMAAgAABWZXJzaW9uIDAwMS4wMDAgAAB0AGUAcwB0ADEAAHRlc3QxAAAAAAACAAAAAAAA/4MAMgAAAAEAAAAAAAAAAAAAAAAAAAAAAAUAAAABAAIBAgEDBkdseXBoMQZHbHlwaDIAAAAAAAH//wACAAEAAAAOAAAAHgAAAAAAAgACAAMAAwABAAQABAADAAQAAAACAAAAAAABAAAACgAcAB4AAURGTFQACAAEAAAAAP//AAAAAAAAAAEAAAAKAB4ALAABREZMVAAIAAQAAAAA//8AAQAAAAFtYXJrAAgAAAABAAAAAQAEAAQAAAABAAgAAQAcABYAAQAiAAwAAQAEAAEB9AIeAAEAAQADAAEAAQAEAAEAAAAGAAEBzAAoAAAAAAABAAAAAMbULpkAAAAAydbSQAAAAADJ1te5);

Did you create this font yourself?  (I am worried about licensing)

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:64
> +    ComplexTextController(const TextRun&, unsigned, unsigned, const Font*);

Is it necessary to take the startingY as an argument since the users always want 0?  (Now that I look, maybe both of these unsigned arguments aren't needed; they might be left over from previous refactorings.)

[Tony Chang]

Comment on attachment 90682 [details]
Patch V0

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

>> LayoutTests/platform/chromium-linux/fast/text/arabic-vertical-offset.html:7
>> +    src: url(data:font/opentype;base64,AAEAAAAQAQAABAAARkZUTVqB2JMAAAZoAAAAHEdERUYAOQAQAAAFuAAAACZHUE9TtHzC6QAABgAAAABmR1NVQkR2THUAAAXgAAAAIE9TLzJ8FWiUAAABiAAAAFZjbWFwB9EI6gAAAfQAAAFKY3Z0IAAhAnkAAANAAAAABGdhc3D//wADAAAFsAAAAAhnbHlmCNrWUwAAA1AAAACMaGVhZPax+w0AAAEMAAAANmhoZWEGOgOZAAABRAAAACRobXR4DQQAwwAAAeAAAAASbG9jYQBiAJoAAANEAAAADG1heHAASQA5AAABaAAAACBuYW1ldi8ayAAAA9wAAAGVcG9zdCbbGCcAAAV0AAAAOgABAAAAAQAAriJ80l8PPPUAiwPoAAAAAMnW2/EAAAAAydbb8QAh/6wDRAKaAAAACAACAAAAAAAAAAEAAAKa/6wAWgPoAAAAAANEAAEAAAAAAAAAAAAAAAAAAAAEAAEAAAAFAAgAAgAAAAAAAgAAAAEAAQAAAEAALgAAAAAAAQPoAfQABQAAAooCvAAAAIwCigK8AAAB4AAxAQIAAAIABgkAAAAAAAAAACAAAAAAAAAAAAAAAAAAUGZFZABABgYG4QMg/zgAWgKaAFQAAAABAAAAAAAAA+gAIQAAAAAD6AAAA+gAogFMAAAAAAADAAAAAwAAABwAAQAAAAAARAADAAEAAAAcAAQAKAAAAAYABAABAAIGBgbh//8AAAYGBuH///n9+SMAAQAAAAAAAAAAAQYAAAEAAAAAAAAAAQIAAAACAAAAAAAAAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAhAnkAAAAqACoAKgA4AEYAAgAhAAABKgKaAAMABwAusQEALzyyBwQA7TKxBgXcPLIDAgDtMgCxAwAvPLIFBADtMrIHBgH8PLIBAgDtMjMRIREnMxEjIQEJ6MfHApr9ZiECWAAAAQCiAPsDRAFRAAMAABMhFSGiAqL9XgFRVgAAAAEBTP+sAkwAqAADAAAlIRUhAUwBAP8AqPwAAAAAAA4ArgABAAAAAAAAAAAAAgABAAAAAAABAAUADwABAAAAAAACAAYAIwABAAAAAAADACEAbgABAAAAAAAEAAUAnAABAAAAAAAFABAAxAABAAAAAAAGAAUA4QADAAEECQAAAAAAAAADAAEECQABAAoAAwADAAEECQACAAwAFQADAAEECQADAEIAKgADAAEECQAEAAoAkAADAAEECQAFACAAogADAAEECQAGAAoA1QAAAAB0AGUAcwB0ADEAAHRlc3QxAABNAGUAZABpAHUAbQAATWVkaXVtAABGAG8AbgB0AEYAbwByAGcAZQAgADIALgAwACAAOgAgAHQAZQBzAHQAMQAgADoAIAAyADIALQA0AC0AMgAwADEAMQAARm9udEZvcmdlIDIuMCA6IHRlc3QxIDogMjItNC0yMDExAAB0AGUAcwB0ADEAAHRlc3QxAABWAGUAcgBzAGkAbwBuACAAMAAwADEALgAwADAAMAAgAABWZXJzaW9uIDAwMS4wMDAgAAB0AGUAcwB0ADEAAHRlc3QxAAAAAAACAAAAAAAA/4MAMgAAAAEAAAAAAAAAAAAAAAAAAAAAAAUAAAABAAIBAgEDBkdseXBoMQZHbHlwaDIAAAAAAAH//wACAAEAAAAOAAAAHgAAAAAAAgACAAMAAwABAAQABAADAAQAAAACAAAAAAABAAAACgAcAB4AAURGTFQACAAEAAAAAP//AAAAAAAAAAEAAAAKAB4ALAABREZMVAAIAAQAAAAA//8AAQAAAAFtYXJrAAgAAAABAAAAAQAEAAQAAAABAAgAAQAcABYAAQAiAAwAAQAEAAEB9AIeAAEAAQADAAEAAQAEAAEAAAAGAAEBzAAoAAAAAAABAAAAAMbULpkAAAAAydbSQAAAAADJ1te5);
> 
> Did you create this font yourself?  (I am worried about licensing)

Is it possible to load the font from disk?  We normally put extra test files in the resources subdirectory (for example, platform/chromium-linux-fast/text/resources/).

[Kenichi Ishibashi]

Hi Evan,

Thank you for taking a look of this patch.

(In reply to comment #2)
> Did you create this font yourself?  (I am worried about licensing)

Yes, I've created it with FontForge from scratch so I think there is no licensing issue.  I'll add an explanation of it.

> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h:64
> > +    ComplexTextController(const TextRun&, unsigned, unsigned, const Font*);
> 
> Is it necessary to take the startingY as an argument since the users always want 0?  (Now that I look, maybe both of these unsigned arguments aren't needed; they might be left over from previous refactorings.)

I might have wrong idea, but I couldn't find other way to pass the starting position to ComplexTextController.  As far as I checked, the starting point is not always zero at FontLinux::drawComplexText().

[Kenichi Ishibashi]

Hi Tony,

(In reply to comment #3)
> Is it possible to load the font from disk?  We normally put extra test files in the resources subdirectory (for example, platform/chromium-linux-fast/text/resources/).

Thank you letting me know that.  I'll put the font on resources directory in the next patch.

[Tony Chang]

Comment on attachment 90682 [details]
Patch V0

I think the change is fine, but please update the ChangeLog to say that you created the font and split the font into a separate file in resources/.

[Kenichi Ishibashi]

Created attachment 91057 [details]
Patch V1

[Kenichi Ishibashi]

Created attachment 91058 [details]
Thai script rendering result

Hi,

I've updated the patch to address Tony's comments. Thanks!

BTW, I noticed that the rendering results of some Thai scripts on Chromium (w/this patch) and Firefox are slightly different.  Chromium w/this patch renders some marks a bit higher than Firefox.  The attached image shows the rendering results of text U+0E17 U+0E4D U+0E2B U+0E49 (I don't understand Thai language so the text might be meaningless).  This difference comes from the shaping result (the return value of HB_ShapeItem()) and I'm not sure this is correct rendering.

Regards,

[Evan Martin]

See https://bugs.webkit.org/show_bug.cgi?id=53637 for some more background on a recent bug we had with getting a font metric wrong that caused glyph positions to be subtly off.

I think we should land this patch, since we know it is correct for at least Arabic, and worry about the Thai issue separately if it's an issue at all.  I know that Firefox usually has correct rendering so it is probably worth looking into, but until we fixed the above bug (which we only fixed a few months ago), Thai rendered completely wrong.  So I don't worry about upsetting our nonexistent Thai userbase.  :)

[Kenichi Ishibashi]

Comment on attachment 91057 [details]
Patch V1

Thank you for review, Tony! Could you cq+?

[Kenichi Ishibashi]

Hi Evan,

I get it. Thank you for the information!

(In reply to comment #9)
> See https://bugs.webkit.org/show_bug.cgi?id=53637 for some more background on a recent bug we had with getting a font metric wrong that caused glyph positions to be subtly off.
> 
> I think we should land this patch, since we know it is correct for at least Arabic, and worry about the Thai issue separately if it's an issue at all.  I know that Firefox usually has correct rendering so it is probably worth looking into, but until we fixed the above bug (which we only fixed a few months ago), Thai rendered completely wrong.  So I don't worry about upsetting our nonexistent Thai userbase.  :)

[WebKit Commit Bot]

Comment on attachment 91057 [details]
Patch V1

Clearing flags on attachment: 91057

Committed r85013: <http://trac.webkit.org/changeset/85013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.