1056441 - Remove MediaSource's WaitForData seeking mechanism and replace with async use of MSDM task queue

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

Doesn't seem to be a bug covering this piece of work, so filing one.

WaitForData was a stop-gap mechanism to gain some functionality until there was time to complete a better solution.  This bug covers the better solution.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: avoid-waiting-for-data

Does this seem like what we want?

Still a few unanswered questions and things that need tidying up.

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8514027 [details] [diff] [review]
avoid-waiting-for-data

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

This looks good, although (not having thought too deeply about how to implement this) I'm surprised it doesn't require changes in the MediaDecoderStateMachine.  It looks like returning NS_OK from Seek and processing it later could confuse the MDSM, since after Seek returns it assumes the Seek has completed successfully and kicks off a bunch of decoding tasks.

:cajbir was going to work on this (see bug 1002297 comment 1), so he may have more thoughts to add.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8514027 [details] [diff] [review]
avoid-waiting-for-data

f+ as this part looks good

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> Comment on attachment 8514027 [details] [diff] [review]
> avoid-waiting-for-data
> 
> Review of attachment 8514027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, although (not having thought too deeply about how to
> implement this) I'm surprised it doesn't require changes in the
> MediaDecoderStateMachine.  It looks like returning NS_OK from Seek and
> processing it later could confuse the MDSM, since after Seek returns it
> assumes the Seek has completed successfully and kicks off a bunch of
> decoding tasks.
> 
> :cajbir was going to work on this (see bug 1002297 comment 1), so he may
> have more thoughts to add.

You're right, it does need changes to the state machine, I think I was just getting lucky when this worked the other day.

The problem is that the state machine keeps requesting video frames from the reader while we're waiting for the data to become available.

If we get to EOS before we actually perform the seek, then MediaSourceReader::OnVideoEOS will silently swallow the notification (since SwitchVideoReader fails, but IsEnded is false). MediaDecoderStateMachine never gets a reply, so it waits for the OnVideoDecoded/OnVideoEOS notification forever.

My plan is to change the api for MediaDecoderReader::Seek to be properly asynchronous with an 'OnSeekCompleted' callback via the RequestSampleCallback interface. MDSM can then avoid requesting video frames while we're waiting for the seek to be completed.

We might also want to handle the case where MediaSourceReader::OnVideoEOS doesn't forward the notification onto MSDM, since it seems possible for that to happen in other situations. There might be a bug for that already, it sounds familiar.

Comment 5

[Matt Woodrow (:mattwoodrow)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)

> We might also want to handle the case where MediaSourceReader::OnVideoEOS
> doesn't forward the notification onto MSDM, since it seems possible for that
> to happen in other situations. There might be a bug for that already, it
> sounds familiar.

Yep, looks like cajbir ran into this already (bug 1062055 comment 11).

Comment 6

[cajbir (:cajbir)]

Also see bug 1090991 for bholley's take on the same issue.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Make seeking asynchronous

I'm not sure if adding a proper MDSM state would be better than the bool, but it seemed more error prone.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Tested this one more thoroughly, and seems to work really well. It also works when seeking a second time before the first one has completed.

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8516363 [details] [diff] [review]
Make seeking asynchronous

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

Awesome.

::: dom/media/MediaDataDecodedListener.h
@@ +79,5 @@
> +    RefPtr<nsIRunnable> task(NS_NewRunnableMethodWithArg<nsresult>(mTarget,
> +                                                                   &Target::OnSeekCompleted,
> +                                                                   aResult));
> +    if (NS_FAILED(mTaskQueue->Dispatch(task))) {
> +      NS_WARNING("Failed to dispatch OnAudioDecoded task");

Please update the warning text.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2139,5 @@
> +        mReader->Seek(seekTime,
> +                      mStartTime,
> +                      mEndTime,
> +                      mCurrentTimeBeforeSeek);
> +        mWaitingForDecoderSeek = true;

mWaitingForDecoderSeek needs to be updated with the lock held.

::: dom/media/MediaDecoderStateMachine.h
@@ +915,5 @@
>    // True if we need to decode forwards to the seek target inside
>    // mCurrentSeekTarget.
>    bool mDecodeToSeekTarget;
>  
> +  bool mWaitingForDecoderSeek;

Add a comment for this please.

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +391,5 @@
> +{
> +  // It's possible that we race when reading this, but we should
> +  // only get false positives and post a task that won't end up doing
> +  // anything.
> +  if (mWaitingForSeekData) {

It's preferable to avoid accessing this unlocked to avoid warnings from Helgrind/TSAN/etc.

@@ +411,5 @@
> +    GetCallback()->OnSeekCompleted(NS_ERROR_FAILURE);
> +    return;
> +  }
> +
> +  if (!mAudioTrack && !mVideoTrack) {

I'm not sure this is possible, since MSR::ReadMetadata needs at least one track to succeed.

@@ +444,5 @@
> +MediaSourceReader::OnSeekCompleted(nsresult aResult)
> +{
> +  mPendingSeeks--;
> +  // Keep the most recent failed result (if any)
> +  if (!NS_SUCCEEDED(aResult)) {

NS_FAILED?

@@ +458,5 @@
> +
> +void
> +MediaSourceReader::AttemptSeek()
> +{
> +  // MDSM task thread

Assert this?

::: dom/media/mediasource/MediaSourceReader.h
@@ +150,5 @@
> +  // result we're going to forward onto our callback.
> +  uint32_t mPendingSeeks;
> +  nsresult mSeekResult;
> +
> +

Remove extra newline.

::: dom/media/wmf/WMFReader.h
@@ +1,1 @@
> +

Accidental removal?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8516363 [details] [diff] [review]
Make seeking asynchronous

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +211,5 @@
>    mDecodeThreadWaiting(false),
>    mDropAudioUntilNextDiscontinuity(false),
>    mDropVideoUntilNextDiscontinuity(false),
>    mDecodeToSeekTarget(false),
> +  mWaitingForDecoderSeek(false),

It looks like this flag can be combined with mDecodingFrozenAtStateDecoding which also prevents from dispatching decoding task while seeking is in the near future after coming back from dormant state.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> (In reply to Matthew Gregan [:kinetik] from comment #2)
> > Comment on attachment 8514027 [details] [diff] [review]
> > avoid-waiting-for-data
> > 
> > Review of attachment 8514027 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good, although (not having thought too deeply about how to
> > implement this) I'm surprised it doesn't require changes in the
> > MediaDecoderStateMachine.  It looks like returning NS_OK from Seek and
> > processing it later could confuse the MDSM, since after Seek returns it
> > assumes the Seek has completed successfully and kicks off a bunch of
> > decoding tasks.
> > 
> > :cajbir was going to work on this (see bug 1002297 comment 1), so he may
> > have more thoughts to add.
> 
> You're right, it does need changes to the state machine, I think I was just
> getting lucky when this worked the other day.
> 
> The problem is that the state machine keeps requesting video frames from the
> reader while we're waiting for the data to become available.
> 
> If we get to EOS before we actually perform the seek, then
> MediaSourceReader::OnVideoEOS will silently swallow the notification (since
> SwitchVideoReader fails, but IsEnded is false). MediaDecoderStateMachine
> never gets a reply, so it waits for the OnVideoDecoded/OnVideoEOS
> notification forever.

I fixed this in bug 1090991, and refactored the callback interface.

> My plan is to change the api for MediaDecoderReader::Seek to be properly
> asynchronous with an 'OnSeekCompleted' callback via the
> RequestSampleCallback interface. MDSM can then avoid requesting video frames
> while we're waiting for the seek to be completed.

That sounds sensible.

> We might also want to handle the case where MediaSourceReader::OnVideoEOS
> doesn't forward the notification onto MSDM, since it seems possible for that
> to happen in other situations. There might be a bug for that already, it
> sounds familiar.

My goal in bug 1090991 was for Request{Audio,Video}Data to always be answered with either On{Audio,Video}Decoded or OnNotDecoded. I think that should be the case now, so please file bugs if you find other places where we can drop this stuff on the floor.

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Bobby Holley (:bholley) from comment #11)
> 
> My goal in bug 1090991 was for Request{Audio,Video}Data to always be
> answered with either On{Audio,Video}Decoded or OnNotDecoded. I think that
> should be the case now, so please file bugs if you find other places where
> we can drop this stuff on the floor.

I think you've fixed it, I hadn't rebased against m-c tip when I posted that.

Comment 13

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82171e97db6e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82171e97db6e