Bug 45571 – Add AudioNode files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add AudioNode files

[Chris Rogers]

Created attachment 67257 [details]
Patch

[Chris Rogers]

This is the implementation for the AudioNode object as defined in the web audio specification:

http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioNode-section

[Kenneth Russell]

Comment on attachment 67257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67257&action=prettypatch

Here are my thoughts upon initial review. I'd like to hear your comments before we decide how to proceed with the review.

> WebCore/webaudio/AudioNode.cpp:54
> +        memset(s_nodeCount, 0, sizeof(s_nodeCount));
Static data in C is specified as being initialized to zero, so the memset isn't necessary.

> WebCore/webaudio/AudioNode.cpp:149
> +    // These requests would be handled in the real-time audio thread.
We've talked somewhat offline about what the real thread safety issues are throughout this code; for example, that using WTF's atomicIncrement/atomicDecrement in ref/deref wouldn't help because higher-level locking is needed for consistency of the overall audio graph. The fact that this code works is certainly a strong point in favor of its current structure, but I think it's very important to have good documentation and as many verified invariants as possible to ensure its thread safety, because multithreading bugs and crashes are among the hardest to debug. For one thing, AudioNode.h should document which threads call which methods, and if it is the case that certain methods are only called by one thread (like the real-time audio thread), there should be an assertion to that effect.

> WebCore/webaudio/AudioNode.cpp:202
> +                // Garbage collection - we've already be disconnected
be -> been

> WebCore/webaudio/AudioNode.h:50
> +class AudioNode {
As mentioned in my comments in the .cpp file, this class should document which methods are called on which threads (i.e., the main thread or the real-time audio thread). If a given method is only called by one type of thread or the other, there should be assertions to that effect.

> WebCore/webaudio/AudioNode.h:105
> +    AudioNodeInput* input(unsigned);
These should match the order of numberOfInputs() / numberOfOutputs().

> WebCore/webaudio/AudioNode.h:136
> +    Vector<OwnPtr<AudioNodeInput> > m_inputs;
> +    Vector<OwnPtr<AudioNodeOutput> > m_outputs;
Because these members are protected rather than private, it is difficult to make any guarantees about the overall thread safety of this code. It appears that AudioNodeInputs and AudioNodeOutputs are instantiated by subclasses or other classes, since I don't see any creation in the reviews for AudioNode, AudioNodeInput or AudioNodeOutput. Is there any way to make these members private and encapsulate their creation within the AudioNode class?

> WebCore/webaudio/AudioNode.h:143
> +    int m_disabledRefCount;
These should be marked volatile.

> WebCore/webaudio/AudioNode.idl:33
> +        readonly attribute AudioContext context;
This attribute isn't in the Web Audio API.

[Chris Rogers]

Created attachment 67611 [details]
Patch

[Chris Rogers]

> Here are my thoughts upon initial review. I'd like to hear your comments before we decide how to proceed with the review.

After discussing this with you in some detail, I think that a global recursive lock is the right approach for the connect(), disconnect(), ref(), and deref() methods.  The lock can be taken normally on the main thread, but in the real-time audio thread I will use tryLock().  If this tryLock() fails then the AudioNode being referenced or de-referenced will be added to a queue and will be tried again the next time the tryLock() is successfully acquired.  Because the real-time audio thread is being called very frequently and the tryLock() very rarely fails, this extra queue will rarely be used and will never grow that big.




> WebCore/webaudio/AudioNode.cpp:54
> +        memset(s_nodeCount, 0, sizeof(s_nodeCount));
Static data in C is specified as being initialized to zero, so the memset isn't necessary.

FIXED




> WebCore/webaudio/AudioNode.cpp:149
> +    // These requests would be handled in the real-time audio thread.
We've talked somewhat offline about what the real thread safety issues are throughout this code; for example, that using WTF's atomicIncrement/atomicDecrement in ref/deref wouldn't help because higher-level locking is needed for consistency of the overall audio graph. The fact that this code works is certainly a strong point in favor of its current structure, but I think it's very important to have good documentation and as many verified invariants as possible to ensure its thread safety, because multithreading bugs and crashes are among the hardest to debug. For one thing, AudioNode.h should document which threads call which methods, and if it is the case that certain methods are only called by one thread (like the real-time audio thread), there should be an assertion to that effect.

ADDED ASSERTIONS TO IMPORTANT METHODS: connect(), disconnect(), processIfNecessary(), pullInputs()




> WebCore/webaudio/AudioNode.cpp:202
> +                // Garbage collection - we've already be disconnected
be -> been

FIXED



> WebCore/webaudio/AudioNode.h:50
> +class AudioNode {
As mentioned in my comments in the .cpp file, this class should document which methods are called on which threads (i.e., the main thread or the real-time audio thread). If a given method is only called by one type of thread or the other, there should be assertions to that effect.

ADDED COMMENTS TO IMPORTANT THREAD SAFETY METHODS




> WebCore/webaudio/AudioNode.h:105
> +    AudioNodeInput* input(unsigned);
These should match the order of numberOfInputs() / numberOfOutputs().

FIXED




> WebCore/webaudio/AudioNode.h:136
> +    Vector<OwnPtr<AudioNodeInput> > m_inputs;
> +    Vector<OwnPtr<AudioNodeOutput> > m_outputs;
Because these members are protected rather than private, it is difficult to make any guarantees about the overall thread safety of this code. It appears that AudioNodeInputs and AudioNodeOutputs are instantiated by subclasses or other classes, since I don't see any creation in the reviews for AudioNode, AudioNodeInput or AudioNodeOutput. Is there any way to make these members private and encapsulate their creation within the AudioNode class?


There shouldn't be any thread safety issues with m_inputs and m_outputs since inputs and outputs are only created in the constructor and never dynamically changed during operation.


> WebCore/webaudio/AudioNode.h:143
> +    int m_disabledRefCount;
These should be marked volatile.

FIXED



> WebCore/webaudio/AudioNode.idl:33
> +        readonly attribute AudioContext context;
This attribute isn't in the Web Audio API.

ADDED ATTRIBUTE TO SPEC

[Chris Rogers]

Created attachment 67732 [details]
Patch

[Chris Rogers]

I've added the locking that I mentioned in the previous comment.  This involves a locking mechanism implemented in AudioContext, please see this patch for details:

https://bugs.webkit.org/show_bug.cgi?id=44890

[Chris Rogers]

Created attachment 68282 [details]
Patch

[Chris Rogers]

This patch adds more rigorous thread safety.  I've added extensive asserts about threads and locks being held in the important methods.  I've tested this in both multicore and simulated single-core modes (by turning off all but one thread using hwprefs) under stress tests, repeatedly re-loading the page etc.  I have tested by adding test code to simulate the try locks failing much more frequently than normal to make sure we handle those code paths.  I've run with the instrumentation to make sure that the reference counting doesn't create any memory leaks.

[Kenneth Russell]

Comment on attachment 68282 [details]
Patch

We've discussed both this patch and that for AudioContext extensively offline and at this point I think this looks very good. One comment for the record: the reason that the atomic increments in AudioNode::ref() aren't guarded by the graph lock is covered by the FIXME in that method; this would only be necessary if waking up of a note which has already played were supported, which currently it is not.

[WebKit Commit Bot]

Comment on attachment 68282 [details]
Patch

Clearing flags on attachment: 68282

Committed r68438: <http://trac.webkit.org/changeset/68438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.