1058429 - Improve the performance when suspend/resume a HTMLMediaElement on B2G.

Status: RESOLVED DUPLICATE of bug 1062134

(Core :: Audio/Video, defect)

Description

[Benjamin Chen [:bechen]]

Fork from bug 1050664 comment 9.

On B2G platform, use browser to play a streaming video, suspend/resume a MediaElement will trigger the StateMachine change state from DORMANT to METADATA. The StateMachine assumes that the first decoded data after METADATA state is "the beginning of the steam" because StateMachine wants to update mStartTime. The assumption make the underlying decoder must rewind the stream, otherwise the mStartTime is wrong.

If we can reduce the "rewind" behavior or "update mStartTime", we may save one network operation for RTSP, and one extra HTTP download for HTTP streaming(if MediaCache miss).

Comment 1

[Benjamin Chen [:bechen]]

Attachment: WIP-bug-1058429.v01.patch

Comment 2

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

Comment on attachment 8479736 [details] [diff] [review]
WIP-bug-1058429.v01.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1479,5 @@
>    // in that case MediaDecoder shouldn't be calling us.
>    NS_ASSERTION(mState != DECODER_STATE_SEEKING,
>                 "We shouldn't already be seeking");
> +  NS_ASSERTION(mState >= DECODER_STATE_DECODING ||
> +               mState == DECODER_STATE_DECODING_METADATA,

If we allow |mState == DECODER_STATE_DECODING_METADATA| here, it will be possible for Seek() to change state to |DECODER_STATE_SEEKING| even before CallDecodeMetadata() is executed which is wrong.

@@ +1924,5 @@
>      SetStartTime(0);
>      res = FinishDecodeMetadata();
>      NS_ENSURE_SUCCESS(res, res);
> +  } else if (mStartTime != -1) {
> +    res = FinishDecodeMetadata();

Don't you need to call SetStartTime() to ensure other status variables are properly initialized.

@@ +1997,3 @@
>    // Inform the element that we've loaded the metadata and the first frame.
> +  // Sync dispatch this event because The MediaDecoder::MetadataLoaded might
> +  // change the mState to DECODER_STATE_SEEKING when back from dormant state.

The change is not trivial and risky. Can you get some profiling data to show how much benefit we get from this seeking-right-after-leaving-dormant optimization.

::: content/media/omx/RtspMediaCodecReader.cpp
@@ +100,5 @@
>  RtspMediaCodecReader::ReadMetadata(MediaInfo* aInfo,
>                                     MetadataTags** aTags)
>  {
>    // TODO: fix me, we need a SeekTime(0) here because the
>    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.

You don't need this comment anymore, right?

@@ +101,5 @@
>                                     MetadataTags** aTags)
>  {
>    // TODO: fix me, we need a SeekTime(0) here because the
>    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.
> +  EnsureActive();

What is EnsureActive() for?

Comment 3

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #2)
> Comment on attachment 8479736 [details] [diff] [review]
> WIP-bug-1058429.v01.patch
> 
> Review of attachment 8479736 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1479,5 @@
> >    // in that case MediaDecoder shouldn't be calling us.
> >    NS_ASSERTION(mState != DECODER_STATE_SEEKING,
> >                 "We shouldn't already be seeking");
> > +  NS_ASSERTION(mState >= DECODER_STATE_DECODING ||
> > +               mState == DECODER_STATE_DECODING_METADATA,
> 
> If we allow |mState == DECODER_STATE_DECODING_METADATA| here, it will be
> possible for Seek() to change state to |DECODER_STATE_SEEKING| even before
> CallDecodeMetadata() is executed which is wrong.
> 

Hmm, that would be a problem...

> @@ +1924,5 @@
> >      SetStartTime(0);
> >      res = FinishDecodeMetadata();
> >      NS_ENSURE_SUCCESS(res, res);
> > +  } else if (mStartTime != -1) {
> > +    res = FinishDecodeMetadata();
> 
> Don't you need to call SetStartTime() to ensure other status variables are
> properly initialized.
> 

My assumption is: stream's mStartTime won't change.
So if the mStartTime is not -1, means it had been set before. So I don't think we need to call SetStartTime() again.
Or do you mean |SetStartime(mStartTime)| ?

> @@ +1997,3 @@
> >    // Inform the element that we've loaded the metadata and the first frame.
> > +  // Sync dispatch this event because The MediaDecoder::MetadataLoaded might
> > +  // change the mState to DECODER_STATE_SEEKING when back from dormant state.
> 
> The change is not trivial and risky. Can you get some profiling data to show
> how much benefit we get from this seeking-right-after-leaving-dormant
> optimization.
> 
The old path: Dormant -> MetaData -> decoding -> seeking
The new pathc Dormant -> MetaData -> seeking

The extra time on decoding state is about 0.1 second. But if the MediaCache miss when decoding, that would spend about 0.4~0.6 seconds at least.

> @@ +101,5 @@
> >                                     MetadataTags** aTags)
> >  {
> >    // TODO: fix me, we need a SeekTime(0) here because the
> >    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.
> > +  EnsureActive();
> 
> What is EnsureActive() for?

Send a play command to Rtsp server to ensure the streaming data is coming.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: bug-1058429.v02.patch

I add a member |mIsWaitingForSeek|, when leaving from dormant state, we set it true to stop dispatching decoding task. And set it to false when seeking.

Comment 5

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

Comment on attachment 8483227 [details] [diff] [review]
bug-1058429.v02.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1572,5 @@
>  {
>    AssertCurrentThreadInMonitor();
> +  if ((mState != DECODER_STATE_DECODING &&
> +       mState != DECODER_STATE_BUFFERING &&
> +       mState != DECODER_STATE_SEEKING) || mIsWaitingForSeek) {

This approach has a deep coupling with MediaDecoder for it expects MediaDecoder will call Seek() after leaving dormant state. How about exposing a FreezeDecoding() function from MediaDecoderStateMachine for MediaDecoder to call when MediaDecoder know it will seek soon to avoid unnecessary decoding.

Comment 7

[Sotaro Ikeda [:sotaro]]

This bug also blocks bug 1082563.

Comment 8

[Sotaro Ikeda [:sotaro]]

[Blocking Requested - why for this release]: This bug blocks bug 1082563.

Comment 9

[David Flanagan [:djf]]

In bug 1082563, we noticed that we were getting 'loadedmetadata' events when a video element was resumed. (I didn't check for 'canplay', 'seeking', 'seeked' events, etc., but suspect that some of those may be fired as well.) I think that is a bug, but don't know whether it will be fixed here or if a separate bug should be filed.

Comment 10

[Andrew Overholt [:overholt]]

This blocks a 2.1+ bug so I'm marking it 2.1+ as well.

Comment 11

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

(In reply to David Flanagan [:djf] from comment #9)
> In bug 1082563, we noticed that we were getting 'loadedmetadata' events when
> a video element was resumed. (I didn't check for 'canplay', 'seeking',
> 'seeked' events, etc., but suspect that some of those may be fired as well.)
> I think that is a bug, but don't know whether it will be fixed here or if a
> separate bug should be filed.

When suspended, the HW decoder resource is released. Therefore metadata has to be loaded again when resumed as well as the 'loadedmetadata' event.

Comment 12

[Benjamin Chen [:bechen]]

Set duplication to bug 1062134 because most things are done there.