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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Kenneth Russell <kbr at google.com>  2010-09-13 19:24:18 PST ---
(From update of attachment 67257)
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.

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