Mixed subpixel and grayscale AA on windows 8

RESOLVED FIXED in Firefox 16

Status

()

Core
Graphics: Text
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
All
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14-, firefox15-, firefox16+ fixed, firefox17+ fixed, firefox-esr10-)

Details

(Whiteboard: [Win8],)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
tracking-firefox-esr10: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
Hardware: x86_64 → All
(Assignee)

Comment 2

5 years ago
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

5 years ago
Am seeing this when (smooth) scrolling as well.
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).
tracking-firefox-esr10: ? → -
tracking-firefox14: ? → -
tracking-firefox15: ? → -
tracking-firefox16: ? → +
tracking-firefox17: ? → +

Comment 5

5 years ago
Related: Bug 760299
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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.

Updated

5 years ago
Assignee: nobody → bas.schouten
Whiteboard: [Win8]
(Assignee)

Comment 8

5 years ago
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

5 years ago
(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?
Not sure if this is related or a dupe, but please see also bug 793280.
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 664239 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo
Attachment #664239 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

5 years ago
Created attachment 664258 [details] [diff] [review]
Use new ID2D1DeviceContext interface in cairo v2

Updated to include missing file.
Attachment #664239 - Attachment is obsolete: true
Attachment #664239 - Flags: review?(jmuizelaar)
Attachment #664258 - Flags: review?(jmuizelaar)
(Assignee)

Comment 14

5 years ago
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.
Attachment #664258 - Attachment is obsolete: true
Attachment #664258 - Flags: review?(jmuizelaar)
Attachment #664285 - Flags: review?(jmuizelaar)
(Assignee)

Comment 15

5 years ago
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.
Status: NEW → ASSIGNED

Updated

5 years ago
Keywords: verifyme
Whiteboard: [Win8] → [Win8],

Updated

5 years ago
Keywords: qawanted
Attachment #664285 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 16

5 years ago
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
Attachment #664285 - Flags: approval-mozilla-beta?
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.
Attachment #664285 - Flags: approval-mozilla-beta?
Attachment #664285 - Flags: approval-mozilla-beta+
Attachment #664285 - Flags: approval-mozilla-aurora+
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.
Keywords: testcase-wanted
(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.
(Assignee)

Comment 20

5 years ago
(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.
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

5 years ago
Pushed to Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/63dd46624884
status-firefox16: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/9211f1307e00
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 24

5 years ago
(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?
Blocks: 793280
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

5 years ago
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?
(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

5 years ago
Did the fix get pushed to Aurora as well?
(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.
(Assignee)

Comment 30

5 years ago
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

5 years ago
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.

Updated

5 years ago
status-firefox17: --- → affected

Updated

5 years ago
Attachment #664285 - Flags: approval-mozilla-aurora+
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.
(Assignee)

Comment 33

5 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 34

5 years ago
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
Attachment #664285 - Attachment is obsolete: true
Attachment #668769 - Flags: review?(jmuizelaar)
Attachment #668769 - Flags: approval-mozilla-aurora?
Attachment #668769 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/13ec2ee2b447
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 798931
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.
Attachment #668769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 37

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/e054d8a286c6
Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/5e6bff90786c - Win7 had 10 failures in content/canvas/test/test_canvas.html
(Assignee)

Comment 39

5 years ago
I wonder if this patch for some obscure reason needs a full clobber.
(Assignee)

Comment 40

5 years ago
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.
(Assignee)

Comment 41

5 years ago
Repushed with mistake fixed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/b13f3bd4de37

Updated

5 years ago
status-firefox17: affected → fixed
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.
Keywords: qawanted, verifyme

Updated

5 years ago
Blocks: 815658

Updated

4 years ago
Depends on: 812695
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.