Closed
Bug 1153149
Opened 9 years ago
Closed 9 years ago
Get rid of IsWaitingMediaResources() in PlatformDecoderModule
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: ayang, Assigned: ayang)
References
Details
Attachments
(1 file, 3 obsolete files)
9.85 KB,
patch
|
ayang
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ayang
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8595166 -
Flags: review?(jyavenard)
Comment 2•9 years ago
|
||
Comment on attachment 8595166 [details] [diff] [review] remove_IsWaitingMediaResource_from_PDM Review of attachment 8595166 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment applied (you don't need mInited) ::: dom/media/fmp4/MP4Reader.h @@ +218,5 @@ > uint64_t mNumSamplesOutput; > uint32_t mDecodeAhead; > // Whether this stream exists in the media. > bool mActive; > + bool mInited; It's a terrible name ! mInitialized maybe. However, I don't see the point of having this member. mInited always mirror the existence mDecoder. As error are fatals, if mDecoder->Init() failed, we will never again reach the code where we would have mDecoder set but Init() failed. if mDecoder == nullptr -> mInited = false If mDecoder == something -> mInited = true. So testing for mDecoder is enough. If we assumed we could be in a condition where mDecoder is set but mDecoder->Init() failed, then simply set mDecoder to nullptr if mDecoder->Init() failed
Attachment #8595166 -
Flags: review?(jya) → review+
Comment 3•9 years ago
|
||
Some questions for current codec reservation and notifications. I noticed currently GonkVideoDecoderManager is waiting codec reservation in [1] which is a blocking call. So if video codec is already occupied by others, ReadMetadata will be blocked. Would this cause some problems, like user cannot terminate that process or MediaDecoderStateMachine can not be shutdown complete? [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#111
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Blake Wu [:bwu][:blakewu] from comment #3) > Some questions for current codec reservation and notifications. I noticed > currently GonkVideoDecoderManager is waiting codec reservation in [1] which > is a blocking call. So if video codec is already occupied by others, > ReadMetadata will be blocked. Would this cause some problems, like user > cannot terminate that process or MediaDecoderStateMachine can not be > shutdown complete? > > [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/ > GonkVideoDecoderManager.cpp?from=GonkVideoDecoderManager.cpp&case=true#111 The waiting codec reservation is on another thread, not MediaDecoderStateMachine thread. So the problem you stated won't happen.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #2) > Comment on attachment 8595166 [details] [diff] [review] > remove_IsWaitingMediaResource_from_PDM > > Review of attachment 8595166 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comment applied (you don't need mInited) > > ::: dom/media/fmp4/MP4Reader.h > @@ +218,5 @@ > > uint64_t mNumSamplesOutput; > > uint32_t mDecodeAhead; > > // Whether this stream exists in the media. > > bool mActive; > > + bool mInited; > > It's a terrible name ! mInitialized maybe. I'll change the name. > > However, I don't see the point of having this member. mInited always mirror > the existence mDecoder. > As error are fatals, if mDecoder->Init() failed, we will never again reach > the code where we would have mDecoder set but Init() failed. > > if mDecoder == nullptr -> mInited = false > If mDecoder == something -> mInited = true. > > So testing for mDecoder is enough. > > If we assumed we could be in a condition where mDecoder is set but > mDecoder->Init() failed, then simply set mDecoder to nullptr if > mDecoder->Init() failed I think mInited is required in current media resource management. If there are two videos opening in the same page. The first video acquired the codec successfully. The second one will be blocked in the mDecoder->Init(). In this situation, the mDecoder is not nullptr and mDecoder->Init() is blocked before the first video releases the codec. Then MediaDecoderStateMachine could get the wrong value from IsWaitingMediaResources().
Comment 6•9 years ago
|
||
(In reply to Alfredo Yang from comment #5) > I think mInited is required in current media resource management. > If there are two videos opening in the same page. The first video acquired > the codec successfully. The second one will be blocked in the > mDecoder->Init(). In this situation, the mDecoder is not nullptr and > mDecoder->Init() is blocked before the first video releases the codec. > Then MediaDecoderStateMachine could get the wrong value from > IsWaitingMediaResources(). No.. That's not how IsWaitingMediaResources() is being called. If mDecoder->Init() is blocking, then MP4Reader::ReadMetadata() will not return. IsWaitingMediaResources() is only ever called before and after ReadMetadata() (now in MediaDecoderReader::CallReadMetadata()) As MP4Reader::ReadMetadata() is blocked, MP4Reader::IsWaitingMediaResources can't be called for that reader. Testing mDecoder is sufficient.
Comment 7•9 years ago
|
||
Actually, on second thought... Can MP4Reader::IsWaitingMediaResources() ever returns true then ? It doesn't appear that it's possible now. If mDecoder->Init() may not returned and be blocking ReadMetadata() , bwu comment is valid. It will not block the MediaDecoderStateMachine however as it's done in a promise.
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #7) > Actually, on second thought... Can MP4Reader::IsWaitingMediaResources() ever > returns true then ? I try to modify codes as comments and found MediaDecoderStateMachine will be in WAITING_FOR_RESOURCES [1] when resuming from dormant. When resuming from dormant, the mDecoder is nullptr so MP4Reader::IsWaitingMediaResources() returns true, then MediaDecoderStateMachine won't called ReadMetadata(). Without calling ReadMetadata(), decoder won't be initialized and MDSM gets stuck in WAITING_FOR_RESOURCES. I think it can be solved by initializing codec in MP4Reader::PreReadMetadata(). Or other suggestion? [1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderReader.cpp#206
Comment 9•9 years ago
|
||
If as per your earlier comment mDecoder->Init() is blocking until the decoder is ready, you can just get rid of MP4Reader::IsWaitingMediaResources(). If it's not blocking then we can't get rid of MediaDataDecoder::IsWaitingMediaResources yet and you'll have to wait until bug 1148292 is complete. After this, the MP4Reader ReadMetadata() will always return a promise, in which case we will simply have to resolve the promise once the decoder is ready. And we won't need IsWaitingMediaResources() at all..
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #9) > If as per your earlier comment mDecoder->Init() is blocking until the > decoder is ready, you can just get rid of > MP4Reader::IsWaitingMediaResources(). I'll remove MP4Reader::IsWaitingMediaResources(). Thank you for review.
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8595166 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8595867 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Carry r+. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9a4300696a
Attachment #8596276 -
Attachment is obsolete: true
Attachment #8600840 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Hi sheriff, please check in after bug 1154512. Thanks.
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/87329d2ed8ce
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/87329d2ed8ce
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•