Closed Bug 1192675 Opened 5 years ago Closed 5 years ago

Not all MediaDataDecoder are threadsafe

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file, 1 obsolete file)

In bug 1061525 I see some highly suspicious failures.

We've always assumed that Apple's VideoToolbox is thread-safe. But that may be an invalid assumptions as it's accessed from multiple threads at once.

Will need bug 1146086 to properly handle this
Comment on attachment 8645666 [details] [diff] [review]
P1. Ensure VDA/VT APIs are only ever accessed from the same thread.

Actually, cpearce would be a better fit, as it's very similar to the work he did with the WMF decoder
Attachment #8645666 - Flags: review?(matt.woodrow) → review?(cpearce)
Assignee: nobody → jyavenard
Comment on attachment 8645666 [details] [diff] [review]
P1. Ensure VDA/VT APIs are only ever accessed from the same thread.

rebased
Attachment #8645666 - Attachment is obsolete: true
Attachment #8645666 - Flags: review?(cpearce)
Comment on attachment 8646206 [details] [diff] [review]
P1. Ensure VDA/VT APIs are only ever accessed from the same thread.

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

r+ with comments addressed.

::: dom/media/platforms/apple/AppleUtils.h
@@ +64,5 @@
> +  }
> +  // Copy operator
> +  CFRefPtr<T>& operator=(const CFRefPtr<T>& aCFRefPtr)
> +  {
> +    if (mRef) {

What if you go:

CFRefPtr<SomeType> p(new SomeType());
p = p;

Then the CFRelease(mRef) here would null out the incoming ref, and you'd assign null to mRef.

::: dom/media/platforms/apple/AppleVDADecoder.cpp
@@ +119,5 @@
>        aSample->mDuration,
>        aSample->mKeyframe ? " keyframe" : "",
>        aSample->Size());
>  
> +  mInputIncoming = true;

Shouldn't mInputIncoming be a counter instead of a boolean? What if multiple inputs are submitted consecutively and both the AppleVDADecoder::Input() calls run before the first AppleVDADecoder::SubmitFrame() runs? Then AppleVDADecoder::SubmitFrame() will clear mInputIncoming on the first incoming input but before the second call runs, i.e. mInputIncoming will be cleared when there's still a task to call SubmitFrame pending.

@@ +144,5 @@
> +    NS_NewRunnableMethod(this, &AppleVDADecoder::ProcessFlush);
> +  MonitorAutoLock mon(mMonitor);
> +  mTaskQueue->Dispatch(runnable.forget());
> +  while (mIsFlushing) {
> +    mon.Wait();

I look forward to the day where MediaDataDecoder::Flush() returns a MozPromise. :)

@@ +498,5 @@
>      CFRetain(frameInfo);
>    }
>  
>    // Ask for more data.
> +  if (!mInputIncoming) {

There could be a task in the task queue waiting to run that calls AppleVDADecoder::SubmitFrame(). You'll be firing InputExhausted() too early here.

::: dom/media/platforms/apple/AppleVDADecoder.h
@@ +83,5 @@
>  
> +  void DispatchOutputTask(already_AddRefed<nsIRunnable> aTask)
> +  {
> +    nsCOMPtr<nsIRunnable> task = aTask;
> +    if (mIsShutDown || mIsFlushing) {

You need to be holding the monitor here because you're accessing mIsFlushing.

::: dom/media/platforms/apple/AppleVTDecoder.cpp
@@ +102,5 @@
>    }
>    LOG("    sha1 %s", digest.get());
>  #endif // LOG_MEDIA_SHA1
>  
> +  mInputIncoming = true;

Same comment about mInputIncoming not accurately reflecting whether there's input incoming applies here.
Attachment #8646206 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #6)
> Comment on attachment 8646206 [details] [diff] [review]
> P1. Ensure VDA/VT APIs are only ever accessed from the same thread.
> 
> Review of attachment 8646206 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> ::: dom/media/platforms/apple/AppleUtils.h
> @@ +64,5 @@
> > +  }
> > +  // Copy operator
> > +  CFRefPtr<T>& operator=(const CFRefPtr<T>& aCFRefPtr)
> > +  {
> > +    if (mRef) {
> 
> What if you go:
> 
> CFRefPtr<SomeType> p(new SomeType());
> p = p;
> 
> Then the CFRelease(mRef) here would null out the incoming ref, and you'd
> assign null to mRef.

I'm having a headache just reading this, imagine trying to rewrite the code for that :)

Maybe I'll just remove that method, it's not used.

> > +  mInputIncoming = true;
> 
> Shouldn't mInputIncoming be a counter instead of a boolean? What if multiple
> inputs are submitted consecutively and both the AppleVDADecoder::Input()
> calls run before the first AppleVDADecoder::SubmitFrame() runs? Then
> AppleVDADecoder::SubmitFrame() will clear mInputIncoming on the first
> incoming input but before the second call runs, i.e. mInputIncoming will be
> cleared when there's still a task to call SubmitFrame pending.

See below.

> @@ +498,5 @@
> >      CFRetain(frameInfo);
> >    }
> >  
> >    // Ask for more data.
> > +  if (!mInputIncoming) {
> 
> There could be a task in the task queue waiting to run that calls
> AppleVDADecoder::SubmitFrame(). You'll be firing InputExhausted() too early
> here.

mInputIncoming is cleared upon entering in SubmitFrame

This is only to handle the case where between the time SubmitFrame started and the time we test mInputIncoming , Input() was called once again.
In which case there's no need to call InputExhausted

If Input() was called while we were processing SubmitFrame() ; then SubmitFrame() won't call InputExhausted() and the next run of SubmitFrame() will.

Even ifInputExhausted was called too many times, it isn't a problem. The MediaFormatReader will only process it once.


> 
> ::: dom/media/platforms/apple/AppleVDADecoder.h
> @@ +83,5 @@
> >  
> > +  void DispatchOutputTask(already_AddRefed<nsIRunnable> aTask)
> > +  {
> > +    nsCOMPtr<nsIRunnable> task = aTask;
> > +    if (mIsShutDown || mIsFlushing) {
> 
> You need to be holding the monitor here because you're accessing mIsFlushing.

ah yes.. indeed.

> 
> ::: dom/media/platforms/apple/AppleVTDecoder.cpp
> @@ +102,5 @@
> >    }
> >    LOG("    sha1 %s", digest.get());
> >  #endif // LOG_MEDIA_SHA1
> >  
> > +  mInputIncoming = true;
> 
> Same comment about mInputIncoming not accurately reflecting whether there's
> input incoming applies here.

All we care about if there's one incoming pending, in which case we don't have to tell the reader that we need more data.

As such, a boolean is okay. It's just the equivalent to !mTaskQueue->IsEmpty()

The problem here is that now that the VDA or VT callback can queue task on its own and that often the call to VTDecompressionSessionDecodeFrame can immediately call the callback (like if the frame is a keyframe) ; we ended up with mTaskQueue->IsEmpty() returning false, and as such we weren't calling mCallback->RequestingMoreData() which lead to stall.

We can't rely on the task queue being empty to know if another Input is pending.
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> > Comment on attachment 8646206 [details] [diff] [review]
> > P1. Ensure VDA/VT APIs are only ever accessed from the same thread.
> > 
> > Review of attachment 8646206 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r+ with comments addressed.
> > 
> > ::: dom/media/platforms/apple/AppleUtils.h
> > @@ +64,5 @@
> > > +  }
> > > +  // Copy operator
> > > +  CFRefPtr<T>& operator=(const CFRefPtr<T>& aCFRefPtr)
> > > +  {
> > > +    if (mRef) {
> > 
> > What if you go:
> > 
> > CFRefPtr<SomeType> p(new SomeType());
> > p = p;
> > 
> > Then the CFRelease(mRef) here would null out the incoming ref, and you'd
> > assign null to mRef.
> 
> I'm having a headache just reading this, imagine trying to rewrite the code
> for that :)

You can also add a "if (aCFRefPtr.mRef == mRef) return;" check.
Blocks: 1193669
https://hg.mozilla.org/mozilla-central/rev/9ada003cbfbf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1197083
No longer blocks: 1197083
See Also: → 1197083
Blocks: 1197083
Blocks: 1199193
Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
See Also: → 1206568
You need to log in before you can comment on or make changes to this bug.