297074 - Make nsRenderingContext::GetWidth optionally return an array of glyph widths

Status: RESOLVED WONTFIX

(Core Graveyard :: GFX, defect)

Description

[Simon Montagu :smontagu]

The current implementation of letter spacing, word spacing and justification
calls GetWidth() and then DrawString() for each character separately, which
gives very inaccurate results in complex text.

I know that we plan to have a more comprehensive solution in Thebes, but in the
meantime rendering would get a lot better (and I expect that performance would
improve also) if we could measure the whole text in one call to an API which
returned an array of glyph widths mapped to the code points of the input string
and then added the extra spacing in a script-sensitive fashion.

It turns out that on Windows XP this can be done with very little effort. I am
still investigating possibilities for other platforms.

Comment 1

[Simon Montagu :smontagu]

Attachment: Proof of concept

This is not remotely near finished, or worth anyone's time to look at. I'm just
attaching it as a checkpoint.

Comment 2

[Simon Montagu :smontagu]

I don't want to start marking dependencies just yet, but here are a few bugs
that are fixed by this (on XP with complex text support):

Bug 60546
Bug 127661
Bug 185600
Bug 202351
Bug 240914
Bug 248361
Bug 270012

Comment 3

[Simon Montagu :smontagu]

Attachment: Omnibus testcase from the bugs mentioned in comment 2

Sorry, bug 127661 doesn't belong in that list.

Comment 4

[Simon Montagu :smontagu]

Attachment: Screenshot of the testcase

Comment 5

[Simon Montagu :smontagu]

This doesn't really block bug 157967. The work here can probably be leveraged
into future support for Uniscribe etc., but the object is to achieve a better
rendering with vanilla Win32 API, and the equivalent on other platforms if
possible, without explicitly using the Uniscribe API.

Comment 6

[Stephen Blackheath]

Attachment: Fixes Hebrew text with vowels/diacritics on Windows and GTK-Pango

Simon - I am now aware that this probably duplicates some of your work, but
here it is to get it out there.  It fixes the worst of my Hebrew rendering
woes.  Your GetWidth()/GetWidthArray() is the same as my GetCharacterSpacing(),
and you have implemented it on Windows, while I have implemented it in
GTK/Pango.

While you are on vacation, I will work on a new patch that combines the best of
our two.

The changes in my patch are:
* Fix nsTextFrame::RenderString() so it does not assume the width of a string
equals the sum of its component characters (which assumption does not hold true
for Hebrew+vowels).
* Add a new optionally-implemented GetCharacterSpacing() function to the gfx
layer that makes it return the horizontal spacing for each character that the
gfx would use in the absence of justification.
* Fix the layout of RTL text on Windows, which was not right.
* Add NS_RENDERING_HINT_REORDER_SPACED_TEXT on Windows which should be there.

Comment 7

[Stephen Blackheath]

Attachment: Fixes Hebrew text with vowels/diacritics on Windows and GTK-Pango

This new version of the patch combines my work with Simon's.  It resolves the
worst of the Hebrew rendering issues.  I discuss this more on my website
http://blacksapphire.com/firefox-rtl/  I've tested it on Linux and Windows. 
GetWidthArray is now implemented on both Pango and Windows.

I have carefully written it to make sure it doesn't break anything it doesn't
affect, and other than a little more general testing (particularly on Windows),
I would be happy to have this patch committed to CVS.

Simon - please review.

Comment 8

[Stephen Blackheath]

Attachment: Fixes Hebrew+diacritics and other languages, Windows and GTK-Pango

Version 4.9 patch: This fixes the brokenness on Windows 98 by using a more
primitive method for DrawString and for GetWidthArray.

Comment 9

[Stephen Blackheath]

Attachment: Fixes Hebrew+diacritics and other languages, Windows and GTK-Pango

Fixes a problem that was occurring on Linux with Arabic.  Arabic works nicely
now.

Comment 10

[Stephen Blackheath]

Attachment: Patch version 4.11: Fixes multiple complex-layout languages on Windows and GTK-Pango

Fixes a problem with Indic languages on Linux (i.e. GTK/Pango).

Comment 11

[Stephen Blackheath]

Attachment: Patch version 4.12: Fixes multiple complex-layout languages on Windows and GTK-Pango

Comment 12

[Sukh]

I have to say thank you for this patch.  It's made a significant difference to
Indic language support on Firefox.

I've tested it on the latest Deer Park release and it works well.  Originally in
Firefox 1.0, Indic text would break up when selected with the cursor.  This was
subsequently fixed in DP although not perfect yet because for Indic text the
syllables should be selected, not the individual text components.  However with
this patch, non-Justified text is fine when selecting, but Justified text still
breaks up into pieces.  Although I must say that this is considerably better
than the current situation where justified text is always broken up and thus
unreadable!

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's worth thinking about how this change interacts with the work on switching
GFX to cairo.

Comment 14

[Aaron Leventhal]

The current patch doesn't apply cleanly to trunk or branch.

Comment 15

[Aaron Leventhal]

*** Bug 309101 has been marked as a duplicate of this bug. ***

Comment 16

[Aaron Leventhal]

(In reply to comment #14)
> The current patch doesn't apply cleanly to trunk or branch.

FWIW, I think this patch will help quite a bit but we also need bug 309099.
There was a big problem even when text wasn't painted char by char (when using
word-spacing). We shouldn't be repainting the entire screen every time an
outline is painted.

Comment 17

[Aaron Leventhal]

(In reply to comment #16)
> FWIW, I think this patch will help quite a bit
(I was referring to the major screen reader perfomance drag in bug 307545)

Comment 18

[Stuart Parmenter]

For what its worth, all the code in that patch is thrown out for 1.9.  We'll be
providing an interface for getting an array of glyph advances and indexes, but
it will be pretty different than this.

Comment 19

[Stuart Parmenter]

the gfx side of things, that is.

Comment 20

[Aaron Leventhal]

Attachment: Same stuff, but applies cleanly to MOZILLA_1_8_BRANCH

This fixes 95% of the screen reader slowness issue. All the real pages with the
problem behave well enough with this patch. I have a torture test in bug 309099
which is still not perfect, but for the release of 1.5 fixing this would be
good enough.

Comment 21

[Aaron Leventhal]

This greatly improves bug 307545 which is blocking 1.8b5+

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Is this patch's risk is low for landing on 1.8? I don't think so...

Comment 23

[Aaron Leventhal]

(In reply to comment #22)
> Is this patch's risk is low for landing on 1.8? I don't think so...

I don't think so either but Roc asked me to find out if this fixes the problem,
which we do need to deal with for 1.8.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this patch's value is very big for i18n. But, this patch is very
large for after beta release. If we find any regressions, we should block the
release for the regression(s). (And if find after release, we should fix on 1.8
branch for minor release.) If it is guaranteed, I don't have dissenting opinion.

Comment 25

[Simon Fraser [no longer active]]

What impact (if any) does this patch have on platforms that don't implement the
nsRenderingContext::GetWidth() stuff?

Comment 26

[Aaron Leventhal]

Patch is too large, so removing blocking request.

Comment 27

[Stephen Blackheath]

(In reply to comment #25)
> What impact (if any) does this patch have on platforms that don't implement the
> nsRenderingContext::GetWidth() stuff?

I've worked hard to make sure there isn't a performance penalty in that
situation.  Take a look at nsTextFrame_MeasureWalker::visit().  You'll see that
it asks nsRenderingContext::getWidthArray(), and if it isn't implemented, it
uses a method that works the same as the old one, except that it is modified
slightly in that it will behave more intelligently if it sees any characters it
suspects might be complex layout.  So for Latin text, the performance should be
pretty much the same as before.

Aaron - thanks for the cleanup of the patch.  I cleaned it up myself for the
4.12 version, but the merge was broken within a week!

I am continuing to work on this patch - see http://blacksapphire.com/firefox-rtl/
I hope all my work isn't thrown out with the new 1.9 code!  Would it be a better
use of my time to look at the new code?  If so, how?  I can certainly help with
testing.

Sukh - can you provide a URL and screenshots for the issues you mention, and
instructions for someone who doesn't read Indic languages?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Talk to pavlov, he's working on this. Find him on IRC if necessary. Hopefully we
can fold this into the new font code.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try the IRC #gfx channel.

The basic plan is to use Uniscribe on Windows, Pango on Linux and ATSUI on Mac
to turn DOM text into glyph indices and coordinates, and then render the glyphs
through cairo. I guess it will be similar in flavour to this patch. I'm sure
we'd appreciate your help defining the API and then massaging this patch to use
that API.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 197201 [details] [diff] [review]
Same stuff, but applies cleanly to MOZILLA_1_8_BRANCH

I don't think we're taking this for branch.

Comment 31

[Sukh]

I seriously do hope this patch makes it into Firefox 1.5.  At the moment,
Firefox is very poor at rendering Indic text on anything that is spaced text -
to the extent that it is unusable and unreadable.

Stephen:  Visit http://tdil.mit.gov.in/hindi_site/ach-mat.htm and hopefully with
the patch applied the page will look okay.  Ensure your computer system has
complex text rendering support for Indic scripts and that you have a Devanagari
font installed.

Now attempt to select some text.  You'll notice that as you select the text, it
breaks up into pieces.  Now look at
http://www.bbc.co.uk/hindi/sport/story/2005/10/051007_sania_qf.shtml and try to
select the text.  As the text is NOT justified, the selecting the text does not
break it up into pieces.  This is the desired form of selecting text
(technically the best option would be selecting syllables, but that's the least
of our worries in terms of Indic support on Firefox).

Comment 32

[Simon Montagu :smontagu]

This is superseded by the new textframe.