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)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: ayang, Assigned: ayang)
References
Details
Attachments
(1 file, 2 obsolete files)
15.04 KB,
patch
|
ayang
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Any mp4 video and play it. Press home key. Resume the background ap and continue playing the video.
Comment 1•9 years ago
|
||
Allow me to change the title.
Summary: video stops resuming from background → [FFOS] video stops resuming from background with Gonk Platform Decoder
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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 4•9 years ago
|
||
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-
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
(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
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8534901 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
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+
Assignee | ||
Comment 10•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/0435b9bb6482
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0435b9bb6482
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 15•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
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.
Description
•