1336289 - ConvertPlanarYCbCr and ConvertMacIOSurfaceImage shaders fail to compile

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Dzmitry Malyshau [:kvark]]

Original bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1330672
Also possibly related: https://bugzilla.mozilla.org/show_bug.cgi?id=891204, https://bugzilla.mozilla.org/show_bug.cgi?id=1191527

The YUV shader decoding path seems to only happening on Linux.
Both Windows and OSX machines fail to compile the shaders and fall back to software, while still attempting to compile them every frame.

Both cases spew the console log:
"texSubImage2D: Failed to hit GPU-copy fast-path. Falling back to CPU upload." 

 --- OSX case ---
Mac Mini 2010, OSX 10.12, NVidia GeForce 320M

Error (see firefox-osx.trace provided):
"0:2(12): error: extension `GL_ARB_texture_rectangle' unsupported in fragment shader"

Changing the version requirement to 120 gets it going, but produces incorrect image. Preliminary investigation showed that we are not binding the `uCbCrTexture` correctly according to YCBCR_422_APPLE extension. We expect the surface to have 2 planes, but it seems to have a single plane with 2 channels instead.

 --- Windows case ---
Windows 7, Radeon RX 480

Compositing with Direct3D 11, using Angle.
A RenderDoc capture doesn't show any failed shader loading since they are caught by Angle and not reaching D3D11.
(don't have any more info, let me know if needed)

Comment 1

[Dzmitry Malyshau [:kvark]]

Attachment: firefox-win.rdc

RenderDoc capture of non-e10s YCbCR video playback on Windows

Comment 2

[Kelsey Gilbert [:jgilbert]]

Who was working on YCbCr last, Milan?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Dzmitry Malyshau [:kvark] from comment #0)
> ...
> 
> Changing the version requirement to 120 gets it going, but produces
> incorrect image. Preliminary investigation showed that we are not binding
> the `uCbCrTexture` correctly according to YCBCR_422_APPLE extension. We
> expect the surface to have 2 planes, but it seems to have a single plane
> with 2 channels instead.

This would explain Mac, sounds like we need more investigation for Windows.


(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Who was working on YCbCr last, Milan?

I'm not convinced this is a recent regression.  Sotaro has done some recent work, but I'm not sure it's related.  Sotaro, can you take a quick look?

Dzmitry - is there a minimal case that reproduces failed shader compilation?

Jeff - can we have a patch with MOZ_ASSERT when compilation fails, at least for the local testing?

Comment 4

[Dzmitry Malyshau [:kvark]]

> This would explain Mac, sounds like we need more investigation for Windows.

Yes, I haven't set up the build environment on the Windows machine at home, so I didn't test the fix (of bumping the GLSL version).

> Dzmitry - is there a minimal case that reproduces failed shader compilation?

I haven't tried to move the shader code out and compile it independently of Firefox. The test case I used was launching FF and following the link (that decodes YCbCr video).

Comment 5

[Kelsey Gilbert [:jgilbert]]

APPLE_ycbcr_422 definitely looks like it's single-plane interleaved:
[Y0,Cb0], [Y1,Cr1], [Y2,Cb2], [Y3,Cr3]

It's also an RGB format, so we should be sampling as .rgb, not doing conversion ourselves.

I will gladly add asserts.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Uh, wow, this code is a mess.

Comment 7

[Kelsey Gilbert [:jgilbert]]

I'll take this for now.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> I'm not convinced this is a recent regression.  Sotaro has done some recent
> work, but I'm not sure it's related.  Sotaro, can you take a quick look?

I just changed a color matrix coefficient. It should not be related to this bug.

Mac IOSurface problem seems to related to Bug 1248323. It changed color format from NV12 to YUV422. When IOSurface support was added to GLBlitHelper by Bug 1191534, it uses NV12 as color format.

Comment 9

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Fixed by bug 1322746.