Closed
Bug 1154512
Opened 9 years ago
Closed 9 years ago
Don't use MediaTaskQueue::SyncDispatch() in Gonk PlatformDecoderModule
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: cpearce, Assigned: ayang)
References
Details
Attachments
(1 file, 2 obsolete files)
13.89 KB,
patch
|
ayang
:
review+
|
Details | Diff | Splinter Review |
The Gonk PDM is using MediaTaskQueue::SyncDispatch(): http://mxr.mozilla.org/mozilla-central/search?string=SyncDispatch&find=dom%2Fmedia%2Ffmp4%2Fgonk&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central This function is unsafe. If a task queue flush happens at the same time, SyncDispatch() may never return. It seems flushes can and do happen. We saw from Bug 1154133 that removing some sync dispatches from the EME PDM made our MacOSX and Linux EME mochitests more reliable. We should remove MediaTaskQueue::SyncDispatch() calls from the The Gonk PDM.
Reporter | ||
Comment 1•9 years ago
|
||
Alfredo: are you able to take this bug? Once we have removed the SyncDispatch() calls from the Gonk PDM, we can remove SyncDispatch() from MediaTaskQueue.
Flags: needinfo?(ayang)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8592720 -
Flags: review?(cpearce)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8592720 [details] [diff] [review] remove_time_table_and_sync_dispatch Review of attachment 8592720 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp @@ +38,1 @@ > I'm a little wary of holding the mutex for the entire duration of this function, as it calls out to OMX and so we could hold the lock for an unknown duration of time... Can we only lock the mutex while manipulating mQueueSample, rather than for the entire function? Is that safe? It looks like it would be safe. @@ +99,5 @@ > nsresult > GonkDecoderManager::Flush() > { > + MutexAutoLock lock(mMutex); > + mQueueSample.Clear(); The purpose of Flush() is to direct the decoder to drop any samples/frames in its queue. Here you just drop any pending input, and block output... Is there a way that you can cause OMX to discard any samples it's buffered internally? ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.h @@ +58,5 @@ > // It sends MP4Sample to OMX layer. It must be overrided by subclass. > virtual android::status_t SendSampleToOMX(MediaRawData* aSample) = 0; > > + // This mutex protects mQueueSample. > + Mutex mMutex; Nit: trailing space.
Attachment #8592720 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #4) > Comment on attachment 8592720 [details] [diff] [review] > remove_time_table_and_sync_dispatch > > Review of attachment 8592720 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp > @@ +38,1 @@ > > > > I'm a little wary of holding the mutex for the entire duration of this > function, as it calls out to OMX and so we could hold the lock for an > unknown duration of time... Can we only lock the mutex while manipulating > mQueueSample, rather than for the entire function? Is that safe? It looks > like it would be safe. Yes, I'll do that. > > @@ +99,5 @@ > > nsresult > > GonkDecoderManager::Flush() > > { > > + MutexAutoLock lock(mMutex); > > + mQueueSample.Clear(); > > The purpose of Flush() is to direct the decoder to drop any samples/frames > in its queue. Here you just drop any pending input, and block output... Is > there a way that you can cause OMX to discard any samples it's buffered > internally? Yes, OMX has flush() command which is called by Flush() override function in GonkAudioDecoderManager and GonkVideoDecoderManager. The class hierarchy is a little complicated among GonkMediaDataDecoder, GonkAudioDecoderManager and GonkVideoDecoderManager, I plan to remove the hierarchy in bug 1127656. > > ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.h > @@ +58,5 @@ > > // It sends MP4Sample to OMX layer. It must be overrided by subclass. > > virtual android::status_t SendSampleToOMX(MediaRawData* aSample) = 0; > > > > + // This mutex protects mQueueSample. > > + Mutex mMutex; > > Nit: trailing space.
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8592720 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8594673 -
Attachment is patch: true
Assignee | ||
Updated•9 years ago
|
Attachment #8594673 -
Flags: feedback?(cpearce)
Assignee | ||
Comment 7•9 years ago
|
||
Carry r+. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9a4300696a
Attachment #8594673 -
Attachment is obsolete: true
Attachment #8594673 -
Flags: feedback?(cpearce)
Attachment #8600837 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bc8f9c19385
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
•