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.

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.

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

Comment on attachment 8912986 [details] Bug 1395497 - Create SurfaceTexture in detached state, attach on first use https://reviewboard.mozilla.org/r/184348/#review189476

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

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.

WIP with matrices: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04bd0395451e679a5dd0a1eb1aecec34829da825

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.

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

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.

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

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

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.

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?

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.

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1498abcee79d for https://treeherder.mozilla.org/logviewer.html#?job_id=137356665&repo=mozilla-inbound on every flavor of Linux.

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?

Ok, I'm going to land bug 1404536 first. It looks green?

Unblocked! Back to you to rebase and land. (I hope!)

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

https://hg.mozilla.org/mozilla-central/rev/31cc637a34aa https://hg.mozilla.org/mozilla-central/rev/4046d781cacd https://hg.mozilla.org/mozilla-central/rev/b6aad6761527

https://hg.mozilla.org/mozilla-central/rev/31cc637a34aa https://hg.mozilla.org/mozilla-central/rev/4046d781cacd https://hg.mozilla.org/mozilla-central/rev/b6aad6761527

Too late for 57

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)

Backed out from beta on request from gchang and RyanVM: https://hg.mozilla.org/releases/mozilla-beta/rev/b2dca24b1741e6089c7fc1080493e2e230b52cb3

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.

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

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 :)

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?

If we're willing to accept the crash rate I guess so...

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.

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.