Closed Bug 1382292 Opened 7 years ago Closed 7 years ago

canvas drawImage from usermedia upside down

Categories

(Core :: Graphics: CanvasWebGL, defect)

52 Branch
All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
fennec + ---
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 + fixed
firefox57 --- verified

People

(Reporter: ervinserfozo, Assigned: rbarker)

References

Details

Attachments

(2 files)

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

Steps to reproduce:

I have the same issue in my project like the one reported in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1225871 
So to be sure that the problem is not in my code, I tried those steps from above mentioned bug:
1. GoTo: https://rawgit.com/AlexNigl/4340d560ac6a61dca9d8/raw/c3866daac2f3389109686fcc804ee24abc6fe807/index.html
2. allow camera permissions
3. click on video to make a snapshot



Actual results:

The snapshot of the video appears upside-down.


Expected results:

The snapshot of the video should appear as it is. So, not upside-down.
[Tracking Requested - why for this release]: regression of a important web feature

11:38.40 INFO: No more inbound revisions, bisection finished.
11:38.40 INFO: Last good revision: 28bb04d0338d2741bbc951566f156744a50060bc
11:38.40 INFO: First bad revision: bf15d4078c2a6db7df37ab466d28a1e075c9eb4d
11:38.40 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=28bb04d0338d2741bbc951566f156744a50060bc&tochange=bf15d4078c2a6db7df37ab466d28a1e075c9eb4d
Blocks: 1330672
tracking-fennec: --- → ?
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Audio/Video → Canvas: WebGL
OS: Unspecified → Android
Product: Firefox for Android → Core
Hardware: Unspecified → All
Version: 54 Branch → 52 Branch
:kvark, can you comment to the bug? It might be a regression of bug 1330672.
Flags: needinfo?(kvark)
Interesting, I don't see it on 56.0a1 (2017-07-17), Fedora 26.
Same for 56.0a1 (2017-07-20) - the repro steps produce the expected result for me.
Hence, I don't consider it a regression, given that I was able to reproduce the old bug on this very configuration.
Flags: needinfo?(kvark)
Look at the platform, screen shot and the product where this bug was first reported to. This is a Firefox for Android bug. This reproduces on android on every device.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kvark)
Ah, apologies, I was replying to the question if it's a regression from 1330672, which was on Linux.
Randall can you take a look?
Assignee: nobody → rbarker
Flags: needinfo?(rbarker)
I looked at the code and couldn't figure out what's wrong. Going deeper would need me to set up the repro case here.
Basically, there are multiple points through the pipeline where an image can be flipped, and one needs to see what code paths are active on Android, possibly re-evaluating the fix from 1330672.
Flags: needinfo?(kvark)
Track 56+ as regression.
Flags: needinfo?(rbarker)
tracking-fennec: ? → +
Comment on attachment 8893583 [details]
Bug 1382292 - Revert PLANAR_YCBCR case in GLBlitHelper.cpp on Android so that when a canvas uses a video frame as source, it is not flipped

This looks like a hack around the symptoms, but not a solution for the cause. It tries to negate the effect of kvark's patch which caused the regrssion, but only on Android, without answering the question of why the PlanarYCbCrImage is coming in flipped on Android but not other platforms.

If at the cause was understood on Android, and it was a necessary to do a hack like this, at minimum, I would like a comment detailing why Android behaves this way and why we are forced to do this in the first place, rather than having a strange #ifdef in the code for mysterious reasons.

But for now, I will hand this off to kvark to make the final decision on what to do with this.
Attachment #8893583 - Flags: review?(lsalzman) → review?(kvark)
Comment on attachment 8893583 [details]
Bug 1382292 - Revert PLANAR_YCBCR case in GLBlitHelper.cpp on Android so that when a canvas uses a video frame as source, it is not flipped

https://reviewboard.mozilla.org/r/164656/#review171460

hmm, my fix was in GLBlitHelper.cpp, and only for the PLANAR_YCBCR surfaces: https://bug1330672.bmoattachments.org/attachment.cgi?id=8828210
So I don't see this change as a partial revert, and it can break more stuff on Android. Let's make it a real partial revert instead?
Attachment #8893583 - Flags: review?(kvark) → review-
Comment on attachment 8893583 [details]
Bug 1382292 - Revert PLANAR_YCBCR case in GLBlitHelper.cpp on Android so that when a canvas uses a video frame as source, it is not flipped

https://reviewboard.mozilla.org/r/164656/#review175022
Attachment #8893583 - Flags: review?(kvark) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a733646bca35
Revert PLANAR_YCBCR case in GLBlitHelper.cpp on Android so that when a canvas uses a video frame as source, it is not flipped r=kvark
https://hg.mozilla.org/mozilla-central/rev/a733646bca35
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8893583 [details]
Bug 1382292 - Revert PLANAR_YCBCR case in GLBlitHelper.cpp on Android so that when a canvas uses a video frame as source, it is not flipped

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330672 - WebGL Texture is flipped upside down
[User impact if declined]: Canvas snap shots of video will be upside down.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: steps to reproduce are in the but.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]:no, it is reverting a change.
[Why is the change risky/not risky?]: The patch reverts code on the android platform
[String changes made/needed]: none.
Attachment #8893583 - Flags: approval-mozilla-beta?
Kevin, can you verify this on nightly since it looks like you were able to reproduce it earlier? Thanks!
Flags: qe-verify+
Flags: needinfo?(kbrosnan)
Verified using today's nightly on a Galaxy S6.
Flags: needinfo?(kbrosnan)
Comment on attachment 8893583 [details]
Bug 1382292 - Revert PLANAR_YCBCR case in GLBlitHelper.cpp on Android so that when a canvas uses a video frame as source, it is not flipped

Fix verified on nightly, let's uplift this for 56 beta 6.
Attachment #8893583 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.