1388240 - Use DXGIYCbCrTextureHostD3D11 directly with Webrender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

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

+++ This bug was initially created as a clone of Bug #1357299 +++

From https://bugzilla.mozilla.org/show_bug.cgi?id=1357299#c38, we should just pass the YCbCr channel data to WR without using libyuv.

Bug 1322746 extend the eglStream capacity to support the DXGIYCbCrTextureHostD3D11 format. After the bug 1322746, I will use the eglStream to skip the libyuv usage.

Comment 1

[Sotaro Ikeda [:sotaro]]

:jerry, you might want to progress this bug since Bug 1322746 was addressed already.

Comment 2

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

Oh, thanks for the reminding. I will start to handle this soon.

Comment 3

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 4

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

Attachment: [WIP] use EGL stream to support d3d11 A8 format texture.

For DXGIYCbCrTextureHostD3D11, gecko use 3 separated d3d11 A8 textures
to represent the Y, Cb and Cr data. This patch try to use EGL stream to
convert the d3d11 texture to gl handle. Then, WR could use the converted
gl handle to show the video data without buffer copy operation.

--
I haven't find a path to use DXGIYCbCrTextureHostD3D11 when I turn on WR.
The H.264 video will go through NV12 format, and vp8/9 will go through shmem.

Comment 5

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

:jya, do you know which video format(or what pref combinations) will use the DXGIYCbCrTextureHostD3D11? Currently, I only see the nv12 and the shmem usage for video.

Comment 6

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

There is one place which we create the DXGIYCbCrTexture.

https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/dom/media/MediaData.cpp#324

Comment 7

[Jean-Yves Avenard [:jya]]

The default.
Rgb on all but old nvidia and amd cards

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8909245 [details] [diff] [review]
[WIP] use EGL stream to support d3d11 A8 format texture.

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

It would be really nice if you could leverage the existing code in GLBlitHelper.

Comment 9

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

Attachment: P1: update the DXGITextureHostD3D11::Lock() comment for non-compositor use case.

MozReview-Commit-ID: ERo8DHpau6c

Comment 10

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

Attachment: P2: turn to use TextureExternalHandle in DXGIYCbCrTextureHostD3D11.

Create a new type RenderDXGIYCbCrTextureHostOGL for planar-ycbcr format in WR.
That type could convert the 3 d3d11-a8 textures into gl handles. Then, WR could
draw the gl handles directly.

MozReview-Commit-ID: 1CIQO4p8u30

Comment 11

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

Attachment: P3: use EGL stream to support d3d11 A8 format texture.

For DXGIYCbCrTextureHostD3D11, gecko use 3 separated d3d11 A8 textures
to represent the Y, Cb and Cr data. This patch try to use EGL stream to
convert the d3d11 texture to gl handle. Then, WR could use the converted
gl handle to show the video data without buffer copy operation.

MozReview-Commit-ID: C9w9rzufTOj

Comment 12

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

Comment on attachment 8910728 [details] [diff] [review]
P3: use EGL stream to support d3d11 A8 format texture.

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

Why I don't use GLBlitHelper here is that:
The GLBlitHelper will initialize some shaders for bliting. I just need the EGLStream parts.
The BindAnglePlanes is the most suitable one for the usage here. But it mixes the EGLStream creation and KeyedMutex locking together.
In my case, I will create the EGLStream once and wait for the mutex for several times.
Is it worth to update the BindAnglePlanes for my usage?

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.cpp
@@ +186,5 @@
>    if (!EnsureLockable()) {
>      return false;
>    }
>  
> +  if (!mLocked) {

In some case, WR will call Lock() several times for the same texture. So, add a flag to prevent the multiple AcquireSync() call.

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8910727 [details] [diff] [review]
P2: turn to use TextureExternalHandle in DXGIYCbCrTextureHostD3D11.

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

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +1330,5 @@
>  DXGIYCbCrTextureHostD3D11::AddWRImage(wr::ResourceUpdateQueue& aResources,
>                                        Range<const wr::ImageKey>& aImageKeys,
>                                        const wr::ExternalImageId& aExtID)
>  {
>    // TODO - This implementation is very slow (read-back, copy on the copy and re-upload).

nit: I suppose this comment is out of date now.

Comment 14

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

Attachment: P2: turn to use TextureExternalHandle in DXGIYCbCrTextureHostD3D11. r=nical

update for comment 13.

Comment 15

[Kelsey Gilbert [:jgilbert]]

If we don't share code it will be painful when we upstream into ANGLE with differences, and then have to rectify those differences in multiple places.

Leverage the existing code so that it does what you want. In this case I would move the acquire/release into a subclass called AcquireAnglePlanes, and just use the raw BindAnglePlanes here.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8910728 [details] [diff] [review]
P3: use EGL stream to support d3d11 A8 format texture.

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

We should really be transitioning away from A8 to R8 at some point.

It does seem like a bug that WR would lock too many times. In particular, this would break if we did the following:
A: lock
B: lock skipped
B: unlock
// If A expects to be locked still, this is a bug
A: lock skipped
// Everything is fine again after this.

Asserting that the object is locked each time we use it would show us if we hit the bug today. A counting lock would fix this, if we need to.

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.cpp
@@ +186,5 @@
>    if (!EnsureLockable()) {
>      return false;
>    }
>  
> +  if (!mLocked) {

Generally early out in cases like this. (If we're already locked, just return immediately)

@@ +323,5 @@
> +    if (FAILED(device->OpenSharedResource((HANDLE)mHandles[i],
> +                                          __uuidof(ID3D11Texture2D),
> +                                          (void**)(ID3D11Texture2D**)getter_AddRefs(mTextures[i])))) {
> +      NS_WARNING("RenderDXGITextureHostOGL::Lock(): Failed to open shared texture");
> +      return false;

This leaves the other resources open? I assume that we just destroy this object if this function returns false?

@@ +340,5 @@
> +
> +    // Create the EGLStream.
> +    mStreams[i] = egl->fCreateStreamKHR(egl->Display(), nullptr);
> +    MOZ_ASSERT(egl->fGetError() == LOCAL_EGL_SUCCESS);
> +    MOZ_ASSERT(mStreams[i]);

No reason to have both these assertions here, really.
If the returned stream handle is non-zero, it must not have errored. We can rely on this.

@@ +351,5 @@
> +
> +    // Now, we could check the stream status and use the A8 gl handle.
> +    EGLint state;
> +    egl->fQueryStreamKHR(egl->Display(), mStreams[i], LOCAL_EGL_STREAM_STATE_KHR, &state);
> +    MOZ_ASSERT(state == LOCAL_EGL_STREAM_STATE_NEW_FRAME_AVAILABLE_KHR);

Skip this.
If you think you must, make it ifdef debug.

@@ +368,5 @@
> +  }
> +
> +  if (!mLocked) {
> +    if (mKeyedMutexs[0]) {
> +      for (auto mutex : mKeyedMutexs) {

const auto&

@@ +387,5 @@
>  RenderDXGIYCbCrTextureHostOGL::Unlock()
>  {
> +  if (mLocked) {
> +    if (mKeyedMutexs[0]) {
> +      for (auto mutex : mKeyedMutexs) {

const auto&

@@ +412,5 @@
> +  if (aChannelIndex == 0) {
> +    return mSize;
> +  } else {
> +    // The CbCr channel size is a half of Y channel size.
> +    return mSize / 2;

Are you confident these are tightly-packed sizes *and* that size is always even? Otherwise this is wrong. (We can at least assert that mSize is always even)

@@ +419,5 @@
> +
> +void
> +RenderDXGIYCbCrTextureHostOGL::DeleteTextureHandle()
> +{
> +  if (mTextureHandles[0] != 0) {

Early-out! It makes this nested code more readable.

@@ +423,5 @@
> +  if (mTextureHandles[0] != 0) {
> +    if (mGL && mGL->MakeCurrent()) {
> +      mGL->fDeleteTextures(3, mTextureHandles);
> +    }
> +    for(int i = 0; i < 3; ++i) {

for<space>(

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.h
@@ +79,5 @@
> +  void DeleteTextureHandle();
> +
> +  RefPtr<gl::GLContext> mGL;
> +
> +  WindowsHandle mHandles[3];

This should be const.

@@ +89,5 @@
> +
> +  // The gl handles for Y, Cb and Cr data.
> +  GLuint mTextureHandles[3];
> +
> +  gfx::IntSize mSize;

This should be const.

Comment 17

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

Comment on attachment 8910728 [details] [diff] [review]
P3: use EGL stream to support d3d11 A8 format texture.

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

::: gfx/webrender_bindings/RenderD3D11TextureHostOGL.cpp
@@ +323,5 @@
> +    if (FAILED(device->OpenSharedResource((HANDLE)mHandles[i],
> +                                          __uuidof(ID3D11Texture2D),
> +                                          (void**)(ID3D11Texture2D**)getter_AddRefs(mTextures[i])))) {
> +      NS_WARNING("RenderDXGITextureHostOGL::Lock(): Failed to open shared texture");
> +      return false;

Yes, it still keeps other d3d textures' ref-count. This object will be destroyed when its corresponding TextureHost is deleted.

@@ +412,5 @@
> +  if (aChannelIndex == 0) {
> +    return mSize;
> +  } else {
> +    // The CbCr channel size is a half of Y channel size.
> +    return mSize / 2;

The planar yuv420 doesn't need to be tightly-packed. But the size should be even. I add an assertion in the constructor.

Comment 18

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

(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> It does seem like a bug that WR would lock too many times. In particular,
> this would break if we did the following:
> A: lock
> B: lock skipped
> B: unlock
> // If A expects to be locked still, this is a bug
> A: lock skipped
> // Everything is fine again after this.
> 
> Asserting that the object is locked each time we use it would show us if we
> hit the bug today. A counting lock would fix this, if we need to.

The textureHost might be shared by multiple compositableHost in some cases. Then, two primitives in WR will reference to the same renderTextureHost. That's why we might have multiple lock() call for renderTextureHost.

WR will call lock() for all ext-image, and then call unlock() for all ext-image. There is no interleaving lock/unlock() sequence.
But the counting lock will be much better. I will do that later.

Comment 19

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

Attachment: P2: turn to use TextureExternalHandle in DXGIYCbCrTextureHostD3D11. v2. r=nical

rebase.

Comment 20

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

Attachment: P3: use EGL stream to support d3d11 A8 format texture. v2. r=jgilbert

update for comment 16.

Skip the querying from egl stream.
Early out for the DeleteTextureHandle() and lock() function.

Comment 21

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

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c91bd1c947bb023cd31a0d25237aa94d011907

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91808daafa07
update the DXGITextureHostD3D11::Lock() comment for non-compositor use case. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9876a0d19f
turn to use TextureExternalHandle in DXGIYCbCrTextureHostD3D11. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/507fd8ac4448
use EGL stream to support d3d11 A8 format texture. r=jgilbert

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/91808daafa07
https://hg.mozilla.org/mozilla-central/rev/0b9876a0d19f
https://hg.mozilla.org/mozilla-central/rev/507fd8ac4448