Closed
Bug 370588
Opened 18 years ago
Closed 18 years ago
Rework gfxTextRun, part 2
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(3 files, 3 obsolete files)
242.01 KB,
patch
|
Details | Diff | Splinter Review | |
29.00 KB,
text/plain
|
Details | |
33.83 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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)
Comment 2•18 years ago
|
||
The patch works fine on Windows excepted the spacing rendering problem.
# the issue already happens on Linux after landing the new textrun.
Assignee | ||
Comment 3•18 years ago
|
||
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.
Comment 4•18 years ago
|
||
(In reply to comment #3)
> Which problem is that?
I can reproduce that the text is overflowed from nsTextFrame if the text is justified.
Assignee | ||
Comment 5•18 years ago
|
||
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)
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 255622 [details] [diff] [review]
updated patch
Vlad should look at the ATSUI code.
Attachment #255622 -
Flags: review?(vladimir)
Comment 7•18 years ago
|
||
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)
Assignee | ||
Comment 8•18 years ago
|
||
+ 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.
Comment 9•18 years ago
|
||
Running Camino with this patch applied I crashed when viewing an email message in Gmail
STR
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
Comment 10•18 years ago
|
||
OS X 10.4.8 PPC
Camino build, checkout time 11:30 am Japan Standard Time
Assignee | ||
Comment 11•18 years ago
|
||
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?
Comment 12•18 years ago
|
||
(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.
Assignee | ||
Comment 13•18 years ago
|
||
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.
Assignee | ||
Comment 14•18 years ago
|
||
Comment on attachment 256418 [details] [diff] [review]
fix?
Still need you to take a look at the Mac code, vlad...
Attachment #256418 -
Flags: review?(vladimir)
Comment 15•18 years ago
|
||
(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 ElPais.com now fix in the box.)
Comment 16•18 years ago
|
||
Trying to access http://news.bbc.co.uk/hi/arabic/news/ 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
Assignee | ||
Comment 17•18 years ago
|
||
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)
Comment 18•18 years ago
|
||
(In reply to comment #17)
> Created an attachment (id=256555) [details]
> fix
Ok, Arabic now works, but some other pages/languages on http://www.bbc.co.uk/worldservice/languages/
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.
Multiple
###!!! 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]
fix
This looks fine, as far as I was able to understand it.. go forth!
Attachment #256555 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 20•18 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 21•18 years ago
|
||
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
Comment 24•18 years ago
|
||
This regressed Tp on OSX by about 8%. Is that expected?
Assignee | ||
Comment 25•18 years ago
|
||
I'm working on it, see bug 372631. I think I'm close to a fix.
Depends on: 372631
Comment 26•17 years ago
|
||
Could this have caused bug 404789?
You need to log in
before you can comment on or make changes to this bug.
Description
•