Closed Bug 1395497 Opened 7 years ago Closed 7 years ago

WebGL video textures issues for Firefox 55 in android

Categories

(Firefox for Android Graveyard :: Audio/Video, defect)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Version:
55 Branch
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: aelboudali, Assigned: snorp)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Bug 1395497 - Create SurfaceTexture in detached state, attach on first use
59 bytes, text/x-review-board-request
jgilbert
: review+
gchang
: approval-mozilla-beta-
Details
Bug 1395497 - Regenerate JNI binding
59 bytes, text/x-review-board-request
Details
Bug 1395497 - Add AndroidSurfaceTexture support to GLBlitHelper. -
59 bytes, text/x-review-board-request
snorp
: review+
jgilbert
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Steps to reproduce:

When visiting :

https://krpano.com/ios/bugs/ios8-webgl-video-performance/

The html5 video was playing normally on Firefox 54. Upon updating to Firefox 55 on android the video no longer works and displays a black screen. 


Actual results:

The video WebGL texture seems to no longer be playing on Firefox for Android. The sound is playing well, but the screen remains black.


Expected results:

The video WebGL texture should be able to be seen just.
Component: General → Audio/Video
OS: Unspecified → Android
Hardware: Unspecified → All
I was able to reproduce this on all branches with Oneplus Two (Android 6.0.1) and Motorola Nexus 6 (Android 7.1.1). Marking as New. 
Notes: on Chrome is not reproducible.
Status: UNCONFIRMED → NEW
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox57: --- → affected
Ever confirmed: true
Assignee: nobody → snorp
These patches make the situation better. There are some caveats:

1) We do not attempt to handle the situation where both the compositor and canvas (either WebGL or 2d) are trying to consume a video. We know from past experience that trying to quickly switch between to GL contexts results in problems.

2) This won't work in E10s because we won't be able to access the SurfaceTexture in the content process. To fix that, we could create the SurfaceTexture for the video in the content process, then blit that to another SurfaceTexture that lives in the parent process.
Comment on attachment 8912985 [details]
Bug 1395497 - Modify ScopedBindTexture to allow save/restore

https://reviewboard.mozilla.org/r/184346/#review189470

Modify ScopedBindTexture.

::: gfx/gl/ScopedGLHelpers.cpp:477
(Diff revision 1)
> +}
> +
> +void
> +ScopedTextureBindingState::UnwrapImpl()
> +{
> +    mGL->fBindTexture(mBinding, mValue);

This doesn't work at all!
LOCAL_GL_TEXTURE_BINDING_2D: 0x8069
LOCAL_GL_TEXTURE_2D: 0x0DE1
Attachment #8912985 - Flags: review?(jgilbert) → review-
Comment on attachment 8912986 [details]
Bug 1395497 - Create SurfaceTexture in detached state, attach on first use

https://reviewboard.mozilla.org/r/184348/#review189476
Attachment #8912986 - Flags: review?(jgilbert) → review+
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review189474

::: gfx/gl/GLBlitHelper.cpp:75
(Diff revision 1)
> +    uniform mat4 uTexTransform;                                              \n\
> +                                                                             \n\
> +    void main(void)                                                          \n\
> +    {                                                                        \n\
> +        FRAG_COLOR = TEXTURE(uTex0,                                          \n\
> +            (uTexTransform * vec4(vTexCoord0, 0.0, 1.0)).xy);                \n\

This should be in the vert shader.
We should just be uploading a matrix to the vs instead of the current mess of vec2s, though that does mean dealing with constructing matrices in c++.

I can do this for you.

::: gfx/gl/GLBlitHelper.cpp:687
(Diff revision 1)
> +
> +    ScopedTextureBindingState boundTex(mGL, LOCAL_GL_TEXTURE_BINDING_EXTERNAL);
> +    surfaceTexture->UpdateTexImage();
> +
> +    gfx::Matrix4x4 transform;
> +    AndroidSurfaceTexture::GetTransformMatrix(java::sdk::SurfaceTexture::Ref::From(surfaceTexture), transform);

GetTransformMatrix should take a gfx::Matrix4x4*, not gfx::Matrix4x4&.
Attachment #8912987 - Flags: review?(jgilbert) → review-
Comment on attachment 8912985 [details]
Bug 1395497 - Modify ScopedBindTexture to allow save/restore

https://reviewboard.mozilla.org/r/184346/#review189658

Quick drive-by comments about two issues found by our static analysis bot.

::: gfx/gl/ScopedGLHelpers.h:344
(Diff revision 1)
> +protected:
> +    GLenum mBinding;
> +    GLuint mValue;
> +
> +public:
> +    ScopedTextureBindingState(GLContext* aGL, GLenum state = LOCAL_GL_TEXTURE_BINDING_2D);

Our static analysis bot found a problem at this line:

Error: Bad implicit conversion constructor for 'ScopedTextureBindingState' [clang-tidy: mozilla-implicit-constructor]
Note: Consider adding the explicit keyword to the constructor

::: gfx/gl/ScopedGLHelpers.cpp:467
(Diff revision 1)
>  }
>  
>  ////////////////////////////////////////////////////////////////////////
> +// ScopedTextureBindingState
> +
> +ScopedTextureBindingState::ScopedTextureBindingState(GLContext* gl, GLenum binding)

Our static analysis bot found a problem at this line:

Error: Bad implicit conversion constructor for 'ScopedTextureBindingState' [clang-tidy: mozilla-implicit-constructor]
Note: Consider adding the explicit keyword to the constructor
Comment on attachment 8912985 [details]
Bug 1395497 - Modify ScopedBindTexture to allow save/restore

https://reviewboard.mozilla.org/r/184346/#review189470

Ok, do you have a suggestion on what you would like to see? Maybe another constructor with no texture name for the save/restore case?

> This doesn't work at all!
> LOCAL_GL_TEXTURE_BINDING_2D: 0x8069
> LOCAL_GL_TEXTURE_2D: 0x0DE1

Whoops! You're right, of course. That's what I get for trying to clean up the manual fGetIntegerv/fBindTexture pair I had :)
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review189474

> GetTransformMatrix should take a gfx::Matrix4x4*, not gfx::Matrix4x4&.

Any particular reason why? I chose a reference because otherwise we should null check it. We seem to use both quite a bit in gfx code.
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review189474

> This should be in the vert shader.
> We should just be uploading a matrix to the vs instead of the current mess of vec2s, though that does mean dealing with constructing matrices in c++.
> 
> I can do this for you.

This is the same way we've handled this texture transform in the compositor since forever, and the same way it worked in the previous GLBlitHelper implementation. I guess doing it in the vs could perform better, but that's scope creep for this patch.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #13)
> Comment on attachment 8912987 [details]
> Bug 1395497 - Add SurfaceTexture impl in GLBlitHelper
> 
> https://reviewboard.mozilla.org/r/184350/#review189474
> 
> > This should be in the vert shader.
> > We should just be uploading a matrix to the vs instead of the current mess of vec2s, though that does mean dealing with constructing matrices in c++.
> > 
> > I can do this for you.
> 
> This is the same way we've handled this texture transform in the compositor
> since forever, and the same way it worked in the previous GLBlitHelper
> implementation. I guess doing it in the vs could perform better, but that's
> scope creep for this patch.

This is API/architecture creep otherwise. Since we have a bad track record about clean ups, I believe we should fix it immediately.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #12)
> Comment on attachment 8912987 [details]
> Bug 1395497 - Add SurfaceTexture impl in GLBlitHelper
> 
> https://reviewboard.mozilla.org/r/184350/#review189474
> 
> > GetTransformMatrix should take a gfx::Matrix4x4*, not gfx::Matrix4x4&.
> 
> Any particular reason why? I chose a reference because otherwise we should
> null check it. We seem to use both quite a bit in gfx code.

Because it makes it clear at the call site that we're expecting an out-var. A null check isn't needed. I don't even assert on it anymore. (it'll crash all on its own)

Out-vars being contractually non-null is a standard guarantee.
Comment on attachment 8912985 [details]
Bug 1395497 - Modify ScopedBindTexture to allow save/restore

https://reviewboard.mozilla.org/r/184346/#review190220

::: gfx/gl/ScopedGLHelpers.cpp:202
(Diff revision 2)
>  ScopedBindTexture::ScopedBindTexture(GLContext* aGL, GLuint aNewTex, GLenum aTarget)
>          : ScopedGLWrapper<ScopedBindTexture>(aGL)
>          , mTarget(aTarget)
>          , mOldTex(GetBoundTexture(aGL, aTarget))
>  {
> +    if (aNewTex) {

Use -1. There are potential cases where binding 0 is desirable, so using zero as a sentenel could be surprising.
Attachment #8912985 - Flags: review?(jgilbert) → review+
Attachment #8912987 - Flags: review?(dmu)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #27)
> Comment on attachment 8912987 [details]
> Bug XXX - Use mat3s to transform tex coords in GLBlitHelper. -
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/184350/diff/2-3/

It seems :jgilbert has done some review work already. We should let him keep follow-up it.
Attachment #8912987 - Flags: review?(jgilbert)
Comment on attachment 8912985 [details]
Bug 1395497 - Modify ScopedBindTexture to allow save/restore

https://reviewboard.mozilla.org/r/184346/#review190220

> Use -1. There are potential cases where binding 0 is desirable, so using zero as a sentenel could be surprising.

Textures are GLuint, though, so using negative as a sentinel could also be surprising (and we'd have to change the interface to use GLint, which would be weird). I think I'm just going to drop this patch and bind to either 0 or whatever texture the SurfaceTexture is going to bind to.
Attachment #8912985 - Attachment is obsolete: true
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review190648

I think mozreview got screwed up here -- this patch has not been reviewed yet. Jeff wrote it, I'm just uploading it with the rest of the changes for this bug.
Comment on attachment 8914008 [details]
Bug 1395497 - Add AndroidSurfaceTexture support to GLBlitHelper. -

https://reviewboard.mozilla.org/r/185270/#review190720
Attachment #8914008 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #29)
> Comment on attachment 8912985 [details]
> Bug 1395497 - Modify ScopedBindTexture to allow save/restore
> 
> https://reviewboard.mozilla.org/r/184346/#review190220
> 
> > Use -1. There are potential cases where binding 0 is desirable, so using zero as a sentenel could be surprising.
> 
> Textures are GLuint, though, so using negative as a sentinel could also be
> surprising (and we'd have to change the interface to use GLint, which would
> be weird). I think I'm just going to drop this patch and bind to either 0 or
> whatever texture the SurfaceTexture is going to bind to.

Sounds good.
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review190872

LGTM. thanks.

::: gfx/gl/GLBlitHelper.cpp:128
(Diff revision 5)
> +Mat<N>::Zero()
> +{
> +    Mat<N> ret;
> +    for (auto& x : ret.m) {
> +        x = 0.0f;
> +    }

Why not consider to use memcpy instead of assigning by items.

::: gfx/gl/GLBlitHelper.cpp:1046
(Diff revision 5)
>          break;
>      default:
>          gfxCriticalError() << "Unexpected srcTarget: " << srcTarget;
>          return;
>      }
> -    const auto& prog = GetDrawBlitProg(key);
> +    const auto& prog = GetDrawBlitProg({ fragHeader, kFragBody_RGBA});

a redundant space in {} before 'f'.
Attachment #8912987 - Flags: review?(dmu) → review+
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review190222

::: gfx/gl/AndroidSurfaceTexture.cpp:21
(Diff revision 2)
>    auto jarray = jni::FloatArray::LocalRef::Adopt(env, env->NewFloatArray(16));
> -  aSurfaceTexture->GetTransformMatrix(jarray);
> +  surfaceTexture->GetTransformMatrix(jarray);
>  
>    jfloat* array = env->GetFloatArrayElements(jarray.Get(), nullptr);
>  
> -  aMatrix._11 = array[0];
> +  outMatrix->_11 = array[0];

Oh man, gross.

::: gfx/gl/GLBlitHelper.cpp:366
(Diff revision 2)
>          gl->fUniformMatrix4fv(mLoc_uColorMatrix, 1, false, colorMatrix);
>      }
>  
> +    MOZ_ASSERT(bool(argsTexTransform) == (mLoc_uTexTransform != -1));
> +    if (argsTexTransform) {
> +        gl->fUniformMatrix4fv(mLoc_uTexTransform, 1, false, &argsTexTransform->transform._11);

If this works, use memcpy in GetTransformMatrix.
If that isn't valid, this isn't valid.
Attachment #8912987 - Flags: review+
Comment on attachment 8914008 [details]
Bug 1395497 - Add AndroidSurfaceTexture support to GLBlitHelper. -

https://reviewboard.mozilla.org/r/185270/#review191466

Jeff, I made some changes to the y flip based on local testing. Can you look?
Attachment #8914008 - Flags: review?(jgilbert)
Comment on attachment 8912986 [details]
Bug 1395497 - Create SurfaceTexture in detached state, attach on first use

https://reviewboard.mozilla.org/r/184348/#review191474


C/C++ static analysis didn't find any defects in this patch. Hooray!
Comment on attachment 8912987 [details]
Bug 1395497 - Use mat3s to transform tex coords in GLBlitHelper. -

https://reviewboard.mozilla.org/r/184350/#review191480


C/C++ static analysis didn't find any defects in this patch. Hooray!
Comment on attachment 8914008 [details]
Bug 1395497 - Add AndroidSurfaceTexture support to GLBlitHelper. -

https://reviewboard.mozilla.org/r/185270/#review191484


C/C++ static analysis didn't find any defects in this patch. Hooray!
Comment on attachment 8914008 [details]
Bug 1395497 - Add AndroidSurfaceTexture support to GLBlitHelper. -

https://reviewboard.mozilla.org/r/185270/#review192942

::: gfx/gl/GLBlitHelper.cpp:769
(Diff revision 4)
> +    // something wrong.
> +    MOZ_ASSERT(transform3.at(0,2) == 0);
> +    MOZ_ASSERT(transform3.at(1,2) == 0);
> +    MOZ_ASSERT(transform3.at(2,2) == 1);
> +
> +    const OriginPos srcOrigin = srcImage->GetOriginPos();

Prefer const auto& in cases like this, particularly since the type is extremely obvious.
Attachment #8914008 - Flags: review?(jgilbert) → review+
Sorry, I didn't see this re-request!
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8503893cc85
Create SurfaceTexture in detached state, attach on first use r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9fbcd8bd4cb
Regenerate JNI binding r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3dd1e01908
Use mat3s to transform tex coords in GLBlitHelper. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/c99dadd2df7d
Add AndroidSurfaceTexture support to GLBlitHelper. - r=snorp
Jeff, the bustage in comment #53 is probably related to your mat3 patch, since none of the others should affect Linux. Can you take a look?
Flags: needinfo?(jgilbert)
Ok, I'm going to land bug 1404536 first. It looks green?
Flags: needinfo?(jgilbert)
Depends on: 1404536
Attachment #8912987 - Attachment is obsolete: true
Unblocked! Back to you to rebase and land. (I hope!)
Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31cc637a34aa
Create SurfaceTexture in detached state, attach on first use r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4046d781cacd
Regenerate JNI binding r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6aad6761527
Add AndroidSurfaceTexture support to GLBlitHelper. - r=snorp
Depends on: 1413500
Verified as fixed on the latest Beta (58.0b3/build 4) and the latest Nightly (59.0a1 / 2017-11-14)
This issue was tested on a Pixel C (Android 8.0)
status-firefox58: fixed → verified
Ryan/gchang, why was this backed out? Was it due to bug 1413500? I would like to reland this in beta, along with bug 1413500 and 1421313.
Flags: needinfo?(ryanvm)
Flags: needinfo?(gchang)
Comment on attachment 8912986 [details]
Bug 1395497 - Create SurfaceTexture in detached state, attach on first use

Approval Request Comment
[Feature/Bug causing the regression]: Unknown
[User impact if declined]: Crashes, this fix is necessary to uplift bug 1421313
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1413500
[Is the change risky?]: No
[Why is the change risky/not risky?]: Been using on Nightly with no problems
[String changes made/needed]: None
Attachment #8912986 - Flags: approval-mozilla-beta?
Yes, it was to due to the high crash volume on Beta until a suitable fix for bug 1413500 could be found. Re-landing on Beta is Gerry's call, so leaving his NI set :)
Flags: needinfo?(ryanvm)
Hi :snorp,
Yes, like Ryan said, it was due to the high crash volume on beta. Given this issue has been there since 55 and we only have 10 days before RC week (1/15) after we are back from time off. Can we let this ride the train on 59?
Flags: needinfo?(gchang) → needinfo?(snorp)
If we're willing to accept the crash rate I guess so...
Flags: needinfo?(snorp)
Comment on attachment 8912986 [details]
Bug 1395497 - Create SurfaceTexture in detached state, attach on first use

This patch is huge and it's a bit risky to uplift this to 58 (next week is RC). Let's let it ride the train on 59 and won't fix for 58.
Attachment #8912986 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox58: affected → wontfix
status-firefox59: --- → fixed
Target Milestone: Firefox 58 → Firefox 59
Hi, I posted this bug from v56 back in September.
It has been moved forward I see to v59.
Has this been fixed in the current v59?
Yes, it should be fixed in 59.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: