Share a decoding thread pool for all AudioContexts

RESOLVED DUPLICATE of bug 1108701

Status

()

defect
RESOLVED DUPLICATE of bug 1108701
7 years ago
Last year

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla30
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
Right now we use a decoding thread pool per AudioContext.  We should use a global one.
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.
Assignee

Updated

5 years ago
Assignee: nobody → ehsan
Component: Video/Audio → Web Audio
Assignee

Updated

5 years ago
Attachment #8378675 - Flags: review?(paul)
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 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
Attachment #8378675 - Flags: review?(paul) → review+
(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.
Assignee

Comment 6

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/169de60f0b1e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Backed out for shutdown hang reported in bug 1026325.
https://hg.mozilla.org/integration/mozilla-inbound/rev/109ded3295f8
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Depends on: 1097823
No longer depends on: 1097823
Depends on: 1108701
Fixed in bug 1108701.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
No longer depends on: 1108701
Resolution: --- → DUPLICATE
Duplicate of bug: 1108701
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.