Closed Bug 1696761 Opened 3 years ago Closed 3 years ago

Background behind Firefox is visible

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 + fixed
firefox88 + fixed

People

(Reporter: evilpie, Assigned: bradwerth)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

Screenshot
1.19 MB, image/png
Details
body_opacity_0.1.html
376 bytes, text/html
Details
about:support
31.60 KB, text/plain
Details
0001-WIP-Include-image-instance-color-alpha-in-check-for-.patch
1.32 KB, patch
Details | Diff | Splinter Review
Flags: needinfo?(bwerth)

[Tracking Requested - why for this release]:

status-firefox87: --- → affected
status-firefox88: --- → affected

Tried to minimize, this is the best I've come up with:

<!DOCTYPE html>
<html>
    <head>
        <style>
        body::before {
            content: "";
            position: fixed;
            width: 100%;
            height: 100%;
            background-image: url(data:image/gif;base64,R0lGODlhAQABAIAAAAUEBAAAACwAAAAAAQABAAACAkQBADs=);
            opacity: 0.5;
        }
        </style>
    </head>
    <body></body>
</html>

I've tested a transparent PNG, opaque PNG, transparent GIF, opaque GIF, and opaque JPEG. The two PNGs and the transparent GIF didn't exhibit this behavior.

[Tracking Requested - why for this release]:

tracking-firefox87: --- → ?
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)

Brad, do you have an update on this? We're building the last beta for 87 today.

Bug 1669840 looks like it backs out cleanly (as far as hg is concerned, anyway; I haven't pushed to try or anything yet)

Flags: needinfo?(bwerth)

(In reply to Julien Cristau [:jcristau] from comment #5)

Brad, do you have an update on this? We're building the last beta for 87 today.

You can backout patches from Bug 1669840 today and I will try to rebuild them later in this bug with this testcase handled.

Flags: needinfo?(bwerth)

I can't so far reproduce on macOS. Would you please post the contents of your "about:support" as an attachment to this bug? This will capture data on open tabs so take that into consideration.

Flags: needinfo?(evilpies)
Attached file body_opacity_0.1.html —

This is an attachment version of the testcase in comment 2, with opacity changed to 0.1.

Attached file about:support —

I can reliably reproduce this in Nightly on Linux. Even with a new clean profile, for which I attached the about:support information.

I should have clarified this in comment 0, but someone else reported this issue on Matrix.

Flags: needinfo?(evilpies)

Hi Brad, 88 goes to Beta next week. Looks like I can still backout bug 1669840 from it cleanly when it does. I'm guessing that'd be the preferable way to go?

Flags: needinfo?(bwerth)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)

Hi Brad, 88 goes to Beta next week. Looks like I can still backout bug 1669840 from it cleanly when it does. I'm guessing that'd be the preferable way to go?

For now, yes. When I finish the fix, I will include the patched-up parts that were rolled back.

Flags: needinfo?(bwerth)

Per the above comments, backed out from m-c as well for 88+. This regression will be addressed when that bug is ready to re-land.
https://hg.mozilla.org/integration/autoland/rev/f2aa3c52790966347d5206922245ea968bc428e3

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox88: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

Updates on progress. The testcase attachment 9208516 [details] fails in a useful way on macOS when the rolled-back patches from Bug 1669840 are re-applied. My working assumption is that fixing the behavior on macOS will also fix the reported issue on Linux -- easy for me to confirm fixes on Linux, but not as easy to debug and develop there. Presuming this is a valid approach, it's notable that the macOS buggy behavior (displaying full black instead of 0.1 gray) only occurs when gfx.webrender.compositor is set to true. So zeroing in on the root cause.

The problem might be a faulty display list. WR captured frames show the expected differences with the patches applied. One possible point of failure is that the RepeatingImage has the properties:

     alpha_type: PremultipliedAlpha,
     color: (
      r: 1,
      g: 1,
      b: 1,
      a: 0.1,
     ),

Which of course does not look like a premultiplied color, but it's not clear if the alpha_type declares how the color is stored or how it should be processed. My current thinking is one of two sources of failure:

  1. This color should be stored in the display list in premultiplied form as (0.1, 0.1, 0.1, 0.1). I can test this as soon as I can get wrench to play back the scene, which is panicking for me at the moment.
  2. This color is fine and will be processed with premultiplication, but the image decode is not doing that properly. I'm trying to debug how the color is applied during decode.

(In reply to Brad Werth [:bradwerth] from comment #15)

  1. This color should be stored in the display list in premultiplied form as (0.1, 0.1, 0.1, 0.1). I can test this as soon as I can get wrench to play back the scene, which is panicking for me at the moment.

This incorrect color is coming from my patch https://phabricator.services.mozilla.com/D103130 where I calculated the colors improperly. Fixing it is not sufficient to fix the bug. There is still something going on within the compositor and/or decode.

(In reply to Brad Werth [:bradwerth] from comment #15)

The problem might be a faulty display list. WR captured frames show the expected differences with the patches applied. One possible point of failure is that the RepeatingImage has the properties:

     alpha_type: PremultipliedAlpha,
     color: (
      r: 1,
      g: 1,
      b: 1,
      a: 0.1,
     ),

Glenn pointed out that these two properties are independent. The alpha_type property is meant to indicate that the pixels of the image itself have premultiplied alpha. That is not the case here and it shouldn't be set. It also isn't enough to fix the problem, but updated patches will correct this. The rgba values are correct since they shouldn't be premultiplied in the display list.

Depends on D103130

It looks like the background image is being incorrectly detected as an opaque backdrop candidate, as it was not checking the per-instance image color alpha (which I think was unused prior to this patch).

The flow on effect of this is that the compositor code thinks the tile is opaque, and disables alpha blending during drawing this tile, which can cause undefined outputs.

The attached patch adds a check for the image instance color. This fixes the test case for me on Linux, though I haven't verified on macos.

Brad, would you be able to test on mac and see if this resolves the issue there too?

Flags: needinfo?(bwerth)

(In reply to Glenn Watson [:gw] from comment #21)

Created attachment 9221238 [details] [diff] [review]
0001-WIP-Include-image-instance-color-alpha-in-check-for-.patch

Brad, would you be able to test on mac and see if this resolves the issue there too?

Yes, this patch fixes the problem on macOS, noted in comment 15.

Flags: needinfo?(bwerth)

Comment on attachment 9212962 [details]
WIP: Bug 1696761 Part 1 - Allow applying opacity to nsDisplayBackgroundImage items

Revision D92941 was moved to bug 1669840. Setting attachment 9212962 [details] to obsolete.

Attachment #9212962 - Attachment is obsolete: true

Comment on attachment 9212963 [details]
WIP: Bug 1696761 Part 2: Make nsImageRenderer::BuildWebRenderDisplayItems premultiply alpha with provided opacity.

Revision D103130 was moved to bug 1669840. Setting attachment 9212963 [details] to obsolete.

Attachment #9212963 - Attachment is obsolete: true

Comment on attachment 9212964 [details]
WIP: Bug 1696761 Part 3: Update test expectations.

Revision D103938 was moved to bug 1669840. Setting attachment 9212964 [details] to obsolete.

Attachment #9212964 - Attachment is obsolete: true
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: