Closed Bug 1330672 Opened 3 years ago Closed 3 years ago

WebGL Texture is flipped upside down

Categories

(Core :: Canvas: WebGL, defect, P1)

50 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 + wontfix
firefox52 + verified
firefox53 + verified
firefox54 --- verified

People

(Reporter: f.elsner, Assigned: kvark)

References

()

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(11 files, 3 obsolete files)

611.40 KB, image/png
Details
4.74 KB, text/plain
Details
5.39 KB, text/plain
Details
4.81 KB, text/plain
Details
3.22 KB, application/javascript
Details
9.34 KB, text/plain
Details
2.06 MB, application/zip
Details
516.69 KB, image/png
Details
442.88 KB, image/png
Details
836 bytes, patch
jgilbert
: review+
Details | Diff | Splinter Review
47 bytes, text/x-github-pull-request
Details | Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.90 Safari/537.36

Steps to reproduce:

Using the PIXI Library for rendering to canvas and a Video as a WebGL Texture

https://pixijs.github.io/examples/#/basics/video.js


Actual results:

The textures are flipped vertically, even though chrome and other browsers show the textures correctly


Expected results:

The frames of the video should be shown in a similar way the pure html video would be shown
Whiteboard: gfx-noted
(In reply to Alice0775 White from comment #1)
> I can reproduce on Windows10.
> 
> W/ enabled HWA:
> Nightly53.0a1: affected
> Aurora52.0a2: affected
> Beta51.0b13: affected
> Firefox50.1.0: affected
> ESR45.6.0: unaffected
> 
> W/ disabled HWA:
> Nightly53.0a1: unaffected
> Aurora52.0a2: affected
> Beta51.0b13: affected
> Firefox50.1.0: affected
> ESR45.6.0: unaffected
> 
> Regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=4be1c47c9cbc1e827667226574cb00391e1a124c&tochange=b15f
> 05a94c716fdbed45b5f46a0fd5b8ae6443ef
> 
> Regressed by:Bug 1286348

Please leave an about:support.
It works fine for me on Windows 10.

We have fairly extensive testing for this, so this is a surprise that it would be broken.
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Flags: needinfo?(alice0775)
Attached file about:support
Flags: needinfo?(alice0775)
I could reproduce this on every system tested so far, provided about:support of three different systems.

http://krpano.com/ios/bugs/ios8-webgl-video/  does work though.
Tracking for 51 and up, regression since 50.
No apparent correlation to acceleration, ANGLE used or video decoding.  Works for me on Windows 10 with D3D11/D2D1.1/D3D9 DXVA/D3D11 ANGLE, for example, but fails for the same configuration above.
Flags: needinfo?(milan)
Unaffected Nightly53:
Compositing	Direct3D 11
Asynchronous Pan/Zoom	wheel input enabled; touch input enabled; scrollbar drag enabled
WebGL Renderer	Google Inc. -- ANGLE (Intel(R) HD Graphics P530 Direct3D11 vs_5_0 ps_5_0)
WebGL2 Renderer	Google Inc. -- ANGLE (Intel(R) HD Graphics P530 Direct3D11 vs_5_0 ps_5_0)
Hardware H264 Decoding	Yes; Failed to create D3D11 device for decoder; Using D3D9 API
Audio Backend	wasapi
Direct2D	true
DirectWrite	true (10.0.14393.351)
Unaffected Nightly53:
MacOS w/OpenGL compositing, HWA H264 decoding.
I can repro on my Linux/Intel machine. Chromium doesn't show the video at all.
(In reply to Alice0775 White from comment #1)
> 
> W/ enabled HWA:
> Nightly53.0a1: affected
> Aurora52.0a2: affected
> Beta51.0b13: affected
> Firefox50.1.0: affected
> ESR45.6.0: unaffected
> 
> W/ disabled HWA:
> Nightly53.0a1: unaffected
> Aurora52.0a2: affected
> Beta51.0b13: affected
> Firefox50.1.0: affected
> ESR45.6.0: unaffected
> 

Oops, typo.

W/ disabled HWA:
Nightly53.0a1: affected
Aurora52.0a2: affected
Beta51.0b13: affected
Firefox50.1.0: affected
ESR45.6.0: unaffected

W/ enabled HWA:
Nightly53.0a1: unaffected
Aurora52.0a2: affected
Beta51.0b13: affected
Firefox50.1.0: affected
ESR45.6.0: unaffected
Alice, Dzmitry, can you reproduce the similar problem in bug 1322169 comment 0?
Flags: needinfo?(milan)
Flags: needinfo?(kvark)
Flags: needinfo?(alice0775)
(In reply to Milan Sreckovic [:milan] from comment #15)
> Alice, Dzmitry, can you reproduce the similar problem in bug 1322169 comment
> 0?

Detailed STR please?
Flags: needinfo?(alice0775)
We have run across the same problem, but only with webm videos, mp4 videos are displayed correctly.
(In reply to jan.nerad from comment #17)
> We have run across the same problem, but only with webm videos, mp4 videos
> are displayed correctly.

Perfect, thanks for the tip.
(In reply to jan.nerad from comment #17)
> We have run across the same problem, but only with webm videos, mp4 videos
> are displayed correctly.

I cannot confirm this.

I used https://pixijs.github.io/examples/#/basics/video.js to reproduce this bug on different machines and it is using mp4.
Attached file Bug test case
Test case for bug, needs to run on a server
I have uploaded a test case.
It is based on the test from https://bugzilla.mozilla.org/show_bug.cgi?id=1127336.
Line 145, gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, true) seems to be the cause of the flip.
When commented out, the video is displayed correctly.
This pixel storage parameter is set in the PIXI library to true by default.
Duplicate of this bug: 1322169
Assignee: jgilbert → kvark
Priority: -- → P1
Attached image default alpha
I confirm the bug is still there with the latest FF build.
Flags: needinfo?(kvark)
Few comments about UNPACK_PREMULTIPLY_ALPHA_WEBGL and changing the status of this bug to: "wontfix"

1. I can confirm that hardcoding UNPACK_PREMULTIPLY_ALPHA_WEBGL to be always false in pixi.js solves the problem with flipped videos (see related bug 1322169)

2. Forcing UNPACK_PREMULTIPLY_ALPHA_WEBGL to be always false is causing problems with images that have alpha channels (no surprise here). Please see the two screenshots I have attached

3. In my opinion UNPACK_PREMULTIPLY_ALPHA_WEBGL shouldn't affect orientation of video textures anyway. I couldn't find any documentation that would justify such behavior. Also keep in mind that videos in all other browsers are unaffected by that flag - it is specific for Firefox only. So this sounds like browser bug.

4. Although probably we can add firefox specific workaround only for video textures, this code is in pixi.js which is pretty popular library and we will have to maintain custom patch until the pixi creators accept such patch (if they agree at all).

5. There are other rendering libraries that will have to apply similar firefox specific patch

In conclusion I think it will be best for all parties if this is fixed in the browser.
Dimcho,

Please worry not. The bug is only marked as "wontfix" for firefox-51, it still affects the later versions. We take it seriously, and I'm looking into a solution right now.
Status: NEW → ASSIGNED
Attachment #8828209 - Flags: review?(jgilbert)
Attachment #8828210 - Flags: review?(jgilbert)
Providing 2 patches here since I'm not completely sure if the second one is correct, but I hope it is (some QA coverage would be great here). Both fix the test cases we have and do not break anything obvious.

Try results for the patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8da895da3e2c651076c4fcdf30fa41b70c7d968b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db59ae3830df6770c57baecb9914cf76bdb6fc54
Ok, both fixes are incorrect, because they don't explain the difference in reproduction on different hardware... Basically, our tex image has 3 paths, from fastest to slowest:
  1. the GL blit. That path doesn't work correct on Intel for YCbCr.
  2. memcpy path. Works good. That's what we hit if we disable the premultiply alpha flag.
  3. pixel conversion path. Probably works correct, but we don't reach it for the use cases.
(In reply to Dzmitry Malyshau [:kvark] from comment #27)
> Dimcho,
> 
> Please worry not. The bug is only marked as "wontfix" for firefox-51, it
> still affects the later versions. We take it seriously, and I'm looking into
> a solution right now.

Understood. Sorry for the confusion.
Comment on attachment 8828209 [details] [diff] [review]
Added a force flip flag for BlitImageToFramebuffer

Just say "no" to y-flip hell.
Attachment #8828209 - Flags: review?(jgilbert) → review-
Attachment #8828209 - Attachment is obsolete: true
Comment on attachment 8828210 [details] [diff] [review]
Fix for the YCbCr blit origin

Review of attachment 8828210 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good, though I agree it's a little worrisome.

We really just need a bunch of tests to run through all the backends here. (or at least try)
Attachment #8828210 - Flags: review?(jgilbert) → review+
(In reply to Dzmitry Malyshau [:kvark] from comment #31)
> Ok, both fixes are incorrect, because they don't explain the difference in
> reproduction on different hardware... Basically, our tex image has 3 paths,
> from fastest to slowest:
>   1. the GL blit. That path doesn't work correct on Intel for YCbCr.
>   2. memcpy path. Works good. That's what we hit if we disable the
> premultiply alpha flag.
>   3. pixel conversion path. Probably works correct, but we don't reach it
> for the use cases.

It sounds like it might be a bug where YCbCr images don't have consistent origins, which, if true, would constitute the real bug here.
Could someone (who can't repro the original bug) run the testcases with my patch and see if it breaks them?
Maybe it's worth seeing if we can find a regression range for this; as in, did this ever work?
Attachment #8828210 - Attachment is obsolete: true
Comment on attachment 8832218 [details] [diff] [review]
Missing YFlip flag setup for YCbCr blit. It doesn't affect OSX (and presumably Windows), since this blitting code is not executed there, and the texture is converted to RGBA prior to uploading to GPU.

Review of attachment 8832218 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLBlitHelper.cpp
@@ +766,5 @@
>      }
>  
>      float* yuvToRgb = gfxUtils::Get3x3YuvColorMatrix(yuvData->mYUVColorSpace);
>      mGL->fUniformMatrix3fv(mYuvColorMatrixLoc, 1, 0, yuvToRgb);
> +    mGL->fUniform1f(mYFlipLoc, 0.0);

This is set in the caller[1], and I don't see why it should be changed here.

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLBlitHelper.cpp#865
Attachment #8832218 - Flags: review?(jgilbert) → review-
Thanks Jeff!
Good point, it is indeed called. I didn't realize this is the same code path here.
Comment on attachment 8828210 [details] [diff] [review]
Fix for the YCbCr blit origin

I believe this is the correct fix, albeit not a complete one.

Basically, the shaders we provide for `InitTexQuadProgram` fail to compile on Windows/OSX because of the invalid `#version 100` directive. Intel on Linux is happily digesting them, which makes it one of the few configurations to take this fast GPU decoding path that flips the images vertically.

Fixing the shaders appear to be non-trivial, since at least on OSX we are not handling YCBCR_422_APPLE surface properly. We attempt to access the plane=1 of it, but there is none, and the shader produces images that lack in blue color.

I propose to proceed with this simple fix, and then I'll work on Windows and OSX in the scope of separate bugs (perhaps, with less priority). Good thing is - we'll have a much faster video playback on these platforms now ;) Given that we are currently doing a lot of CPU work and even trying to compile those shaders every frame...
Attachment #8828210 - Attachment is obsolete: false
Attachment #8832218 - Attachment is obsolete: true
Jeff, let me know how the plan sounds to you, before I call for a checkin.
Flags: needinfo?(jgilbert)
Woah, failing to compile is something we should fatally assert about.
Let's land this and we can track the failing compiles elsewhere. Please link to the bugs from here.
Flags: needinfo?(jgilbert)
Keywords: checkin-needed
Created a separate bug for the failing GPU decode path:
https://bugzilla.mozilla.org/show_bug.cgi?id=1336289
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65289055a7b
Fix for the YCbCr blit origin. r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a65289055a7b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(kvark)
Comment on attachment 8828210 [details] [diff] [review]
Fix for the YCbCr blit origin

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: flipped YCbCr videos on Linux (some HW configurations)
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes, just follow the link https://pixijs.github.io/examples/#/basics/video.js
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Yes
[Why is the change risky/not risky?]: There might be cases where the old code behaves correctly, but I'm not aware of them.
[String changes made/needed]:
Flags: needinfo?(kvark)
Attachment #8828210 - Flags: approval-mozilla-beta?
Attachment #8828210 - Flags: approval-mozilla-aurora?
Hi Florin, could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
(In reply to Gerry Chang [:gchang] from comment #49)
> Hi Florin, could you help find someone to verify if this issue is fixed as
> expected on a latest Nightly build? Thanks!

Forwarding this to Brindusa.
Flags: needinfo?(florin.mezei) → needinfo?(brindusa.tot)
Comment on attachment 8828210 [details] [diff] [review]
Fix for the YCbCr blit origin

fix webgl video issue on linux

I confirmed the video from comment 0 is upside down on 52.0b3, and displays the right way around on the latest nightly (linux x86_64, intel skylake).  Let's take this in aurora53 and beta52.
Attachment #8828210 - Flags: approval-mozilla-beta?
Attachment #8828210 - Flags: approval-mozilla-beta+
Attachment #8828210 - Flags: approval-mozilla-aurora?
Attachment #8828210 - Flags: approval-mozilla-aurora+
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #50)
> Forwarding this to Brindusa.

Nevermind. We should get this verified on 52.0b4 and 53.0a2.
Flags: needinfo?(brindusa.tot) → qe-verify+
I tested on Ubuntu 16.04 with FF Nighlty 54.0a1(2017-02-08) and I can confirm the fix. 
Please note that I couldn't reproduce this issue on Mac and Windows.
Status: RESOLVED → VERIFIED
I managed to reproduce the issue on Firefox 53.0a1 (2017-01-11), only under Ubuntu OS.
The issue is no longer reproducible on Firefox 52.0b8, or on Firefox 53.0a2 (2017-02-21).
Tests were performed under Mac OS X 10.12.3, Windows 10.x64 and under Ubuntu 14.04x86.
Comment on attachment 8846787 [details] [review]
[treeherder] wlach:1330762 > mozilla:master

Sorry for the noise, please ignore this.
Attachment #8846787 - Attachment is obsolete: true
Depends on: 1382292
You need to log in before you can comment on or make changes to this bug.