1192675 - Not all MediaDataDecoder are threadsafe

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Ensure VDA/VT APIs are only ever accessed from the same thread.

Comment 2

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20ef74e70b24

Comment 3

[Jean-Yves Avenard [:jya]]

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P1. Ensure VDA/VT APIs are only ever accessed from the same thread.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8645666 [details] [diff] [review]
P1. Ensure VDA/VT APIs are only ever accessed from the same thread.

rebased

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Jean-Yves Avenard [:jya]]

(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.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ada003cbfbf

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ada003cbfbf

Comment 11

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0a9391fdb350

Comment 12

[Ralph Giles (:rillian)]

Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c

Comment 13

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c75497d877f2