Closed
Bug 1094614
Opened 10 years ago
Closed 10 years ago
Firefox OS GonkDecoderModule does not release video decoder
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: ajones, Assigned: bwu)
References
Details
Attachments
(1 file, 4 obsolete files)
7.37 KB,
patch
|
bwu
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce: * Enable MP4Reader and GonkDecoderModule * Go to browser * Navigate to http://people.mozilla.org/~cpearce/video/bruce_vs_ironman.mp4 * Press refresh Actual results: Video does not play. Expected results: Video should play
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #1) > Blake - can you take this bug? Sure! Thanks for reminding me.
Assignee: nobody → bwu
Flags: needinfo?(bwu)
Assignee | ||
Comment 3•10 years ago
|
||
Release codec resource properly.
Attachment #8522068 -
Flags: review?(sotaro.ikeda.g)
Comment 4•10 years ago
|
||
Comment on attachment 8522068 [details] [diff] [review] Bug-1094614-Release-codec-resource-properly.patch Review of attachment 8522068 [details] [diff] [review]: ----------------------------------------------------------------- I quickly checked MediaCodecProxy. This class have 2 functions that are almost same. Why is there almost same implementation? - MediaCodecProxy::releaseCodec() - MediaCodecProxy::ReleaseMediaResources() And it seems better that MediaCodecProxy class call "IPCThreadState::self()->flushCommands()" same way as to AwesamePlayer. It forces IPC transaction delivery. Destroying IPC object is async. therefore, such function call becomes necessary. MediaCodecProxy already calling it in destructor. But it seems to be called without concrete meaning.
Attachment #8522068 -
Flags: review?(sotaro.ikeda.g)
Updated•10 years ago
|
Flags: needinfo?(bwu)
Comment 5•10 years ago
|
||
And in MediaCodecProxy, mCodecLock is not correctly held in some functions. They seems to be added later than another functions.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #4) > Comment on attachment 8522068 [details] [diff] [review] > Bug-1094614-Release-codec-resource-properly.patch > > Review of attachment 8522068 [details] [diff] [review]: > ----------------------------------------------------------------- > > I quickly checked MediaCodecProxy. This class have 2 functions that are > almost same. Why is there almost same implementation? > - MediaCodecProxy::releaseCodec() > - MediaCodecProxy::ReleaseMediaResources() > > And it seems better that MediaCodecProxy class call > "IPCThreadState::self()->flushCommands()" same way as to AwesamePlayer. It > forces IPC transaction delivery. Destroying IPC object is async. therefore, > such function call becomes necessary. MediaCodecProxy already calling it in > destructor. But it seems to be called without concrete meaning. Yes. You are right. We should merge this two similiar functions. Originally releaseCodec() is used for codec resource management. ReleaseMediaResources is used and provided for MP4Reader. Currently MediaCodecProxy is used by MP4Reader and MediaCodecReader which have different designs. I will try to make similar functions merged. Thanks for this notice!
Flags: needinfo?(bwu)
Assignee | ||
Comment 7•10 years ago
|
||
Changes according comment 4 and 5.
Attachment #8522068 -
Attachment is obsolete: true
Attachment #8524330 -
Flags: review?(sotaro.ikeda.g)
Comment 8•10 years ago
|
||
Comment on attachment 8524330 [details] [diff] [review] Bug-1094614-Release-codec-resource-properly-v2.patch Review of attachment 8524330 [details] [diff] [review]: ----------------------------------------------------------------- The patch becomes better. After applying the patch, in the following functions, mCodec is accessed without mCodecLock lock held. It need to be addressed. It is OK to fix this lock problem in another bug. In this case, please create a new bug for it. - MediaCodecProxy::IsWaitingResources() - MediaCodecProxy::Output() - MediaCodecProxy::Input() - MediaCodecProxy::UpdateOutputBuffers() And the followings are additional comments. ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp @@ +579,5 @@ > } > > +void GonkVideoDecoderManager::ReleaseMediaResources() { > + ALOG("ReleseMediaResources"); > + ReleaseAllPendingVideoBuffersLocked(); name of this function seems not good. It seems that this function name seems to be borrowed from OmxDecoder::ReleaseAllPendingVideoBuffersLocked(). OmxDecoder::ReleaseAllPendingVideoBuffersLocked() expects that it is called with mSeekLock held. But this function seems not have such precondition. Therefore, it seems better to remove Locked prefix from the function name. ::: dom/media/omx/MediaCodecProxy.cpp @@ +595,5 @@ > void MediaCodecProxy::ReleaseMediaResources() > { > + releaseCodec(); > + // Complete all pending Binder ipc transactions > + IPCThreadState::self()->flushCommands(); It seems better to move "IPCThreadState::self()->flushCommands()" to releaseCodec(). My image is like AwesomePlayer::shutdownVideoDecoder_l(). http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/AwesomePlayer.cpp#1327
Attachment #8524330 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 9•10 years ago
|
||
Thanks for your review! (In reply to Sotaro Ikeda [:sotaro] from comment #8) > Comment on attachment 8524330 [details] [diff] [review] > Bug-1094614-Release-codec-resource-properly-v2.patch > > Review of attachment 8524330 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch becomes better. After applying the patch, in the following > functions, mCodec is accessed without mCodecLock lock held. It need to be > addressed. It is OK to fix this lock problem in another bug. In this case, > please create a new bug for it. > - MediaCodecProxy::IsWaitingResources() Will do! > - MediaCodecProxy::Output() In this function, dequeueOutputBuffer() will be called and there is a mCodecLock protected inside. > - MediaCodecProxy::Input() In this function, dequeueInputBuffer() will be called and there is a mCodecLock protected inside. > - MediaCodecProxy::UpdateOutputBuffers() Similarly, mCodecLock is in the getOutputBuffers() which is called in this function. > > And the followings are additional comments. > > ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp > @@ +579,5 @@ > > } > > > > +void GonkVideoDecoderManager::ReleaseMediaResources() { > > + ALOG("ReleseMediaResources"); > > + ReleaseAllPendingVideoBuffersLocked(); > > name of this function seems not good. It seems that this function name seems > to be borrowed from OmxDecoder::ReleaseAllPendingVideoBuffersLocked(). > OmxDecoder::ReleaseAllPendingVideoBuffersLocked() expects that it is called > with mSeekLock held. But this function seems not have such precondition. > Therefore, it seems better to remove Locked prefix from the function name. I am fine with changing function name. > > ::: dom/media/omx/MediaCodecProxy.cpp > @@ +595,5 @@ > > void MediaCodecProxy::ReleaseMediaResources() > > { > > + releaseCodec(); > > + // Complete all pending Binder ipc transactions > > + IPCThreadState::self()->flushCommands(); > > It seems better to move "IPCThreadState::self()->flushCommands()" to > releaseCodec(). My image is like AwesomePlayer::shutdownVideoDecoder_l(). > http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/ > AwesomePlayer.cpp#1327 OK. Will do!
Assignee | ||
Comment 10•10 years ago
|
||
Make changes according to comment 8 and 9.
Attachment #8524330 -
Attachment is obsolete: true
Attachment #8525197 -
Flags: review?(sotaro.ikeda.g)
Comment 11•10 years ago
|
||
(In reply to Blake Wu [:bwu][:blakewu] from comment #9) > Thanks for your review! > (In reply to Sotaro Ikeda [:sotaro] from comment #8) > > Comment on attachment 8524330 [details] [diff] [review] > > Bug-1094614-Release-codec-resource-properly-v2.patch > > > > Review of attachment 8524330 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > The patch becomes better. After applying the patch, in the following > > functions, mCodec is accessed without mCodecLock lock held. It need to be > > addressed. It is OK to fix this lock problem in another bug. In this case, > > please create a new bug for it. > > - MediaCodecProxy::IsWaitingResources() > Will do! > > - MediaCodecProxy::Output() > In this function, dequeueOutputBuffer() will be called and there is a > mCodecLock protected inside. > > - MediaCodecProxy::Input() > In this function, dequeueInputBuffer() will be called and there is a > mCodecLock protected inside. > > - MediaCodecProxy::UpdateOutputBuffers() > Similarly, mCodecLock is in the getOutputBuffers() which is called in this > function. > > blakewu, for example, the following code is not protected by the lock. It is not safe. Why is it not necessary to protect? > if (mCodec == nullptr) { > ALOG("MediaCodec has not been inited from input!"); > return NO_INIT; > }
Flags: needinfo?(bwu)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #11) > (In reply to Blake Wu [:bwu][:blakewu] from comment #9) > > Thanks for your review! > > (In reply to Sotaro Ikeda [:sotaro] from comment #8) > > > Comment on attachment 8524330 [details] [diff] [review] > > > Bug-1094614-Release-codec-resource-properly-v2.patch > > > > > > Review of attachment 8524330 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > The patch becomes better. After applying the patch, in the following > > > functions, mCodec is accessed without mCodecLock lock held. It need to be > > > addressed. It is OK to fix this lock problem in another bug. In this case, > > > please create a new bug for it. > > > - MediaCodecProxy::IsWaitingResources() > > Will do! > > > - MediaCodecProxy::Output() > > In this function, dequeueOutputBuffer() will be called and there is a > > mCodecLock protected inside. > > > - MediaCodecProxy::Input() > > In this function, dequeueInputBuffer() will be called and there is a > > mCodecLock protected inside. > > > - MediaCodecProxy::UpdateOutputBuffers() > > Similarly, mCodecLock is in the getOutputBuffers() which is called in this > > function. > > > > > blakewu, for example, the following code is not protected by the lock. It is > not safe. Why is it not necessary to protect? It is necessary! You are right. Sorry not to get your point. I will create another bug to fix this. > > > if (mCodec == nullptr) { > > ALOG("MediaCodec has not been inited from input!"); > > return NO_INIT; > > }
Flags: needinfo?(bwu)
Assignee | ||
Comment 13•10 years ago
|
||
Bug 1101565 is created to fix this lock problem.
Comment 14•10 years ago
|
||
Comment on attachment 8525197 [details] [diff] [review] Bug-1094614-Release-codec-resource-properly-v3.patch Review of attachment 8525197 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I understand the remaining problem will be addressed by Bug 1101565.
Attachment #8525197 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Carry r+ from sotaro.
Attachment #8525197 -
Attachment is obsolete: true
Attachment #8525870 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b7475c56426a Testing results look good!
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Hi Blake, seems the patch didn't apply cleanly: patching file dom/media/fmp4/gonk/GonkMediaDataDecoder.h Hunk #1 FAILED at 28 1 out of 1 hunks FAILED -- saving rejects to file dom/media/fmp4/gonk/GonkMediaDataDecoder.h.rej patching file dom/media/fmp4/gonk/GonkVideoDecoderManager.h Hunk #1 FAILED at 44 1 out of 2 hunks FAILED -- saving rejects to file dom/media/fmp4/gonk/GonkVideoDecoderManager.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh Bug-1094614-Release-codec-resource-properly-Final.patch could you take a look, thanks!
Flags: needinfo?(bwu)
Keywords: checkin-needed
Assignee | ||
Comment 18•10 years ago
|
||
Rebase to the latest code base.
Attachment #8525870 -
Attachment is obsolete: true
Attachment #8525943 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Hi Carsten, I have rebased the patch and run the testing again. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e0a82dfbbcbb The testing results look good!
Flags: needinfo?(bwu)
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5261b6dd5487
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5261b6dd5487
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•