Closed
Bug 1058364
Opened 10 years ago
Closed 10 years ago
MediaSource streams don't correctly handle the end of sub decoders
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: cajbir, Assigned: cajbir)
References
Details
Attachments
(1 file, 3 obsolete files)
7.03 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
This bug was spawned from bug 1041374 comment 14.
Assignee | ||
Comment 2•10 years ago
|
||
Don't treat the end of an individual media resource as the end of the entire media source.
Attachment #8478792 -
Flags: review?(kinetik)
Comment 3•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Address review comments. Carry forward r+.
Attachment #8478848 -
Attachment is obsolete: true
Attachment #8478860 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8478860 -
Attachment is obsolete: true
Attachment #8478860 -
Attachment is patch: false
Assignee | ||
Comment 7•10 years ago
|
||
Address review comments. Carry forward r+.
Attachment #8478863 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
Comment 9•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•