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

RESOLVED FIXED in Firefox 54

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jya, Assigned: jya)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
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
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1337046
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-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 10

a year ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1337559
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8834647 - Attachment is obsolete: true

Comment 17

a year ago
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

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b9ab508b1f1
https://hg.mozilla.org/mozilla-central/rev/4df45b91e00a
Status: NEW → RESOLVED
Last Resolved: a year 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.