1330672 - WebGL Texture is flipped upside down

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[f.elsner]

Attachment: Screenshot from 2017-01-12 16-53-32.png

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

Comment 1

[Alice0775 White]

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=b15f05a94c716fdbed45b5f46a0fd5b8ae6443ef

Regressed by:Bug 1286348

Comment 2

[Kelsey Gilbert [:jgilbert]]

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

Comment 3

[Alice0775 White]

Attachment: about:support

Comment 4

[vierzigundzwei]

Attachment: about:support - Desktop, Windows 10, NVIDIA GTX 650

Comment 5

[vierzigundzwei]

Attachment: about:support - Laptop, Windows 10, NVIDIA Quadro K1100M

Comment 6

[vierzigundzwei]

Attachment: about:support - Laptop, Fedora 25, Intel integrated GPU (of an i7-7500U)

Comment 7

[vierzigundzwei]

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.

Comment 8

[f.elsner]

Attachment: about: support Desktop Fedora 24, GeForce GTX 650

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 51 and up, regression since 50.

Comment 10

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

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.

Comment 11

[Kelsey Gilbert [:jgilbert]]

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)

Comment 12

[Kelsey Gilbert [:jgilbert]]

Unaffected Nightly53:
MacOS w/OpenGL compositing, HWA H264 decoding.

Comment 13

[Dzmitry Malyshau [:kvark]]

I can repro on my Linux/Intel machine. Chromium doesn't show the video at all.

Comment 14

[Alice0775 White]

(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

Comment 15

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

Alice, Dzmitry, can you reproduce the similar problem in bug 1322169 comment 0?

Comment 16

[Alice0775 White]

(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?

Comment 17

[jan.nerad]

We have run across the same problem, but only with webm videos, mp4 videos are displayed correctly.

Comment 18

[Kelsey Gilbert [:jgilbert]]

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

Comment 19

[vierzigundzwei]

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

Comment 20

[jan.nerad]

Attachment: Bug test case

Test case for bug, needs to run on a server

Comment 21

[jan.nerad]

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.

Comment 23

[Dimcho Balev]

Attachment: default alpha

Comment 24

[Dimcho Balev]

Attachment: UNPACK_PREMULTIPLY_ALPHA_WEBGL=false

Comment 25

[Dzmitry Malyshau [:kvark]]

I confirm the bug is still there with the latest FF build.

Comment 26

[Dimcho Balev]

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.

Comment 27

[Dzmitry Malyshau [:kvark]]

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.

Comment 28

[Dzmitry Malyshau [:kvark]]

Attachment: Added a force flip flag for BlitImageToFramebuffer

Comment 29

[Dzmitry Malyshau [:kvark]]

Attachment: Fix for the YCbCr blit origin

Comment 30

[Dzmitry Malyshau [:kvark]]

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

Comment 31

[Dzmitry Malyshau [:kvark]]

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.

Comment 32

[Dimcho Balev]

(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 33

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8828209 [details] [diff] [review]
Added a force flip flag for BlitImageToFramebuffer

Just say "no" to y-flip hell.

Comment 34

[Kelsey Gilbert [:jgilbert]]

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)

Comment 35

[Kelsey Gilbert [:jgilbert]]

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

Comment 36

[Dzmitry Malyshau [:kvark]]

Could someone (who can't repro the original bug) run the testcases with my patch and see if it breaks them?

Comment 37

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

Maybe it's worth seeing if we can find a regression range for this; as in, did this ever work?

Comment 38

[Dzmitry Malyshau [:kvark]]

Attachment: 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.

Try results: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e706155dac01970e8eb881f763b0e787d2c62f

Comment 39

[Kelsey Gilbert [:jgilbert]]

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

Comment 40

[Dzmitry Malyshau [:kvark]]

Thanks Jeff!
Good point, it is indeed called. I didn't realize this is the same code path here.

Comment 41

[Dzmitry Malyshau [:kvark]]

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...

Comment 42

[Dzmitry Malyshau [:kvark]]

Jeff, let me know how the plan sounds to you, before I call for a checkin.

Comment 43

[Kelsey Gilbert [: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.

Comment 44

[Dzmitry Malyshau [:kvark]]

Created a separate bug for the failing GPU decode path:
https://bugzilla.mozilla.org/show_bug.cgi?id=1336289

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65289055a7b
Fix for the YCbCr blit origin. r=jgilbert

Comment 46

[Carsten Book [:Tomcat]]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 48

[Dzmitry Malyshau [: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]:

Comment 49

[Gerry Chang [:gchang]]

Hi Florin, could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 50

[Andrei Vaida [:avaida]]

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

Comment 51

[Julien Cristau [:jcristau]]

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.

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65e5f8c2d816

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f367195ced0f

Comment 54

[Andrei Vaida [:avaida]]

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

Comment 55

[ovidiu boca[:Ovidiu]]

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.

Comment 56

[Mihai Boldan, Desktop QA [:mboldan]]

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 57

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1330762 > mozilla:master

Comment 58

[William Lachance (:wlach)]

Comment on attachment 8846787 [details] [review]
[treeherder] wlach:1330762 > mozilla:master

Sorry for the noise, please ignore this.

Comment 59

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1330762 > mozilla:master