Closed Bug 1336947 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1336947: P1. Drain decoder until no more data is returned.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1336947: P2. Fix coding style.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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.
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.
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
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.
Blocks: 1337046
Comment on attachment 8834475 [details]
Bug 1336947: P1. Drain decoder until no more data is returned.

https://reviewboard.mozilla.org/r/110406/#review111704
Attachment #8834475 - Flags: review?(gsquelart) → review+
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??
Attachment #8834646 - Flags: review?(gsquelart) → review+
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'
Attachment #8834647 - Flags: review?(gsquelart) → review+
Blocks: 1337559
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?
Attachment #8834647 - Attachment is obsolete: true
Pushed by jyavenard@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2b9ab508b1f1
https://hg.mozilla.org/mozilla-central/rev/4df45b91e00a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: