Closed Bug 370588 Opened 14 years ago Closed 14 years ago

Rework gfxTextRun, part 2


(Core :: Graphics, defect)

Not set





(Reporter: roc, Assigned: roc)




(3 files, 3 obsolete files)

The following patch does two major things: hoist the platform-independent code from gfxPangoTextRun to gfxTextRun (removing gfxPangoTextRun in the process), and implement gfxTextRun "properly" for Mac and Windows. These have to be done at the same time.
Attached patch patch (obsolete) — Splinter Review
This will probably regress performance on Windows and Mac, because they were caching things already (so no new win there) and this code does a bit more work now (extracting all advances and storing them with the glyph indices). But hopefully it's only a small difference and I'm sure it's a step in the right direction...

For the Windows draw path, I now just use the direct cairo_show_glyphs path always. If this hurts drawing performance, I'd be surprised.
Attachment #255306 - Flags: review?(pavlov)
The patch works fine on Windows excepted the spacing rendering problem.
# the issue already happens on Linux after landing the new textrun.
Which problem is that?

I'm not sure about the handling of missing glyphs on Mac and Windows. I should be calling SetMissingGlyph in some places, but I'm not.
(In reply to comment #3)
> Which problem is that?

I can reproduce that the text is overflowed from nsTextFrame if the text is justified.
Attached patch updated patch (obsolete) — Splinter Review
Updated with missing-glyph detection for Windows. The existing Mac code doesn't have any explicit missing-glyph detection and I'm not sure what to do there so I just left it alone.

I found the bug with justification being off. It's a generic spacing bug that mistakenly added the spacing for the last character in a glyph-run twice. It's fixed in this patch by removing code at the end of gfxFont::Draw.

I noticed that Windows (Uniscribe) is making item boundaries at punctuation even in simple testcases, which is a little weird, although it shouldn't be a problem as long as we process items efficiently. I modified gfxTextRun::AddGlyphRun to merge glyphruns with the same font so that those extra items don't affect the generated textrun.
Attachment #255306 - Attachment is obsolete: true
Attachment #255622 - Flags: review?(pavlov)
Attachment #255306 - Flags: review?(pavlov)
Comment on attachment 255622 [details] [diff] [review]
updated patch

Vlad should look at the ATSUI code.
Attachment #255622 - Flags: review?(vladimir)
Attached patch faster patchSplinter Review
this makes the windows code use its fast paths more often and sets IS_ASCII/IS_8BIT appropriately.  I'm seeing speed results between -2% and +5% on Tp on my machine which may just be noise so I don't think we'll see any real performance hit (at least on Windows).
Attachment #255622 - Attachment is obsolete: true
Attachment #255622 - Flags: review?(vladimir)
Attachment #255622 - Flags: review?(pavlov)
+    if (IsAscii(aString, aLength))
+        params.mFlags |= gfxTextRunFactory::TEXT_IS_ASCII;
+    else
+        params.mFlags |= gfxTextRunFactory::TEXT_IS_8BIT;

8BIT and ASCII aren't mutually exclusive. This should always set 8BIT and the assertion in gfxWindowsFontGroup::MakeTextRun(PRUint8*) could just be removed.

Otherwise, looks good.
Running Camino with this patch applied I crashed when viewing an email message in Gmail

1. enter Gmail
2. view individual message
3. crash while loading

from Apple crash.log
0   org.mozilla.camino       	0x007f92a0 gfxTextRun::ComputeClusterAdvance(unsigned) + 76
1   org.mozilla.camino       	0x007f93c0 gfxTextRun::ComputeLigatureData(unsigned, gfxTextRun::PropertyProvider*) + 236
2   org.mozilla.camino       	0x007f9644 gfxTextRun::ShrinkToLigatureBoundaries(unsigned*, unsigned*) + 104
3   org.mozilla.camino       	0x007f99d0 gfxTextRun::GetAdvanceWidth(unsigned, unsigned, gfxTextRun::PropertyProvider*) + 440
4   org.mozilla.camino       	0x007f3880 nsThebesFontMetrics::GetWidth(unsigned short const*, unsigned, int&, int*, nsThebesRenderingContext*) + 284
5   org.mozilla.camino       	0x0086862c nsRenderingContextImpl::GetPosition(unsigned short const*, unsigned, nsPoint) + 928
6   org.mozilla.camino       	0x008686f8 nsRenderingContextImpl::GetPosition(unsigned short const*, unsigned, nsPoint) + 1132
7   org.mozilla.camino       	0x003bfe48 nsTextFrame::MeasureText(nsPresContext*, nsHTMLReflowState const&, nsTextTransformer&, nsTextStyle&, nsTextFrame::TextReflowData&) + 1596
8   org.mozilla.camino       	0x003c0784 nsTextFrame::MeasureText(nsPresContext*, nsHTMLReflowState const&, nsTextTransformer&, nsTextStyle&, nsTextFrame::TextReflowData&) + 3960
9   org.mozilla.camino       	0x00396abc nsLineLayout::ReflowFrame(nsIFrame*, unsigned&, nsHTMLReflowMetrics*, int&) + 1096
10  org.mozilla.camino       	0x00361808 nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) + 132
11  org.mozilla.camino       	0x00363400 nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) + 480
OS X 10.4.8 PPC
Camino build, checkout time 11:30 am Japan Standard Time
Blocks: 371504
Do you get any assertions displayed on the console if you run it from the command line? (I assume this is a debug build.) Anything funny about the email message?
(In reply to comment #11)
> Do you get any assertions displayed on the console if you run it from the
> command line? (I assume this is a debug build.) 

It was an opt build unfortunately.
I'll patch my debug build tonight.

> Anything funny about the email
> message?

Nope, plain text messages typical from mailing lists.
A couple of rules in userContent.css to bump up the font-size in Gmail.

Attached patch fix? (obsolete) — Splinter Review
Updates to Mac code. Remove the gfxAtsuiFonts.h/.cpp part from the latest patch Stuart posted, replace them with this, and apply to your tree.

I found a crash viewing my own GMail. It was triggered by ATSUI's behaviour encountering DEVANAGARI VOWEL SIGN I; ATSUI was reversing the direction of the vowel sign + consonant pair, because the vowel precedes the consonant visually even though it follows the consonant in the text. I can handle this (and other swapped pairs) by forcing them to cluster.

I don't see any email with the Devanagari in it, but Google was definitely sending me the text. With this update I don't crash even when viewing messages in my highly multicultural spam folder.
Comment on attachment 256418 [details] [diff] [review]

Still need you to take a look at the Mac code, vlad...
Attachment #256418 - Flags: review?(vladimir)
(In reply to comment #13)
> Created an attachment (id=256418) [details]
> fix?

Yep, no more crashes in Gmail.
(and whoot ! the headlines of articles on now fix in the box.)
Depends on: 371380
Trying to access crashes, same stack as comment #10
I get these:
WARNING: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file /Users/phiw/cocoafox/mozilla/layout/base/nsPresContext.h, line 924
###!!! ASSERTION: Continuation at the start of a run??: 'index > 0', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp, line 584
###!!! ASSERTION: First character must be the start of a cluster and can't be a ligature continuation!: 'aCharIndex > 0 || (aGlyph.IsClusterStart() && !aGlyph.IsLigatureContinuation())', file ../../../dist/include/thebes/gfxFont.h, line 863
###!!! ASSERTION: Ligature at the start of the run??: 'ligStart > 0', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxFont.cpp, line 575
###!!! ASSERTION: Unknown character type!: 'glyphData->IsComplexCluster()', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxFont.cpp, line 553
Attached patch fixSplinter Review
This should fix that crash. I regressed the detection of the ATSUI bug ... the code to check whether the string character is a space was off by one.
Attachment #256418 - Attachment is obsolete: true
Attachment #256555 - Flags: review?(vladimir)
Attachment #256418 - Flags: review?(vladimir)
(In reply to comment #17)
> Created an attachment (id=256555) [details]
> fix

Ok, Arabic now works, but some other pages/languages on
still go boom, particularly Hindi and Tamil (I gave up after those 2 ...).
No problems with CJK pages, Cyrilic (tested the BBC), any Roman language.

###!!! ASSERTION: Lost some glyphs: 'glyph->originalOffset >= 2*index', file /Users/phiw/cocoafox/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp, line 590

Comment on attachment 256555 [details] [diff] [review]

This looks fine, as far as I was able to understand it.. go forth!
Attachment #256555 - Flags: review?(vladimir) → review+
I made additional changes to fix Mac crashes and to fix reftest failures. The Mac crashes were caused by glyph reordering like in comment #13 but with more general situations where K glyphs were moved to the left of J glyphs, for values of K or J greater than one. So I generalized the checks to work for K up to 10 and any J.

The reftest failures were caused by the fact that gfxTextRun::Draw and DrawToPath round to the nearest pixel. Because ATSUI uses subpixel glyph placement, this was causing the markup "abc<!-->def" (two Draw calls) to render differently to "abcdef". I fixed this by removing the rounding in Draw and DrawToPath and having gfxAtsuiFontGroup round each glyph advance up to the nearest appunit so that aligning frames to appunits doesn't cause problems.

I just checked this all in.
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 372636
roc, as mentioned in bug 367884, Khmer unicode rendering has not been working on trunk builds for a very long time... High hopes every time I see you applying new text rendering related patches but always the same result.

I confirm builds with this patch applied does not fix the problem either.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a3pre) Gecko/20070305 Minefield/3.0a3pre
Duplicate of this bug: 368296
Duplicate of this bug: 368068
Depends on: 372838
This regressed Tp on OSX by about 8%.  Is that expected?
I'm working on it, see bug 372631. I think I'm close to a fix.
Depends on: 372631
Depends on: 373179
Depends on: 373321
Blocks: 358732
Depends on: 375594
Blocks: 361581
Depends on: 377201
Could this have caused bug 404789?
Depends on: 404789
Depends on: 413928
Depends on: 422253
You need to log in before you can comment on or make changes to this bug.