1384495 - Decoded video frames could be missing or pending forever.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, enhancement)

Description

[John Lin [:jhlin][:jolin]]

Missing frames: During draining, RemoteDataDecoder::ReturnDecodedData() could incorrectly return empty decoded data before draining is actually complete. When that happens, no more Drain() will be called and all pending frames will be lost. It should check if mDrainStatus is DRAINED or not.

Pending forever: CodecProxy stops notifying input status once it receives EOS input. That was necessary for the old onInputExhausted() callback but is not the right behavior for onInputStatus() which native implementation requires to resolve promise.

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1384495 - p1: remove end of input flag.

The flag is needed only for the old onInputExhausted() callback and should not be used for onInputStatus(). (It prevents native implementation from receiving notification required for resolving promise.)

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

Comment 2

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1384495 - p2: resolve drain promise with empty decoded data only when draining complete.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8890299 [details]
Bug 1384495 - p2: resolve drain promise with empty decoded data only when draining complete.

https://reviewboard.mozilla.org/r/161416/#review166720

::: dom/media/platforms/android/RemoteDataDecoder.cpp:612
(Diff revision 1)
>    // We only want to clear mDecodedData when we have resolved the promises.
>    if (!mDecodePromise.IsEmpty()) {
>      mDecodePromise.Resolve(mDecodedData, __func__);
>      mDecodedData.Clear();
>    } else if (!mDrainPromise.IsEmpty()) {
> +    if (!mDecodedData.IsEmpty() || mDrainStatus == DrainStatus::DRAINED) {

please merge the two logical expressions (so it's all in else if (...)

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8890298 [details]
Bug 1384495 - p1: remove end of input flag.

https://reviewboard.mozilla.org/r/161414/#review166750

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8890298 [details]
Bug 1384495 - p1: remove end of input flag.

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

Comment 6

[John Lin [:jhlin][:jolin]]

Comment on attachment 8890299 [details]
Bug 1384495 - p2: resolve drain promise with empty decoded data only when draining complete.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c847269e1c8
p1: remove end of input flag. r=esawin
https://hg.mozilla.org/integration/autoland/rev/e53245c9e2a8
p2: resolve drain promise with empty decoded data only when draining complete. r=jya

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/4c847269e1c8
https://hg.mozilla.org/mozilla-central/rev/e53245c9e2a8