Closed Bug 1127336 Opened 9 years ago Closed 9 years ago

Using <video> as WebGL texture results in upside-down texture

Categories

(Core :: Graphics: CanvasWebGL, defect)

35 Branch
ARM
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: vlad, Assigned: jgilbert)

References

Details

Attachments

(4 files)

There's a testcase at http://krpano.com/ios/bugs/ios8-webgl-video/

Texturing from video results in the texture being loaded upside down in Firefox for Android.  Works fine on desktop.  Reproduced in 38, and I have a report of it happening in 35 as well.
Jeff, can you reproduce this?
Flags: needinfo?(jgilbert)
Yep, definitely upside-down.

This is probably related to our video->texture fast path.
Jeff, something you can fix?  Or should we send it to a/v team?
Assignee: nobody → jgilbert
(In reply to Milan Sreckovic [:milan] from comment #3)
> Jeff, something you can fix?  Or should we send it to a/v team?

It's a bug in WebGL code probably. I can probably take a look.
Dan, do you have a device to reproduce this, and do you have spare cycles to take a quick look?
Assignee: jgilbert → nobody
Flags: needinfo?(jgilbert) → needinfo?(dglastonbury)
Yeah, it appears upside-down in Fennec 37.0.1
Flags: needinfo?(dglastonbury)
You win!
Assignee: nobody → dglastonbury
This seems to be happening with mp4 source video (like in the example), but not ogv source video.  It doesn't happen with either on the desktop with GL compositor/native GL webgl.
Adding some logging to GLBlitHelper...

For mp4 source video, I see a stream of:

I/GeckoConsole(23545): ==== rAF loop start
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type EGLImage
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type SurfaceTexture
I/GeckoConsole(23545): ==== rAF loop start
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type EGLImage
I/GeckoConsole(23545): ==== rAF loop start
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type EGLImage
I/GeckoConsole(23545): ==== rAF loop start
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type EGLImage
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type SurfaceTexture
...

Sometimes there's just an EGLImage blit, sometimes there's both an EGLImage and a SurfaceTexture blit

For ogv source video, I see a stream of:

I/GeckoConsole(23545): ==== rAF loop start
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type PlanarYCbCr
I/GeckoConsole(23545): ==== rAF loop start
I/GeckoBlitHelper(23545): BlitImageToFramebuffer yflip 0 type PlanarYCbCr
...

So now I have an additional question -- why is the mp4 source video causing the SurfaceTexture blits?  And why not every frame?
the SurfaceTexture blit is coming from:

#0  mozilla::gl::GLBlitHelper::BlitSurfaceTextureImage (this=this@entry=0x87023a90, 
    stImage=stImage@entry=0x8f8a5420, yflip=yflip@entry=false)
    at /home/vladimir/proj/moz/mozilla-central/gfx/gl/GLBlitHelper.cpp:738
#1  0x9cd7283c in mozilla::gl::GLBlitHelper::BlitImageToFramebuffer (this=this@entry=0x87023a90, 
    srcImage=srcImage@entry=0x8f8a5420, destSize=..., destFB=1, yflip=yflip@entry=false, xoffset=xoffset@entry=0, 
    yoffset=yoffset@entry=0, cropWidth=cropWidth@entry=0, cropHeight=cropHeight@entry=0)
    at /home/vladimir/proj/moz/mozilla-central/gfx/gl/GLBlitHelper.cpp:897
#2  0x9cd728ce in mozilla::gl::GLBlitHelper::BlitImageToTexture (this=0x87023a90, srcImage=0x8f8a5420, 
    destSize=..., destTex=1, destTarget=3553, yflip=false, xoffset=0, yoffset=0, cropWidth=0, cropHeight=0)
    at /home/vladimir/proj/moz/mozilla-central/gfx/gl/GLBlitHelper.cpp:928
#3  0x9d34de3a in mozilla::VideoDataDecoder::CopySurface (this=this@entry=0x8ef93210, img=img@entry=0x8f8a5420)
    at /home/vladimir/proj/moz/mozilla-central/dom/media/fmp4/android/AndroidDecoderModule.cpp:84
#4  0x9d34e81a in PostOutput (aDuration=33334, aInfo=..., this=0x8ef93210, aFormat=...)
    at /home/vladimir/proj/moz/mozilla-central/dom/media/fmp4/android/AndroidDecoderModule.cpp:121
#5  mozilla::VideoDataDecoder::PostOutput (this=0x8ef93210, aInfo=..., aFormat=..., aDuration=33334)
    at /home/vladimir/proj/moz/mozilla-central/dom/media/fmp4/android/AndroidDecoderModule.cpp:103
#6  0x9d34d358 in mozilla::MediaCodecDataDecoder::DecoderLoop (this=0x8ef93210)
    at /home/vladimir/proj/moz/mozilla-central/dom/media/fmp4/android/AndroidDecoderModule.cpp:547
#7  0x9d34dc4a in apply<mozilla::MediaCodecDataDecoder, void (mozilla::MediaCodecDataDecoder::*)()> (m=
    (void (mozilla::MediaCodecDataDecoder::*)(mozilla::MediaCodecDataDecoder * const)) 0x9d34ceed <mozilla::MediaCodecDataDecoder::DecoderLoop()>, o=<optimized out>, this=<optimized out>)
    at ../../../dist/include/nsThreadUtils.h:574
#8  nsRunnableMethodImpl<void (mozilla::MediaCodecDataDecoder::*)(), true>::Run (this=<optimized out>)
    at ../../../dist/include/nsThreadUtils.h:666
And here's the culprit:
  https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/android/AndroidDecoderModule.cpp#111

This decoder module sets the GLImage origin to BottomLeft; nothing else does.  BlitHelpers don't take this into account.  Should they?  Right now the "yflip" argument to blit helpers mean "flip the Y axis".  But that doesn't really help if you don't know where the origin is on the input image, and that detail got hidden deep in the GLImage classes.  I think we have a few options:

1 - we change the blit helper "yflip" argument meaning to say "in the output, I want to Y axis to be flipped from the natural orientation".  For most images, or ones where we don't know the origin, we assume top-left input.
2 - we change "yflip" to be a gl::OriginPos, so that we can be explicit about what we want
3 - we expose an OriginPos on the base Image class, so that callers can identify what the origin of any Image is, and do any y flipping that they need
Here's a patch that fixes the problem for me.  This roughly does #1 above.  Regular (non-webgl) playback of mp4 and ogv files continues to work fine.

I don't know if there are any callsites where this could break though -- basically the same code pattern that's seen here that required changing the order data param will need changing elsewhere too (create GLImage, set order to BottomLeft; use BlitHelpers to blit; set order to BottomLeft in destination).
Attachment #8589701 - Flags: review?(dglastonbury)
Comment on attachment 8589701 [details] [diff] [review]
Take into account image origin when using yflip

Oh my god you guys, you all broke this in bug 1117777.  GUILTY ALL OF YOU (except dan).  That bug's name was even "Fix inverted MP4 videos on Android" and noone said "hey, videos are now inverted in webgl, wonder if it's related?"!
Attachment #8589701 - Flags: review?(snorp)
Attachment #8589701 - Flags: review?(jgilbert)
snorp: pretty sure the BlitImageToFramebuffer call in GLImage::GetAsSourceSurface is now incorrect, and that "true" should be a "false".  I mean, it was *always* incorrect because it never bothered to check the origin, because it couldn't, since only the subclasses knew about it, but I think it was accidentally working before because all of the other things always had y inverted.
This does an explicit OriginPos argument for the destination.  I kinda like this one better... some of the callsites (e.g. the CanvasRenderingContext2D one, that just had a "true" for the yflip) had me scratching my head to figure out what it really wanted.  I still don't know if it wanted top left or bottom left.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #11)
> And here's the culprit:
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/android/
> AndroidDecoderModule.cpp#111
> 
> This decoder module sets the GLImage origin to BottomLeft; nothing else
> does.  BlitHelpers don't take this into account.  Should they?  Right now
> the "yflip" argument to blit helpers mean "flip the Y axis".  But that
> doesn't really help if you don't know where the origin is on the input
> image, and that detail got hidden deep in the GLImage classes.  I think we
> have a few options:
> 
> 1 - we change the blit helper "yflip" argument meaning to say "in the
> output, I want to Y axis to be flipped from the natural orientation".  For
> most images, or ones where we don't know the origin, we assume top-left
> input.
Never again! This is where bugs live and breed.

> 2 - we change "yflip" to be a gl::OriginPos, so that we can be explicit
> about what we want
I don't think we generally want this.

> 3 - we expose an OriginPos on the base Image class, so that callers can
> identify what the origin of any Image is, and do any y flipping that they
> need
This is ideal. We should do the minimal amount of flipping.
Comment on attachment 8589701 [details] [diff] [review]
Take into account image origin when using yflip

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

BlitFooToBar do not care about which origin the source and dest have, only with whether they should yflip or not.
The caller should determine from the src and dest origins whether or not yflip is required.

::: gfx/gl/GLBlitHelper.cpp
@@ +730,5 @@
>  {
>      AndroidSurfaceTexture* surfaceTexture = stImage->GetData()->mSurfTex;
>  
> +    if (stImage->GetData()->mOriginPos == OriginPos::BottomLeft) {
> +        yflip = !yflip;

Say "no" to yflip hell.
Attachment #8589701 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> Comment on attachment 8589701 [details] [diff] [review]
> Take into account image origin when using yflip
> 
> Review of attachment 8589701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BlitFooToBar do not care about which origin the source and dest have, only
> with whether they should yflip or not.
> The caller should determine from the src and dest origins whether or not
> yflip is required.

I'll make a once-over patch for the callers of these.
(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #11)
> > And here's the culprit:
> >  
> > https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/android/
> > AndroidDecoderModule.cpp#111
> > 
> > This decoder module sets the GLImage origin to BottomLeft; nothing else
> > does.  BlitHelpers don't take this into account.  Should they?  Right now
> > the "yflip" argument to blit helpers mean "flip the Y axis".  But that
> > doesn't really help if you don't know where the origin is on the input
> > image, and that detail got hidden deep in the GLImage classes.  I think we
> > have a few options:
> > 
> > 1 - we change the blit helper "yflip" argument meaning to say "in the
> > output, I want to Y axis to be flipped from the natural orientation".  For
> > most images, or ones where we don't know the origin, we assume top-left
> > input.
> Never again! This is where bugs live and breed.
> 
> > 2 - we change "yflip" to be a gl::OriginPos, so that we can be explicit
> > about what we want
> I don't think we generally want this.
Actually, since this is only on the BlitImageToFoo functions, we might want to have a `gl::OriginPos destOrigin`. For no flip, just pass in Image->mOriginPos.
> 
> > 3 - we expose an OriginPos on the base Image class, so that callers can
> > identify what the origin of any Image is, and do any y flipping that they
> > need
> This is ideal. We should do the minimal amount of flipping.
Comment on attachment 8589716 [details] [diff] [review]
Version with explicit destination OriginPos

I like this route.
Attachment #8589716 - Flags: review+
Assignee: dglastonbury → jgilbert
Comment on attachment 8589701 [details] [diff] [review]
Take into account image origin when using yflip

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

::: dom/media/fmp4/android/AndroidDecoderModule.cpp
@@ +143,4 @@
>        data.mSync = eglSync;
>        data.mOwns = true;
>        data.mSize = gfx::IntSize(mConfig.display_width, mConfig.display_height);
> +      data.mOriginPos = gl::OriginPos::TopLeft; // the blit already did the flipping

This seems kinda flaky, does it not?

::: gfx/gl/GLBlitHelper.cpp
@@ +765,5 @@
>      EGLImage eglImage = image->GetData()->mImage;
>      EGLSync eglSync = image->GetData()->mSync;
>  
> +    if (image->GetData()->mOriginPos == OriginPos::BottomLeft) {
> +        yflip = !yflip;

Cut'n'paste code? Can we do this in one place, so not requiring these kinds of checks to be sprinkled through out the blitters?
Attachment #8589701 - Flags: review?(dglastonbury) → review-
Comment on attachment 8589962 [details] [diff] [review]
0002-Simplify-and-cleanup.patch (based on "Version with explicit destination OriginPos")

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4266,4 @@
>        gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);
>        gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
>      }
> +    const gl::OriginPos destOrigin = gl::OriginPos::TopLeft;

This seems a bit pointless to me.

::: gfx/gl/GLBlitHelper.cpp
@@ +861,5 @@
>          return false;
>      }
>  
> +    const bool needsYFlip = (srcOrigin != destOrigin);
> +    mGL->fUniform1f(mYFlipLoc, needsYFlip ? (float)1.0 : (float)0.0);

I like this version.

@@ -874,5 @@
>      mGL->fViewport(0, 0, destSize.width, destSize.height);
> -    if (xoffset != 0 && yoffset != 0 && cropWidth != 0 && cropHeight != 0) {
> -        mGL->fEnable(LOCAL_GL_SCISSOR_TEST);
> -        mGL->fScissor(xoffset, yoffset, (GLsizei)cropWidth, (GLsizei)cropHeight);
> -    }

Is this code not needed?

@@ +953,4 @@
>      bool good = UseTexQuadProgram(type, srcSize);
>      if (!good) {
>          // We're up against the wall, so bail.
> +        MOZ_CRASH("Fatal Error: Failed to prepare to blit texture->framebuffer.\n");

Is it really a good idea to crash the browser? Can't we just bail or render black?
(In reply to Dan Glastonbury :djg :kamidphish from comment #22)
> 
> Review of attachment 8589701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/android/AndroidDecoderModule.cpp
> @@ +143,4 @@
> >        data.mSync = eglSync;
> >        data.mOwns = true;
> >        data.mSize = gfx::IntSize(mConfig.display_width, mConfig.display_height);
> > +      data.mOriginPos = gl::OriginPos::TopLeft; // the blit already did the flipping
> 
> This seems kinda flaky, does it not?

No flakier than before -- it only feels flaky because the actual flip is hidden in the CopySurface() function.  To make it look a little less flaky, could make CopySurface take an explicit destOrigin param, pass TopLeft there, so that it's obvious what's happening.
Comment on attachment 8589962 [details] [diff] [review]
0002-Simplify-and-cleanup.patch (based on "Version with explicit destination OriginPos")

Looks good to me, though I agree that the const destOrigin (when it's static, like in Canvas2D) is a little superfluous.. I get the intent, but at that point you may as well have const GLenum destTextureTarget = LOCAL_GL_TEXTURE_2D too etc :)

I didn't move the uniform setting in the Framebuffer blit because of the need to do the casts to the specific Image* subclasses there, and also becuase nothing else sets a uniform directly there.  But it's the only (one of the few?) uniforms that are set anyway, and the other functions are implementation details anyway, so whatever.
Attachment #8589962 - Flags: review?(vladimir) → review+
Comment on attachment 8589701 [details] [diff] [review]
Take into account image origin when using yflip

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

I agree with the other comments here and like the other patch a lot better.
Attachment #8589701 - Flags: review?(snorp) → review-
Let's get the fix here in on aurora too, since we just recently branched.
(In reply to Dan Glastonbury :djg :kamidphish from comment #23)
> Comment on attachment 8589962 [details] [diff] [review]
> 0002-Simplify-and-cleanup.patch (based on "Version with explicit destination
> OriginPos")
> 
> Review of attachment 8589962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +4266,4 @@
> >        gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);
> >        gl->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
> >      }
> > +    const gl::OriginPos destOrigin = gl::OriginPos::TopLeft;
> 
> This seems a bit pointless to me.
The `const` is not necessary[1], but it's useful for readability to call out what the var is named. (similar to how we don't like raw `true` and `false` args)

[1]: This is just me wishing that C++ had an immutable-by-default option, so that this didn't need to be so verbose. The majority of local vars are functionally const, 

> 
> @@ -874,5 @@
> >      mGL->fViewport(0, 0, destSize.width, destSize.height);
> > -    if (xoffset != 0 && yoffset != 0 && cropWidth != 0 && cropHeight != 0) {
> > -        mGL->fEnable(LOCAL_GL_SCISSOR_TEST);
> > -        mGL->fScissor(xoffset, yoffset, (GLsizei)cropWidth, (GLsizei)cropHeight);
> > -    }
> 
> Is this code not needed?
No. No callers actually change the default values for these args, and this code only runs if the args have non-default values. (The the conditional should almost certainly have been OR'd not AND'd anyways!)

> 
> @@ +953,4 @@
> >      bool good = UseTexQuadProgram(type, srcSize);
> >      if (!good) {
> >          // We're up against the wall, so bail.
> > +        MOZ_CRASH("Fatal Error: Failed to prepare to blit texture->framebuffer.\n");
> 
> Is it really a good idea to crash the browser? Can't we just bail or render
> black?
I'd rather have crash reports and fix it rather than let it fester. I'd rather have a "report runtime assertion" but we don't really have that option. (I'll follow up on this idea later)

Regardless of whether we crash or bail/black-out, the user still won't have a functional webpage. A crash is more aggressive, but also more informative to us. I argue that it's likely we'll adversely affect fewer users by crashing aggressively (and then fixing) on a few users rather than not functioning on many users for a longer time.

Would you be more amenable to having it only crash on nightly/dev and try to bail on beta/release?
Flags: needinfo?(dglastonbury)
(In reply to Jeff Gilbert [:jgilbert] from comment #28)
> Would you be more amenable to having it only crash on nightly/dev and try to
> bail on beta/release?

Yes.
Flags: needinfo?(dglastonbury)
vlad, jeff et al

I am also experiencing this issue. My project was actually working fine several versions back. Then, in once update, the videos were vertically inverted. Then, in the update after that, they wouldn't render at all. 

My project: www.villmer.com/tools
I'm running Android Kit Kat 4.4.4 on a Galaxy Tab 2 Plus.

Regards,
Mr. Villmer
www.villmer.com
mail@villmer.com
Attached patch 0003-fixup.patchSplinter Review
This bug seems to be related primarily to mp4 files as I'm seemingly able to load webm files without issue. The mp4 files are technically loading but are just not rendering.

Firefox 37.0.2
Android Kit Kat 4.4.4 on a Galaxy Tab 2 Plus.

Regards,
Mr. Villmer
www.villmer.com
mail@villmer.com
https://hg.mozilla.org/mozilla-central/rev/737853697fe3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assuming this issue is, in fact, resolved, why wait to implement the solution until version 40? Considering we are only at Android version 37, doesn't look like this fix is coming any time soon. 3 major versions later to implement this?
(In reply to villmer from comment #36)
> Assuming this issue is, in fact, resolved, why wait to implement the
> solution until version 40? Considering we are only at Android version 37,
> doesn't look like this fix is coming any time soon. 3 major versions later
> to implement this?

We can backport it to 39, but it's a minor problem that's not worth perturbing 37, nor 38 at this point, I think.
How does the implementation of reparations perturbe anything?
(In reply to villmer from comment #38)
> How does the implementation of reparations perturbe anything?

Because we do not have absolute faith in the correctness of code. Our previous 'fix' for this issue broke it in a different way. If we would have shipped that to everyone, it may have broken otherwise-working content.
Wouldn't this be the case regardless of what version you released the code at? It could break whatever version you released it under, no? So, why not get it out the door soon so you can begin resolving it sooner if it doesn't work?
(In reply to villmer from comment #40)
> Wouldn't this be the case regardless of what version you released the code
> at? It could break whatever version you released it under, no? So, why not
> get it out the door soon so you can begin resolving it sooner if it doesn't
> work?

We can't thrash updates daily to Beta and Release, whereas we can easily fix it on Nightly and Dev. If you disagree with our uplifting standards, please take it up with Release Engineering.
For what it's worth, I disagree with your uplifting standards. I don't consider the implementation of a solution to be thrashing. There won't ever be a time when releasing a a solution to one problem will guarantee it won't affect something else.
We are in what our Release Engineering team considers late beta for Firefox 38. In late beta teams look to reduce risk of changes. Supporting 6 versions of Android OS and various combinations of device manufacturers, mobile chipsets and the drivers creates a varied problem space. The risk is more widespread that just Firefox for Android, Firefox for Windows, OS X and Linux are built off the same code base. Introducing unintended regressions to those releases is a risk.

Examples of changes that are acceptable in late beta, backing out bad code to a known good state, crash fixes that have shown on Aurora and Nightly crash reporting that the crash no longer exists. There is no known good state here, backing out bug 1117777 would reintroduce another bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: