821062 - Share a decoding thread pool for all AudioContexts

Status: RESOLVED DUPLICATE of bug 1108701

(Core :: Web Audio, defect)

Description

[(no longer active)]

Right now we use a decoding thread pool per AudioContext.  We should use a global one.

Comment 1

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

I have added content/media/SharedThreadPool.h which is adds the ability for threadsafe singleton shared thread pools to be acquired by a nsCString name, so they can be easily shared and disposed of with minimal fuss. I recommend you use them, rather than rolling your own again.

Comment 2

[(no longer active)]

Attachment: Share a decoding thread pool for all AudioContexts; r=padenot

Comment 3

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

Comment on attachment 8378675 [details] [diff] [review]
Share a decoding thread pool for all AudioContexts; r=padenot

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

Can you change the "MOZ_ASSERT(!mThreadPool == NS_IsMainThread()," assertions in MediaDecodeTask::Decode() to a !NS_IsMainThread() assertion? The fact that this code still implies that it could run on the main thread worries me.

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8378675 [details] [diff] [review]
Share a decoding thread pool for all AudioContexts; r=padenot

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

::: content/media/webaudio/MediaBufferDecoder.h
@@ +91,5 @@
>  private:
>    bool EnsureThreadPoolInitialized();
>  
>  private:
> +  RefPtr<SharedThreadPool> mThreadPool;

Hm, didn't khuey recently said we should not use RefPtr ? [1] Can we just change that to an nsRefPtr instead?

[1]: http://logbot.glob.com.au/?c=mozilla%23media&s=18+Feb+2014&e=18+Feb+2014&h=MOZ_COUNT_CTOR#c48666

Comment 5

[Paul Adenot (:padenot)]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8378675 [details] [diff] [review]
> Share a decoding thread pool for all AudioContexts; r=padenot
> 
> Review of attachment 8378675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you change the "MOZ_ASSERT(!mThreadPool == NS_IsMainThread(),"
> assertions in MediaDecodeTask::Decode() to a !NS_IsMainThread() assertion?
> The fact that this code still implies that it could run on the main thread
> worries me.

This can only happen if one fiddles with a pref that is not even in the default list of pref. It might even not work anymore, but I seem to recall ehsan had a good reason to no remove this code.

Comment 6

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #5)
> (In reply to Chris Pearce (:cpearce) from comment #3)
> > Comment on attachment 8378675 [details] [diff] [review]
> > Share a decoding thread pool for all AudioContexts; r=padenot
> > 
> > Review of attachment 8378675 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can you change the "MOZ_ASSERT(!mThreadPool == NS_IsMainThread(),"
> > assertions in MediaDecodeTask::Decode() to a !NS_IsMainThread() assertion?
> > The fact that this code still implies that it could run on the main thread
> > worries me.
> 
> This can only happen if one fiddles with a pref that is not even in the
> default list of pref. It might even not work anymore, but I seem to recall
> ehsan had a good reason to no remove this code.

I think we should remove it when Blink ships the unprefixed AudioContext implementation and remove their webkitAudioContext implementation, which is going to happen soon.

(In reply to Paul Adenot (:padenot) from comment #4)
> Comment on attachment 8378675 [details] [diff] [review]
> Share a decoding thread pool for all AudioContexts; r=padenot
> 
> Review of attachment 8378675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/MediaBufferDecoder.h
> @@ +91,5 @@
> >  private:
> >    bool EnsureThreadPoolInitialized();
> >  
> >  private:
> > +  RefPtr<SharedThreadPool> mThreadPool;
> 
> Hm, didn't khuey recently said we should not use RefPtr ? [1] Can we just
> change that to an nsRefPtr instead?
> 
> [1]:
> http://logbot.glob.com.au/
> ?c=mozilla%23media&s=18+Feb+2014&e=18+Feb+2014&h=MOZ_COUNT_CTOR#c48666

This code forces you to use RefPtr (because of the TemporaryRef return value) but I'm fixing that mess in bug 935778.  One more usage won't hurt that much.

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/169de60f0b1e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/169de60f0b1e

Comment 9

[Karl Tomlinson (:karlt)]

Try run for backout
https://tbpl.mozilla.org/?tree=Try&rev=c170ac4f53bf
https://tbpl.mozilla.org/?tree=Try&rev=f6e6a9649ea1

Comment 10

[Karl Tomlinson (:karlt)]

Backed out for shutdown hang reported in bug 1026325.
https://hg.mozilla.org/integration/mozilla-inbound/rev/109ded3295f8

Comment 11

[Karl Tomlinson (:karlt)]

Fixed in bug 1108701.

Comment 12

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.