This issue was discovered when I was implementing bug 1713276. When setting mPicSize on a PlanarYCbCrData, that attribute should suppose to be used for cliping image into a smaller size later in the gfx pipeline. The usage here is many decoders would allocate larger size spaces for data buffer because of their own alignment rule. Eg. FFmpeg will align height 1080 to 1088.

Therefore, when bliting happens there is an attribute to clip the image, which should be the mPicSize.

However, it didn't work on my case, and we found out that that should be a bug for WR. Based on following offline disucssion with Kesley, this bug is used to handle that WR cliping problem.

Kesley:
I talked to gw about the cliprect thing
He thinks it's fixable, and not too much work, but also not zero work
eventually, he points out, we should really be uploading this data to the gpu before we hand it off to WR, probably, and that if we do that, clip-rect would work

:gw, can you comment to this bug?

Yup, to expand on what Kelsey said above:

It is possible to add support to (fix) WR for this case, since it should really be handled correctly.

However:

Uploading texture data through that code path was never really intended for video data frames anyway. That code path results in extra copies of the intermediate texture being made (at least once, possibly more than that) and allocated and placed in the main texture atlas cache (which is not designed to handle very large images, it's meant for caching many small images in one texture page).

So, given how inefficient using that path is for video, the question is whether to spend engineering resources making it handle the clipping correctly, or whether we should instead change Gecko to use the intended video upload path here, which already handles the clipping case and is much more efficient.

I would estimate fixing the slow WR path for correctness is ~3 days work, not sure about instead doing the Gecko work mentioned above.

I'm not sure of the urgency of this bug, or which of the above options we want to put engineering time into. Jim, Bob, thoughts?

Let me also add some context here, we (media playback) are not intended to use this "inefficient" way to upload the texture, tbh we don't even know what path the web render would choose to upload the texture. The thing we did in bug 1713276 is, to eliminate the copying of decoded data, and anything beyond that (uploading texture) is not something we plan to change. See this to see more details.

(In reply to Glenn Watson [:gw] from comment #2)

or whether we should instead change Gecko to use the intended video upload path here, which already handles the clipping case and is much more efficient.

What's the name of the intended video upload path? Where is it? Searchfox link(s) please.

bug 1539043 and its dependencies might be relevant.

(In reply to Darkspirit from comment #4)

(In reply to Glenn Watson [:gw] from comment #2)

or whether we should instead change Gecko to use the intended video upload path here, which already handles the clipping case and is much more efficient.

What's the name of the intended video upload path? Where is it? Searchfox link(s) please.

Instead of using the RawData variant in ExternalImageSource [1], ideally Gecko would instead provide a native texture handle (the NativeTexture variant).

[1] https://searchfox.org/mozilla-central/source/gfx/wr/webrender_api/src/image.rs#66

As I have zero knowledge about web render (and the details about how we propagate the image to the compositor), I don't know where web render determines the external image source, but could web render send that RawData image to GPU process again and upload image there, not from CPU?

For more context in the media playback side, usually we would copy data into this image (cropping will be done by us when copying data) which has a shmem texture. In bug 1713276, we skip this step and allocate shmem earlier for decoder, so that we can share that shmem later directly without copying them into shmem again.

Jeff, do you know who / which team would own the code / responsibility for doing the upload of these video surfaces to a GL surface? Is it something we could consider doing instead of fixing up WR to handle the slow path correctly?

I wonder if we could do upload video surfaces to GL surfaces like Bug 1536515.

I am not sure how to implement any of the approaches being discussed here. If anybody posts a clearer sketch of a solution, I'm willing to take it on.

(In reply to Sotaro Ikeda [:sotaro] from comment #9)

I wonder if we could do upload video surfaces to GL surfaces like Bug 1536515.

It could be implemented like D137724. But if platform supports cross process direct binding texture, it seems better to use it.
For example, on Windows, D3D11YCbCrImage exists. It is nice if we could use it directly instead of SharedPlanarYCbCrImage.

Couldn't we just limit the size when instantiating RenderTextureHost, which would essentially handle mPicSize without this huge amount of changes being proposed here adding new RenderTextureHosts? I also am worried that for RenderCompositorSWGL, where we don't support hardware surfaces at all, that this fix won't work.

Do we have a testcase that can be used to illustrate the problem, so that alternative fixes could be explored?

STR.

1. comment out these lines and rebuild Firefox
2. set media.hardware-video-decoding.enabled=false
3. ensure media.ffmpeg.customized-buffer-allocation is true
4. go to this video
5. select its resolution to 1080p or below
6. You will see the result like this

Note. if you don't see the issue, ensure you're using ffvpx vp9 decoder, not the platform decoder. That can be confirmed by using MediaPanel.

Currently we already use an alternative workaround which is to explicitly crop the surface.

This makes WR properly handle mPicSize when RenderBufferTextureHost is used.
The main change is that we need to take care to pass in display().Size() from
the descriptor, and then further use that to carefully limit the size of the
CbCr texture, as it doesn't necessarily maintain an appropriate half-sized
scale with respect to the Y texture if it is padded.

Given that mPicSize should now actually work, we should no longer need any
of the previous mCroppedSize mechanisms that were added to work around this,
and so they are removed in this patch.

https://hg.mozilla.org/mozilla-central/rev/015fea9ab135