Closed
Bug 1330672
Opened 8 years ago
Closed 8 years ago
WebGL Texture is flipped upside down
Categories
(Core :: Graphics: CanvasWebGL, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla54
People
(Reporter: f.elsner, Assigned: kvark)
References
()
Details
(Keywords: regression, Whiteboard: gfx-noted)
Attachments
(11 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
application/javascript
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
jgilbert
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
text/x-github-pull-request
|
Details |
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
Updated•8 years ago
|
Whiteboard: gfx-noted
Comment hidden (obsolete) |
Comment 2•8 years ago
|
||
(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)
Updated•8 years ago
|
Flags: needinfo?(alice0775)
Comment 3•8 years ago
|
||
Flags: needinfo?(alice0775)
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(milan)
Updated•8 years ago
|
Comment 11•8 years ago
|
||
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•8 years ago
|
||
Unaffected Nightly53:
MacOS w/OpenGL compositing, HWA H264 decoding.
Assignee | ||
Comment 13•8 years ago
|
||
I can repro on my Linux/Intel machine. Chromium doesn't show the video at all.
Comment 14•8 years ago
|
||
(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)
Comment 16•8 years ago
|
||
(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)
Comment 17•8 years ago
|
||
We have run across the same problem, but only with webm videos, mp4 videos are displayed correctly.
Comment 18•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
Test case for bug, needs to run on a server
Comment 21•8 years ago
|
||
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.
Updated•8 years ago
|
Assignee: jgilbert → kvark
Updated•8 years ago
|
Priority: -- → P1
Updated•8 years ago
|
Comment 23•8 years ago
|
||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
I confirm the bug is still there with the latest FF build.
Flags: needinfo?(kvark)
Comment 26•8 years ago
|
||
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.
Assignee | ||
Comment 27•8 years ago
|
||
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
Assignee | ||
Comment 28•8 years ago
|
||
Attachment #8828209 -
Flags: review?(jgilbert)
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8828210 -
Flags: review?(jgilbert)
Assignee | ||
Comment 30•8 years ago
|
||
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
Assignee | ||
Comment 31•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8828209 -
Attachment is obsolete: true
Comment 34•8 years ago
|
||
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+
Comment 35•8 years ago
|
||
(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.
Assignee | ||
Comment 36•8 years ago
|
||
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?
Assignee | ||
Comment 38•8 years ago
|
||
Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e706155dac01970e8eb881f763b0e787d2c62f
Attachment #8832218 -
Flags: review?(jgilbert)
Assignee | ||
Updated•8 years ago
|
Attachment #8828210 -
Attachment is obsolete: true
Comment 39•8 years ago
|
||
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-
Assignee | ||
Comment 40•8 years ago
|
||
Thanks Jeff!
Good point, it is indeed called. I didn't realize this is the same code path here.
Assignee | ||
Comment 41•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8832218 -
Attachment is obsolete: true
Assignee | ||
Comment 42•8 years ago
|
||
Jeff, let me know how the plan sounds to you, before I call for a checkin.
Flags: needinfo?(jgilbert)
Comment 43•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 44•8 years ago
|
||
Created a separate bug for the failing GPU decode path:
https://bugzilla.mozilla.org/show_bug.cgi?id=1336289
Comment 45•8 years ago
|
||
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
Comment 46•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 47•8 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(kvark)
Assignee | ||
Comment 48•8 years ago
|
||
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?
Comment 49•8 years ago
|
||
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)
Comment 50•8 years ago
|
||
(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 51•8 years ago
|
||
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+
Comment 52•8 years ago
|
||
bugherder uplift |
Comment 53•8 years ago
|
||
bugherder uplift |
Comment 54•8 years ago
|
||
(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+
Comment 55•8 years ago
|
||
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
Comment 56•8 years ago
|
||
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•8 years ago
|
||
Comment 58•8 years ago
|
||
Comment on attachment 8846787 [details]
[treeherder] wlach:1330762 > mozilla:master
Sorry for the noise, please ignore this.
Attachment #8846787 -
Attachment is obsolete: true
Comment 59•8 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•