Closed Bug 1094614 Opened 5 years ago Closed 5 years ago

Firefox OS GonkDecoderModule does not release video decoder

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ajones, Assigned: bwu)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Blake - can you take this bug?
Flags: needinfo?(bwu)
(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)
Release codec resource properly.
Attachment #8522068 - Flags: review?(sotaro.ikeda.g)
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)
Flags: needinfo?(bwu)
And in MediaCodecProxy, mCodecLock is not correctly held in some functions. They seems to be added later than another functions.
(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)
Changes according comment 4 and 5.
Attachment #8522068 - Attachment is obsolete: true
Attachment #8524330 - Flags: review?(sotaro.ikeda.g)
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)
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!
Make changes according to comment 8 and 9.
Attachment #8524330 - Attachment is obsolete: true
Attachment #8525197 - Flags: review?(sotaro.ikeda.g)
(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)
(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)
Bug 1101565 is created to fix this lock problem.
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+
Carry r+ from sotaro.
Attachment #8525197 - Attachment is obsolete: true
Attachment #8525870 - Flags: review+
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
Rebase to the latest code base.
Attachment #8525870 - Attachment is obsolete: true
Attachment #8525943 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/5261b6dd5487
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.