Closed Bug 1098114 Opened 5 years ago Closed 5 years ago

Remove ReferenceKeeperRunnable in MediaCodecReader.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bechen, Assigned: bechen)

Details

Attachments

(1 file, 1 obsolete file)

ReferenceKeeperRunnable class is an unnecessary class because we have NS_ProxyRelease function, and also the code logic doesn't guarantee the reference released on main thread.
I have tried to NS_ProxyRelease but compile failed because the nsRefPtr<MediaCodecReader> mReader is not a nsISupports.
Hi Bruce:
Do you know why the MediaCoderReader must be destroyed at main thread? I guess we can relax the restriction.
Flags: needinfo?(brsun)
Hi Benjamin:

The idea is borrowed from OmxDecoder. I check the log history, this is for releasing MediaResource on main thread:
 - https://bugzilla.mozilla.org/show_bug.cgi?id=1033915#c3
 - https://bugzilla.mozilla.org/show_bug.cgi?id=947905#c0
 - https://bugzilla.mozilla.org/show_bug.cgi?id=929029#c14

Thanks for your remainding, apparently the reference counting of MediaResource is not handled well in MediaCodecReader currently. This will be another follow-up bug, and we'd better to fix this problem before changing media.omx.async.enabled pref. on by default.
Flags: needinfo?(brsun)
One follow-up created as mentioned: https://bugzilla.mozilla.org/show_bug.cgi?id=1098659
Attached patch bug-1098114.patch (obsolete) — Splinter Review
Attachment #8522793 - Flags: review?(jwwang)
Comment on attachment 8522793 [details] [diff] [review]
bug-1098114.patch

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

The patch doesn't do as described in the bug title. Can you explain what this patch do?
(In reply to JW Wang [:jwwang] from comment #7)
> Comment on attachment 8522793 [details] [diff] [review]
> bug-1098114.patch
> 
> Review of attachment 8522793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch doesn't do as described in the bug title. Can you explain what
> this patch do?

At beginning, I want to remove the |ReferenceKeeperRunnable| at MediaCodecReader::ProcessCachedDataTask::Run() because there is logic flaw inside and I think the MediaCodecReader can be released on other thread.

Comment 5 explain the mExtractor needs to be released on main thread, so I restore the ReferenceKeeperRunnable class back to the cpp file.
Comment on attachment 8522793 [details] [diff] [review]
bug-1098114.patch

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

Please update the bug title to reflect what this bug tries to fix.

::: dom/media/omx/MediaCodecReader.cpp
@@ +1089,5 @@
>  
>  void
>  MediaCodecReader::DestroyExtractor()
>  {
> +  // Ensure the MediaResource destroyed on main thread.

Add a comment to describe how mExtractor is related to MediaResource. The comment is confusing which talks about MediaResource but the code below speaks to mExtractor.
Attachment #8522793 - Flags: review?(jwwang) → review+
(In reply to Benjamin Chen [:bechen] from comment #5)
> I found a reference link to the MediaResource in MediaCodecReader.
> http://dxr.mozilla.org/mozilla-central/source/dom/media/omx/MediaCodecReader.
> cpp?from=MediaCodecReader.cpp#1167

FYI, releasing the reference of mExtractor in main thread seems not enough; the reference count of MediaResource in MediaCodecReader::ProcessCachedData() should be handled properly as well: https://bugzilla.mozilla.org/show_bug.cgi?id=1098659#c1
I think we can assert delete-in-main-thread in the destructors of MediaResource to help us catch such violations.
(In reply to JW Wang [:jwwang] from comment #11)
> I think we can assert delete-in-main-thread in the destructors of
> MediaResource to help us catch such violations.

MediaResource has threadsafe refcounting, and it uses NS_IMPL_RELEASE_WITH_DESTROY to ensure that its destructor runs on the main thread. I'm not sure what the problem here you're trying to solve is, but MediaResource doesn't need to be released on the main thread; it's Release() takes care of that for you.
Since the MediaResourec will be destroyed at main thread by NS_IMPL_RELEASE_WITH_DESTROY,comment 12, I can remove the |ReferenceKeeperRunnable| class and the MediaDecoderReader can be destroyed at any thread.
Attachment #8522793 - Attachment is obsolete: true
Attachment #8523678 - Flags: review?(jwwang)
Comment on attachment 8523678 [details] [diff] [review]
bug-1098114.v02.patch

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

::: dom/media/omx/MediaCodecReader.cpp
@@ +298,5 @@
>    mHandler = new MessageHandler(this);
>    mVideoListener = new VideoResourceListener(this);
>  }
>  
>  MediaCodecReader::~MediaCodecReader()

Can we delete this empty destructor?
Attachment #8523678 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #14)
> Comment on attachment 8523678 [details] [diff] [review]
> bug-1098114.v02.patch
> 
> Review of attachment 8523678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/omx/MediaCodecReader.cpp
> @@ +298,5 @@
> >    mHandler = new MessageHandler(this);
> >    mVideoListener = new VideoResourceListener(this);
> >  }
> >  
> >  MediaCodecReader::~MediaCodecReader()
> 
> Can we delete this empty destructor?

I suggest not to delete the destructor otherwise we will encounter "incomplete type" compile warning/error I remember.
How? I can't find where a pure virtual destructor is defined.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ef2372fc405
Status: ASSIGNED → 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.