The default bug view has changed. See this FAQ.

MSE YouTube demo player doesn't seek into unbuffered range

RESOLVED FIXED in mozilla32

Status

()

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

People

(Reporter: kentuckyfriedtakahe, Assigned: cajbir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

Fix
10.63 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
Steps to reproduce:

* Navigate to http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings2.mpd
* Enter 60 into the box to the left of the seek button
* Click on seek button

Expected results:

The video should start playing from the 60 second mark in the video

Actual results:

Video shows "Video can't be played because the file is corrupt"
Log window shows:

7413: 1, Seeking to time, 60
7437: 1, resetSourceBuffer
7443: Error (3)


Alternative results:

Video stops playing. Log window shows:

5605: 1, Seeking to time, 60
5608: 1, resetSourceBuffer
6004: Paused, auto=false
6094: 2, Sent XHR: url=http://cd.pn/mse/ytdemo/feelings_vp9-20130806-242.webm, range=bytes=4459979-4478155
6094: No range in progress callback
6538: 3, Appending init
6587: Ending stream
7086: 2, Deregistering progress timer
Blocks: 1000608
Blocks: 974362
Blocks: 1000686
(Assignee)

Comment 1

3 years ago
Created attachment 8429052 [details] [diff] [review]
x.patch

Get seeking in unbuffered ranges working. This is a minimal patch that works but with a not very nice way of handling the waiting for additional data to arrive. It loops using a sleep.

The hope is to land this to allow MSE testing with seeking to work and I'll refactor the remainder of the patch to work on top of the async work in bug 979104. If you're ok with this I'll fill in the XXXXXX in the patch with a bug raised for that work.

If you're not ok with it can you give feedback on it and I'll continue the rebase on top of 979104 here.
Attachment #8429052 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Comment on attachment 8429052 [details] [diff] [review]
x.patch

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

Looks good, just a question and a few nits.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3054,5 @@
>    return mCORSMode != CORS_NONE;
>  }
>  
> +#ifdef PR_LOGGING
> +static const char* gReadyStateToString[] = {

This doesn't seem to need to move, or am I missing something?

::: content/media/mediasource/MediaSourceDecoder.cpp
@@ +396,5 @@
> +  {
> +  }
> +
> +  NS_IMETHOD Run() MOZ_OVERRIDE MOZ_FINAL {
> +    auto owner = mDecoder->GetOwner();

First use of auto in content/media?

\o/

@@ +404,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  AbstractMediaDecoder* mDecoder;

This should be an nsRefPtr, otherwise the decoder could disappear out from under us.

@@ +422,5 @@
> +  // MediaDecoderStateMachine. Bug 979104 implements what we need and
> +  // we'll remove this for an async approach based on that in bug XXXXXXX.
> +  while (!mMediaSource->ActiveSourceBuffers()->AllContainsTime (aTime / USECS_PER_S)
> +         && !IsShutdown()) {
> +    PR_Sleep(PR_MillisecondsToInterval(100));

I know this is a short term fix, but how would you feel about adding a monitor to the MediaSource object which we can Wait() on here and NotifyAll() on in each SourceBuffer after an AppendBuffer()?
Attachment #8429052 - Flags: review?(kinetik)
(Assignee)

Comment 3

3 years ago
(In reply to Matthew Gregan [:kinetik] from comment #2)
> This doesn't seem to need to move, or am I missing something?

This may be the remains of my ripping out the changes to the decoder state machine for this patch. I'll remove if needed in the updated patch.

> I know this is a short term fix, but how would you feel about adding a
> monitor to the MediaSource object which we can Wait() on here and
> NotifyAll() on in each SourceBuffer after an AppendBuffer()?

Sure, no problem. I'll do this tomorrow, along with your other suggestions and put a new patch up. Thanks!
(Assignee)

Comment 4

3 years ago
Created attachment 8432807 [details] [diff] [review]
Fix

Addresses review comments.
Attachment #8429052 - Attachment is obsolete: true
Attachment #8432807 - Flags: review?(kinetik)
Attachment #8432807 - Flags: review?(kinetik) → review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1e2a3d0c6f
https://hg.mozilla.org/mozilla-central/rev/3d1e2a3d0c6f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.