1351954 - gmp-clearkey's WMF decoder doesn't allocate its video frame buffer optimally

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, enhancement, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

gmp-clearkey's VideoDecoder::SampleToVideoFrame() function just allocates buffers to send video frames back to Gecko to be the same size that WMF's H.264 decoder outputs:

https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/media/gmp-clearkey/0.1/VideoDecoder.cpp#220

The WMF decoder outputs the planes 16-row aligned for some reason.

Us allocating with 16 row aligned means we waste memory.

Also it conflicts with the work I'm doing in bug 1351953. I'm removing the intr IPC to allocate shmems by pre-allocating the video frame shmems in the content process. This means I need to know what size to pre-allocate the shmems. But Widevine allocates the optimal size buffers. It would be easier if both Clearkey and Widevine had allocated buffers the same size. So since I can't patch Widevine's CDM, I'll patch ours so that gmp-clearkey and the Widevine CDM allocate buffers the same size.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1351954 - Change ClearKey CDM to allocate its video frame buffers optimally.

The WMF decoder gives us video frames in buffers with the planes 16 row-aligned
for some reason. If we allocate our video frames the same size we waste memory.
So let's have the ClearKey CDM not allocate its video frames with the extra
padding rows.

Excluding the padding in our copy of the decoded data also makes my work in bug
1351953 easier.

Review commit: https://reviewboard.mozilla.org/r/124940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124940/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8852773 [details]
Bug 1351954 - Change ClearKey CDM to allocate its video frame buffers optimally.

https://reviewboard.mozilla.org/r/124940/#review127534

::: media/gmp-clearkey/0.1/VideoDecoder.cpp:228
(Diff revision 1)
>    // i.e., Y, then U, then V.
>    uint32_t padding = 0;
>    if (aHeight % 16 != 0) {
>      padding = 16 - (aHeight % 16);
>    }
> -  uint32_t ySize = stride * (aHeight + padding);
> +  uint32_t srcYSize = stride * (aHeight + padding);

This looks wrong to me.

The y-plane size of the source frame should be: stride * (mDecoder->GetFrameHeight() + padding);

But now you pass mDecoder->GetPictureRegion().height to SampleToVideoFrame() at #172.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, I think you're right. Thanks for pointing that out!

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8852773 [details]
Bug 1351954 - Change ClearKey CDM to allocate its video frame buffers optimally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124940/diff/1-2/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

It would be good to have a class to encapsulate the low level and subtle calculation which is easy to get wrong.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Well bear in mind that the frame size padding is a WMF specific issue, so it's not clear to me how much we'd benefit by extracting that logic and reusing it elsewhere.

Regardless, it would be good if we can do that in a follow up, and get the fix here landed.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8852773 [details]
Bug 1351954 - Change ClearKey CDM to allocate its video frame buffers optimally.

Redirecting review to Gerald, since JW will be on holiday today and tomorrow...

I've filed bug 1352925 to address the refactoring JW suggested in a follow up.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8852773 [details]
Bug 1351954 - Change ClearKey CDM to allocate its video frame buffers optimally.

https://reviewboard.mozilla.org/r/124940/#review128354

::: media/gmp-clearkey/0.1/VideoDecoder.cpp:248
(Diff revision 2)
> -  uint64_t bufferSize = ySize + 2 * uSize;
> +  // Note: We allocate the minimal sized buffer required to send the
> +  // frame back over to the parent process. This is so that we request the
> +  // same sized frame as the buffer allocator expects.
> +  using mozilla::CheckedUint32;
> +  CheckedUint32 bufferSize = CheckedUint32(stride) * aPictureHeight +
> +                             (CheckedUint32(stride) * aPictureHeight) / 2;

x/4*2 is not always equal to x/2 (because of rounding down after the division), so you should do `/ 4 * 2`, unless you don't care about allocating one extra byte in probably-rare cases.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8852773 [details]
Bug 1351954 - Change ClearKey CDM to allocate its video frame buffers optimally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124940/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2d106b73e6a
Change ClearKey CDM to allocate its video frame buffers optimally. r=gerald

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2d106b73e6a