Closed Bug 1002297 Opened 7 years ago Closed 7 years ago

MSE YouTube demo player doesn't seek into unbuffered range

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ajones, Assigned: cajbir)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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: 974362
Attached patch x.patch (obsolete) — Splinter Review
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)
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)
(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!
Attached patch FixSplinter Review
Addresses review comments.
Attachment #8429052 - Attachment is obsolete: true
Attachment #8432807 - Flags: review?(kinetik)
Attachment #8432807 - Flags: review?(kinetik) → review+
https://hg.mozilla.org/mozilla-central/rev/3d1e2a3d0c6f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.