Last Comment Bug 784382 - Mixed subpixel and grayscale AA on windows 8
: Mixed subpixel and grayscale AA on windows 8
Status: RESOLVED FIXED
[Win8],
:
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: unspecified
: All Windows 8
: -- normal with 6 votes (vote)
: mozilla18
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on: 798931 812695
Blocks: 815658 793280
  Show dependency treegraph
 
Reported: 2012-08-21 09:20 PDT by Bas Schouten (:bas.schouten)
Modified: 2015-10-16 12:01 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
+
fixed
+
fixed
-


Attachments
Screenshot showing mixed rendering (7.33 KB, image/png)
2012-08-21 09:20 PDT, Bas Schouten (:bas.schouten)
no flags Details
Use new ID2D1DeviceContext interface in cairo (163.42 KB, patch)
2012-09-24 15:07 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Use new ID2D1DeviceContext interface in cairo v2 (235.37 KB, patch)
2012-09-24 16:15 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Use new ID2D1DeviceContext interface in cairo v3 (13.44 KB, patch)
2012-09-24 17:12 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Part 2: Use new ID2D1DeviceContext interface in Azure (16.45 KB, patch)
2012-10-06 05:11 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-08-21 09:20:45 PDT
Created attachment 653788 [details]
Screenshot showing mixed rendering

On windows 8 when we animate (or type in a textbox), we re-render glyphs, seemingly randomly, with grayscale and subpixel AA. This looks very bad.

See attachment. This happens on Release and Nightly.
Comment 1 Bas Schouten (:bas.schouten) 2012-08-21 09:22:29 PDT
Technically this is not really a regression from the perspective of our code, however it kind of is since the problem only starts being exposed with Windows 8.
Comment 2 Bas Schouten (:bas.schouten) 2012-08-21 12:39:05 PDT
Having done some more investigation here this appears to be a Direct2D bug. As soon as we push a layer, even when we specify D2D1_LAYER_OPTIONS_INITIALIZE_FOR_CLEARTYPE on pushing the layer, we seem to stop getting subpixel-AA on subsequent DrawGlyphRun calls. I don't have a good explanation for this and will create a stand-alone test-case that I can send to Microsoft.
Comment 3 timbugzilla 2012-08-21 18:49:22 PDT
Am seeing this when (smooth) scrolling as well.
Comment 4 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-22 09:54:53 PDT
Will track this for 16 and 17, this falls outside of the ESR criteria and it's not a chemspill-worthy issue (14) or a release blocker (15).
Comment 5 redsign 2012-08-22 11:19:51 PDT
Related: Bug 760299
Comment 6 Bas Schouten (:bas.schouten) 2012-08-22 15:03:26 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #4)
> Will track this for 16 and 17, this falls outside of the ESR criteria and
> it's not a chemspill-worthy issue (14) or a release blocker (15).

My main concern is if these D2D changes are backported to Windows 7, suddenly our entire accelerated userbase would be effected. Which seems like it would be a considerable issue.
Comment 7 Bas Schouten (:bas.schouten) 2012-08-28 09:35:58 PDT
I've reproduced this issue in a stand-alone application and I've contacted people at Microsoft. Will report back here when I know more about this.
Comment 8 Bas Schouten (:bas.schouten) 2012-09-04 08:59:10 PDT
I've received word I'll get getting an answer to my questions soon. I'll update the bug once I have them.
Comment 9 JP Rosevear [:jpr] 2012-09-20 06:03:53 PDT
(In reply to Bas Schouten (:bas) from comment #8)
> I've received word I'll get getting an answer to my questions soon. I'll
> update the bug once I have them.

I believe we got an answer Bas?
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-21 12:58:28 PDT
Not sure if this is related or a dupe, but please see also bug 793280.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-21 13:08:39 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #10)
> Not sure if this is related or a dupe, but please see also bug 793280.

Bug 760299 could also be related.
Comment 12 Bas Schouten (:bas.schouten) 2012-09-24 15:07:22 PDT
Created attachment 664239 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo
Comment 13 Bas Schouten (:bas.schouten) 2012-09-24 16:15:10 PDT
Created attachment 664258 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo v2

Updated to include missing file.
Comment 14 Bas Schouten (:bas.schouten) 2012-09-24 17:12:24 PDT
Created attachment 664285 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo v3

Due to an extremely complex dependency graph (and sadly some files actually being different with same names), I couldn't get the complete version included well. I created a hackier version which I'm testing now.
Comment 15 Bas Schouten (:bas.schouten) 2012-09-24 17:30:35 PDT
I've tested this patch locally on Windows 8 and Windows 7 and it seems to do the job. I've fired a job at try to make a try build off the beta branch.
Comment 16 Bas Schouten (:bas.schouten) 2012-09-25 08:11:30 PDT
Comment on attachment 664285 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Regressed by Windows 8
User impact if declined: Significantly disturbing text drawing
Testing completed (on m-c, etc.): QA has a test build
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: None
Comment 17 Alex Keybl [:akeybl] 2012-09-25 10:32:03 PDT
Comment on attachment 664285 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo v3

[Triage Comment]
Let's land on all branches (central, then aurora & beta if green) since we'd like to take this in Beta 5. If QA comes back with a major regression, we can back out as necessary.
Comment 18 juan becerra [:juanb] 2012-09-25 13:42:52 PDT
I haven't been able to observe the problem on the machines we have in the QA lab, yet. So it's going to be difficult to verify without an example where we can see the before and after fix.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-25 13:49:27 PDT
(In reply to Alex Keybl [:akeybl] from comment #17)
> Let's land on all branches (central, then aurora & beta if green) since we'd
> like to take this in Beta 5. If QA comes back with a major regression, we
> can back out as necessary.

It would be great if we could be advised on what sorts of things this is potential to regress so that we could build up a manual regression test to guide future beta/release testing (at the very least to be run for the remaining 16 betas and the upcoming 17 betas).

(In reply to juan becerra [:juanb] from comment #18)
> I haven't been able to observe the problem on the machines we have in the QA
> lab, yet. So it's going to be difficult to verify without an example where
> we can see the before and after fix.

After I have verified bug 793280 tomorrow on my Win8 machine I can try to see if I can come up with a testcase for *this* bug. Unfortunately, this does have to wait until tomorrow because I won't have access to my Win8 machine until then.
Comment 20 Bas Schouten (:bas.schouten) 2012-09-25 14:20:40 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> (In reply to Alex Keybl [:akeybl] from comment #17)
> > Let's land on all branches (central, then aurora & beta if green) since we'd
> > like to take this in Beta 5. If QA comes back with a major regression, we
> > can back out as necessary.
> 
> It would be great if we could be advised on what sorts of things this is
> potential to regress so that we could build up a manual regression test to
> guide future beta/release testing (at the very least to be run for the
> remaining 16 betas and the upcoming 17 betas).

Considering I don't really see what could realistically break here I don't really know what you should look for. If something breaks it will be on Windows 8, not earlier.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-25 14:35:37 PDT
Thanks Bas, I'm hoping our usual web-compat exploratory tests that we run every Beta/Release will catch regressions, if any (note that Win8 is integrated as an officially tested platform since 16b1). Otherwise, I think we'll all have to keep an eye out on Bugzilla, Input, and Support for Win8 specific feedback.

Note that I will still be attempting to verify bug 793280 tomorrow morning, regardless.
Comment 22 Scoobidiver (away) 2012-09-26 02:44:12 PDT
Pushed to Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/63dd46624884
Comment 23 Mounir Lamouri (:mounir) 2012-09-26 04:10:05 PDT
https://hg.mozilla.org/mozilla-central/rev/9211f1307e00
Comment 24 timbugzilla 2012-09-26 04:16:08 PDT
(In reply to Mounir Lamouri (:mounir) from comment #23)
> https://hg.mozilla.org/mozilla-central/rev/9211f1307e00

Isn't this only fixed for cairo, and not for azure-content?
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-26 10:04:17 PDT
Bas' tryserver build for this bug fixes my facebook issue (bug 793280). I'm not sure if that qualifies as valid testcase for this bug or not. Bas, any feedback you can give here?

Assuming this testcase is valid, I will use it to verify this fixed across branches when builds are available.
Comment 26 nasko_naskov 2012-09-26 10:30:28 PDT
The trybuild seems to have fixed all of the text issues i had on win8 RTM when i force enable the preffs to override the blocking of my 12.8 driver.Is this fix already in the nightly builds?
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-26 10:51:14 PDT
(In reply to nasko_naskov from comment #26)
> Is this fix already in the nightly builds?

Based on comment 23, I would expect this to appear fixed in tomorrow's Firefox 18.0a1 Nightly.
Comment 28 Doug Halamay 2012-09-26 11:00:37 PDT
Did the fix get pushed to Aurora as well?
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-26 11:05:44 PDT
(In reply to Doug Halamay from comment #28)
> Did the fix get pushed to Aurora as well?

It's received approval for Aurora but I don't see a commit yet. I would expect that to come today and be available in tomorrow's build.
Comment 30 Bas Schouten (:bas.schouten) 2012-09-26 12:30:00 PDT
An equivalent fix for Aurora and Nightly still needs to be prepared. We use different drawing codepaths there so this patch won't help there.
Comment 31 nasko_naskov 2012-09-27 07:44:40 PDT
This fix just landed on today's nightly,but apparently,as bas said,it will only work for beta and aurora and nightly will need "An equivalent fix".I hope a fix for aurora and nightly lands soon.
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-28 16:04:04 PDT
So is this actually fixed on Nightly then?  How can this be fixed there and not have a patch requesting approval for Aurora.  We've got about a week left before merge day and it's time to fix this now, while it's still in Aurora.
Comment 33 Bas Schouten (:bas.schouten) 2012-09-28 16:10:01 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #32)
> So is this actually fixed on Nightly then?  How can this be fixed there and
> not have a patch requesting approval for Aurora.  We've got about a week
> left before merge day and it's time to fix this now, while it's still in
> Aurora.

The story is complicated. This is -not- fixed on nightly, because this patch has no effect on nightly since we're using Azure content drawing there. There will be a very similar patch (to this one) for nightly and aurora, before the migration.

Reopening since this is not actually fixed on nightly.
Comment 34 Bas Schouten (:bas.schouten) 2012-10-06 05:11:20 PDT
Created attachment 668769 [details] [diff] [review]
Part 2: Use new ID2D1DeviceContext interface in Azure

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Azure Content
User impact if declined: Annoying subpixel AA effects, see screenshots
Testing completed (on m-c, etc.): Analogous patch for cairo tested on beta
Risk to taking this patch (and alternatives if risky): Extremely low
String or UUID changes made by this patch: None
Comment 35 Phil Ringnalda (:philor, back in August) 2012-10-06 22:18:27 PDT
https://hg.mozilla.org/mozilla-central/rev/13ec2ee2b447
Comment 36 Alex Keybl [:akeybl] 2012-10-07 13:57:57 PDT
Comment on attachment 668769 [details] [diff] [review]
Part 2: Use new ID2D1DeviceContext interface in Azure

[Triage Comment]
Approving this low risk fix for a tracked bug - please land early Monday to make the next merge.
Comment 37 Bas Schouten (:bas.schouten) 2012-10-07 15:21:01 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/e054d8a286c6
Comment 38 Phil Ringnalda (:philor, back in August) 2012-10-07 18:21:33 PDT
Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/5e6bff90786c - Win7 had 10 failures in content/canvas/test/test_canvas.html
Comment 39 Bas Schouten (:bas.schouten) 2012-10-07 23:35:19 PDT
I wonder if this patch for some obscure reason needs a full clobber.
Comment 40 Bas Schouten (:bas.schouten) 2012-10-07 23:58:44 PDT
This is a silly mistake in the patch. I wonder how the canvas tests on Inbound and Try didn't catch this. Something to investigate later.
Comment 41 Bas Schouten (:bas.schouten) 2012-10-08 00:31:46 PDT
Repushed with mistake fixed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/b13f3bd4de37
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-15 16:15:16 PDT
The testcase I used to verify this before was fixed back in comment 23. Unless a full testcase can be provided for the specific issues we are trying to fix in Aurora and Nightly, this will not be able to be verified. Please advise.

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