Closed Bug 1366502 Opened 2 years ago Closed 2 years ago

Use WR planar-YCbCr format for software-decoded video

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 9 obsolete files)

2.63 KB, patch
jerry
: review+
Details | Diff | Splinter Review
4.33 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
8.48 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
3.75 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
13.30 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.72 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
6.04 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
3.49 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.23 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
7.33 KB, patch
jerry
: review+
Details | Diff | Splinter Review
5.49 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.58 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.05 KB, patch
kats
: review+
Details | Diff | Splinter Review
Currently, gecko still uses libyuv to convert the video texture to rgb format. We could just pass that ycbcr texture to WR without the color format conversion.
WR supports the planar-ycbcr image format. We could get rid of the software ycbcr to rgb color format conversion(using libyuv) in gecko.

MozReview-Commit-ID: 2Fn7cEmc9On
Attachment #8869741 - Flags: review?(nical.bugzilla)
Attachment #8869741 - Flags: review?(nical.bugzilla) → review?(sotaro.ikeda.g)
Comment on attachment 8869741 [details] [diff] [review]
Use WR PushYCbCrPlanarImage() for software-video-decoded buffer.

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

The patch looks good. But it is not clear what we need to do if we want to use wrench.

From Bug 1352657, if we want to use wrench, it seems that we need to disable external image id handling locally.
Can you explain how we could disable external image id handling with this patch?
See Also: → 1352657
Flags: needinfo?(hshih)
I think I could update the wrench to support the external image for some regular gl textures, but I will hit the problem for some special gl texture handles(e.g. the TEXTURE_EXTERNAL texture target). Because we don't know the stride, internal format and size for that special gl texture. So, maybe it's hard to support the external image for wrench recently.

So, I will try to do the similar thing as bug 1352657. Just use GetAsSurface() to get the buffer and pass to WR using add_image(). That calls will copy the image buffer to the WR's internal buffer. Then, we don't need to handle the platform specific texture problem.

         WebRenderTextureHost* wrTexture = texture->AsWebRenderTextureHost();
-        if (wrTexture) {
+        if (false && wrTexture) {

Before the dynamic video pipeline id, we should just hardcode the texture format in WebrenderImageLayer. So, I will just use a preference in WebrenderImageLayer to force to put a RGBA image display item at content side, and use GetAsSurface() and add_image() to pass the converted RGBA data to WR.

Sotaro, what do you think for this?
Flags: needinfo?(hshih) → needinfo?(sotaro.ikeda.g)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #3)
> Before the dynamic video pipeline id, we should just hardcode the texture
> format in WebrenderImageLayer. So, I will just use a preference in
> WebrenderImageLayer to force to put a RGBA image display item at content
> side, and use GetAsSurface() and add_image() to pass the converted RGBA data
> to WR.
> 
> Sotaro, what do you think for this?

It sounds good!
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8869741 - Flags: review?(sotaro.ikeda.g) → review+
Bug 1367689 will try to handle the external image with wrench.
Hi Sotaro,
There are a lot of webm video format test fails in [1].
The problem is that webm decoder is just output the single channel B8G8R8A8 format, but I assume all software decoder will output the 3 channel YCbCr buffer. So, I hit the assert at AddWRImage in [2].

  MOZ_ASSERT(aImageKeys.length() == 1);

Should we modify the output format as the WR video test workaround until the dynamic pipeline id? Should we wait the dynamic pipeline id patch for this problem?

[1]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75c321e179c1834b9af9cefc202797d15235fa1b
[2]
https://bugzilla.mozilla.org/attachment.cgi?id=8869741&action=diff#a/gfx/layers/composite/TextureHost.cpp_sec2
Flags: needinfo?(sotaro.ikeda.g)
Software decoder does not always output 3 channel YCbCr. If video has alpha, the decoder output 4 channel data. In current gecko, the 4 channel output is converted to B8G8R8A8 format. It was added by Bug 1321076. Can't we add a code to detect format to WebRenderImageLayer?
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(hshih)
See Also: → 1321076
MozReview-Commit-ID: LcSHVkBbM6G
Attachment #8872514 - Flags: review?(sotaro.ikeda.g)
This patch gets the image format from the image container and push the proper WR commands according to the image format.

MozReview-Commit-ID: JrzwvvACGW9
Attachment #8872515 - Flags: review?(sotaro.ikeda.g)
Flags: needinfo?(hshih)
Attachment #8872514 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8872515 - Flags: review?(sotaro.ikeda.g) → review+
Hi Sotaro,

There are still some reftests failed(the R6 and R8). It looks like the yuv-to-rgb conversion precision problem. Before this patch, we use [1] to convert yuv to rgb. Now, we are trying to use shader[2] for color conversion.
I'm trying to do the similar calculation in shader, but there is a big lookup table used in [1]. It's not a good practice to use the big table in shader.

I would like to increase the fuzzy comparing value for R8 and skip some test if we are use webrender for R6.

This is the current testing setting:

fails-if(layersGPUAccelerated&&!webrender) fails-if(webrender&&browserIsRemote) skip-if(Android) == object-fit-contain-webm-001.html object-fit-contain-webm-001-ref.html

I'm trying to set fails-if if we have webrender.

Sotaro, what do you think?

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

[1]
https://dxr.mozilla.org/mozilla-central/source/gfx/ycbcr/yuv_convert.cpp#67
[2]
https://dxr.mozilla.org/mozilla-central/source/gfx/webrender/res/ps_yuv_image.fs.glsl?q=path%3Aps_yuv_image.fs.glsl&redirect_type=single#20
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #12)
> Hi Sotaro,
> 
> There are still some reftests failed(the R6 and R8). It looks like the
> yuv-to-rgb conversion precision problem. Before this patch, we use [1] to
> convert yuv to rgb. Now, we are trying to use shader[2] for color conversion.
> I'm trying to do the similar calculation in shader, but there is a big
> lookup table used in [1]. It's not a good practice to use the big table in
> shader.
> 
> I would like to increase the fuzzy comparing value for R8 and skip some test
> if we are use webrender for R6.
> 
> Sotaro, what do you think?

Yea, it is reasonable.
Flags: needinfo?(sotaro.ikeda.g)
Blocks: 1368556
Handle the ImageFormat::CAIRO_SURFACE case for some videos.
Attachment #8873286 - Flags: review+
Attachment #8872515 - Attachment is obsolete: true
Attached patch update reftest list. (obsolete) — Splinter Review
We use gpu for yuv color conversion now. There are some precision problems in gpu path. Mark them fails-if and update the fuzzy-test value.

MozReview-Commit-ID: 75KdZmePRzb
Attachment #8873287 - Flags: review?(sotaro.ikeda.g)
Attachment #8873287 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8869741 - Attachment is obsolete: true
Attachment #8873286 - Attachment is obsolete: true
Wait for bug 1345054.
Depends on: 1345054
Attachment #8872514 - Attachment is obsolete: true
With the video pipeline, we could call the WR image functions in compositor thread.
Attachment #8874362 - Flags: review?(sotaro.ikeda.g)
It's awful to put all combination of image key and WR command settings in the same place. Make the settings go back to textureHosts.
Each textureHost should implement GetWRImageKeys() and PushExternalImage() function.
Attachment #8874363 - Flags: review?(sotaro.ikeda.g)
These 2 functions are used for WR.
The GetWRImageKeys() will return the proper image keys according to the textureHost format.
The PushExternalImage() will put all necessary WR commands into DisplayListBuilder for the textureHost rendering.
Attachment #8874364 - Flags: review?(sotaro.ikeda.g)
WR supports the planar-ycbcr image format. We turn to use the planar-ycbcr image to get rid of the software-ycbcr-to-rgb color format conversion(using libyuv) in gecko.

The BufferTextureHost will use 3 image keys for SurfaceFormat::YUV format.
The RenderBufferTextureHost will also use 3 DataSourceSurfaces to represent the 3 channel data in planar-ycbcr format.
Attachment #8874365 - Flags: review?(sotaro.ikeda.g)
Attachment #8873749 - Attachment is obsolete: true
Attachment #8873750 - Attachment is obsolete: true
MozReview-Commit-ID: 5cu8cYoTMxT
Attachment #8874367 - Flags: review?(sotaro.ikeda.g)
Update for DXGITextureHostD3D11 and DXGIYCbCrTextureHostD3D11.
Attachment #8874368 - Flags: review?(sotaro.ikeda.g)
The RenderTextureHost might calls some thread-specific functions(e.g. OpenGL calls) in ~RenderTextureHost(). Add a checking here to prevent this problem.
Attachment #8874370 - Flags: review?(sotaro.ikeda.g)
If we call UnregisterExternalImage() at non-render-thread and decrease the RenderTextureHost's ref-count to zero, the RenderTextureHost will be released in non-render-thread.
That will cause some problems if we use some thread-specific functions in ~RenderTextureHost().
This patch uses a message loop in UnregisterExternalImage() to resolve this problem.
Attachment #8874371 - Flags: review?(sotaro.ikeda.g)
Attachment #8873287 - Attachment is obsolete: true
Attachment #8874362 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874363 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874364 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874365 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874367 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874368 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874369 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8874371 [details] [diff] [review]
P10: Update the thread model for RegisterExternalImage(), UnregisterExternalImage() and GetRenderTexture() call.

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

::: gfx/webrender_bindings/RenderThread.cpp
@@ +289,5 @@
>  {
> +  if (!IsInRenderThread()) {
> +    Loop()->PostTask(NewRunnableMethod<uint64_t>(
> +      this, &RenderThread::UnregisterExternalImage, aExternalImageId
> +    ));

In RenderBufferTextureHost case, if 	
RenderThread::UnregisterExternalImage() is called, buffer of RenderBufferTextureHost becomes invalid, since lifetime of buffer is controlled by BufferTextureHost. It means, RenderBufferTextureHost hold invalid buffer until it is unregistered. Can we ensure that the invalid buffer is not used  by WebRender?
Attachment #8874371 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> Comment on attachment 8874371 [details] [diff] [review]
> P10: Update the thread model for RegisterExternalImage(),
> UnregisterExternalImage() and GetRenderTexture() call.
> 
> Review of attachment 8874371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/RenderThread.cpp
> @@ +289,5 @@
> >  {
> > +  if (!IsInRenderThread()) {
> > +    Loop()->PostTask(NewRunnableMethod<uint64_t>(
> > +      this, &RenderThread::UnregisterExternalImage, aExternalImageId
> > +    ));
> 
> In RenderBufferTextureHost case, if 	
> RenderThread::UnregisterExternalImage() is called, buffer of
> RenderBufferTextureHost becomes invalid, since lifetime of buffer is
> controlled by BufferTextureHost. It means, RenderBufferTextureHost hold
> invalid buffer until it is unregistered. Can we ensure that the invalid
> buffer is not used  by WebRender?


void
TextureParent::Destroy()
{
  if (!mTextureHost) {
    return;
  }

  // ReadUnlock here to make sure the ReadLock's shmem does not outlive the
  // protocol that created it.
  mTextureHost->ReadUnlock();

  if (mTextureHost->GetFlags() & TextureFlags::DEALLOCATE_CLIENT) {
    mTextureHost->ForgetSharedData();
  }

  mTextureHost->mActor = nullptr;
  mTextureHost = nullptr;
}

void TextureHost::Finalize()
{
  if (!(GetFlags() & TextureFlags::DEALLOCATE_CLIENT)) {
    DeallocateSharedData();
    DeallocateDeviceData();
  }
}

void
WebRenderCompositableHolder::Update(const wr::PipelineId& aPipelineId, const wr::Epoch& aEpoch)
{
  ...
  // Release TextureHosts based on Epoch
  while (!holder->mTextureHosts.empty()) {
    if (aEpoch <= holder->mTextureHosts.front().mEpoch) {
      break;
    }
    holder->mTextureHosts.pop();
  }
}

If the buffer/shmemTextureHost doesn't use TextureFlags::DEALLOCATE_CLIENT flag, it's no affect for its internal buffer data(shmem, raw buffer and hw handle). The textureHost is held by WebRenderCompositableHolder and will be released by the WebRenderCompositableHolder::Update() call. So, the textureHost is only released when the WR doesn't use that textureHost.
Flags: needinfo?(sotaro.ikeda.g)
If it is not touched by WR, why do we need to destroy RenderBufferTextureHost on Render Thread? I am just not fun of keeping invalid buffer.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> If it is not touched by WR, why do we need to destroy
> RenderBufferTextureHost on Render Thread? I am just not fun of keeping
> invalid buffer.

The ~RenderTextureHostOGL() will use the thread-specific gl context which should run at render thread. I saw a deadlock problem when we call gl::MakeCurrent() in both render and compositor thread.
Flags: needinfo?(sotaro.ikeda.g)
Yes, gl related things should be deleted on render thread. But for me, it could be a separate problem. I prefer not to defer unregister ExternalImage. Can we destroy gl related things after unregister ExternalImage?
Flags: needinfo?(sotaro.ikeda.g)
To simplify the code, if you want to destroy RenderBufferTextureHost on render thread, it might better to add a comment about it and I still perter not to defer unregister ExternalImage.
Attachment #8874371 - Attachment is obsolete: true
MozReview-Commit-ID: KjZIGRzRomT
Attachment #8874754 - Flags: review?(sotaro.ikeda.g)
Attachment #8874754 - Attachment is obsolete: true
Attachment #8874754 - Flags: review?(sotaro.ikeda.g)
Attachment #8874756 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874753 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8874370 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f4d82e3d79
Rename mImageClientTypeContainer into mImageClientContainerType. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b20aebef47d
Update the thread checking for WR image functions. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/82f5a21b53a6
Move the various of image key and WR command settings from WebRenderCompositableHolder into textureHosts. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5225d81b832
Add GetWRImageKeys() and PushExternalImage() in textureHost. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8846dac9ee35
Update BufferTextureHost and RenderBufferTextureHost for video pipeline. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/22408b6a1ad1
Update MacIOSurfaceTextureHostOGL for video pipeline. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fcc15d8f90
Update TextureD3D11 for video pipeline. v2. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f21ee861e5
Update WebRenderTextureHost for video pipeline. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf598918bb1b
Make sure the RenderTextureHost is released in render thread. r=sotaro.
https://hg.mozilla.org/integration/mozilla-inbound/rev/265e39153027
Update the thread model for RegisterExternalImage(), UnregisterExternalImage() and GetRenderTexture() call. v2. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f98b7f60e58
update reftest list. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/42350bacb0bc
Make sure all wrapped textureHosts doesn't use TextureFlags::DEALLOCATE_CLIENT flag. v2. r=sotaro
Here is another try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=475b157dac42510010dd76f6bfbe642eb63ca8c8&selectedJob=105092098
Only 1 failed from 11 tests.

Finally, I know the reason why we have the intermittent failed.

The test in [1] will play a big dimension video(9841x705) and check if gecko could handle that. But it also reset the video src to a empty string in onloadedmetadata(). Thus, we can't make sure the first frame of the video is always shown on the screen.

Currently, webrender can't handle the image size more than 8192. We have a assertion to check that [2]. I'm going to skip this crash test for webrender. I will also create the follow-up bugs to resolve the intermittent video status and the maximum image size in webrender.

[1]
https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/dom/media/test/crashtests/789075-1.html
[2]
https://dxr.mozilla.org/mozilla-central/source/gfx/webrender/src/texture_cache.rs#664
The video size in this case is (9841x705), but the maximun dimension in WR is 8192.
So, skip this test here.

MozReview-Commit-ID: B9Bg7eeKymj
Attachment #8875222 - Flags: review?(bugmail)
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf47ea8ae7b
Rename mImageClientTypeContainer into mImageClientContainerType. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/a03ce839ba8f
Update the thread checking for WR image functions. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/abac100c9ba4
Move the various of image key and WR command settings from WebRenderCompositableHolder into textureHosts. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aee72988801
Add GetWRImageKeys() and PushExternalImage() in textureHost. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46936cf531e
Update BufferTextureHost and RenderBufferTextureHost for video pipeline. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/82db60ad7fbc
Update MacIOSurfaceTextureHostOGL for video pipeline. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c998ef6aa6bb
Update TextureD3D11 for video pipeline. v2. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/45279b23564d
Update WebRenderTextureHost for video pipeline. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/636b348d5129
Make sure the RenderTextureHost is released in render thread. r=sotaro.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f1d270a475
Update the thread model for RegisterExternalImage(), UnregisterExternalImage() and GetRenderTexture() call. v2. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/7627b9aa45d2
update reftest list. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/185a1ac1e281
Make sure all wrapped textureHosts doesn't use TextureFlags::DEALLOCATE_CLIENT flag. v2. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/00db286bf5b5
Skip dom/media/test/crashtests/789075-1.html for webrender. r=kats
Create bug 1370839 for intermittent video status and bug 1370838 for big size video.
Blocks: 1370546
Blocks: 1369678
Blocks: 1370838
You need to log in before you can comment on or make changes to this bug.