1336792 - RemoteDataDecoder setting wrong duration on decoded frame

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

The Video RemoteDataDecoder to set the duration on the decoded frame, on Input enter the frame's duration into the queue.
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/media/platforms/android/RemoteDataDecoder.cpp#270
(Note that the duration is entered in the queue after inputting the data to the decoder, this is racy, there's no guarantee that the decoder won't be finished prior the duration being set. When this happens, the decoded frames will be incorrectly dropped)

The frame's duration is then retrieved when the video is decoded:
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/media/platforms/android/RemoteDataDecoder.cpp#151

This is incorrect.
The frames comes into decode order, and so are the duration. The decoder however returns data in presentation order. The two orders aren't the same.
As such, the duration you first input, doesn't necessarily matches the duration of the first decoded frame.

We can't use a queue to store the durations. We must use a map from the pts to the duration. Similar to what is done with the ffmpeg video decoder.

Comment 1

[Blake Wu [:bwu][:blakewu]]

Yeah. Display order and decoded order are normally not the same. 
Good catch!
Ni John to check it.

Comment 2

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

Attachment: Bug 1336792 - part 1: extract DurationMap and make it thread safe.

Also fix a bug that outputs not sent for rendering are not released immediately.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834823 [details]
Bug 1336792 - part 1: extract DurationMap and make it thread safe.

https://reviewboard.mozilla.org/r/110658/#review112552

Looks good, but let's reuse existing structure to do that job.

Can remove all your input duration map stuff. Though would have to make it thread safe.
Or create the VideoData, create a slightly different Output that will set the pts properly, and then the duration map can be monitor/mutex free.

::: dom/media/platforms/android/RemoteDataDecoder.cpp:22
(Diff revision 1)
>  #include "nsPromiseFlatString.h"
>  #include "nsIGfxInfo.h"
>  
>  #include "prlog.h"
>  
> -#include <deque>
> +#include <map>

FWIW, considering the number of frames typically queued (about 4 on average) a simple array would provide a faster average access to it.

I have written such DurationMap class in the FFmpeg video decoder.
What about you use it instead?

extract it and use it.

https://hg.mozilla.org/mozilla-central/file/tip/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h#l90

::: dom/media/platforms/android/RemoteDataDecoder.cpp:154
(Diff revision 1)
>      }
>  
>      void HandleOutput(Sample::Param aSample) override
>      {
> -      Maybe<int64_t> durationUs = mDecoder->mInputDurations.Get();
> -      if (!durationUs) {
> +      UniquePtr<VideoData::Listener> releaseSample(
> +        new RenderOrReleaseOutput(mDecoder->mJavaDecoder, aSample));

I'm assuming here you're fixing an apparent leak of the EOS sample ?

maybe that would be better of split and put in another commit.

Comment 4

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

Attachment: Bug 1336792 - part 2: use pts to index input duration.

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

Comment 5

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

Attachment: Bug 1336792 - part 3: release outputs not sent for rendering.

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

Comment 6

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

Comment on attachment 8834823 [details]
Bug 1336792 - part 1: extract DurationMap and make it thread safe.

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

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834823 [details]
Bug 1336792 - part 1: extract DurationMap and make it thread safe.

https://reviewboard.mozilla.org/r/110658/#review112786

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8835926 [details]
Bug 1336792 - part 2: use pts to index input duration.

https://reviewboard.mozilla.org/r/111480/#review112790

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8835927 [details]
Bug 1336792 - part 3: release outputs not sent for rendering.

https://reviewboard.mozilla.org/r/111482/#review112792

you're going to bitrot my changes to RemoteDataDecoder now :(

Comment 10

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

Comment on attachment 8835927 [details]
Bug 1336792 - part 3: release outputs not sent for rendering.

https://reviewboard.mozilla.org/r/111482/#review112792

Thank goodness Git is good at rebasing. :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bea086dc1a72
part 1: extract DurationMap and make it thread safe. r=jya
https://hg.mozilla.org/integration/autoland/rev/6f2246b26c3f
part 2: use pts to index input duration. r=jya
https://hg.mozilla.org/integration/autoland/rev/1de67ff4b2b9
part 3: release outputs not sent for rendering. r=jya

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bea086dc1a72
https://hg.mozilla.org/mozilla-central/rev/6f2246b26c3f
https://hg.mozilla.org/mozilla-central/rev/1de67ff4b2b9