SVG icon artifacts in the identity popup on Windows 8

VERIFIED FIXED in Firefox 55

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: mchang)

Tracking

({regression, regressionwindow-wanted})

unspecified
mozilla56
All
Windows 8
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

2 years ago
Flags: needinfo?(mchang)
(Assignee)

Comment 1

2 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

2 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)
(Reporter)

Comment 3

2 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?
Some of the screenshots look like we may be using a surface with uninitialized memory.
(Assignee)

Updated

2 years ago
Assignee: nobody → mchang
Does it only happen on Windows 8 and not other versions of Windows?
Flags: needinfo?(jhofmann)
(Reporter)

Comment 6

2 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)
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

2 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

2 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

2 years ago
Can you see if the test environments are disabling the GPU process for some reason? Thanks!
Flags: needinfo?(jhofmann)
(Reporter)

Comment 12

2 years ago
Forwarding that question to Joel... Is Windows 8 not using the GPU process?
Flags: needinfo?(jhofmann) → needinfo?(jmaher)
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

2 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

2 years ago
Sorry, can we get an about:Support of the Windows 8 Machines please?
Flags: needinfo?(jmaher)
(Assignee)

Comment 17

2 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)
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.
(Assignee)

Comment 20

2 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)
(Assignee)

Comment 24

2 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)
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

2 years ago
Fallback in the parent process. Probably not worth finding the bug here.
Attachment #8881867 - Flags: review?(jmuizelaar)
Attachment #8881867 - Flags: review?(jmuizelaar) → review+
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?

Comment 30

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/008167964a21
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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

2 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 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+
(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.