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

RESOLVED DUPLICATE of bug 1062134

Status

()

Core
Audio/Video
RESOLVED DUPLICATE of bug 1062134
3 years ago
3 years ago

People

(Reporter: bechen, Assigned: bechen)

Tracking

unspecified
x86_64
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+)

Details

(Whiteboard: [caf priority: p2][CR 744386])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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).
(Assignee)

Comment 1

3 years ago
Created attachment 8479736 [details] [diff] [review]
WIP-bug-1058429.v01.patch
Attachment #8479736 - Flags: feedback?(jwwang)
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?
Attachment #8479736 - Flags: feedback?(jwwang) → feedback-
(Assignee)

Updated

3 years ago
Assignee: nobody → bechen
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
Created attachment 8483227 [details] [diff] [review]
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.
Attachment #8479736 - Attachment is obsolete: true
Attachment #8483227 - Flags: feedback?(jwwang)
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.
Attachment #8483227 - Flags: feedback?(jwwang)
(Assignee)

Updated

3 years ago
Depends on: 1062134
(Assignee)

Updated

3 years ago
See Also: → bug 1084629

Updated

3 years ago
Duplicate of this bug: 1084629

Updated

3 years ago
Blocks: 1082563
This bug also blocks bug 1082563.
[Blocking Requested - why for this release]: This bug blocks bug 1082563.
blocking-b2g: --- → 2.1?
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.
This blocks a 2.1+ bug so I'm marking it 2.1+ as well.
blocking-b2g: 2.1? → 2.1+
(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.

Updated

3 years ago
Blocks: 1074290

Updated

3 years ago
Whiteboard: [CR 741684]

Updated

3 years ago
Whiteboard: [CR 741684] → [caf priority: p2][CR 741684]

Updated

3 years ago
Whiteboard: [caf priority: p2][CR 741684] → [caf priority: p2][CR 744386]
(Assignee)

Comment 12

3 years ago
Set duplication to bug 1062134 because most things are done there.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1062134
(Assignee)

Updated

3 years ago
See Also: → bug 1088524

Updated

3 years ago
No longer blocks: 1074290
You need to log in before you can comment on or make changes to this bug.