1357299 - Support NV12 format DXGITextureHostD3D11 with Webrender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: Fix the wrong ext string for QueryDisplayAttribEXT. r=jgilbert

MozReview-Commit-ID: 3lZG2GrxXHF

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: Support DXGI texture type for WR.

MozReview-Commit-ID: F4mPCALj1OY

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Pass KnowsCompositor instead of ShadowForwarder to media decoder.

If we use WebRender, there is no ShadowForwarder with this configuration. So, use the AsKnowsCompositor() instead.

MozReview-Commit-ID: 7skHx7SxMuR

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: Turn on DXVA with LAYERS_WR backend.

MozReview-Commit-ID: EOOp0Dzenub

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

These is the WIP patch.

The converted gl texture from dx11 nv12 texture is always green in this patch. I'm checking this problem.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Bad green and nv12 is a combination we see outside of webrender.  Perhaps unrelated, but just in case, :mstange can fill in the details.

Comment 8

[Markus Stange [:mstange]]

Bug 1352016 is about a green border showing up with nv12 on some machines. That's really all I know about it.

Comment 9

[Kelsey Gilbert [:jgilbert]]

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.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 11

[Kelsey Gilbert [:jgilbert]]

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.

Comment 12

[Sotaro Ikeda [:sotaro]]

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

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: Fix the wrong ext string for QueryDisplayAttribEXT. v2. r=jgilbert

Update for comment 9.

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: Remove the unused IsWrappingNativeHandle() function in WebRenderTextureHost.

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

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Support DXGI texture type for WR.

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

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: Get the gl handle from d3d shared texture handle.

MozReview-Commit-ID: F4mPCALj1OY

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Pass KnowsCompositor instead of ShadowForwarder to media decoder.

If we use WebRender, there is no ShadowForwarder with this configuration. So, use the AsKnowsCompositor() instead.

MozReview-Commit-ID: 7skHx7SxMuR

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Turn on DXVA with LAYERS_WR backend.

MozReview-Commit-ID: EOOp0Dzenub

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7: Fix unified-build build break.

MozReview-Commit-ID: GSUxyWUfBVt

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P8: Add some function result checkings for DXVA2 video decoding.

Check the buffer appending status for the video sample object.
Check for the IMFTransform output status.

MozReview-Commit-ID: J0bn6NB7gi0

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P9: Do the refactoring for SyncObject.

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

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P10: Update MLGDeviceD3D11 and CompositorD3D11 to use RendererSyncObject.

MozReview-Commit-ID: 1a0Ho7smkAx

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P11: Update layers, dxva and vr module to use TextureSyncObject.

MozReview-Commit-ID: 4HTPz0YcYHq

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P12: Add SyncObject in RendererOGL.

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

Comment 25

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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 26

[Matt Woodrow (:mattwoodrow)]

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?

Comment 27

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Turn on DXVA with LAYERS_WR and ANGLE backend. v2.

Turn on DXVA for WebRender-ANGLE backend.

Comment 28

[Nicolas Silva [:nical]]

Comment on attachment 8886753 [details] [diff] [review]
P2: Remove the unused IsWrappingNativeHandle() function in WebRenderTextureHost.

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

Nice!

Comment 29

[Nicolas Silva [:nical]]

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

Comment 30

[Nicolas Silva [:nical]]

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 31

[Kelsey Gilbert [:jgilbert]]

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.

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 34

[Nicolas Silva [:nical]]

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

Comment 35

[Nicolas Silva [:nical]]

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.

Comment 36

[Nicolas Silva [:nical]]

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.

Comment 37

[Kelsey Gilbert [:jgilbert]]

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

Comment 38

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 39

[Nicolas Silva [:nical]]

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

Comment 40

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Support DXGI texture type for WR. v2.

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.");

Comment 41

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: Get the gl handle from d3d shared texture handle. v2.

update for comment 31.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Pass KnowsCompositor instead of ShadowForwarder to media decoder. v2. r=nical

rebase

Comment 43

[David Anderson [:dvander] - inactive, e-mail if emergency]

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"?

Comment 44

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P9: Do the refactoring for SyncObject. v2.

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.

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P10: Update MLGDeviceD3D11 and CompositorD3D11 to use SyncObjectHost. v2.

Update for comment 43.

Comment 46

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P11: Update layers, dxva and vr module to use SyncObjectChild. v2.

Update for comment 43.

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P12: Add SyncObject in RendererOGL. v2. r=nical

Update for comment 43.

Comment 48

[Matt Woodrow (:mattwoodrow)]

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.

Comment 49

[Sotaro Ikeda [:sotaro]]

:jgilbert, review ping.

Comment 50

[Kelsey Gilbert [: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.

Comment 51

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P0: Fix unified build break for WebRenderUserData.

MozReview-Commit-ID: MSpicRD2Xf

Comment 52

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1-1: Fix the wrong return value for DestroyStreamKHR().

Before bug 1322746, we should fix the wrong return value of DestroyStreamKHR().

Comment 53

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4-1: Get the gl handle from d3d shared texture handle.

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.

Comment 54

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P10-1: Update MLGDeviceD3D11 and CompositorD3D11 to use SyncObjectHost.

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

Comment 55

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 56

[Kelsey Gilbert [:jgilbert]]

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)

Comment 57

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51e7dad660de4e30767a02202ded7e126761b01&selectedJob=121348773

Comment 58

[Pulsebot]

Pushed by [e-mail]:
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

Comment 59

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3a09c2c2a46
https://hg.mozilla.org/mozilla-central/rev/5d97c974cc64
https://hg.mozilla.org/mozilla-central/rev/9801c56ba0f6
https://hg.mozilla.org/mozilla-central/rev/1299e43ad528
https://hg.mozilla.org/mozilla-central/rev/c39eda1b5bb3
https://hg.mozilla.org/mozilla-central/rev/21362b350189
https://hg.mozilla.org/mozilla-central/rev/063c7a48396d
https://hg.mozilla.org/mozilla-central/rev/4d7aa3188d9c
https://hg.mozilla.org/mozilla-central/rev/590df10254b0
https://hg.mozilla.org/mozilla-central/rev/840c6188982f
https://hg.mozilla.org/mozilla-central/rev/0b405a6fb948
https://hg.mozilla.org/mozilla-central/rev/5e5b83e1cf8d
https://hg.mozilla.org/mozilla-central/rev/5e453e18f5f3
https://hg.mozilla.org/mozilla-central/rev/6ca40fe1ef4c