1002297 - MSE YouTube demo player doesn't seek into unbuffered range

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 1

[cajbir (:cajbir)]

Attachment: 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.

Comment 2

[Matthew Gregan [:kinetik]]

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()?

Comment 3

[cajbir (:cajbir)]

(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!

Comment 4

[cajbir (:cajbir)]

Attachment: Fix

Addresses review comments.

Comment 5

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1e2a3d0c6f

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d1e2a3d0c6f