Last Comment Bug 722206 - Change layout/reftests/bidi/718236-1.html to avoid subpixel failures on win7 unaccelerated (and 718236-2.html, 718236-3.html)
: Change layout/reftests/bidi/718236-1.html to avoid subpixel failures on win7 ...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt] (catching up after vacation)
:
Mentors:
Depends on:
Blocks: 722617
  Show dependency treegraph
 
Reported: 2012-01-29 17:41 PST by Jonathan Watt [:jwatt] (catching up after vacation)
Modified: 2012-03-29 12:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (1.31 KB, patch)
2012-01-29 17:41 PST, Jonathan Watt [:jwatt] (catching up after vacation)
smontagu: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
patch with style="letter-spacing: 3px;" (2.69 KB, patch)
2012-01-30 03:29 PST, Jonathan Watt [:jwatt] (catching up after vacation)
no flags Details | Diff | Splinter Review
reftest log of failure with letter-spacing (47.13 KB, text/plain)
2012-01-30 07:12 PST, Jonathan Watt [:jwatt] (catching up after vacation)
no flags Details
patch - change problem character (2.76 KB, patch)
2012-01-31 06:55 PST, Jonathan Watt [:jwatt] (catching up after vacation)
no flags Details | Diff | Splinter Review
failure lines from reftest log with character changed (47.03 KB, text/plain)
2012-01-31 10:53 PST, Jonathan Watt [:jwatt] (catching up after vacation)
no flags Details

Description Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-29 17:41:37 PST
Created attachment 592561 [details] [diff] [review]
patch

I've been having some Try problems with the layout/reftests/bidi/718236-x.html reftests running on the Windows 7 machine without hardware acceleration. Basically imperceptible subpixel differences. Simon suggested changing the references for the tests in the way I've changed them in the patch, and indeed that fixes the problem.
Comment 1 Simon Montagu :smontagu 2012-01-29 20:52:37 PST
Comment on attachment 592561 [details] [diff] [review]
patch

Review of attachment 592561 [details] [diff] [review]:
-----------------------------------------------------------------

For the record, I suggested this fix to Jonathan based on bug 696672 comment 3: "in the reference we render all of the text in one call which causes the blending of the overlap between the ':' and the 'M' to be different than if we render the strings in two steps".

Looking at that again, I wonder if increasing the letter-spacing would be a more elegant fix for this and also bug 69667[123], but that can be a follow-up bug so as not to hold up bug 416581
Comment 2 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-30 03:29:18 PST
Created attachment 592653 [details] [diff] [review]
patch with style="letter-spacing: 3px;"

This hasn't passed Try yet, but assuming it does, does this look okay?

https://tbpl.mozilla.org/?tree=Try&rev=875a31fd0056
Comment 3 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-30 03:31:33 PST
(In reply to Simon Montagu from comment #1)
> but that can be a follow-up bug so as not to hold up bug 416581

Certainly appreciated, but dbaron is really busy right now, so I think there's likely plenty of time before he gets to reviewing that patch. :)
Comment 4 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-30 07:12:49 PST
Created attachment 592704 [details]
reftest log of failure with letter-spacing

Hmm, letter-spacing did not fix the problem in this case. It seems that it's just something to do with the painting of the top left of the Hebrew glyph "٣". Maybe we should just change that letter to something that doesn't cause problems?
Comment 5 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-30 07:15:17 PST
For what it's worth, I'm noticing that these three tests are failing for people quite often on Try on that Windows 7 box when run without hardware acceleration. Strange it doesn't seem to be happening on m-c tbpl.
Comment 6 Simon Montagu :smontagu 2012-01-30 08:25:33 PST
(In reply to Jonathan Watt [:jwatt] from comment #4)
> Hmm, letter-spacing did not fix the problem in this case. It seems that it's
> just something to do with the painting of the top left of the Hebrew glyph
> "٣". Maybe we should just change that letter to something that doesn't cause
> problems?

For the purposes of what the test is testing, there just needs to be two different characters in the range ٠ to ٩ (or anything with the bidi property AN, but those are the most familiar). JFTR, these are Arabic digits, not Hebrew.
Comment 7 Bas Schouten (:bas.schouten) 2012-01-30 21:42:12 PST
Could the new fuzzy-if help here?
Comment 8 Simon Montagu :smontagu 2012-01-31 00:29:10 PST
If I'm understanding the documentation of fuzzy-if correctly (and I'm not sure that I am), it won't help. In the results in comment 4, there are two pixels with different values, one with 0xffffff/0xffffb6 and one with 0xffffff/0xffffdb
Comment 9 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-31 06:55:02 PST
Created attachment 593081 [details] [diff] [review]
patch - change problem character

I pushed another changeset to Try, this time changing the character that's causing the problem to something else:

https://tbpl.mozilla.org/?tree=Try&rev=32dd21a768a4

(The changeset isn't showing up there because it was actually from the previous push which I cancelled.)

Requesting review, assuming that passes Try.
Comment 10 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-31 10:53:27 PST
Created attachment 593161 [details]
failure lines from reftest log with character changed
Comment 11 Jonathan Watt [:jwatt] (catching up after vacation) 2012-01-31 11:01:35 PST
I pushed the original (if less than ideal) patch to m-i:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7cef99596d92
Comment 12 Ed Morley [:emorley] 2012-02-01 11:24:12 PST
https://hg.mozilla.org/mozilla-central/rev/7cef99596d92
Comment 13 Phil Ringnalda (:philor, back in August) 2012-02-12 20:15:00 PST
Comment on attachment 592561 [details] [diff] [review]
patch

[Approval Request Comment]
Test-only change, needed on aurora and beta so that we can start running Windows unaccelerated reftests somewhere other than just on tryserver, like we started to do in November 2010.
Comment 14 Phil Ringnalda (:philor, back in August) 2012-02-12 21:38:23 PST
Comment on attachment 592561 [details] [diff] [review]
patch

Wups, only needed on aurora, since those tests aren't on beta.
Comment 15 Alex Keybl [:akeybl] 2012-02-14 11:27:28 PST
Comment on attachment 592561 [details] [diff] [review]
patch

[Triage Comment]
Changes to reftests and low risk - approved for Aurora 12.
Comment 16 Phil Ringnalda (:philor, back in August) 2012-02-20 15:30:34 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/77b10632390c

Note You need to log in before you can comment on or make changes to this bug.