1708937 - Some Twitter or WebRTC video shows as green or black square on Sandybridge/gen6

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Barry K. Nathan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.93 Safari/537.36 Edg/90.0.818.51

Steps to reproduce:

1. Use a computer with Intel HD Graphics 3000 running Windows (7, 8.1, and 10 all reproduce the bug).
   2a. Watch the GIF in this Tweet (the bug happens with this particular video, but many other Twitter GIFs/videos are unaffected): https://twitter.com/cryptonator1337/status/1269260767913795584
   2b. Or try to use this WebRTC test site: https://janus.conf.meetecho.com/echotest.html

Actual results:

With the Tweet, the GIF video shows up as a green rectangle. With the WebRTC test, both the left video and the right video show up as big green squares (well, technically rectangles).

(There are other WebRTC failure cases where one of the videos shows up as green and the other as black, but it's not something I can link in public.)

Expected results:

I was expecting Firefox 88 to behave like Firefox 87, where the videos are visible and play properly on this same hardware. (Since Firefox 87 works, this is a regression.)

From a user's point of view, this looks like bug 1678903 came back to life (so this is a regression). I ran mozregression and attached the log output to that bug. I think it was the commit to fix bug 1695912 that caused the regression, but it's my first time using mozregression so I would suggest checking the log output in case I misinterpreted it.

Comment 1

[Barry K. Nathan]

On my affected Windows 10 machine (an Early 2011 13" MacBook Pro, which has Intel HD Graphics 3000), I have now set up a Firefox build environment, so soon I'll start playing around on that and see if I can make any progress in figuring out what's going on. (By the way, in case I need it at any point, on one of my desktop PCs I have set up a build environment for GeckoView, and I have a few Android phones, including an LG Nexus 5, which has an Adreno 330 GPU if I'm not mistaken.)

Also, I might take my affected Windows 7 machine (another Early 2011 13" MacBook Pro), blow away Windows 7 (I can restore it from backup later if necessary), install some version of Linux, and see if this bug reproduces on that operating system.

Comment 2

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

I see switch statement changes in the patch for bug 1695912, which as I recall is what's caused issues in the past. Jamie, can you please take a look?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I can reproduce this locally.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

I confirmed that reverting the hunk that touches gfx/wr/webrender/res/brush_yuv_image.glsl fixes it.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1708937. Work around a gen6 shader compiler bug.

This was found through experimentation. Hopefully,
we'll get a better understanding of what's going on in the future.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1f578358932
Work around a gen6 shader compiler bug. r=gw

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9219961 [details]
Bug 1708937. Work around a gen6 shader compiler bug.

Beta/Release Uplift Approval Request

• User impact if declined: Green video some of the time
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This works around a shader miscompilation and could potentially expose a miscompilation with some other shader compiler but given the type of change that doesn't seem that likely
• String changes made/needed:

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/d1f578358932

Comment 9

[Barry K. Nathan]

I tried the patch and it works for me, by the way.

Comment 10

[Barry K. Nathan]

Attachment: a different type of workaround

For what it's worth, I did a bunch of experiments, and while nearly all of them failed, this one succeeded. (This patch is against mozilla-central before the other patch landed, but for the moment that's good enough to show what this workaround is, as well as a possible explanation for why it works.)

In case it's not clear from the diff, I just switched the order of the first two blocks of the if-else, so that the YUV_FORMAT_NV12 block comes before the YUV_FORMAT_PLANAR block.

Before this patch, the if-else was basically this (check the values of the YUV_FORMAT_* constants in yuv.glsl if this doesn't make sense):

if (x == 1) {
    ...
} else if (x == 0) {
    ...
} else if (x == 2) {
    ...
}

And after this patch, it becomes this:

if (x == 0) {
    ...
} else if (x == 1) {
    ...
} else if (x == 2) {
    ...
}

I'm kind of guessing that maybe x == {0, 1, 2} is a more common order for if-else statements than x == {1, 0, 2}, so perhaps it's less likely to trigger a compiler bug. In any case, this fixes the problem for me.

I'm fine with the current workaround, but since this experiment worked, I figured I should present this approach in case it might be more elegant or less risky or anything like that.

Comment 11

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9219961 [details]
Bug 1708937. Work around a gen6 shader compiler bug.

Approved for 88.0.1 and 89.0b8.

Comment 12

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-release/rev/179e1482851c

Comment 13

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/19f335e29b24