1058364 - MediaSource streams don't correctly handle the end of sub decoders

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

When a subdecoder in a media source is ended due to switching streams or seeking the entire media source is treated by the decoding pipeline as having ended. 

When new data for the stream being switched too is added it is ignored as a result. This causes video to stop playback when seeking on YouTube.

Comment 1

[cajbir (:cajbir)]

This bug was spawned from bug 1041374 comment 14.

Comment 2

[cajbir (:cajbir)]

Attachment: Don't make end of a stream as end of the media source

Don't treat the end of an individual media resource as the end of the entire media source.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8478792 [details] [diff] [review]
Don't make end of a stream as end of the media source

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

::: content/media/mediasource/MediaSourceDecoder.cpp
@@ +130,5 @@
>  
>  void
> +MediaSourceDecoder::Ended()
> +{
> +  mReader->Ended();

Need to hold the monitor.

::: content/media/mediasource/MediaSourceReader.cpp
@@ +133,5 @@
>              this, mVideoReader.get(), mDecoders.Length());
>    if (SwitchReaders(SWITCH_FORCED)) {
>      // Success! Resume decoding with next video decoder.
>      RequestVideoData(false, mTimeThreshold);
> +  } else if (mEnded) {

The monitor isn't held here, so mEnded is being access unprotected.

@@ +139,5 @@
>      MSE_DEBUG("MediaSourceReader(%p)::OnVideoEOS reader=%p EOS (readers=%u)",
>                this, mVideoReader.get(), mDecoders.Length());
>      GetCallback()->OnVideoEOS();
>    }
> +  else {

} else {

@@ +159,5 @@
> +    };
> +    MSE_DEBUG("MediaSourceReader(%p)::OnVideoEOS reader=%p ended, media source not ended",
> +              this, mVideoReader.get());
> +
> +    RefPtr<nsIRunnable> task(new RequestVideoDataTask(this, mTimeThreshold));

Why can't you call RequestVideoData directly?

If a new decoder isn't ready to respond with frames yet, we're going to keep hitting this path at 1/frame_duration Hz, which is not ideal but acceptable for now.  Need a follow up bug to address that issue.

::: content/media/mediasource/MediaSourceReader.h
@@ +86,5 @@
>  
>    // Return true if any of the active decoders contain data for the given time
>    bool DecodersContainTime(double aTime);
>  
> +  // Mark the reader to indicate that the MediaSource has completed.

"indicate that EndOfStream has been called on our MediaSource" or something

@@ +111,5 @@
>  
>    nsRefPtr<MediaDecoderReader> mAudioReader;
>    nsRefPtr<MediaDecoderReader> mVideoReader;
> +
> +  bool mEnded;

Seek's while loop should bail if this is true too, since there's no point waiting for more data.

Comment 4

[cajbir (:cajbir)]

Attachment: Don't make end of a stream as end of the media source

Address review comments. Raised bug 1058422 for frequency of reading video data issue.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8478848 [details] [diff] [review]
Don't make end of a stream as end of the media source

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

r+ with comments addressed

::: content/media/mediasource/MediaSourceDecoder.cpp
@@ +131,5 @@
>  void
> +MediaSourceDecoder::Ended()
> +{
> +  ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> +  mReader->Ended();

mon.NotifyAll() since we're using this as a signal to bail out of the WaitForData() loop.

::: content/media/mediasource/MediaSourceReader.cpp
@@ +140,5 @@
>                this, mVideoReader.get(), mDecoders.Length());
>      GetCallback()->OnVideoEOS();
> +  } else {
> +    // If a new decoder isn't ready to respond with frames yet, we're going to
> +    // keep hitting this path at 1/frame_duration Hz. Bug XXXXXX is raised to

Fill in bug #.

@@ +423,5 @@
>      static_cast<MediaSourceDecoder*>(mDecoder)->WaitForData();
>      SwitchReaders(SWITCH_FORCED);
>    }
>  
> +  if (IsShutdown() || IsEnded()) {

This will cause the seek to bail any time the MediaSource is marked EOS.

@@ +428,1 @@
>      return NS_OK;

This should return NS_ERROR_FAILURE.

Comment 6

[cajbir (:cajbir)]

Attachment: Don't make end of a stream as end of the media source

Address review comments. Carry forward r+.

Comment 7

[cajbir (:cajbir)]

Attachment: Don't make end of a stream as end of the media source

Address review comments. Carry forward r+.

Comment 8

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa272c7141e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfa272c7141e