[Webkit-unassigned] [Bug 45571] Add AudioNode files

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Chris Rogers <crogers at google.com>  2010-09-14 15:08:32 PST ---
> 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.