Closed
Bug 1357299
Opened 8 years ago
Closed 7 years ago
Support NV12 format DXGITextureHostD3D11 with Webrender
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jerry, Assigned: jerry)
References
Details
Attachments
(15 files, 13 obsolete files)
(deleted),
patch
|
jerry
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jgilbert
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jerry
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jerry
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: 3lZG2GrxXHF
Attachment #8880333 -
Flags: review?(jgilbert)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: F4mPCALj1OY
Assignee | ||
Comment 4•7 years ago
|
||
If we use WebRender, there is no ShadowForwarder with this configuration. So, use the AsKnowsCompositor() instead.
MozReview-Commit-ID: 7skHx7SxMuR
Assignee | ||
Comment 5•7 years ago
|
||
MozReview-Commit-ID: EOOp0Dzenub
Assignee | ||
Comment 6•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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+
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8880333 -
Attachment is obsolete: true
Assignee | ||
Comment 14•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8880334 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8880335 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
MozReview-Commit-ID: F4mPCALj1OY
Attachment #8886755 -
Flags: review?(jgilbert)
Assignee | ||
Updated•7 years ago
|
Attachment #8880336 -
Attachment is obsolete: true
Assignee | ||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
MozReview-Commit-ID: EOOp0Dzenub
Attachment #8886757 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 19•7 years ago
|
||
MozReview-Commit-ID: GSUxyWUfBVt
Attachment #8886758 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Assignee | ||
Comment 22•7 years ago
|
||
MozReview-Commit-ID: 1a0Ho7smkAx
Attachment #8886761 -
Flags: review?(matt.woodrow)
Attachment #8886761 -
Flags: review?(dvander)
Assignee | ||
Comment 23•7 years ago
|
||
MozReview-Commit-ID: 4HTPz0YcYHq
Attachment #8886762 -
Flags: review?(matt.woodrow)
Attachment #8886762 -
Flags: review?(dvander)
Assignee | ||
Comment 24•7 years ago
|
||
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)
Assignee | ||
Comment 25•7 years ago
|
||
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•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8886758 -
Flags: review?(matt.woodrow) → review+
Updated•7 years ago
|
Attachment #8886759 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 27•7 years ago
|
||
Turn on DXVA for WebRender-ANGLE backend.
Attachment #8886907 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•7 years ago
|
Attachment #8886757 -
Attachment is obsolete: true
Attachment #8886757 -
Flags: review?(matt.woodrow)
Comment 28•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8886756 -
Flags: review?(nical.bugzilla) → review+
Comment 29•7 years ago
|
||
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 30•7 years ago
|
||
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•7 years ago
|
||
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-
Assignee | ||
Comment 32•7 years ago
|
||
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.
Assignee | ||
Comment 33•7 years ago
|
||
(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)
Comment 34•7 years ago
|
||
> 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)
Comment 35•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(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)
Assignee | ||
Comment 38•7 years ago
|
||
(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)
Comment 39•7 years ago
|
||
> 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)
Assignee | ||
Comment 40•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8886754 -
Attachment is obsolete: true
Attachment #8886754 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 41•7 years ago
|
||
update for comment 31.
Attachment #8888187 -
Flags: review?(jgilbert)
Assignee | ||
Updated•7 years ago
|
Attachment #8886755 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8886756 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
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
Updated•7 years ago
|
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"?
Assignee | ||
Comment 44•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8886760 -
Attachment is obsolete: true
Attachment #8886760 -
Flags: review?(matt.woodrow)
Attachment #8886760 -
Flags: review?(dvander)
Assignee | ||
Comment 45•7 years ago
|
||
Update for comment 43.
Attachment #8888656 -
Flags: review?(matt.woodrow)
Attachment #8888656 -
Flags: review?(dvander)
Assignee | ||
Updated•7 years ago
|
Attachment #8886761 -
Attachment is obsolete: true
Attachment #8886761 -
Flags: review?(matt.woodrow)
Attachment #8886761 -
Flags: review?(dvander)
Assignee | ||
Comment 46•7 years ago
|
||
Update for comment 43.
Attachment #8888657 -
Flags: review?(matt.woodrow)
Attachment #8888657 -
Flags: review?(dvander)
Assignee | ||
Updated•7 years ago
|
Attachment #8886762 -
Attachment is obsolete: true
Attachment #8886762 -
Flags: review?(matt.woodrow)
Attachment #8886762 -
Flags: review?(dvander)
Assignee | ||
Updated•7 years ago
|
Attachment #8886763 -
Attachment is obsolete: true
Attachment #8888655 -
Flags: review?(dvander) → review+
Attachment #8888656 -
Flags: review?(dvander) → review+
Attachment #8888657 -
Flags: review?(dvander) → review+
Updated•7 years ago
|
Attachment #8886907 -
Flags: review?(matt.woodrow) → review+
Updated•7 years ago
|
Attachment #8888655 -
Flags: review?(matt.woodrow) → review+
Updated•7 years ago
|
Attachment #8888656 -
Flags: review?(matt.woodrow) → review+
Comment 48•7 years ago
|
||
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+
Comment 50•7 years ago
|
||
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-
Updated•7 years ago
|
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 51•7 years ago
|
||
MozReview-Commit-ID: MSpicRD2Xf
Attachment #8893232 -
Flags: review?(mtseng)
Assignee | ||
Comment 52•7 years ago
|
||
Before bug 1322746, we should fix the wrong return value of DestroyStreamKHR().
Attachment #8893245 -
Flags: review?(jgilbert)
Assignee | ||
Comment 53•7 years ago
|
||
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)
Assignee | ||
Comment 54•7 years ago
|
||
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+
Updated•7 years ago
|
Blocks: stage-wr-trains
Updated•7 years ago
|
Attachment #8893245 -
Flags: review?(jgilbert) → review+
Comment 56•7 years ago
|
||
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+
Assignee | ||
Comment 57•7 years ago
|
||
Comment 58•7 years ago
|
||
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
Comment 59•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Attachment #8893232 -
Flags: review?(mtseng)
Assignee | ||
Updated•7 years ago
|
Attachment #8893232 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•