Closed Bug 1105209 Opened 9 years ago Closed 9 years ago

[FFOS] video stops resuming from background with Gonk Platform Decoder

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(1 file, 2 obsolete files)

Any mp4 video and play it.
Press home key.
Resume the background ap and continue playing the video.
Allow me to change the title.
Summary: video stops resuming from background → [FFOS] video stops resuming from background with Gonk Platform Decoder
Attached patch pdm_dormant_fix (obsolete) — Splinter Review
This patch addresses 3 problems.
1. add media resource request in PlatformDecoderModule.
2. Reset MP4Reader mUpdateScheduled in Flush()
3. Reset mAudioRequestPending and mVideoRequestPending in MediaDecoderStateMachine::ResetPlay() because DecodeAudio runnable could be cancelled by MediaDecoderStateMachine::FlushDecoding().
Attachment #8534901 - Flags: review?(cpearce)
Comment on attachment 8534901 [details] [diff] [review]
pdm_dormant_fix

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1497,5 @@
>    // are clearing the queue and causes crash for no samples to be popped.
>    MOZ_ASSERT(!mAudioSink);
>  
>    mVideoFrameEndTime = -1;
> +  mVideoRequestPending = false;

MediaDecoderStateMachine::FlushDecoding is only called in SHUTDOWN state. I fail to see the reason resetting this flag since we're not decoding anymore.

Since ResetPlayback is also called by DecodeSeek, resetting the flag could result in multiple decode tasks in MediaTaskQueue at the same time.
Comment on attachment 8534901 [details] [diff] [review]
pdm_dormant_fix

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

B2G resource management is better to be reviewed by ajones.
Attachment #8534901 - Flags: review?(cpearce) → review?(ajones)
Comment on attachment 8534901 [details] [diff] [review]
pdm_dormant_fix

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

There seems to be information missing from this patch. Does it even apply to mozilla-inbound?

::: dom/media/fmp4/MP4Reader.cpp
@@ +254,5 @@
>    nsString mInitDataType;
>  };
>  #endif
>  
> +void MP4Reader::RequestCodecResource() {

This patch is missing changes to MP4Reader.h and the code that calls RequestMediaCodecResource()

@@ +331,5 @@
>           mPlatform->SupportsVideoMimeType(aMimeType);
>  }
>  
> +void
> +MP4Reader::PreReadMetadata()

I don't understand the purpose of this function because I don't see the code that calls it. It is not obvious from the name.

::: dom/media/omx/MediaCodecProxy.cpp
@@ +602,5 @@
>  }
>  
>  bool MediaCodecProxy::IsWaitingResources()
>  {
> +  if (mResourceHandler != nullptr) {

nit: the Mozilla style is |if (mResourceHandler) {| without the |!= nullptr|
Attachment #8534901 - Flags: review?(ajones) → review-
(In reply to JW Wang [:jwwang] from comment #3)
> Comment on attachment 8534901 [details] [diff] [review]
> pdm_dormant_fix
> 
> Review of attachment 8534901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1497,5 @@
> >    // are clearing the queue and causes crash for no samples to be popped.
> >    MOZ_ASSERT(!mAudioSink);
> >  
> >    mVideoFrameEndTime = -1;
> > +  mVideoRequestPending = false;
> 
> MediaDecoderStateMachine::FlushDecoding is only called in SHUTDOWN state. I
> fail to see the reason resetting this flag since we're not decoding anymore.

FlushDecoding is called in DECODER_STATE_DORMANT state either.

> 
> Since ResetPlayback is also called by DecodeSeek, resetting the flag could
> result in multiple decode tasks in MediaTaskQueue at the same time.

I'll move mVideoRequestPending and mAudioRequestPending to MediaDecoderStateMachine::FlushDecoding().
The problem here is mAudioRequestPending/mVideoRequestPending should be cleared when OnAudioDecoded/OnVideoDecoded in normal cases. However, FlushDecoding() cancels the jobs without reset these flags and it causes the audio/video decode task won't be dispatched anymore.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #5)
> Comment on attachment 8534901 [details] [diff] [review]
> pdm_dormant_fix
> 
> Review of attachment 8534901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There seems to be information missing from this patch. Does it even apply to
> mozilla-inbound?

No. I don't direct checkin code to mozilla-inbound.

> 
> ::: dom/media/fmp4/MP4Reader.cpp
> @@ +254,5 @@
> >    nsString mInitDataType;
> >  };
> >  #endif
> >  
> > +void MP4Reader::RequestCodecResource() {
> 
> This patch is missing changes to MP4Reader.h and the code that calls
> RequestMediaCodecResource()
> 
> @@ +331,5 @@
> >           mPlatform->SupportsVideoMimeType(aMimeType);
> >  }
> >  
> > +void
> > +MP4Reader::PreReadMetadata()
> 
> I don't understand the purpose of this function because I don't see the code
> that calls it. It is not obvious from the name.

MP4Reader::PreReadMetadata() is called by MediaDecoderStateMachine::DecodeMetadata() [1] before checking hardware resource.
I use PreReadMetadata() to send hardware codec request and then MediaDecoderStateMachine will check the resource is available or not [2]. 


[1] http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#2029
[2] http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#2031
Attached patch pdm_dormant_fix (obsolete) — Splinter Review
Attachment #8534901 - Attachment is obsolete: true
Attachment #8536396 - Flags: review?(ajones)
Comment on attachment 8536396 [details] [diff] [review]
pdm_dormant_fix

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

::: dom/media/fmp4/MP4Reader.h
@@ +44,5 @@
>  
>    virtual bool HasAudio() MOZ_OVERRIDE;
>    virtual bool HasVideo() MOZ_OVERRIDE;
>  
> +  virtual void PreReadMetadata() MOZ_OVERRIDE;

The function name isn't obvious so add your description in a comment here.
Attachment #8536396 - Flags: review?(ajones) → review+
Attached patch pdm_dormant_fixSplinter Review
Thanks for reviews. Update comments and go try.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ccf64a98dbe9
Attachment #8536396 - Attachment is obsolete: true
Attachment #8536987 - Flags: review+
Try is good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0435b9bb6482
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1105913
Comment on attachment 8536987 [details] [diff] [review]
pdm_dormant_fix

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: landed on m-c.
[Risks and why]: This affects media decoding outside of the MSE work, particularly on Firefox OS, as well as fixing the MSE-specific bug.
[String/UUID change made/needed]: None.

Uplift of MSE bug 1110534 depends on this change.
Attachment #8536987 - Flags: approval-mozilla-aurora?
Attachment #8536987 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.