Closed Bug 1154512 Opened 5 years ago Closed 5 years ago

Don't use MediaTaskQueue::SyncDispatch() in Gonk PlatformDecoderModule

Categories

(Core :: Audio/Video, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: cpearce, Assigned: ayang)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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)
Sure, no problem.
Assignee: nobody → ayang
Flags: needinfo?(ayang)
Attachment #8592720 - Flags: review?(cpearce)
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+
(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.
Attachment #8592720 - Attachment is obsolete: true
Attachment #8594673 - Attachment is patch: true
Attachment #8594673 - Flags: feedback?(cpearce)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8bc8f9c19385
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.