Bug 59182 - [Chromium] Vertical positions are off for some Arabic glyphs on Linux
Summary: [Chromium] Vertical positions are off for some Arabic glyphs on Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://crbug.com/76458
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-22 02:08 PDT by Kenichi Ishibashi
Modified: 2011-04-26 21:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch V0 (21.58 KB, patch)
2011-04-22 02:25 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (21.32 KB, patch)
2011-04-25 22:47 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Thai script rendering result (16.64 KB, image/png)
2011-04-25 22:55 PDT, Kenichi Ishibashi
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2011-04-22 02:08:16 PDT
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.
Comment 1 Kenichi Ishibashi 2011-04-22 02:25:58 PDT
Created attachment 90682 [details]
Patch V0
Comment 2 Evan Martin 2011-04-22 09:12:12 PDT
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.)
Comment 3 Tony Chang 2011-04-22 10:06:41 PDT
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/).
Comment 4 Kenichi Ishibashi 2011-04-24 18:12:46 PDT
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().
Comment 5 Kenichi Ishibashi 2011-04-24 18:14:17 PDT
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.
Comment 6 Tony Chang 2011-04-25 10:17:26 PDT
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/.
Comment 7 Kenichi Ishibashi 2011-04-25 22:47:18 PDT
Created attachment 91057 [details]
Patch V1
Comment 8 Kenichi Ishibashi 2011-04-25 22:55:02 PDT
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,
Comment 9 Evan Martin 2011-04-26 10:21:50 PDT
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.  :)
Comment 10 Kenichi Ishibashi 2011-04-26 16:46:15 PDT
Comment on attachment 91057 [details]
Patch V1

Thank you for review, Tony! Could you cq+?
Comment 11 Kenichi Ishibashi 2011-04-26 16:54:02 PDT
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.  :)
Comment 12 WebKit Commit Bot 2011-04-26 21:50:08 PDT
Comment on attachment 91057 [details]
Patch V1

Clearing flags on attachment: 91057

Committed r85013: <http://trac.webkit.org/changeset/85013>
Comment 13 WebKit Commit Bot 2011-04-26 21:50:13 PDT
All reviewed patches have been landed.  Closing bug.