1336947 - Provide ability to MediaDataDecoder to resolve a promise before the task has fully completed

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jean-Yves Avenard [:jya]]

Following bug 1319987, the MediaDataDecoder returns a promise when Decode and Drain are called.

It is expected that once either one of these promises are resolved, there's no more things the decoder can do until a new input is provided.

This can cause quite a problem, in particular with Drain in that there may be a high number of decoded frames to returned at once.

On Windows, the MFT typically holds about 30+ frames, so when draining we have to wait for all those frames to have finished being converted to BGRA, which can take a long time.
On Android, the RemoteDataDecoder uses a limited amount of memory buffer to hold decoded frames, which means that it may not be able to return all decoded frames at once as it would need to wait for the first drained frames' buffer to be available once again.

We should provide a mechanism that allows to return partial data and inform the MediaDataDecoder client (here the MediaFormatReader) that there are still some data pending, that it should restart the prior operation for the MediaDataDecoder to continue.

Comment 1

[Jean-Yves Avenard [:jya]]

One scenario I'm currently leaning toward would be for the MediaDataDecoder to resolve the DecodePromise with a new type of MediaData (say ResumeMediaData), that would indicate that the previous operation hasn't fully completed and that it needs to start again.

We would have a new API added to RefPtr<DecodePromise> MediaDataDecoder::Resume() that would be resolved once again.
MediaDataDecoder::Resume() would be called for as long as the decoder returns a ResumeMediaData inside the DecodedData array.
If we don't want to add a new API to MediaDataDecoder, and we wanted to resume MediaDataDecoder::Decode, we could pass a nullptr to it. But that's kind of messy.

Comment 2

[Jean-Yves Avenard [:jya]]

Issue with scenario above is that we would no longer be able to chain DecodePromise; may as well go back to a callback system and revert the entirety of bug 1319987

Comment 3

[Jean-Yves Avenard [:jya]]

Solution to be adopted:
Drain will be called several times until the DecodedData array returned is empty.

On Android, the DecodePromise will be resolved whenever a frame is returned, we won't always wait for onInputExhausted to be called.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1336947: P1. Drain decoder until no more data is returned.

It may not be possible for a decoder to return all drained samples in one go. This is particularly a problem on android where there's a limited amount of memory buffers available.

So we no longer expect the operation to complete in one go, we call Drain until there are no more samples returned.

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

Comment 5

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

Comment on attachment 8834475 [details]
Bug 1336947: P1. Drain decoder until no more data is returned.

https://reviewboard.mozilla.org/r/110406/#review111704

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1336947: P2. Fix coding style.

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

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1336947: P3. Only attempt to drain FFmpeg decoder when decoder.

The FFmpeg can't be drained if the decoder just got created or if it has already been drained.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834475 [details]
Bug 1336947: P1. Drain decoder until no more data is returned.

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

Comment 9

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

Comment on attachment 8834646 [details]
Bug 1336947: P2. Fix coding style.

https://reviewboard.mozilla.org/r/110498/#review111758

r+ with fix needed:

::: dom/media/platforms/ffmpeg/FFmpegDecoderModule.h:75
(Diff revision 1)
>      if (aConfig.IsVideo() &&
> -        (aConfig.mMimeType.EqualsLiteral("video/avc") ||
> +        && (aConfig.mMimeType.EqualsLiteral("video/avc")

Remove the '&&' from the first line. How could this build??

Comment 10

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

Comment on attachment 8834647 [details]
Bug 1336947: P3. Only attempt to drain FFmpeg decoder when decoder.

https://reviewboard.mozilla.org/r/110500/#review111760

r+ with typo fixed:

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h:90
(Diff revision 1)
>    };
>  
>    PtsCorrectionContext mPtsContext;
>    int64_t mLastInputDts;
>  
> +  // Set to false if the decoder has anything to drained.

'to drained' -> 'to drain' or 'to be drained'

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834646 [details]
Bug 1336947: P2. Fix coding style.

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

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834647 [details]
Bug 1336947: P3. Only attempt to drain FFmpeg decoder when decoder.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834647 [details]
Bug 1336947: P3. Only attempt to drain FFmpeg decoder when decoder.

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

Comment 14

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

Comment on attachment 8834647 [details]
Bug 1336947: P3. Only attempt to drain FFmpeg decoder when decoder.

https://reviewboard.mozilla.org/r/110500/#review111798

(didn't notice before)

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h:1
(Diff revision 3)
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Summary in commit description is missing some words!
"... when decoder." -- When decoder what?

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834475 [details]
Bug 1336947: P1. Drain decoder until no more data is returned.

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

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834646 [details]
Bug 1336947: P2. Fix coding style.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b9ab508b1f1
P1. Drain decoder until no more data is returned. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4df45b91e00a
P2. Fix coding style. r=gerald

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/2b9ab508b1f1
https://hg.mozilla.org/mozilla-central/rev/4df45b91e00a