Last Comment Bug 1002297 - MSE YouTube demo player doesn't seek into unbuffered range
: MSE YouTube demo player doesn't seek into unbuffered range
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Linux
-- normal with 1 vote (vote)
: mozilla32
Assigned To: cajbir (:cajbir)
:
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
http://cd.pn/mse/ytdemo/dash-player.h...
Depends on:
Blocks: MSE 974362 1000608 1000686
  Show dependency treegraph
 
Reported: 2014-04-27 19:07 PDT by Anthony Jones (:kentuckyfriedtakahe, :k17e)
Modified: 2014-06-03 05:55 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
x.patch (7.30 KB, patch)
2014-05-26 23:31 PDT, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
Fix (10.63 KB, patch)
2014-06-02 14:46 PDT, cajbir (:cajbir)
kinetik: review+
Details | Diff | Splinter Review

Description User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-04-27 19:07:12 PDT
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 User image cajbir (:cajbir) 2014-05-26 23:31:44 PDT
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.
Comment 2 User image Matthew Gregan [:kinetik] 2014-05-27 21:49:39 PDT
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 User image cajbir (:cajbir) 2014-05-28 02:06:33 PDT
(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 User image cajbir (:cajbir) 2014-06-02 14:46:45 PDT
Created attachment 8432807 [details] [diff] [review]
Fix

Addresses review comments.
Comment 6 User image Carsten Book [:Tomcat] 2014-06-03 05:44:07 PDT
https://hg.mozilla.org/mozilla-central/rev/3d1e2a3d0c6f

Note You need to log in before you can comment on or make changes to this bug.