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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Assignee: nobody → cajbir.bugzilla
Blocks: 1056440
Blocks: 1041374
This bug was spawned from bug 1041374 comment 14.
Don't treat the end of an individual media resource as the end of the entire media source.
Attachment #8478792 - Flags: review?(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.
Attachment #8478792 - Flags: review?(kinetik) → review-
Address review comments. Raised bug 1058422 for frequency of reading video data issue.
Attachment #8478792 - Attachment is obsolete: true
Attachment #8478848 - Flags: review?(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.
Attachment #8478848 - Flags: review?(kinetik) → review+
Address review comments. Carry forward r+.
Attachment #8478848 - Attachment is obsolete: true
Attachment #8478860 - Flags: review+
Attachment #8478860 - Attachment is obsolete: true
Attachment #8478860 - Attachment is patch: false
Address review comments. Carry forward r+.
Attachment #8478863 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bfa272c7141e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.