Add AudioContext files
Created attachment 65948 [details] Patch
I just realized I need to add the vendor prefix "webkit" to the IDL. But anybody interested please go ahead and review the patch (minus this one detail :)
Sorry nevermind, Dimitri Glazkov just told me that it's DOMWindow.idl which will handle the "webkit" prefix, so that's not an issue with this patch.
Implements AudioContext as described in the web audio specification: http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioContext-section
Comment on attachment 65948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=65948&action=prettypatch > WebCore/webaudio/AudioContext.h:31 > + I think you want #if ENABLE(WEB_AUDIO) here as well, no? > WebCore/webaudio/AudioContext.h:62 > + static PassRefPtr<AudioContext> create(Document* document); nit: no need to name this parameter > WebCore/webaudio/AudioContext.h:75 > + float currentTime() { return (float)m_destinationNode->currentTime(); } > + float sampleRate() { return (float)m_destinationNode->sampleRate(); } I'm not sure why these have to cast, but please use C++ style static_cast<float>() instead of C-style casts. > WebCore/webaudio/AudioContext.h:101 > + void notifyNodeFinishedProcessing(AudioNode* node); nit: no need to name this parameter in the header > WebCore/webaudio/AudioContext.h:105 > + void markForDeletion(AudioNode* node); nit: no need to name this parameter in the header > WebCore/webaudio/AudioContext.h:109 > + // Gets called everytime an AudioNode is connected. > + void nodeConnected(AudioNode* /*source*/, AudioNode* /*destination*/, unsigned /*outputIndex*/, unsigned /*inputIndex*/) This is a little odd. Does this function need a FIXME? It doesn't really seem to do what the signature suggests it might. > WebCore/webaudio/AudioContext.h:125 > + AudioContext(Document* document); > + void lazyInitialize(); > + void uninitialize(); > + > + // The context itself keeps a reference to all source nodes. The source nodes, then reference all nodes they're connected to, and so on... > + void refNode(AudioNode* node); > + void derefNode(AudioNode* node); > + void derefUnfinishedSourceNodes(); nit: no need to name these parameters in the header
Created attachment 66969 [details] Patch
James, thanks for the initial review. I've addressed all of your comments. I didn't put a feature enable: #if ENABLE(WEB_AUDIO) in the header file because I was told to only put them in the .cpp files.
Comment on attachment 66969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66969&action=prettypatch Looking good overall. You're right about #if ENABLE()s in a header file, not sure why I thought otherwise. r- because of the RefPtr<Document> issue - I want to be sure we are handling that correctly since if it's subtly wrong it could be very hard to debug and fix down the road. > WebCore/webaudio/AudioContext.cpp:51 > +#include "PlatformString.h" Include <wtf/text/WTFString.h> to get the string class (it moved fairly recently). > WebCore/webaudio/AudioContext.cpp:60 > +PassRefPtr<CachedAudio> AudioContext::createAudioRequest(const String &url, bool mixToMono) Do we have to use strings for URLs? I think this will make the security folks sad pandas. As a general note, prefer enums to bools for parameters. It's very difficult to tell what the 'false' in createAudioContext(..., false) means. Something like createAudioContext(..., MixToMono/MixToStereo) or whatever the appropriate two values would be is easier to understand. > WebCore/webaudio/AudioContext.h:119 > + AudioContext(Document* document); nit: don't name this parameter > WebCore/webaudio/AudioContext.h:123 > + // The context itself keeps a reference to all source nodes. The source nodes, then reference all nodes they're connected to, and so on... This comment sort of trails off. I'm not sure what it's trying to say. > WebCore/webaudio/AudioContext.h:149 > + // FIXME: check if we really need to hold on to document - we may need to, so be careful! > + RefPtr<Document> m_document; This makes me a little bit nervous. What's the plan to resolve this FIXME?
Created attachment 67279 [details] Patch
James, I've removed the createAudioRequest() method since this is just a temporary hack to access audio file binary data in an XHR-like way. I'll be looking to replace it with XHR with binary data capabilities. I think I've addressed all your other comments. I got rid of the RefPtr on Document, since I was able to locate why this was necessary and check that the document is still there by adding a hasDocument() method (I clear m_document when stop() is called).
Created attachment 67734 [details] Patch
I've added a locking mechanism with lock(), tryLock(), and unlock() methods.
Created attachment 68279 [details] Patch
This patch addresses some more work which was done in AudioNode. I've added extensive asserts about threads and locks being held 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. and it works well.
Comment on attachment 68279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68279&action=review We've gone through the locking semantics of this class as well as AudioNode at length offline, and at this point I think this looks great. Marking r+; a couple of minor issues that aren't really necessary before commit, so leave them up to you. > WebCore/webaudio/AudioContext.cpp:60 > +const int UndefinedThreadIdentifier = 0xffffffff; From looking at ThreadingPthreads.cpp and ThreadingWin.cpp, I think that 0 is the correct value to use here. Regardless it would be good to define that explicitly in WTF. > WebCore/webaudio/AudioContext.cpp:114 > + printf("%p: AudioContext::~AudioContext()\n", this); Should use LOG_ERROR or similar. > WebCore/webaudio/AudioContext.cpp:352 > + m_graphOwnerThread = thisThread; This could be placed in the "else" clause to avoid mutating this member over and over; not a big deal. > WebCore/webaudio/AudioContext.cpp:437 > + ASSERT(isAudioThread()); Could use an ASSERT(isGraphOwner()) too.
Comment on attachment 68279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68279&action=review > WebCore/webaudio/AudioContext.idl:60 > + RealtimeAnalyserNode createAnalyser(); One more minor comment: I don't see this method in the Web Audio spec.
Comment on attachment 68279 [details] Patch Clearing flags on attachment: 68279 Committed r68163: <http://trac.webkit.org/changeset/68163>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/68163 might have broken GTK Linux 32-bit Debug The following changes are on the blame list: http://trac.webkit.org/changeset/68163 http://trac.webkit.org/changeset/68164 http://trac.webkit.org/changeset/68165 http://trac.webkit.org/changeset/68166 http://trac.webkit.org/changeset/68167 http://trac.webkit.org/changeset/68168 http://trac.webkit.org/changeset/68169
So the main interface is non-prefixed? It certainly should be prefixed with webkit, because there is no specification for Audio yet, just proposals.
The webkit prefix will be in the patch for adding the AudioContext constructor in DOMWindow.idl. So an AudioContext will be constructed as follows: var context = new webkitAudioContext();
Ok, great.