Closed
Bug 1372577
Opened 7 years ago
Closed 7 years ago
SVG icon artifacts in the identity popup on Windows 8
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | fixed |
firefox56 | --- | fixed |
People
(Reporter: johannh, Assigned: mchang)
References
Details
(Keywords: regression, regressionwindow-wanted)
Attachments
(3 files, 1 obsolete file)
11.99 KB,
patch
|
Details | Diff | Splinter Review | |
1.12 KB,
patch
|
Details | Diff | Splinter Review | |
855 bytes,
patch
|
mchang
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We're consistently seeing strange SVG icon artifacts in the identity popup on Windows 8 through mozscreenshots (our visual regression tool):
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/80d36c5e839ccf3f73c79f910ec37c4753ceca069655d162412cf1a7c719f0da4a49b3e23c159614e4dea2452a3f96a6a1d0ceda7c35285eab64aa60fb10b683
All screenshots on https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=f223e1fd2044a026c740434df95f37a7f7accf48&newProject=mozilla-central&newRev=6491fb29e7fcc9e02cc179ae2856f426a3552385&filter=%5Ewindows8.*controlcenter
The pushlog for the regression is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f223e1fd2044a026c740434df95f37a7f7accf48&tochange=6491fb29e7fcc9e02cc179ae2856f426a3552385
These are not intermittent, they consistently occur on Windows 8 which leads me to believe they also occur outside of our test environment.
jwatt pointed out in bug 1361593 comment 14 that bug 1359527 is likely responsible for those.
The affected icons are in this folder: https://searchfox.org/mozilla-central/source/browser/themes/shared/controlcenter/
Note that we're planning on refactoring the SVG markup of those icons to not use :not(:target) rules and try to avoid masking.
mchang, can you take a look please? Let me know if you need more information.
Flags: needinfo?(mchang)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mchang)
Assignee | ||
Comment 1•7 years ago
|
||
I had a bad mismerge which was fixed. Is this still happening after https://bugzilla.mozilla.org/show_bug.cgi?id=1359527#c54 ?
Flags: needinfo?(jhofmann)
Reporter | ||
Comment 2•7 years ago
|
||
Yup, it's still happening as of today:
https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=2a3a253806d129c0bb6f2b76bf75630457a24492&newProject=mozilla-central&newRev=5293e5f8935831e2e0d91ac9b69c1b473b3c0177&filter=%5Ewindows8.*controlcenter
(Uncheck "similar")
I tried pushing a local backout to try but I get build errors (and --setenv still doesn't work properly, see bug 1371315)
Ideally someone could confirm this locally on Windows 8 and get us a regression window.
Flags: needinfo?(jhofmann)
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 3•7 years ago
|
||
I guess another question is: Does this affect only these icons in the identity popup or could it affect content SVGs (or other chrome SVGs that we don't test) as well?
Comment 4•7 years ago
|
||
Some of the screenshots look like we may be using a surface with uninitialized memory.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mchang
Comment 5•7 years ago
|
||
Does it only happen on Windows 8 and not other versions of Windows?
Flags: needinfo?(jhofmann)
Reporter | ||
Comment 6•7 years ago
|
||
The screenshots are looking fine on Windows 7 and I wasn't able to reproduce on my Windows 10 VM (we're not running mozscreenshots on Windows 10 yet, bug 1332945). It seems like it's only on Windows 8.
Flags: needinfo?(jhofmann)
Reporter | ||
Comment 7•7 years ago
|
||
In fact, Windows 10 screenshots seem to be working now and I can confirm it's not happening there:
https://public-artifacts.taskcluster.net/LeijsLN3TtWvvozC4zUGtA/0/public/test_info/20170502031133-controlCenter_52_lightLWT_mixedSubView.png
https://public-artifacts.taskcluster.net/LeijsLN3TtWvvozC4zUGtA/0/public/test_info/20170502031133-controlCenter_47_lightLWT_https.png
Comment 8•7 years ago
|
||
Mozilla/5.0 (Windows NT 6.2; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170615030208
I can't reproduce this issue on Windows 8 machine using the latest Nightly 56.0a1 - I haven't seen any SVG icon artifacts in the identity popup.
Assignee | ||
Comment 9•7 years ago
|
||
So generally, you won't see this on nightly on Windows because we have the GPU process enabled, and we'll fallback to the old path. If you disable the GPU process (about:config, pref "layers.gpu-process.enabled" to false), then go to a bunch of secured and non-secured sites, it'll eventually pop up. I've been able to reproduce it and keep seeing that the effect to luminance output is sometimes for some pixels outputting inaccurate pixel data. This effect only produces this inside Gecko. I have a test case outside Gecko that doesn't do this, so I'm still digging.
Assignee | ||
Comment 10•7 years ago
|
||
Just in case someone wants to try to figure this out.
Apparently, only in the parent process, doing a readback after drawing the luminance image produces random alpha values. e.g., if I set the input image to be transparent black, the resulting luminance output will be 0 for some pixels, 2, for others, 19 for some, etc. This only happens in the parent process. I tried on 3 machines (2 running windows 10, 1 running windows 8). All 3 exhibit this behavior with the SVG control panel, and all 3 give different random values. When rendering an SVG in the content process, I get the expected correct values.
I tried a bunch of things to see what was wrong:
1) Stop using Multi threaded optimizations when creating the device context
2) Paint to an RGBA surface instead of an alpha only surface and readback the alpha values there
3) Don't create a new device context in SourceSurfaceD2D1 when ensuring a realized bitmap.
4) Try to reproduce the error outside of gecko
I was unsuccessful in all of these.
Users generally shouldn't see this issue since the GPU process is enabled by default, so they will fallback to the CPU path. I suspect the test environment is disabling the GPU process for some reason? I verified on Windows 8 that this issue does not reproduce unless I disable the GPU process.
My suspicion is that since we have the GPU process, we probably have some regression that introduced a race condition somewhere with the compositor and drawtarget w/o the GPU process.
Assignee | ||
Comment 11•7 years ago
|
||
Can you see if the test environments are disabling the GPU process for some reason? Thanks!
Flags: needinfo?(jhofmann)
Reporter | ||
Comment 12•7 years ago
|
||
Forwarding that question to Joel... Is Windows 8 not using the GPU process?
Flags: needinfo?(jhofmann) → needinfo?(jmaher)
Comment 13•7 years ago
|
||
in automation we do not edit layers.gpu-process.enabled, so the default value is what we use. For windows 8 automation that we see in treeherder, this is on hardware machines (not vms like windows 7/windows 10). If there are drivers that blacklist this, or other factors that force the gpu process off, then I could see that being an issue.
For win7/win10 we have 2 types of vms available: low end, and gpu (dedicated gpu for the VM). we typically run gpu vms for reftest, webgl, and a few other tests.
Hope this helps.
Flags: needinfo?(jmaher)
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #13)
> in automation we do not edit layers.gpu-process.enabled, so the default
> value is what we use. For windows 8 automation that we see in treeherder,
> this is on hardware machines (not vms like windows 7/windows 10). If there
> are drivers that blacklist this, or other factors that force the gpu process
> off, then I could see that being an issue.
>
> For win7/win10 we have 2 types of vms available: low end, and gpu (dedicated
> gpu for the VM). we typically run gpu vms for reftest, webgl, and a few
> other tests.
>
> Hope this helps.
My understanding is that the D2D should be disabled in the parent process without the GPU process in all configurations now. Is there anyway we can get an about:support of the Windows 8 machines?
@David - do we support d2d in the parent process without the GPU process anymore?
Flags: needinfo?(dvander)
Assignee | ||
Comment 15•7 years ago
|
||
Sorry, can we get an about:Support of the Windows 8 Machines please?
Flags: needinfo?(jmaher)
Comment 16•7 years ago
|
||
oh, I have this already:
https://bug1371211.bmoattachments.org/attachment.cgi?id=8875858
Flags: needinfo?(jmaher)
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #16)
> oh, I have this already:
> https://bug1371211.bmoattachments.org/attachment.cgi?id=8875858
Hmm this is on Firefox 55 it seems? Is there an updated one?
Flags: needinfo?(jmaher)
Comment 18•7 years ago
|
||
that was 12 days ago on a loaner, I can get another loaner- I wouldn't think <2 weeks would make a difference- let me know.
Flags: needinfo?(jmaher)
(In reply to Mason Chang [:mchang] from comment #14)
> (In reply to Joel Maher ( :jmaher) from comment #13)
>
> My understanding is that the D2D should be disabled in the parent process
> without the GPU process in all configurations now. Is there anyway we can
> get an about:support of the Windows 8 machines?
>
> @David - do we support d2d in the parent process without the GPU process
> anymore?
Answered in IRC but for posterity: basically no, due to how the GPU process is configured. We should only be getting d2d in the UI when e10s is not active, or the gpu process has been pref'd off by the user.
Flags: needinfo?(dvander)
Assignee | ||
Comment 20•7 years ago
|
||
Can you try this patch on your test servers? The about:config posted by Joel makes no sense with the output you're seeing and the local testing I've done. I just want to verify this.
Thanks
Flags: needinfo?(jhofmann)
Reporter | ||
Comment 21•7 years ago
|
||
Assignee | ||
Comment 22•7 years ago
|
||
This is tier 3 - I think this is the proper link to see it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c2a8146e97cf283bef1a6961aa331d2398fb645&exclusion_profile=Tier-3&visibility=excluded
Reporter | ||
Comment 23•7 years ago
|
||
Yup, so your assertion is being hit on Windows 8 for some reason:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c2a8146e97cf283bef1a6961aa331d2398fb645&exclusion_profile=Tier-3&visibility=excluded&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=109279189
Happy debugging :)
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 24•7 years ago
|
||
Joel, is there something special about Windows 8 x64 for for machine - t-w864-ix-107 or the specific suite we have here? Is there a way to get an about:support for this machine? Thanks!
Flags: needinfo?(jmaher)
Comment 25•7 years ago
|
||
the only way to get about support is to get a loaner. Win8 should be standard config for all machines and is the same hardware as win10 and win7. There is a bios change from win8->win10 though.
Do you want me to get about:support for t-w864-ix-107 or any other specific machine?
Flags: needinfo?(jmaher)
Assignee | ||
Comment 26•7 years ago
|
||
Fallback in the parent process. Probably not worth finding the bug here.
Attachment #8881867 -
Flags: review?(jmuizelaar)
Updated•7 years ago
|
Attachment #8881867 -
Flags: review?(jmuizelaar) → review+
Comment 27•7 years ago
|
||
Comment on attachment 8881867 [details] [diff] [review]
Stop using D2D alpha effect in the parent process
Review of attachment 8881867 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetD2D1.cpp
@@ +121,5 @@
> {
> + // See bug 1372577, some race condition where we get invalid
> + // results with D2D in the parent process. Fallback in that case.
> + if ((aLuminanceType != LuminanceType::LUMINANCE) ||
> + XRE_IsParentProcess()) {
The comment looks like it applies to the entire |if| condition. Perhaps insert the comment into the condition immediately before the parent check line?
Assignee | ||
Comment 28•7 years ago
|
||
Attachment #8881867 -
Attachment is obsolete: true
Attachment #8881953 -
Flags: review+
Assignee | ||
Comment 29•7 years ago
|
||
Comment 30•7 years ago
|
||
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/008167964a21
Fallback to CPU alpha to luminance on the parent process. r=jrmuizel
Comment 31•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter | ||
Comment 32•7 years ago
|
||
I can confirm this worked, thank you!
https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a578ce873d805743283df9a3627372939d8c8b2c&newProject=mozilla-central&newRev=d536973fe668c6c6046fc3fda82e24f3379e3713&filter=%5Ewindows8.*controlcenter
Status: RESOLVED → VERIFIED
Comment 33•7 years ago
|
||
Hi :mchang,
Since this is a new regression in 55, do you think it's worth uplifting to Beta55?
Flags: needinfo?(mchang)
Assignee | ||
Comment 34•7 years ago
|
||
Comment on attachment 8881953 [details] [diff] [review]
Stop using D2D alpha effect in the parent process
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1359527
[User impact if declined]: Some graphics artifacts in chrome UI
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: bug 1375452
[Is the change risky?]: No
[Why is the change risky/not risky?]: This falls back to using the CPU to render SVG luminance on the parent process, which was the behavior before bug 1359527
[String changes made/needed]: None
Flags: needinfo?(mchang)
Attachment #8881953 -
Flags: approval-mozilla-beta?
Comment 35•7 years ago
|
||
Comment on attachment 8881953 [details] [diff] [review]
Stop using D2D alpha effect in the parent process
fix a graphics issue with svg on windows, beta55+
Attachment #8881953 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 36•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 37•7 years ago
|
||
(In reply to Mason Chang [:mchang] from comment #34)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No
Setting qe-verify- based on Mason's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•