WebGL texture-from-video uploads are really slow right now because we lost our gpu-gpu blit path.

Comment on attachment 8887304 [details] Bug 1322746 - Expose DXGI HANDLEs for GPU_VIDEO. - https://reviewboard.mozilla.org/r/158108/#review163930

Looking green for m-gl: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a443a6f77764a74ed7e7875a933c00689785156c

(In reply to Jeff Gilbert [:jgilbert] from comment #5) > Looking green for m-gl: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a443a6f77764a74ed7e7875a933c00689785156c This is the wrong bug.

Comment on attachment 8887305 [details] Bug 1322746 - Add general ID3D11Texture2D to EGLStream support to ANGLE. - https://reviewboard.mozilla.org/r/158110/#review164004 ::: commit-message-6b8a6:4 (Diff revision 1) > +Bug 1322746 - Add general ID3D11Texture2D to EGLStream support to ANGLE. - r=jerry > + > +MozReview-Commit-ID: IYzPAFEc84d > + I don't know why we try to add the rgb consumer type for D3D11NV12 stream object. From https://msdn.microsoft.com/en-us/library/windows/desktop/bb173059(v=vs.85).aspx There is no direct resource view to get the rgb format data from a nv12 d3d texture. And from https://bugzilla.mozilla.org/show_bug.cgi?id=1357299#c37 , could we also have a new set of planar-yuv-420 stream function for DXGIYCbCrTextureHostD3D11? ::: gfx/angle/src/libANGLE/renderer/d3d/d3d11/StreamProducerNV12.cpp:65 (Diff revision 1) > + case DXGI_FORMAT_R8G8B8A8_UNORM: > + ret.internalFormat = GL_RGBA8; > + break; > + > + default: > + *(uint8_t*)0 = 1; How about use the assertion macro in ANGLE? https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/common/debug.h#123 And I think the ANGLE should not call assert or crash for the unsupported format. The original validateD3DNV12Texture() just returns an error code, but we could hit the crash in getGLDescFromTex().

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review164020 ::: gfx/gl/GLBlitHelperD3D.cpp:314 (Diff revision 1) > + const RefPtr<ID3D11Texture2D> texList[3] = { > + OpenSharedTexture(d3d, handleList[0]), > + OpenSharedTexture(d3d, handleList[1]), > + OpenSharedTexture(d3d, handleList[2]) > + }; > + const BindAnglePlanes bindPlanes(this, 3, texList); Does nv12 stream support A8 texture format?

Comment on attachment 8887305 [details] Bug 1322746 - Add general ID3D11Texture2D to EGLStream support to ANGLE. - https://reviewboard.mozilla.org/r/158110/#review164004 > I don't know why we try to add the rgb consumer type for D3D11NV12 stream object. > > From https://msdn.microsoft.com/en-us/library/windows/desktop/bb173059(v=vs.85).aspx > There is no direct resource view to get the rgb format data from a nv12 d3d texture. > > And from https://bugzilla.mozilla.org/show_bug.cgi?id=1357299#c37 , could we also have a new set of planar-yuv-420 stream function for DXGIYCbCrTextureHostD3D11? "An app using the YUY 4:2:0 formats must map the luma (Y) plane separately from the chroma (UV) planes. Developers do this by calling ID3D12Device::CreateShaderResourceView twice for the same texture and passing in 1-channel and 2-channel formats." You can see in the patch we just ask for PLANE_OFFSET=0 and PLANE_OFFSET=1. Because of the switch on DXGI_FORMAT_NV12 with `plane+mPlaneOffset`, we create R8 and RG8 textures respectively. When pulling from NV12 format, this gives us y=>r8 and uv=>rg8. (they aren't actually RGB. ANGLE just uses YUV to mean `planes>1`. (really ==2) We use RGB we just want access to the texture data directly. "just give me the values, I'll handle it myself") > How about use the assertion macro in ANGLE? > https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/common/debug.h#123 > > And I think the ANGLE should not call assert or crash for the unsupported format. > > The original validateD3DNV12Texture() just returns an error code, but we could hit the crash in getGLDescFromTex(). Sorry, this was left in from testing.

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review164020 > Does nv12 stream support A8 texture format? Not right now. Are you sure it needs to? I was seeing R8 textures. It's easy enough to add A8 support.

Comment on attachment 8887305 [details] Bug 1322746 - Add general ID3D11Texture2D to EGLStream support to ANGLE. - https://reviewboard.mozilla.org/r/158110/#review164552 ::: commit-message-6b8a6:4 (Diff revision 2) > +Bug 1322746 - Add general ID3D11Texture2D to EGLStream support to ANGLE. - r=jerry > + > +MozReview-Commit-ID: IYzPAFEc84d > + Please add some comments to describe that we use nv12 stream object with rgb consumer type to adapt the various formats of d3d textures to gl handle. ::: gfx/angle/src/libANGLE/renderer/d3d/d3d11/StreamProducerNV12.cpp:21 (Diff revision 2) > { > > +static egl::Stream::GLTextureDescription getGLDescFromTex(ID3D11Texture2D* tex, > + UINT planeIndex) > +{ > + // The UV plane of NV12 textures has half the width/height of the Y plane Please move this comment into the NV12 section(line 35). ::: gfx/angle/src/libANGLE/renderer/d3d/d3d11/StreamProducerNV12.cpp:36 (Diff revision 2) > + ret.mipLevels = 0; > + > + UINT maxPlaneIndex = 0; > + switch (desc.Format) { > + case DXGI_FORMAT_NV12: > + if (desc.Width < 1 || desc.Height < 1 || I think the size checking(desc.Width < 1 || desc.Height < 1) should be applied to all formats. ::: gfx/angle/src/libANGLE/renderer/d3d/d3d11/StreamProducerNV12.cpp:105 (Diff revision 2) > - { > - return egl::Error(EGL_BAD_PARAMETER, "Texture is of size 0"); > - } > - if ((desc.Width % 2) != 0 || (desc.Height % 2) != 0) > { > - return egl::Error(EGL_BAD_PARAMETER, "Texture dimensions are not even"); > + return egl::Error(EGL_BAD_PARAMETER, "Unsupported texture format or plane"); Could we show more info for the failed reason(e.g. the dimension is not even)?

The attachment 8887306 [details] is a big patch set. I'm still reviewing that.

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review165062 ::: commit-message-f8999:1 (Diff revision 2) > +Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE. - r=jerry How about support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper? ::: gfx/gl/GLBlitHelper.h:23 (Diff revision 2) > +#include <windows.h> > +#endif > + > namespace mozilla { > > namespace layers { Do we miss the "ID3D11Device" forward declaration in this header? ::: gfx/gl/GLBlitHelper.cpp:644 (Diff revision 2) > + > bool > -GLBlitHelper::BlitPlanarYCbCrImage(layers::PlanarYCbCrImage* yuvImage) > +GuessDivisors(const gfx::IntSize& ySize, const gfx::IntSize& uvSize, > + gfx::IntSize* const out_divisors) > +{ > + const uint8_t widthDivisor = (ySize.width == uvSize.width ) ? 1 : 2; Where do we use the width/heightDivisor? ::: gfx/gl/GLBlitHelper.cpp:653 (Diff revision 2) > + if (uvSize.width * divisors.width != ySize.width || > + uvSize.height * divisors.height != ySize.height) > -{ > + { > - ScopedBindTextureUnit boundTU(mGL, LOCAL_GL_TEXTURE0); > - const PlanarYCbCrData* yuvData = yuvImage->GetData(); > + return false; > + } > + *out_divisors = divisors; Please check whether the "out_divisors" is a valid address. ::: gfx/gl/GLBlitHelper.cpp:798 (Diff revision 2) > - mGL->fActiveTexture(LOCAL_GL_TEXTURE0 + i); > - mGL->fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldTex[i]); > + Maybe<YUVColorSpace> colorSpace = Nothing(); > + if (pixelFormat == '420v') { > + type = DrawBlitType::TexRectNV12; > + planes = 2; > + colorSpace = Some( > + } else if (pixelFormat == '2vuy') { For '2vuy', there are two case for this type. Please check https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/gfx/2d/MacIOSurface.cpp#546 . We need two shaders to handle this type correctly. ::: gfx/gl/GLBlitHelperD3D.cpp:16 (Diff revision 2) > +#include "GLContext.h" > +#include "GLLibraryEGL.h" > +#include "GPUVideoImage.h" > +#include "ScopedGLHelpers.h" > + > +#include "../layers/D3D11YCbCrImage.h" https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/gfx/layers/moz.build#72 https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/gfx/layers/moz.build#163 They are already export in "mozilla.layers". I would not prefer to use relative path. ::: gfx/gl/GLBlitHelperD3D.cpp:32 (Diff revision 2) > + const auto& display = egl.Display(); > + const auto stream = egl.fCreateStreamKHR(display, nullptr); > + MOZ_ASSERT(stream); > + if (!stream) > + return 0; > + EGLBoolean ok = 1; Please use EGL_TRUE instead. ::: gfx/gl/GLBlitHelperD3D.cpp:33 (Diff revision 2) > + const auto stream = egl.fCreateStreamKHR(display, nullptr); > + MOZ_ASSERT(stream); > + if (!stream) > + return 0; > + EGLBoolean ok = 1; > + MOZ_ALWAYS_TRUE( ok &= egl.fStreamConsumerGLTextureExternalAttribsNV(display, stream, I think there is no space after the 'MOZ_ALWAYS_TRUE(' in mozilla coding style. ::: gfx/gl/GLBlitHelperD3D.cpp:42 (Diff revision 2) > + MOZ_ALWAYS_TRUE( ok &= egl.fStreamPostD3DTextureNV12ANGLE(display, stream, texD3D, > + postAttribs) ); > + if (ok) > + return stream; > + > + (void)egl.fDestroyStreamKHR(display, stream); What's the purpose for the "(void)"? ::: gfx/gl/GLBlitHelperD3D.cpp:109 (Diff revision 2) > + > + auto& mutex = mMutexList[i]; > + texD3DList[i]->QueryInterface(_uuidof(IDXGIKeyedMutex), > + (void**)getter_AddRefs(mutex)); > + if (mutex) { > + mutex->AcquireSync(0, 100); Please check the return value for AcquireSync(). ::: gfx/gl/GLBlitHelperD3D.cpp:248 (Diff revision 2) > + > + const auto srcOrigin = OriginPos::BottomLeft; > + const gfx::IntRect clipRect(0, 0, clipSize.width, clipSize.height); > + const auto colorSpace = YUVColorSpace::BT601; > + > + if (format != gfx::SurfaceFormat::NV12) { Are we going to replace the eglCreatePbufferFromClientBuffer with eglStream for the regular rgba d3d texture case? ::: gfx/gl/GLBlitHelperD3D.cpp:265 (Diff revision 2) > + const EGLAttrib postAttribs1[] = { > + LOCAL_EGL_NATIVE_BUFFER_PLANE_OFFSET_IMG, 1, > + LOCAL_EGL_NONE > + }; > + const EGLAttrib* const postAttribsList[2] = { postAttribs0, postAttribs1 }; > + // /layers/d3d11/CompositorD3D11.cpp uses bt601 for EffectTypes::NV12. I don't see the BlitAngleNv12() functoin call in CompositorD3D11.cpp, so I don't think this comment here is useful. ::: gfx/gl/GLContext.h:1427 (Diff revision 2) > + } else { > + fDisable(cap); > + } > + } > + > + bool PushEnabled(const GLenum cap, const bool newVal) { I don't have the good name for this function, but this name is not clear for the behavior "get the old value and set with the new one". ::: gfx/gl/GLLibraryEGL.h:295 (Diff revision 2) > > //KHR_stream > EGLStreamKHR fCreateStreamKHR(EGLDisplay dpy, const EGLint* attrib_list) const > WRAP( fCreateStreamKHR(dpy, attrib_list) ) > > - EGLStreamKHR fDestroyStreamKHR(EGLDisplay dpy, EGLStreamKHR stream) const > + EGLBoolean fDestroyStreamKHR(EGLDisplay dpy, EGLStreamKHR stream) const Sorry for the wrong return type for this function. ::: gfx/gl/GLLibraryEGL.cpp:468 (Diff revision 2) > if (mIsANGLE) { > MOZ_ASSERT(IsExtensionSupported(ANGLE_platform_angle_d3d)); > const GLLibraryLoader::SymLoadStruct angleSymbols[] = { > { (PRFuncPtr*)&mSymbols.fANGLEPlatformInitialize, { "ANGLEPlatformInitialize", nullptr } }, > { (PRFuncPtr*)&mSymbols.fANGLEPlatformShutdown, { "ANGLEPlatformShutdown", nullptr } }, > + SYMBOL(QueryDisplayAttribEXT), Should we load the "QueryDisplayAttribEXT" symbol here? It's already loaded at: https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/gfx/gl/GLLibraryEGL.cpp#632 ::: gfx/layers/client/GPUVideoTextureClient.h:53 (Diff revision 2) > RefPtr<dom::VideoDecoderManagerChild> mManager; > SurfaceDescriptorGPUVideo mSD; > gfx::IntSize mSize; > + > +public: > + const decltype(mSD)& SD() const { return mSD; } Why don't we just use the explicit type of mSD here? ::: gfx/layers/ipc/LayersSurfaces.ipdlh:92 (Diff revision 2) > null_t; > }; > > struct SurfaceDescriptorGPUVideo { > uint64_t handle; > - GPUVideoSubDescriptor desc; > + GPUVideoSubDescriptor subdesc; how about use lower camel style here?

Is there a plan to add automated testing for these? It'd be nice if we could detect if we're hitting the fast path (either by measuring perf in talos, or just exposing it somehow) and using that to ensure this doesn't regress again.

Comment on attachment 8888603 [details] Bug 1322746 - Support blit from IOSurfaces. - https://reviewboard.mozilla.org/r/159606/#review165932 ::: gfx/2d/MacIOSurface.h:147 (Diff revision 1) > static size_t GetMaxHeight(); > > private: > friend class nsCARenderer; > - const void* mIOSurfacePtr; > +public: > + const IOSurfacePtr mIOSurfacePtr; Why are you exposing this, and using all the static functions on the library? Can't we just use the member functions on MacIOSurface (like GetPlaneCount, GetWidth etc) and avoid exposing the internals? MacIOSurfaceLib really shouldn't be in the header at all. ::: gfx/gl/GLBlitHelper.cpp:53 (Diff revision 1) > +const char kFragHeader_Tex2DRect[] = "\ > + #define SAMPLER sampler2DRect \n\ > + #if __VERSION__ >= 130 \n\ > + #define TEXTURE texture \n\ > + #else \n\ > + #define TEXTURE texture2DRect \n\ texture2DRect takes coords in (0,width/height) space not (0,1), so we normally need to adjust for this (by multiplying the tex coords by the texture size). I can't see anywhere in this code that does that, so it seems like this path wouldn't work.

(In reply to Matt Woodrow (:mattwoodrow) from comment #18) > Is there a plan to add automated testing for these? > > It'd be nice if we could detect if we're hitting the fast path (either by > measuring perf in talos, or just exposing it somehow) and using that to > ensure this doesn't regress again. We have tests for this. (we took the latter approach)

Comment on attachment 8888603 [details] Bug 1322746 - Support blit from IOSurfaces. - https://reviewboard.mozilla.org/r/159606/#review165932 > Why are you exposing this, and using all the static functions on the library? > > Can't we just use the member functions on MacIOSurface (like GetPlaneCount, GetWidth etc) and avoid exposing the internals? > > MacIOSurfaceLib really shouldn't be in the header at all. MacIOSurface does a bunch of stuff in CGLTexImageIOSurface2D, so we can't use that directly without heavily modifying it. I don't want to hunt down how this changes other callers, so I opted to bypass it and use the API directly. > texture2DRect takes coords in (0,width/height) space not (0,1), so we normally need to adjust for this (by multiplying the tex coords by the texture size). > > I can't see anywhere in this code that does that, so it seems like this path wouldn't work. The trick is that we usually have to divide our clipped pixel coords by the size of the textures before sampling with sampler2Ds. With sampler2DRects, we need to /not/ divide, which is why texRectNormFactor is {1,1}. I should probably change the names of the uTexSize0 vars to uTexCoordDivisor0, or something. This path empirically works, it's just not the easiest to follow. (but hey, coord systems, right?)

(In reply to Jeff Gilbert [:jgilbert] from comment #20) > (In reply to Matt Woodrow (:mattwoodrow) from comment #18) > > Is there a plan to add automated testing for these? > > > > It'd be nice if we could detect if we're hitting the fast path (either by > > measuring perf in talos, or just exposing it somehow) and using that to > > ensure this doesn't regress again. > > We have tests for this. (we took the latter approach) Wonderful! (In reply to Jeff Gilbert [:jgilbert] from comment #21) > > MacIOSurface does a bunch of stuff in CGLTexImageIOSurface2D, so we can't > use that directly without heavily modifying it. I don't want to hunt down > how this changes other callers, so I opted to bypass it and use the API > directly. Indeed it does. It's meant to just know the sizes itself, and it infers the appropriate GL formats from the number of planes etc (same as your code is doing). I would expect all callers to want to the same behaviour here, but I can see how there might be subtle differences or at least a complicated web of things to untangle. How about just adding a second overload for MacIOSurface::CGLTexImageIOSurface2D that allows you to specify the explicit formats you want (shouldn't need the sizes though) so you can override the implicit format picking behaviour of the existing function. I think with that, you should be able to use the existing getters on MacIOSurface as input to your format choosing code, and then just call the new function. I'd much prefer that over accessing MacIOSurfaceLib directly. > > > texture2DRect takes coords in (0,width/height) space not (0,1), so we normally need to adjust for this (by multiplying the tex coords by the texture size). > > > > I can't see anywhere in this code that does that, so it seems like this path wouldn't work. > > The trick is that we usually have to divide our clipped pixel coords by the > size of the textures before sampling with sampler2Ds. With sampler2DRects, > we need to /not/ divide, which is why texRectNormFactor is {1,1}. I should > probably change the names of the uTexSize0 vars to uTexCoordDivisor0, or > something. > > This path empirically works, it's just not the easiest to follow. (but hey, > coord systems, right?) Coord systems indeed. Ok, that works!

This seems to be what passes for green these days: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3a36bfb9676c696330f4b72e91a33bce56e95bb

Comment on attachment 8900155 [details] Bug 1322746 - dom/base/test/test_anonymousContent_canvas.html should not assume webgl works. - https://reviewboard.mozilla.org/r/171546/#review176690 r=me

Here's the branch: https://github.com/jdashg/gecko-cinn/commits/_bt_gpu-video-desc Green so far on try-all: https://treeherder.mozilla.org/#/jobs?repo=try&revision=028e9fabba07b8a44629796ce43883f90b814ffe Here's the android fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad4df655410e8a099d83814d00dd6ec87367c2d7

jerry, mattwoodrow: Are there any concerns here strictly blocking r+? I'd prefer to land it in its current state and deal with the remaining nits with fix-up bugs, given the trouble I've had getting the tests green with these and related changes. We're trying to ensure this hits 57, and I'm on PTO for the next week.

I will review this patch soon tomorrow. Sorry for the blocking.

I'd really prefer if you address the comments around accessing private members of MacIOSurface before landing. They shouldn't be functional changes, so should be easy to do without regressing the try run.

Comment on attachment 8900115 [details] Bug 1322746 - Disable webgl reftest on Android. - https://reviewboard.mozilla.org/r/171492/#review177240

(In reply to Matt Woodrow (:mattwoodrow) from comment #37) > I'd really prefer if you address the comments around accessing private > members of MacIOSurface before landing. They shouldn't be functional > changes, so should be easy to do without regressing the try run. If they aren't functional changes, grant r+ so this doesn't risk missing 57. I appreciate your concerns, and will address them, but I don't want to risk it at this point. I have been burned too many times by this. We need to land this sooner rather than later. Try runs have not been enough to ensure code doesn't bounce off inbound, which can add days to the troubleshooting process. We don't have that many days before we shouldn't be landing large or risky things anymore. I cannot myself address your concerns until the 30th due to PTO, but it's easy enough after I get back.

pchang: Please find someone to push this forward while I'm on PTO.

Comment on attachment 8900114 [details] Bug 1322746 - Fix android blitting. - https://reviewboard.mozilla.org/r/171490/#review177296 LGTM

Comment on attachment 8900112 [details] Bug 1322746 - Add extern decls for DrawBlitProg::Key. - https://reviewboard.mozilla.org/r/171486/#review177300 r=me, please make sure you wanna internalFBs to be false after removing the last argument of BlitTextureToFramebuffer().

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review177204 ::: gfx/gl/GLBlitHelper.cpp:191 (Diff revision 3) > + ScopedDrawBlitState(GLContext* const gl, const gfx::IntSize& destSize) > + : mGL(*gl) > + , blend (mGL.PushEnabled(LOCAL_GL_BLEND, false)) > + , cullFace (mGL.PushEnabled(LOCAL_GL_CULL_FACE, false)) > + , depthTest (mGL.PushEnabled(LOCAL_GL_DEPTH_TEST, false)) > + , dither (mGL.PushEnabled(LOCAL_GL_DITHER, true)) Why we need to enable dithering here? The original code doesn't enable this. ::: gfx/gl/GLBlitHelper.cpp:597 (Diff revision 3) > bool > -GLBlitHelper::BlitSurfaceTextureImage(layers::SurfaceTextureImage* stImage) > +GLBlitHelper::BlitImage(layers::SurfaceTextureImage* srcImage) > { > // FIXME > + const auto& srcOrigin = srcImage->GetOriginPos(); > + (void)srcOrigin; If you are try to skip the unused warning, please use "mozilla/Unused.h". ::: gfx/gl/GLBlitHelper.cpp:645 (Diff revision 3) > +bool > +GuessDivisors(const gfx::IntSize& ySize, const gfx::IntSize& uvSize, > + gfx::IntSize* const out_divisors) > +{ > + const uint8_t widthDivisor = (ySize.width == uvSize.width ) ? 1 : 2; > + const uint8_t heightDivisor = (ySize.height == uvSize.height) ? 1 : 2; Again, the widthDivisor and heightDivisor is not used. Why we leave them here. ::: gfx/gl/GLBlitHelper.cpp:798 (Diff revision 3) > - mGL->fActiveTexture(LOCAL_GL_TEXTURE0 + i); > - mGL->fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldTex[i]); > + Maybe<YUVColorSpace> colorSpace = Nothing(); > + if (pixelFormat == '420v') { > + type = DrawBlitType::TexRectNV12; > + planes = 2; > + colorSpace = Some( > + } else if (pixelFormat == '2vuy') { With webrender, we will not use gl compatibility profile. Then, we can't use the TexRectRGB shader here. Please check https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/gfx/2d/MacIOSurface.cpp#546 . You should handle that here. Or you need to fix that in anoter bug. ::: gfx/gl/GLBlitHelper.cpp:813 (Diff revision 3) > > - mGL->fActiveTexture(LOCAL_GL_TEXTURE0); > - mGL->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE_ARB, textures[0]); > - mGL->fTexParameteri(LOCAL_GL_TEXTURE_RECTANGLE_ARB, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE); > - mGL->fTexParameteri(LOCAL_GL_TEXTURE_RECTANGLE_ARB, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE); > - surf->CGLTexImageIOSurface2D(mGL, > + if (!mIOSurfaceTexs[0]) { > + mGL->fGenTextures(2, &mIOSurfaceTexs); > + const ScopedBindTexture bindTex(mGL, mIOSurfaceTexs[0], LOCAL_GL_TEXTURE_RECTANGLE); > + mGL->TexParams_SetClampNoMips(LOCAL_GL_TEXTURE_RECTANGLE); > + mGL->fBindTexture(LOCAL_GL_TEXTURE_RECTANGLE, mIOSurfaceTexs[1]); Why don't we use ScopedBindTexture for mIOSurfaceTexs[1]? ::: gfx/gl/GLBlitHelperD3D.cpp:42 (Diff revision 3) > + MOZ_ALWAYS_TRUE( ok &= egl.fStreamPostD3DTextureNV12ANGLE(display, stream, texD3D, > + postAttribs) ); > + if (ok) > + return stream; > + > + (void)egl.fDestroyStreamKHR(display, stream); Could you describe why we have a "(void)" here? ::: gfx/gl/GLContext.h:1430 (Diff revision 3) > + } > + } > + > + bool PushEnabled(const GLenum cap, const bool newVal) { > + const auto oldVal = fIsEnabled(cap); > + SetEnabled(cap, newVal); If the new value is equal to the old one, we could skip the setting. ::: gfx/layers/D3D11YCbCrImage.h:53 (Diff revision 3) > Maybe<gfx::IntSize> mCbCrSize; > }; > > class D3D11YCbCrImage : public Image > { > + friend class gl::GLBlitHelper; If you make GLBlitHelper be a friend of D3D11YCbCrImage, the function GetData() could be private. Expose the internal data is not a good ieda. The GPUVideoImage has the same problem. ::: gfx/layers/client/GPUVideoTextureClient.h:53 (Diff revision 3) > RefPtr<dom::VideoDecoderManagerChild> mManager; > SurfaceDescriptorGPUVideo mSD; > gfx::IntSize mSize; > + > +public: > + const decltype(mSD)& SD() const { return mSD; } I still prefer to use SurfaceDescriptorGPUVideo type instead of decltype(mSD).

I have followed some review comments to fix the patches. I don't want to take-over it because I don't hope to do the review process again.

(In reply to Jeff Gilbert [:jgilbert] from comment #39) > (In reply to Matt Woodrow (:mattwoodrow) from comment #37) > > I'd really prefer if you address the comments around accessing private > > members of MacIOSurface before landing. They shouldn't be functional > > changes, so should be easy to do without regressing the try run. > > If they aren't functional changes, grant r+ so this doesn't risk missing 57. > I appreciate your concerns, and will address them, but I don't want to risk > it at this point. I have been burned too many times by this. > > We need to land this sooner rather than later. Try runs have not been enough > to ensure code doesn't bounce off inbound, which can add days to the > troubleshooting process. We don't have that many days before we shouldn't be > landing large or risky things anymore. > > I cannot myself address your concerns until the 30th due to PTO, but it's > easy enough after I get back. Actually, we have MacIOSurface::GetIOSurfacePtr() already. After rebasing the source code from m-c, it would solve the concern from :mattwoodrow.

(In reply to Daosheng Mu[:daoshengmu] from comment #54) > Actually, we have MacIOSurface::GetIOSurfacePtr() already. After rebasing > the source code from m-c, it would solve the concern from :mattwoodrow. That was added for VR, since they need to pass the internal object to another API. For this, the code is just extracting the internal object, and then duplicating all the functionality of MacIOSurface using the library. I guess it's ok to land like this, but please file an assigned follow-up bug for fixing it.

:jgilbert Please consider to apply this patch for the follow-up works. I still have some issues need to be fixed as below: Part 2: 1) Fix the commit message 2) Show more info for the failed reason - I am considering to drop it. Part 3: 1) Commit msg 2) A8 format support. - Probably, we can open an another bug and drop it. 3) ID3D11Device" forward declaration - We can drop it. I have confirmed we don't need it. 4) EGL_TRUE - We can't see EGL_TRUE in GLBlitHelperD3D.cpp... 5) Load QueryDisplayAttribEXT. - I am not sure if we should keep it. 6) Could you describe why we have a "(void)" here? - I don't know why you have (void) either. 7) gl compatibility profile support - Probably, we can open a follow-up bug to work on it. Part 4: As https://bugzilla.mozilla.org/show_bug.cgi?id=1322746#c54, matt seems to agree to use MacIOSurface::GetIOSurfacePtr(). But, we still need to open a follow-up bug to fix it.

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b5e0c86e8c053a8a0f06738817b9f6245744ba

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review177204 > Why we need to enable dithering here? The original code doesn't enable this. We should enable dithering in case our source and dest formats don't have the same element types/sizes. > Why don't we use ScopedBindTexture for mIOSurfaceTexs[1]? ScopedBindTexture foo(gl, texName) is just a way to abbreviate ScopedBindTexture+fBindTexture. If we already have a ScopedBindTexture in scope, we should just use fBindTexture, not another ScopedBindTexture. > If the new value is equal to the old one, we could skip the setting. Cast-to-void is the common way to mark a statement's result as explicitly unused.

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review165062 > Do we miss the "ID3D11Device" forward declaration in this header? That type isn't used in this file. > Where do we use the width/heightDivisor? I accidentally left them in! > Please check whether the "out_divisors" is a valid address. The function call is useless without supplying a valid address for the out-var. Adding an assert here would be overly paranoid. > For '2vuy', there are two case for this type. > Please check https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/gfx/2d/MacIOSurface.cpp#546 . > We need two shaders to handle this type correctly. I tested both core profile and legacy profile, and the code here works. Since we currently suspect the code you link to is incorrect, I don't think we should cargo-cult its logic here. > I think there is no space after the 'MOZ_ALWAYS_TRUE(' in mozilla coding style. I think it helps readability here and elsewhere. It's perfectly acceptable to add whitespace to improve readability of expressions, particularly to disambiguate nested parentheses. > What's the purpose for the "(void)"? It's the common c/c++ way to explicitly ignore a return value. > Please check the return value for AcquireSync(). What do you recommend happens if it fails? > Are we going to replace the eglCreatePbufferFromClientBuffer with eglStream for the regular rgba d3d texture case? Yes. > I don't see the BlitAngleNv12() functoin call in CompositorD3D11.cpp, so I don't think this comment here is useful. It's not called from there, but this comment states where we get our assumption here that NV12 is always bt601, not bt709. Later, when we're curious why we're always treading nv12 as bt601, it will tell us where we got that assumption. > I don't have the good name for this function, but this name is not clear for the behavior "get the old value and set with the new one". It's clear enough from the code, though. > Why don't we just use the explicit type of mSD here? I try to avoid copy-pasting where I can! Trivial getters are all of this form, and restating the type here is redundant. > how about use lower camel style here? `subdesc` is lower-camel-case already. 'Subdescription' is one word. It looks like rather my previous patch was incorrect, and should be `GPUVideoSubdescriptor`.

Comment on attachment 8887306 [details] Bug 1322746 - Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - https://reviewboard.mozilla.org/r/158112/#review177204 > If you are try to skip the unused warning, please use "mozilla/Unused.h". Unused is probably going away soon, since I don't see that it's needed anymore. I certainly don't see a reason to continue that code-smell here, without a reason to use it over the standard C++ approach. > With webrender, we will not use gl compatibility profile. Then, we can't use the TexRectRGB shader here. Please check https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/gfx/2d/MacIOSurface.cpp#546 . > > You should handle that here. Or you need to fix that in anoter bug. We don't use compat profiles at all in WebGL+Mac anymore. > Could you describe why we have a "(void)" here? https://stackoverflow.com/questions/689677/why-cast-unused-return-values-to-void > Cast-to-void is the common way to mark a statement's result as explicitly unused. Well, we could, but we always reset it anyway. I don't mind, though I slightly prefer the simplicity of it as-is. > I still prefer to use SurfaceDescriptorGPUVideo type instead of decltype(mSD). const decltype(mFoo)& Foo() const { return mFoo; } is the standard form of a getter. Being explicit about the type isn't necessary.

(In reply to Jeff Gilbert [:jgilbert] from comment #60) > > I still prefer to use SurfaceDescriptorGPUVideo type instead of decltype(mSD). > > const decltype(mFoo)& Foo() const { return mFoo; } is the standard form of a > getter. Being explicit about the type isn't necessary. As far as I can tell we're not using this style anywhere else in mozilla code, so please use the full declaration for gfx code.

Comment on attachment 8888603 [details] Bug 1322746 - Support blit from IOSurfaces. - https://reviewboard.mozilla.org/r/159606/#review179702 As above, r=me as long as there is an assigned follow-up bug to stop accessing the IOSurface library functions directly.

Comment on attachment 8903368 [details] Bug 1322746 - No fast uploads for x/y/zOffset!=0 yet. - https://reviewboard.mozilla.org/r/175172/#review180246 LGTM

Comment on attachment 8903408 [details] Bug 1322746 - Remove video->canvas2d fastpath for SkiaGL. - https://reviewboard.mozilla.org/r/175256/#review180260

Comment on attachment 8903370 [details] Bug 1322746 - SkiaGL should ask for a blit to OriginPos::BottomLeft. - https://reviewboard.mozilla.org/r/175176/#review180262

Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf739e7c76b8 Expose DXGI HANDLEs for GPU_VIDEO. - r=mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/b202e9377e24 Add general ID3D11Texture2D to EGLStream support to ANGLE. - r=jerry https://hg.mozilla.org/integration/mozilla-inbound/rev/61c7478af98c Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - r=jerry https://hg.mozilla.org/integration/mozilla-inbound/rev/a4901ec3c327 Support blit from IOSurfaces. - r=mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8352ffd2ca Add extern decls for DrawBlitProg::Key. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/b97bb7a2e555 Explicitly reject D3D9_RGB32_TEXTURE for fast blitting. https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1dd5775850 Fix android blitting. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed5b75a6f5e Disable webgl reftest on Android. - r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/87e7f25a089c dom/base/test/test_anonymousContent_canvas.html should not assume webgl works. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/882cd05b7064 Mark mp4->webgl as fast on Mac. https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bd6bbf5653 No fast uploads for x/y/zOffset!=0 yet. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/88a28c4ebc6b Add common func addLoadEvent to mochi-to-testcase.py. https://hg.mozilla.org/integration/mozilla-inbound/rev/992b2173bda7 SkiaGL should ask for a blit to OriginPos::BottomLeft. - r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/a10fcb139377 Remove video->canvas2d fastpath for SkiaGL. - r=lsalzman

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3e55e4de032f for four unexpected passes on Win8, https://treeherder.mozilla.org/logviewer.html#?job_id=127956182&repo=mozilla-inbound

Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/999778d76314 Expose DXGI HANDLEs for GPU_VIDEO. - r=mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/42a578d05c23 Add general ID3D11Texture2D to EGLStream support to ANGLE. - r=jerry https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7ed67195b4 Support PLANAR_YCBCR, GPU_VIDEO, and D3D11_YCBCR_IMAGE in GLBlitHelper. - r=jerry https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1214b63207 Support blit from IOSurfaces. - r=mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/2c1d5bec1bd0 Add extern decls for DrawBlitProg::Key. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/f93a58285b95 Explicitly reject D3D9_RGB32_TEXTURE for fast blitting. https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0e9ad88c9c Fix android blitting. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee5aa09d486 Disable webgl reftest on Android. - r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/64f3957a47a6 dom/base/test/test_anonymousContent_canvas.html should not assume webgl works. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/7715e44f38ef Mark mp4->webgl as fast on Mac. https://hg.mozilla.org/integration/mozilla-inbound/rev/1b61f5cdc350 No fast uploads for x/y/zOffset!=0 yet. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb37ec59e76 Add common func addLoadEvent to mochi-to-testcase.py. https://hg.mozilla.org/integration/mozilla-inbound/rev/2c1bcee7784b SkiaGL should ask for a blit to OriginPos::BottomLeft. - r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ca494f6c5a Remove video->canvas2d fastpath for SkiaGL. - r=lsalzman https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9d1811487a Mark windows as passing video fast-upload tests.

https://hg.mozilla.org/mozilla-central/rev/999778d76314 https://hg.mozilla.org/mozilla-central/rev/42a578d05c23 https://hg.mozilla.org/mozilla-central/rev/0a7ed67195b4 https://hg.mozilla.org/mozilla-central/rev/0c1214b63207 https://hg.mozilla.org/mozilla-central/rev/2c1d5bec1bd0 https://hg.mozilla.org/mozilla-central/rev/f93a58285b95 https://hg.mozilla.org/mozilla-central/rev/9e0e9ad88c9c https://hg.mozilla.org/mozilla-central/rev/9ee5aa09d486 https://hg.mozilla.org/mozilla-central/rev/64f3957a47a6 https://hg.mozilla.org/mozilla-central/rev/7715e44f38ef https://hg.mozilla.org/mozilla-central/rev/1b61f5cdc350 https://hg.mozilla.org/mozilla-central/rev/ebb37ec59e76 https://hg.mozilla.org/mozilla-central/rev/2c1bcee7784b https://hg.mozilla.org/mozilla-central/rev/a2ca494f6c5a https://hg.mozilla.org/mozilla-central/rev/fc9d1811487a

:jgilbert this new assertion is being triggered in totally unrelated tests (and incidentally cause my changes to be backed out) This is an intermittent failure. https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=de401d17868d6fb35b71dc77879331b530c80bd3 what's up with that?

I think your changes accidentally open up a new gpu-decode path that was previously software decoded. The version of android emulator that we use doesn't seem to have the mentioned GLES extension. I can add support back in for this, but let's do that in a new bug.

It's *extremely* unlikely. Those changes were about changing the prototyping of a debugging function from const char* to nsCString If this bug causes failures to prevent new media code to get in, it's going to be an issue very quickly.

(In reply to Jean-Yves Avenard [:jya] from comment #106) > It's *extremely* unlikely. Those changes were about changing the prototyping > of a debugging function from const char* to nsCString > > If this bug causes failures to prevent new media code to get in, it's going > to be an issue very quickly. That's what that crash stack tells me. It doesn't seem to be happening without your changes. I'll add a fallback this week, but it seems to be something in your changes that's causing the execution path to change. If that's unexpected, it's probably worth taking a look at why. Sorry to be the bearer of bad news!

It certainly looked like jya's patch made it frequent, but it's still there without his patch, bug 1396704

This brought some big improvements on our glvideo test: == Change summary for alert #9189 (as of September 03 2017 04:53 UTC) == Improvements: 83% glvideo Mean tick time across 100 ticks: windows10-64 pgo e10s 30.45 -> 5.16 83% glvideo Mean tick time across 100 ticks: windows10-64 opt e10s 30.40 -> 5.18 75% glvideo Mean tick time across 100 ticks: osx-10-10 opt e10s 17.37 -> 4.41 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9189

The ANGLE changeset in this bug has been upstreamed to Google. https://github.com/google/angle/commit/3dddccff31be90fe090a65568e1da61bd8b05402