Closed Bug 1336792 Opened 6 years ago Closed 6 years ago

RemoteDataDecoder setting wrong duration on decoded frame

Categories

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

Unspecified
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jya, Assigned: jhlin)

References

Details

Attachments

(3 files)

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.
Yeah. Display order and decoded order are normally not the same. 
Good catch!
Ni John to check it.
Flags: needinfo?(jolin)
Assignee: nobody → jolin
Flags: needinfo?(jolin)
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.
Attachment #8834823 - Flags: review?(jyavenard)
Comment on attachment 8834823 [details]
Bug 1336792 - part 1: extract DurationMap and make it thread safe.

https://reviewboard.mozilla.org/r/110658/#review112786
Attachment #8834823 - Flags: review?(jyavenard) → review+
Comment on attachment 8835926 [details]
Bug 1336792 - part 2: use pts to index input duration.

https://reviewboard.mozilla.org/r/111480/#review112790
Attachment #8835926 - Flags: review?(jyavenard) → review+
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 :(
Attachment #8835927 - Flags: review?(jyavenard) → review+
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. :)
Pushed by jolin@mozilla.com:
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
Depends on: 1338932
You need to log in before you can comment on or make changes to this bug.