397454 - [10.5] at some text sizes, only the first letter of an element appears

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

When Mozilla is built using the 10.5 SDK, some elements at certain text sizes display only the first letter of the text of that element.  The rest of the text is broken across several lines with the first letter of each word showing.

Steps:

1. Run the attached testcase with a 10.5 build

Result: "This is 9pt text" displays correctly, "This is 8pt text" displays as -

T
i
8
t

H1, H2, H4 headings display correctly, H3 headings do not.

The following layout debug spew shows up in the console:

nsLineLayout: Text(0)@0x238a1580 metrics=3495360,660!
nsLineLayout: Text(0)@0x23802304 metrics=1311360,660!
nsLineLayout: Text(0)@0x23802370 metrics=2512320,660!
nsLineLayout: Text(0)@0xe56410 metrics=2948160,660!
nsLineLayout: Text(0)@0xe5549c metrics=2513280,1200!
nsLineLayout: Text(0)@0xe56608 metrics=2512320,1200!
nsLineLayout: Text(0)@0xe56674 metrics=6665280,1200!
nsLineLayout: Text(0)@0xe55b3c metrics=3716160,1320!
nsLineLayout: Text(0)@0xe5686c metrics=1311360,1320!
nsLineLayout: Text(0)@0xe568d8 metrics=983040,1320!
nsLineLayout: Text(0)@0xe56944 metrics=3714240,1320!
nsLineLayout: Text(0)@0xe569b0 metrics=2512320,1320!
nsLineLayout: Text(0)@0xe56a1c metrics=6665280,1320!

Note: this does *not* occur with a Mozilla built under 10.4 using the 10.4 SDK.

This may actually be a GFX problem, but I'm sticking it under Layout for now.

Comment 1

[John Daggett (:jtd)]

Attachment: testcase

Comment 2

[John Daggett (:jtd)]

Attachment: testcase screenshot running with a 10.5 build

Comment 3

[John Daggett (:jtd)]

Attachment: testcase screenshot running with a 10.4 build

Comment 4

[John Daggett (:jtd)]

This may be the same issue as bug 385215 but this is a far smaller, simpler
testcase to work with.

Comment 5

[John Daggett (:jtd)]

With 10.4 and 10.5 build, the text run for 'This is 9pt text' is *exactly* the same, the sequence of advance values match (values here are in pixels):

0x3e7b04f0(serif) TEXTRUN "This is 9pt text" ENDTEXTRUN
    [54 68 69 73 20 69 73 20 39 70 74 20 74 65 78 74 ]

advance: 7.333333
advance: 6.000000
advance: 3.333333
advance: 4.666667
advance: 3.000000
advance: 3.333333
advance: 4.666667
advance: 3.000000
advance: 6.000000
advance: 6.000000
advance: 3.333333
advance: 3.000000
advance: 3.333333
advance: 5.333333
advance: 6.000000
advance: 3.333333

But for the next line in the testcase, 'This is 8pt text', the advances from the 10.5 build are insane.

10.4 build:

0x3f196760(serif) TEXTRUN "This is 8pt text" ENDTEXTRUN
    [54 68 69 73 20 69 73 20 38 70 74 20 74 65 78 74 ]

advance: 6.516667
advance: 5.333333
advance: 2.966667
advance: 4.150000
advance: 2.666667
advance: 2.966667
advance: 4.150000
advance: 2.666667
advance: 5.333333
advance: 5.333333
advance: 2.966667
advance: 2.666667
advance: 2.966667
advance: 4.733333
advance: 5.333333
advance: 2.966667


10.5 build:

0x1ea6df40(serif) TEXTRUN "This is 8pt text" ENDTEXTRUN
    [54 68 69 73 20 69 73 20 38 70 74 20 74 65 78 74 ]

advance: 16383.000000
advance: 7280.000000
advance: -3648.000000
advance: 4560.000000
advance: -912.000000
advance: -3648.000000
advance: 4560.000000
advance: -8192.000000
advance: 0.000000
advance: 7280.000000
advance: 912.000000
advance: -912.000000
advance: -5440.000000
advance: -1840.000000
advance: 7280.000000
advance: 9104.000000

Will investigate this more tomorrow.

Comment 6

[John Daggett (:jtd)]

Attachment: testcase, just 8pt text

Comment 7

[John Daggett (:jtd)]

Add in debug code to dump out ATSGlyphInfoFlags info.  Under 10.5, for 8pt text a mysterious 0x00100000 bit is set and the advance info is whacked.  No idea what that bit implies, it's not in the public header file.

0x1e36d640(serif) TEXTRUN "This is 9pt text" ENDTEXTRUN
    [54 68 69 73 20 69 73 20 39 70 74 20 74 65 78 74 ]

	advance: 7.333333 (08000002)
	advance: 6.000000 (08000002)
	advance: 3.333333 (08000002)
	advance: 4.666667 (00040002)
	advance: 3.000000 (08000002)
	advance: 3.333333 (08000002)
	advance: 4.666667 (00040002)
	advance: 3.000000 (00000002)
	advance: 6.000000 (08000002)
	advance: 6.000000 (08000002)
	advance: 3.333333 (00040002)
	advance: 3.000000 (08000002)
	advance: 3.333333 (08000002)
	advance: 5.333333 (08000002)
	advance: 6.000000 (08000002)
	advance: 3.333333 (00080000)

0x1ec1fba0(serif) TEXTRUN "This is 8pt text" ENDTEXTRUN
    [54 68 69 73 20 69 73 20 38 70 74 20 74 65 78 74 ]

	advance: 16383.000000 (08100002)
	advance: 7280.000000 (08100002)
	advance: -3648.000000 (08100002)
	advance: 4560.000000 (00140002)
	advance: -912.000000 (08100002)
	advance: -3648.000000 (08100002)
	advance: 4560.000000 (00140002)
	advance: -8192.000000 (00100002)
	advance: 0.000000 (08100002)
	advance: 7280.000000 (08100002)
	advance: 912.000000 (00140002)
	advance: -912.000000 (08100002)
	advance: -5440.000000 (08100002)
	advance: -1840.000000 (08100002)
	advance: 7280.000000 (08100002)
	advance: 9104.000000 (00080000)

Comment 8

[John Daggett (:jtd)]

Cool!  In the whacked advance case we're actually setting the font size to 32768pt!!  Smokin'!

Comment 9

[John Daggett (:jtd)]

Sweet Jesus!  Somebody at Apple decided to redefine FloatToFixed!!!

In _cairo_atsui_font_create_scaled we set the font size using the following logic:

    double xscale = 1.0;
    double yscale = 1.0;

    _cairo_matrix_compute_scale_factors (&font->base.scale, 
					 &xscale, &yscale, 1);
					 
    font->size = FloatToFixed ((float)xscale);

The bug here is that we are using FloatToFixed with double's, not float's.  The definitions of this macro changed from 10.4 to 10.5:

10.4 SDK

#define FloatToFixed(a)     ((Fixed)((float)(a) * fixed1))

10.5 SDK

#define FloatToFixed(a) (_IntSaturate((a) * fixed1))

With doubles, this new version calculates wildly incorrect results.  Not quite sure what _IntSaturate does but throwing a (float) cast corrects the screwed up calculation.  Workaround is probably to use our own version of FloatToFixed instead.  Will write up a testcase for Apple tomorrow.

Comment 10

[Josh Aas]

Nice catch! We should definitely confirm with Apple that they meant to do this ASAP.

Comment 11

[John Daggett (:jtd)]

Attachment: patch v.0.1

Workaround the new defintion of FloatToFixed by redefining it the old way in the two files where it's used.  For 10.4 builds, this will have no impact.

Comment 12

[John Daggett (:jtd)]

This bug blocks a blocker, bug 385215.  Fixing this completely resolves bug 385215.

Comment 13

[John Daggett (:jtd)]

Comment on attachment 282490 [details] [diff] [review]
patch v.0.1

passing over to roc for review...

Comment 14

[John Daggett (:jtd)]

Bug logged with Apple, rdar://problem/5508190

Contents of the bug:

The definition of FloatToFixed was changed between 10.4 and 10.5 in a way that causes incorrect results.

10.4 SDK

#define FloatToFixed(a)     ((Fixed)((float)(a) * fixed1))

10.5 SDK

#define FloatToFixed(a) (_IntSaturate((a) * fixed1))

Steps:

1. Create a new Command Line Utility > Standard Tool project in PB
2. Add the following code:


#include <stdio.h>
#include <Carbon/Carbon.h>

#define FloatToFixed_10_4(a)		((Fixed)((float)(a) * fixed1))
#define FloatToFixed_10_5(a)		(_IntSaturate((a) * fixed1))
#define FloatToFixed_10_5_fixed(a)	(_IntSaturate((float)(a) * fixed1))

int main (int argc, char * const argv[]) {

	UInt32 fixedVal;
	
	printf( "Starting...\n" );
	for ( fixedVal = 0x800000; fixedVal < 0x800100; fixedVal++ ) {
		Fixed  f104, f105, f105_fixed;
		
		f104 = FloatToFixed_10_4(1.0 * FixedToFloat(fixedVal));
		f105 = FloatToFixed_10_5(1.0 * FixedToFloat(fixedVal));
		f105_fixed = FloatToFixed_10_5_fixed(1.0 * FixedToFloat(fixedVal));
		if ( f104 != f105 ) {
			printf( "Fixed: %x ==> %x, %x (%x)\n", fixedVal, f104, f105, f105_fixed );
		}
	}
	printf( "Done\n" );

    return 0;
}

3. Build and run

Result:

The program outputs to the console values for which the 10.5 version of the macro results in an incorrect result.  The columns here are original fixed 16.16 value, value after fixed to float -> mult x 1.0 -> float to fixed with 10.4 macro, with 10.5 macro, and with the corrected version of the 10.5 macro.  The 10.5 macro dropped the (float) cast and it doesn't seem to return correct results when the argument passed in is a double.  Because intermediate calculations in C are implicitly converted to doubles, this can occur very easily.

Fixed: 800003 ==> 800003, 7fffffff (800003)
Fixed: 80000b ==> 80000b, 7fffffff (80000b)
Fixed: 800013 ==> 800013, 7fffffff (800013)
Fixed: 80001b ==> 80001b, 7fffffff (80001b)
Fixed: 800023 ==> 800023, 7fffffff (800023)
Fixed: 80002b ==> 80002b, 7fffffff (80002b)
Fixed: 800033 ==> 800033, 7fffffff (800033)
Fixed: 80003b ==> 80003b, 7fffffff (80003b)

I'm not quite sure what the original intent of inserting _IntSaturate here was but I would urge those who made this change to reconsider.  FloatToFixed is only used with legacy API's, right?  It's been around in this form for at least 15 years so I don't quite understand the need to change it now.  Something to do with 64-bit support perhaps?

Comment 15

[Stuart Parmenter]

Comment on attachment 282490 [details] [diff] [review]
patch v.0.1

vlad should make sure this gets upstream in to cairo.

Comment 16

[Josh Aas]

landed on trunk

Comment 17

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O 10.5; en-US; rv:1.9b2pre) Gecko/2007120304 Minefield/3.0b2pre. I verified by running https://bugzilla.mozilla.org/attachment.cgi?id=282213, which displays properly.