Make MP4Reader decode audio and video data asynchronous

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpearce, Assigned: eflores)

Tracking

29 Branch
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8448465 [details] [diff] [review]
WIP patch, bitrotten

MP4Reader should be made asynchronous, so that decoding video doesn't block audio decoding concurrently.

The infrastructure for this landed in bug 979104.

We can achieve this by implementing MediaDecoderReader::RequestAudioData() and MediaDecoderReader::RequestVideoData(), and returning decoded samples via the RequestSampleCallback.

The attached patch is a WIP patch that I tested bug 979104 with. It is based on an older version of my patch for bug 979104, so does not apply cleanly.
(Reporter)

Comment 1

4 years ago
Created attachment 8459331 [details] [diff] [review]
WIP unbitrotten, seeking still unreliable

I updated this patch over the weekend. Playback and seeking work, but seeking is unreliable if you scrub the seek bar quickly.
Attachment #8448465 - Attachment is obsolete: true
Created attachment 8517855 [details] [diff] [review]
wip rebased

Seeking still broken.

Seems to break when a seek is interrupted by another seek, causing a SEEKING->SEEKING transition. Works fine when state goes SEEKING->PLAYING->SEEKING.
Attachment #8459331 - Attachment is obsolete: true
(Reporter)

Comment 3

3 years ago
The problem with seeking seems to be that we're not receiving a RequestAudioData() call after seeking (usually only when scrubbing seeking). Calling RequestAudioData() after the SeekAudio() call in MP4Reader::Seek() makes seeking work, but we should fix the base problem (probably in the StateMachine), of RequestAudioData() not always being called after a seek.

Comment 4

3 years ago
Check the mAudioRequestPending or mVideoRequestPending flag. See https://bugzilla.mozilla.org/show_bug.cgi?id=1090991#c0.
(Reporter)

Comment 5

3 years ago
Created attachment 8517871 [details] [diff] [review]
Seek stalling fix

Figured out the seek stalling problem. Here's a fix...

We track whether we've called RequestAudioData() by setting the flag MediaDecoderStateMachine::mAudioRequestPending. We reset this flag in MDSM::OnAudioDecoded() and MDSM::OnNotDecoded().

The sequence of events leading up to the failure is:
* We queue a task to call RequestAudioData(), and set mAudioRequestPending.
* The task runs.
* Before the MP4Reader can output a sample, we seek.
* The MP4Reader flushes its state, and cancels forgets that it's supposed to deliver output.
* At the end of MDSM::DecodeSeek() we call DispatchDecodeTasksIfNeeded(), which sees that mAudioRequestPending is true, and refrains from requesting another audio decode task.
* Ergo, RequestAudioData never gets called, and we stall.

We can either reset mAudioRequestPending after we seek, or we can have MDR::Seek() implementations calls OnNotDecoded() if they cancel delivering samples due to a seek request...

I'm more inclined towards the former approach, as I think that the next time we make a read async, we'll forget to call OnNotDecoded() if we take the later approach, and we'll end up encountering this bug again...
(Reporter)

Comment 6

3 years ago
(In reply to cajbir (:cajbir) from comment #4)
> Check the mAudioRequestPending or mVideoRequestPending flag. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1090991#c0.

Ah yes, that's the same, thanks!
(Reporter)

Comment 7

3 years ago
Resetting m*RequestPending:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84038141ea06

With OnNotDecoded called instead:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=58a1ee007386

I'm not sure which of these is better, as there's potential for state getting out of sync either way.
(Reporter)

Comment 8

3 years ago
Turns out that disabling non-MP4 codecs to speed up testing locally causes all kinds of errors on TBPL...

Try again with OnNotDecoded called:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bdf75aeb0320
the patch comment contains references to bug 1038031 in place of 1032633.

Also, note that the current patch breaks HE-AAC playback. With this video http://www-tvline-com.vimg.net/streaming/tvline/MAA101-Medianet.mp4 Audio is played at half its framerate.

It is imperative that the audio player is configured and created only after the first audio frame has been decoder, as otherwise MP4Reader::ReadMetadata will incorrectly reports a frame rate that is half of what it should be.
(Reporter)

Comment 10

3 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Also, note that the current patch breaks HE-AAC playback. With this video
> http://www-tvline-com.vimg.net/streaming/tvline/MAA101-Medianet.mp4 Audio is
> played at half its framerate.
> 
> It is imperative that the audio player is configured and created only after
> the first audio frame has been decoder, as otherwise MP4Reader::ReadMetadata
> will incorrectly reports a frame rate that is half of what it should be.

This should not be an issue once Bug 1065827 has landed, right?
(In reply to Chris Pearce (:cpearce) from comment #10)
> This should not be an issue once Bug 1065827 has landed, right?

it will still be...

with bug 1065827, we call MediaDecoderReader::ReadMetadata and if we have a duration and video dimensions we immediately emit loadedmetadata.
Then we queue for the first audio/video frame. When those arrive the MDSM calls the new method MediaDecoderReader::ReadUpdatedMetadata which gets the updated MediaInfo (and the right framerate). Then loadeddata is fired. 

Currently, only then is the audio output setup.

I haven't looked in detail on how the patch is doing the async decode, it may be just a matter of calling MediaDecoderReader::ReadUpdatedMetadata in the right spot (and it's also possible I didn't rebase it properly and broke it).

BTW, I've also noticed that with this patch applied, often Youtube with DASH/MP4 has regressed, it often jumps to the next video too early, or stall etc...
(Reporter)

Comment 12

3 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #11)
> (In reply to Chris Pearce (:cpearce) from comment #10)
> > This should not be an issue once Bug 1065827 has landed, right?
> 
> it will still be...
> 
> with bug 1065827, we call MediaDecoderReader::ReadMetadata and if we have a
> duration and video dimensions we immediately emit loadedmetadata.
> Then we queue for the first audio/video frame. When those arrive the MDSM
> calls the new method MediaDecoderReader::ReadUpdatedMetadata which gets the
> updated MediaInfo (and the right framerate).

Shouldn't the framerate be in the container format's init data?

> Then loadeddata is fired. 
> 
> Currently, only then is the audio output setup.

Based on what you described, I'd expect it to work. There must be a bug somewhere...


> 
> I haven't looked in detail on how the patch is doing the async decode, it
> may be just a matter of calling MediaDecoderReader::ReadUpdatedMetadata in
> the right spot (and it's also possible I didn't rebase it properly and broke
> it).
> 
> BTW, I've also noticed that with this patch applied, often Youtube with
> DASH/MP4 has regressed, it often jumps to the next video too early, or stall
> etc...

I think you should wait until we get this patch green on Try before you start worrying about that. Edwin already fixed one issue relating to reporting error instead of EOS, which may cause the problem you're seeing here.
(In reply to Chris Pearce (:cpearce) from comment #12)
> > duration and video dimensions we immediately emit loadedmetadata.
> > Then we queue for the first audio/video frame. When those arrive the MDSM
> > calls the new method MediaDecoderReader::ReadUpdatedMetadata which gets the
> > updated MediaInfo (and the right framerate).
> 
> Shouldn't the framerate be in the container format's init data?
> 

It does, but only of the basic AAC-LC stream. So in the video linked to above that's 24kHz. When you start decoding it, the SBR part is detected and the decoder sees it as 48kHz
(Reporter)

Comment 14

3 years ago
Oh, sorry I thought you meant the video frame rate, not the audio sample rate. Having to update the audio sample rate makes sense.
Created attachment 8520392 [details] [diff] [review]
mp4reader-async-folded.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=858c4350f79f
Attachment #8517855 - Attachment is obsolete: true
Attachment #8519694 - Attachment is obsolete: true
Attachment #8520392 - Flags: review?(kinetik)
Attachment #8517871 - Attachment is obsolete: true
Comment on attachment 8520392 [details] [diff] [review]
mp4reader-async-folded.patch

Review of attachment 8520392 [details] [diff] [review]:
-----------------------------------------------------------------

r+, just a few nits

::: dom/media/fmp4/MP4Reader.cpp
@@ +36,5 @@
> +
> +#define VLOG(...) \
> +    PR_BEGIN_MACRO \
> +      if (PR_GetEnv("LOG_MP4_SAMPLES")) { \
> +        PR_LOG(GetDemuxerLog(), PR_LOG_DEBUG, (__VA_ARGS__)); \

This ends up being kind of complicated, you need to know the log module, level, *and* a special env var.  And I suspect calling PR_GetEnv in hotter areas of code is a bad idea and we should stop doing it, especially since with bug 806819 logging is enabled by default in all builds.

Can we just make this PR_LOG_DEBUG+1 like we do elsewhere for extra-verbose logging?

@@ +189,5 @@
>    if (mPlatform) {
> +    // PDMs are supposed to be destroyed on the main thread...
> +    nsRefPtr<DestroyPDMTask> task(new DestroyPDMTask(mPlatform));
> +    MOZ_ASSERT(!mPlatform);
> +    NS_DispatchToMainThread(task, NS_DISPATCH_NORMAL);

NS_DispatchToMainThread(task);

@@ +667,5 @@
> +    GetCallback()->OnNotDecoded(MediaData::AUDIO_DATA,
> +                                RequestSampleCallback::END_OF_STREAM);
> +  } else if (aTrack == kVideo) {
> +    GetCallback()->OnNotDecoded(MediaData::VIDEO_DATA,
> +                                RequestSampleCallback::END_OF_STREAM);

GetCallback()->OnNotDecoded(aTrack == kVideo ? MediaData::VIDEO_DATA : MediaData::AUDIO_DATA,
                            RequestSampleCallback::END_OF_STREAM);

to avoid duplicating the code.

@@ +756,5 @@
> +    MonitorAutoLock mon(data.mMonitor);
> +    data.mError = true;
> +  }
> +  GetCallback()->OnNotDecoded(aTrack == kVideo ?
> +                                MediaData::VIDEO_DATA : MediaData::AUDIO_DATA,

Indentation.

::: dom/media/fmp4/MP4Reader.h
@@ +84,5 @@
> +  void Update(TrackType aTrack);
> +
> +  // Enqueues a task to call Update(aTrack) on the decoder task queue.
> +  // Lock for corresponding track must be held.
> +  void locked_ScheduleUpdate(TrackType aTrack);

Remove the locked_ prefix, since there's no unlocked ScheduleUpdate and the comment and has-lock assertion in the code are sufficient.

@@ +199,5 @@
>    nsAutoPtr<mp4_demuxer::MP4Sample> mQueuedVideoSample;
>  
> +  // Returns true when the decoder for this track needs input.
> +  // aDecoder.mMonitor must be locked.
> +  bool locked_NeedInput(DecoderData& aDecoder);

Same thing as locked_ScheduleUpdate.
Attachment #8520392 - Flags: review?(kinetik) → review+
Backed out for causing Windows leaks (took me awhile to find the bug given how the commit message was pointing to the wrong one too...). Double-check before pushing in the future kthxbye.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26d34236c4e

https://treeherder.mozilla.org/logviewer.html#?job_id=3913858&repo=mozilla-inbound
Assignee: nobody → edwin
Created attachment 8526356 [details] [diff] [review]
Rebased patch

https://tbpl.mozilla.org/?tree=Try&rev=3f541bf0ac9e

Rebased before refcounted MediaData landed. Leak is almost certainly because mOutput queue is holding raw pointers and we don't always use all the frames.
Created attachment 8526464 [details] [diff] [review]
Rebased and leak fixed

Rebased on top of the refcounted MediaData changed, and fixed the leak.

The change is that MP4Reader::Update returns any available output frames before returning EOS rather than the other way around.
Attachment #8526356 - Attachment is obsolete: true
I also switched std::queue to nsTArray so we could refcount the array.
Attachment #8526464 - Flags: review?(cpearce)
(Reporter)

Comment 23

3 years ago
Comment on attachment 8526464 [details] [diff] [review]
Rebased and leak fixed

Review of attachment 8526464 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the changes since last review. Ship it!
Attachment #8526464 - Flags: review?(cpearce) → review+
(Reporter)

Updated

3 years ago
Depends on: 1103648
Depends on: 1106763
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.