Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Description • 18 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	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.

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	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.

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 18 years ago

Comment on attachment 255622 [details] [diff] [review]
updated patch

Vlad should look at the ATSUI code.

	Stuart Parmenter
	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).

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	philippe (part-time)
	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

	philippe (part-time)
	Comment 10 • 18 years ago

OS X 10.4.8 PPC
Camino build, checkout time 11:30 am Japan Standard Time

	Robert O'Callahan (:roc) (email my personal email if necessary) 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?

	philippe (part-time)
	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.

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	Robert O'Callahan (:roc) (email my personal email if necessary) 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...

	philippe (part-time)
	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.)

	philippe (part-time)
	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

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	philippe (part-time)
	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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 19 • 18 years ago

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

This looks fine, as far as I was able to understand it.. go forth!

	Robert O'Callahan (:roc) (email my personal email if necessary) 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.

	Mathieu Pellerin
	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

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 18 years ago

This regressed Tp on OSX by about 8%.  Is that expected?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 25 • 18 years ago

I'm working on it, see bug 372631. I think I'm close to a fix.

	Steve England [:stevee]
	Comment 26 • 17 years ago

Could this have caused bug 404789?