Support NV12 format DXGITextureHostD3D11 with Webrender

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(15 attachments, 13 obsolete attachments)

5.44 KB, patch
jerry
: review+
Details | Diff | Splinter Review
3.94 KB, patch
nical
: review+
Details | Diff | Splinter Review
1.55 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.71 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.92 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
16.18 KB, patch
nical
: review+
Details | Diff | Splinter Review
10.46 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
1.13 KB, patch
jerry
: review+
Details | Diff | Splinter Review
16.93 KB, patch
mattwoodrow
: review+
dvander
: review+
Details | Diff | Splinter Review
11.64 KB, patch
mattwoodrow
: review+
dvander
: review+
Details | Diff | Splinter Review
16.93 KB, patch
mattwoodrow
: review+
dvander
: review+
Details | Diff | Splinter Review
10.69 KB, patch
jerry
: review+
Details | Diff | Splinter Review
2.59 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
6.95 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.05 KB, patch
dvander
: review+
Details | Diff | Splinter Review
Some HW decoders use NV12 format. This bug will implement NV12 format in WR. NV12 is not supported directly in GL/GLES. Gecko uses ANGLE(it's GLES) at windows platform.

Here is the current NV12 usage with ANGLE:
https://chromium.googlesource.com/angle/angle/+/bda7559731631686cd8d59f91ee212c2cec644ac/src/tests/egl_tests/EGLStreamTest.cpp#357

Here are the WR issues:
Support NV12 texture format
https://github.com/servo/webrender/issues/1131
Add gl::TEXTURE_EXTERNAL_OES support in WR
https://github.com/servo/webrender/issues/1130
Depends on: 1366512
https://github.com/servo/webrender/issues/1131
https://github.com/servo/webrender/issues/1130

All WR dependence is ready now. I will send the gecko part for reviewing later.
Depends on: 1372489
Depends on: 1372803
MozReview-Commit-ID: 3lZG2GrxXHF
Attachment #8880333 - Flags: review?(jgilbert)
MozReview-Commit-ID: F4mPCALj1OY
If we use WebRender, there is no ShadowForwarder with this configuration. So, use the AsKnowsCompositor() instead.

MozReview-Commit-ID: 7skHx7SxMuR
MozReview-Commit-ID: EOOp0Dzenub
These is the WIP patch.

The converted gl texture from dx11 nv12 texture is always green in this patch. I'm checking this problem.
Bad green and nv12 is a combination we see outside of webrender.  Perhaps unrelated, but just in case, :mstange can fill in the details.
Flags: needinfo?(mstange)
Bug 1352016 is about a green border showing up with nv12 on some machines. That's really all I know about it.
Flags: needinfo?(mstange)
Comment on attachment 8880333 [details] [diff] [review]
P1: Fix the wrong ext string for QueryDisplayAttribEXT. r=jgilbert

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

::: gfx/gl/GLLibraryEGL.h
@@ +466,4 @@
>          EGLBoolean (GLAPIENTRY * fQueryDisplayAttribEXT)(EGLDisplay dpy,
>                                                           EGLint attribute,
>                                                           EGLAttrib* value);
> +

Remove this empty newline, now that they're from the same ext.
Attachment #8880333 - Flags: review?(jgilbert) → review+
In my experiment, if we create a texture with keyed-mutex and then copy the decoder's output into that texture, WR will always get a green result. If we use non-keyed-mutex texture, I could see the normal video in WR. But in this case, the video is flickering.
 
Currently, we don't handle the directx texture synchronization in WR. I'm going to have that one.
ANGLE may not support keyed-mutexs for this API, but since we're giving ANGLE the texture directly, we should be able to acquire and release the mutex outside of ANGLE.

Though since these are all write-once textures, there's not much point to using keyed-mutexes.
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #10)
> If we use non-keyed-mutex texture, I could see the normal video in WR. But in this
> case, the video is flickering.

It might be better to check what will happen when recycling of texture is disabled.
Blocks: 1380979
Attachment #8880333 - Attachment is obsolete: true
All WR texture related codes are move into GetWRImageKeys(), AddWRImage() and PushExternalImage().
Each texture type could generate its WR commands individually. So, this "mIsWrappingNativeHandle" flag is not used anymore.

MozReview-Commit-ID: 1TITkGRemAr
Attachment #8886753 - Flags: review?(nical.bugzilla)
Attachment #8880334 - Attachment is obsolete: true
Create the corresponding RenderTextureHost type and WR commands for DXGI texture type.

The DXGITextureHostD3D11 will use 1 or 2 image keys for non-nv12 and nv12 texture format.

The DXGIYCbCrTextureHostD3D11 is a special case. The WR uses ANGLE in windows platform,
but the ANGLE doesn't support A8 format directx texture. So, we use libyuv to convert the
DXGIYCbCrTextureHostD3D11 texture buffer into RGBA format buffer and use WR::AddImage()
for that image.

The whole RenderD3D11TextureHostOGL implementation is in the next patch.

MozReview-Commit-ID: F4mPCALj1OY
Attachment #8886754 - Flags: review?(nical.bugzilla)
Attachment #8880335 - Attachment is obsolete: true
MozReview-Commit-ID: F4mPCALj1OY
Attachment #8886755 - Flags: review?(jgilbert)
Attachment #8880336 - Attachment is obsolete: true
If we use WebRender, there is no ShadowForwarder with this configuration. So, use the AsKnowsCompositor() instead.

MozReview-Commit-ID: 7skHx7SxMuR
Attachment #8886756 - Flags: review?(nical.bugzilla)
MozReview-Commit-ID: EOOp0Dzenub
Attachment #8886757 - Flags: review?(matt.woodrow)
MozReview-Commit-ID: GSUxyWUfBVt
Attachment #8886758 - Flags: review?(matt.woodrow)
Check the buffer appending status for the video sample object.
Check for the IMFTransform output status.

MozReview-Commit-ID: J0bn6NB7gi0
Attachment #8886759 - Flags: review?(matt.woodrow)
MLGDeviceD3D11, CompositorD3D11 and TextureClient use the same synchronization mechanism.
Create the new RendererSyncObject and TextureSyncObject type for reusing code.

Add SyncObject.cpp/h and create two new data types: TextureSyncObject and RendererSyncObject.
The TextureSyncObject is used for client side. The RendererSyncObject is used for renderer such
as MLGDeviceD3D11 and CompositorD3D11.

MozReview-Commit-ID: 3l56WK1aZ15
Attachment #8886760 - Flags: review?(matt.woodrow)
Attachment #8886760 - Flags: review?(dvander)
MozReview-Commit-ID: 1a0Ho7smkAx
Attachment #8886761 - Flags: review?(matt.woodrow)
Attachment #8886761 - Flags: review?(dvander)
MozReview-Commit-ID: 4HTPz0YcYHq
Attachment #8886762 - Flags: review?(matt.woodrow)
Attachment #8886762 - Flags: review?(dvander)
With DXVA2 hardware-video-decoding, the RendererOGL should have a
synchronization mechanism to prevent the flickering of video texture.
Create a SyncObject in RendererOGL to do the texture synchronization.

The WebRenderAPI also exposes the RendererOGL's SyncHandle to
WebRenderBridgeParent. Then, the WebRenderBridgeParent could pass this SyncHandle
to the video decoding module for texture synchronization.

MozReview-Commit-ID: toQ2mO5fzG
Attachment #8886763 - Flags: review?(nical.bugzilla)
With P1-P12, I could play the h.264 video with webrender.

Please check the following pref:
"media.hardware-video-decoding.failed"
=> This pref should be true.

"media.windows-media-foundation.use-nv12-format"
=> This pref could be true or false. If we set false, the decoder will use Media Foundation to convert the nv12 format to rgba format. The WR could handle both nv12 or rgba texture format. 

"media.windows-media-foundation.use-sync-texture"
=> This pref should be true.

Here is my test video:
https://www.youtube.com/watch?v=nH34gErxr6s

In this patch set, I create a syncObject in RendererOGL and pass it to the decoder side. So, the d3d texture is not created with keyed-mutex. And I also use that syncObject as what we do in CompositorD3D11 to prevent the flickering for video texture.
Comment on attachment 8886757 [details] [diff] [review]
P6: Turn on DXVA with LAYERS_WR backend.

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

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp
@@ +451,5 @@
>      return false;
>    }
>    MOZ_ASSERT(!mDXVA2Manager);
>    LayersBackend backend = GetCompositorBackendType(mKnowsCompositor);
> +  if (backend != LayersBackend::LAYERS_D3D11 && backend != LayersBackend::LAYERS_WR) {

So we're guaranteeing here that WR will always be able to efficiently read from a D3D11 texture?

Do we ever run native OGL with WR? Or software in the future?
Attachment #8886758 - Flags: review?(matt.woodrow) → review+
Attachment #8886759 - Flags: review?(matt.woodrow) → review+
Turn on DXVA for WebRender-ANGLE backend.
Attachment #8886907 - Flags: review?(matt.woodrow)
Attachment #8886757 - Attachment is obsolete: true
Attachment #8886757 - Flags: review?(matt.woodrow)
Comment on attachment 8886753 [details] [diff] [review]
P2: Remove the unused IsWrappingNativeHandle() function in WebRenderTextureHost.

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

Nice!
Attachment #8886753 - Flags: review?(nical.bugzilla) → review+
Attachment #8886756 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8886763 [details] [diff] [review]
P12: Add SyncObject in RendererOGL.

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

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +172,5 @@
>    if (!wrApi) {
>      return nullptr;
>    }
>  
> +  return RefPtr<WebRenderAPI>(new WebRenderAPI(wrApi, id, maxTextureSize, useANGLE, syncHandle)).forget();

nit: the code would look less surprising if it used MakeAndAddRef<WebRenderAPI>(...)
Attachment #8886763 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8886754 [details] [diff] [review]
P3: Support DXGI texture type for WR.

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

I don't think that we can afford to do yuv->rgb conversion on the CPU (in places where we currently don't). My understanding is that the majority of Windows users get this format for big sites like youtube, and the extra cost of converting each frame of the CPU would be too big a regression. I am sure that there is a way somehow to avoid this cost. I wonder how chromium deals with this.
Comment on attachment 8886755 [details] [diff] [review]
P4: Get the gl handle from d3d shared texture handle.

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

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.cpp
@@ +16,5 @@
>  RenderDXGITextureHostOGL::RenderDXGITextureHostOGL(WindowsHandle aHandle,
>                                                     gfx::SurfaceFormat aFormat,
>                                                     gfx::IntSize aSize)
> +  : mHandle(aHandle)
> +  , mTextureHandle{ 0, 0 }

{0} will fill with zeros without needing to set each one like {0,0}.

@@ +68,5 @@
> +    };
> +    mSurface = egl->fCreatePbufferFromClientBuffer(egl->Display(),
> +                                                   LOCAL_EGL_D3D_TEXTURE_2D_SHARE_HANDLE_ANGLE,
> +                                                   reinterpret_cast<EGLClientBuffer>(mHandle),
> +                                                   gl::GLContextEGL::Cast(mGL.get())->mConfig,

This config is *probably* fine. Theoretically you want a config that matches the format of your d3d texture. I don't know as ANGLE checks though.
You should either assert mSurface or return false if mSurface is 0.

@@ +73,5 @@
> +                                                   pbufferAttributes);
> +
> +    // Query the keyed-mutex.
> +    void* keyedMutex = nullptr;
> +    egl->fQuerySurfacePointerANGLE(egl->Display(), mSurface, LOCAL_EGL_DXGI_KEYED_MUTEX_ANGLE, &keyedMutex);

s/&keyedMutex/(void**)getter_AddRefs(mKeyedMutex)/, unless you have a reason not to use that directly.

@@ +76,5 @@
> +    void* keyedMutex = nullptr;
> +    egl->fQuerySurfacePointerANGLE(egl->Display(), mSurface, LOCAL_EGL_DXGI_KEYED_MUTEX_ANGLE, &keyedMutex);
> +    mKeyedMutex = static_cast<IDXGIKeyedMutex*>(keyedMutex);
> +    if (mKeyedMutex) {
> +      mKeyedMutex->AcquireSync(0, 10000);

Do you really want to wait for 10s? This should probably be 100ms max, and assert if it fails.

@@ +85,5 @@
> +    mGL->fBindTexture(LOCAL_GL_TEXTURE_2D, mTextureHandle[0]);
> +    mGL->fTexParameterf(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE);
> +    mGL->fTexParameterf(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE);
> +    mGL->fTexParameterf(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_NEAREST);
> +    mGL->fTexParameterf(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_NEAREST);

You can just use mGL->TexParams_SetClampNoMips().

@@ +98,5 @@
> +    egl->fQueryDisplayAttribEXT(egl->Display(), LOCAL_EGL_DEVICE_EXT, (EGLAttrib*)&eglDevice);
> +    MOZ_ASSERT(eglDevice);
> +    ID3D11Device* device = nullptr;
> +    egl->fQueryDeviceAttribEXT(eglDevice, LOCAL_EGL_D3D11_DEVICE_ANGLE, (EGLAttrib*)&device);
> +    MOZ_ASSERT(device);

MOZ_RELEASE_ASSERT here. There's a chance this might fail if we end up on d3d9 angle for some reason.

@@ +109,5 @@
> +      return false;
> +    }
> +
> +    // Create the EGLStream.
> +    const EGLint streamAttributes[] = {

I don't think ANGLE supports these really. Just pass nullptr.

@@ +136,5 @@
> +    mGL->fGenTextures(2, mTextureHandle);
> +    mGL->fActiveTexture(LOCAL_GL_TEXTURE0);
> +    mGL->fBindTexture(LOCAL_GL_TEXTURE_EXTERNAL_OES, mTextureHandle[0]);
> +    mGL->fTexParameteri(LOCAL_GL_TEXTURE_EXTERNAL_OES, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
> +    mGL->fActiveTexture(LOCAL_GL_TEXTURE1);

You probably don't want to leave TEXTURE1 selected instead of TEXTURE0 on exiting this function.

@@ +144,5 @@
> +    MOZ_ASSERT(eglResult);
> +    EGLAttrib producerAttributes[] = {
> +        LOCAL_EGL_NONE,
> +    };
> +    eglResult = egl->fCreateStreamProducerD3DTextureNV12ANGLE(egl->Display(), mStream, producerAttributes);

s/producerAttributes/nullptr/

@@ +149,5 @@
> +    MOZ_ASSERT(eglResult);
> +
> +    // Insert the NV12 texture.
> +    EGLAttrib frameAttributes[] = {
> +        LOCAL_EGL_D3D_TEXTURE_SUBRESOURCE_ID_ANGLE, 0,

IIRC, this isn't necessary. Just pass nullptr to StreamPost.

@@ +153,5 @@
> +        LOCAL_EGL_D3D_TEXTURE_SUBRESOURCE_ID_ANGLE, 0,
> +        LOCAL_EGL_NONE,
> +    };
> +    eglResult = egl->fStreamPostD3DTextureNV12ANGLE(egl->Display(), mStream, (void*)mTexture.get(), frameAttributes);
> +    MOZ_ASSERT(eglResult);

Instead of setting eglResult and calling MOZ_ASSERT(eglResult) separately, use:
MOZ_ALWAYS_TRUE( egl->fStreamPost...(...) );

@@ +158,5 @@
> +
> +    // Now, we could check the stream status and use the NV12 gl handle.
> +    EGLint state;
> +    egl->fQueryStreamKHR(egl->Display(), mStream, LOCAL_EGL_STREAM_STATE_KHR, &state);
> +    MOZ_ASSERT(state == LOCAL_EGL_STREAM_STATE_NEW_FRAME_AVAILABLE_KHR);

I don't think ANGLE supports this not being true. Regardless, only do this check on DEBUG builds.

@@ +182,5 @@
> +{
> +  if (mTextureHandle[0] != 0 && mGL && mGL->MakeCurrent()) {
> +    mGL->fDeleteTextures(2, mTextureHandle);
> +    for(int i = 0; i < 2; ++i) {
> +      mTextureHandle[i] = 0;

You should zero mTextureHandle even if MakeCurrent fails.

@@ +187,5 @@
> +    }
> +
> +    gl::GLLibraryEGL* egl = &gl::sEGLLibrary;
> +    if (mSurface) {
> +      egl->fDestroySurface(egl->Display(), mSurface);

fDestroySurface doesn't care about MakeCurrent. We should always attempt to destroy the surface.

@@ +192,5 @@
> +      mSurface = 0;
> +    }
> +
> +    if (mStream) {
> +      egl->fStreamConsumerReleaseKHR(egl->Display(), mStream);

fStreamConsumerReleaseKHR also should be run even if !MakeCurrent.

You need to destroy the stream, as well.
Attachment #8886755 - Flags: review?(jgilbert) → review-
Comment on attachment 8886763 [details] [diff] [review]
P12: Add SyncObject in RendererOGL.

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

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +172,5 @@
>    if (!wrApi) {
>      return nullptr;
>    }
>  
> +  return RefPtr<WebRenderAPI>(new WebRenderAPI(wrApi, id, maxTextureSize, useANGLE, syncHandle)).forget();

The WebRenderAPI's ctor is protected, and we use factory pattern here. So, I prefer not to expose the ctor to use MakeAndAddRef or make MakeAndAddRef as a friend function.
(In reply to Nicolas Silva [:nical] from comment #30)
> Comment on attachment 8886754 [details] [diff] [review]
> P3: Support DXGI texture type for WR.
> 
> Review of attachment 8886754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think that we can afford to do yuv->rgb conversion on the CPU (in
> places where we currently don't). My understanding is that the majority of
> Windows users get this format for big sites like youtube, and the extra cost
> of converting each frame of the CPU would be too big a regression. I am sure
> that there is a way somehow to avoid this cost. I wonder how chromium deals
> with this.

Does gecko still use DXGIYCbCrTextureHostD3D11 instead of BufferTextureHost for software video decoding?

Jeff, do you have idea about using DXGIYCbCrTextureHostD3D11(uses 3 separated d3d A8 texutres with ANGLE)?
The eglCreatePbufferFromClientBuffer() can't use A8 format.
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(jgilbert)
> Does gecko still use DXGIYCbCrTextureHostD3D11 instead of BufferTextureHost for software video decoding?

Yes, we use it when we get software decoding for vp8 and vp9 which amounts to the majority of youtube traffic. There are also some users that get hardware accelerated video decoding on youtube but my understanding is that it's not the majority.
Flags: needinfo?(nical.bugzilla)
Sorry for the confusion. Most of these users get BufferTextureHost with planar ycbcr data (rather than rgb data). It's not the DXGIYCbCr version.
I don't how much we use DXGI surfaces that are in YUV formats.
We do appear to use DXGIYCbCr textures with WMF (so HWA h264). I am told that it is the second most common decoder for us after vp8/vp9.
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #33)
> (In reply to Nicolas Silva [:nical] from comment #30)
> > Comment on attachment 8886754 [details] [diff] [review]
> > P3: Support DXGI texture type for WR.
> > 
> > Review of attachment 8886754 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't think that we can afford to do yuv->rgb conversion on the CPU (in
> > places where we currently don't). My understanding is that the majority of
> > Windows users get this format for big sites like youtube, and the extra cost
> > of converting each frame of the CPU would be too big a regression. I am sure
> > that there is a way somehow to avoid this cost. I wonder how chromium deals
> > with this.
> 
> Does gecko still use DXGIYCbCrTextureHostD3D11 instead of BufferTextureHost
> for software video decoding?
> 
> Jeff, do you have idea about using DXGIYCbCrTextureHostD3D11(uses 3
> separated d3d A8 texutres with ANGLE)?
> The eglCreatePbufferFromClientBuffer() can't use A8 format.

I needed to make additions to ANGLE in bug 1322746. Depend on those changes and this should be straight-forward.
Flags: needinfo?(jgilbert)
(In reply to Nicolas Silva [:nical] from comment #36)
> We do appear to use DXGIYCbCr textures with WMF (so HWA h264). I am told
> that it is the second most common decoder for us after vp8/vp9.

For the time being gecko uses libyuv for both DXGIYCbCrTexture and DXGITexture, but I will create another bug to skip the libyuv conversion after bug 1322746.
Is it fine that just use libyuv now?
Flags: needinfo?(nical.bugzilla)
> Is it fine that just use libyuv now?

Sounds good to me as long as we don't forget about it and ship the regression.
Flags: needinfo?(nical.bugzilla)
rebase.

The DXGIYCbCrTextureHostD3D11 uses libyuv for format conversion in this patch.
It's a slow path. From comment 30, 38 and 39, we will refine this path in another
bug. We also add a performance warning message for this path.

+  NS_WARNING("WR doesn't support DXGIYCbCrTextureHostD3D11 directly. It's a slower path.");
Attachment #8888185 - Flags: review?(nical.bugzilla)
Attachment #8886754 - Attachment is obsolete: true
Attachment #8886754 - Flags: review?(nical.bugzilla)
Attachment #8886755 - Attachment is obsolete: true
Attachment #8886756 - Attachment is obsolete: true
Attachment #8888189 - Attachment description: P5: Pass KnowsCompositor instead of ShadowForwarder to media decoder. v2. → P5: Pass KnowsCompositor instead of ShadowForwarder to media decoder. v2. r=nical
Attachment #8888185 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8886760 [details] [diff] [review]
P9: Do the refactoring for SyncObject.

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

The names in this patch are confusing... maybe it's because they're not symmetric. How about "SyncObjectHost" and "SyncObjectClient"?
From comment 43, create the new SyncObjectClient/Host type for texture synchronization.

-----

The MLGDeviceD3D11, CompositorD3D11 and TextureClient use the same synchronization mechanism.
Create the new SyncObjectClient/Host types for reusing code.

Add SyncObject.cpp/h and create two new data types: SyncObjectClient and SyncObjectHost.
The SyncObjectClient is used for the TextureClient synchronization at client side.
The SyncObjectHost is used for the TextureHost synchronization in renderers such
as MLGDeviceD3D11 and CompositorD3D11.
Attachment #8888655 - Flags: review?(matt.woodrow)
Attachment #8888655 - Flags: review?(dvander)
Attachment #8886760 - Attachment is obsolete: true
Attachment #8886760 - Flags: review?(matt.woodrow)
Attachment #8886760 - Flags: review?(dvander)
Attachment #8886761 - Attachment is obsolete: true
Attachment #8886761 - Flags: review?(matt.woodrow)
Attachment #8886761 - Flags: review?(dvander)
Attachment #8886762 - Attachment is obsolete: true
Attachment #8886762 - Flags: review?(matt.woodrow)
Attachment #8886762 - Flags: review?(dvander)
Attachment #8886763 - Attachment is obsolete: true
Attachment #8888655 - Flags: review?(dvander) → review+
Attachment #8888656 - Flags: review?(dvander) → review+
Attachment #8888657 - Flags: review?(dvander) → review+
Attachment #8886907 - Flags: review?(matt.woodrow) → review+
Attachment #8888655 - Flags: review?(matt.woodrow) → review+
Attachment #8888656 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8888657 [details] [diff] [review]
P11: Update layers, dxva and vr module to use SyncObjectChild. v2.

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

::: dom/media/platforms/wmf/DXVA2Manager.cpp
@@ +711,5 @@
>        // and because it allows color conversion ocurring directly from this texture
>        // DXVA does not seem to accept IDXGIKeyedMutex textures as input.
>        mSyncObject =
> +        layers::SyncObjectClient::CreateSyncObjectClient(layers::ImageBridgeChild::GetSingleton()->GetTextureFactoryIdentifier().mSyncHandle,
> +                                                         mDevice);

Can you try break this long line up a bit and fix the indenting?

Same with the call below.
Attachment #8888657 - Flags: review?(matt.woodrow) → review+
:jgilbert, review ping.
Flags: needinfo?(jgilbert)
Comment on attachment 8888187 [details] [diff] [review]
P4: Get the gl handle from d3d shared texture handle. v2.

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

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.cpp
@@ +28,5 @@
> +    case gfx::SurfaceFormat::R8G8B8:
> +    case gfx::SurfaceFormat::B8G8R8:
> +      return LOCAL_EGL_TEXTURE_RGB;
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("GetEGLTextureFormat(): unexpected texture format");

This is not hot code, so don't use MOZ_ASSERT_UNREACHABLE. Use gfxCriticalError.

@@ +99,5 @@
> +                                   LOCAL_EGL_DXGI_KEYED_MUTEX_ANGLE,
> +                                   (void**)getter_AddRefs(mKeyedMutex));
> +
> +    if (mKeyedMutex) {
> +      HRESULT hr = mKeyedMutex->AcquireSync(0, 100);

Not here. This has to be executed every Lock(). You should take all this init code an make an EnsureLockable helper function. Then Lock becomes:

Lock()
{
  if (!EnsureLockable())
    return false;
  	
  if (mKeyedMutex) {
    HRESULT hr = mKeyedMutex->AcquireSync(0, 100);
    if (hr != S_OK) {
      gfxCriticalError() << "RenderDXGITextureHostOGL AcquireSync timeout, hr=" << gfx::hexa(hr);
      return false;
    }
  }
  return true;
}

@@ +183,5 @@
>  void
>  RenderDXGITextureHostOGL::Unlock()
>  {
> +  if (mTextureHandle[0]) {
> +    if (mKeyedMutex) {

You should be able to solely test mKeyedMutex here.

@@ +200,5 @@
> +    for(int i = 0; i < 2; ++i) {
> +      mTextureHandle[i] = 0;
> +    }
> +
> +    gl::GLLibraryEGL* egl = &gl::sEGLLibrary;

(const?) auto& egl = gl::sEGLLibrary; (or &gl::sEGLLibrary, if you really want it to act like a pointer)

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.h
@@ +5,5 @@
>  
>  #ifndef MOZILLA_GFX_RENDERD3D11TEXTUREHOSTOGL_H
>  #define MOZILLA_GFX_RENDERD3D11TEXTUREHOSTOGL_H
>  
> +#include <d3d11.h>

Just forward-declare the types you need.
Attachment #8888187 - Flags: review?(jgilbert) → review-
Flags: needinfo?(jgilbert)
MozReview-Commit-ID: MSpicRD2Xf
Attachment #8893232 - Flags: review?(mtseng)
Before bug 1322746, we should fix the wrong return value of DestroyStreamKHR().
Attachment #8893245 - Flags: review?(jgilbert)
Update for the comment 50. I will squash the P4 and P4-1 after reviewing.

Extract the eglStream init work into the EnsureLockable() helper function.
Attachment #8893246 - Flags: review?(jgilbert)
From bug 1163440, there is an additional AcquireSync() call around the swapChain::Present(). Export the KeyedMutex from SyncObjectD3D11Host for this synchronization.

MozReview-Commit-ID: 8mPs4jKj67W
Attachment #8893248 - Flags: review?(dvander)
Comment on attachment 8893248 [details] [diff] [review]
P10-1: Update MLGDeviceD3D11 and CompositorD3D11 to use SyncObjectHost.

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

Looks this new sync code wasn't ported back to Advanced Layers. Left a comment in bug 1163440.
Attachment #8893248 - Flags: review?(dvander) → review+
Attachment #8893245 - Flags: review?(jgilbert) → review+
Comment on attachment 8893246 [details] [diff] [review]
P4-1: Get the gl handle from d3d shared texture handle.

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

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.cpp
@@ +71,5 @@
>    if (mTextureHandle[0]) {
>      return true;
>    }
>  
> +  auto egl = &gl::sEGLLibrary;

auto&
auto without the reference should be avoided when dealing with non-value types.

@@ +213,5 @@
>      for(int i = 0; i < 2; ++i) {
>        mTextureHandle[i] = 0;
>      }
>  
> +    auto egl = &gl::sEGLLibrary;

auto& (really const auto& to const the pointer value)
Attachment #8893246 - Flags: review?(jgilbert) → review+
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a09c2c2a46
P1: Fix the wrong ext string for QueryDisplayAttribEXT. v2. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d97c974cc64
P1-1: Fix the wrong return value for DestroyStreamKHR(). r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/9801c56ba0f6
P2: Remove the unused IsWrappingNativeHandle() function in WebRenderTextureHost. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/1299e43ad528
P3: Support DXGI texture type for WR. v2. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39eda1b5bb3
P4: Get the gl handle from d3d shared texture handle. v2. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/21362b350189
P5: Pass KnowsCompositor instead of ShadowForwarder to media decoder. v2. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/063c7a48396d
P6: Turn on DXVA with LAYERS_WR and ANGLE backend. v2. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7aa3188d9c
P7: Fix unified-build build break. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/590df10254b0
P8: Add some function result checkings for DXVA2 video decoding. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/840c6188982f
P9: Do the refactoring for SyncObject. v2. r=mattwoodrow,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b405a6fb948
P10: Update MLGDeviceD3D11 and CompositorD3D11 to use SyncObjectHost. v2. r=mattwoodrow,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5b83e1cf8d
P10-1: Update MLGDeviceD3D11 and CompositorD3D11 to use SyncObjectHost. r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e453e18f5f3
P11: Update layers, dxva and vr module to use SyncObjectChild. v3. r=mattwoodrow,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca40fe1ef4c
P12: Add SyncObject in RendererOGL. v2. r=nical
Duplicate of this bug: 1356954
Blocks: 1388240
Attachment #8893232 - Attachment is obsolete: true
Depends on: 1391481
Depends on: 1409176
You need to log in before you can comment on or make changes to this bug.