Closed
Bug 1098114
Opened 10 years ago
Closed 10 years ago
Remove ReferenceKeeperRunnable in MediaCodecReader.
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bechen, Assigned: bechen)
Details
Attachments
(1 file, 1 obsolete file)
3.36 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
I have tried to NS_ProxyRelease but compile failed because the nsRefPtr<MediaCodecReader> mReader is not a nsISupports.
Assignee | ||
Comment 2•10 years ago
|
||
Hi Bruce: Do you know why the MediaCoderReader must be destroyed at main thread? I guess we can relax the restriction.
Flags: needinfo?(brsun)
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
One follow-up created as mentioned: https://bugzilla.mozilla.org/show_bug.cgi?id=1098659
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8522793 -
Flags: review?(jwwang)
Comment 7•10 years ago
|
||
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?
Assignee | ||
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
(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
Comment 11•10 years ago
|
||
I think we can assert delete-in-main-thread in the destructors of MediaResource to help us catch such violations.
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
How? I can't find where a pure virtual destructor is defined.
Assignee | ||
Comment 17•10 years ago
|
||
try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7892d5fcb014
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef2372fc405
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ef2372fc405
Status: ASSIGNED → 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
•