Bug 44890 – Add AudioContext files

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

[Chris Rogers]

Add AudioContext files

[Chris Rogers]

Created attachment 65948 [details]
Patch

[Chris Rogers]

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 :)

[Chris Rogers]

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.

[Chris Rogers]

Implements AudioContext as described in the web audio specification:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioContext-section

[James Robinson]

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

[Chris Rogers]

Created attachment 66969 [details]
Patch

[Chris Rogers]

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.

[James Robinson]

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?

[Chris Rogers]

Created attachment 67279 [details]
Patch

[Chris Rogers]

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).

[Chris Rogers]

Created attachment 67734 [details]
Patch

[Chris Rogers]

I've added a locking mechanism with lock(), tryLock(), and unlock() methods.

[Chris Rogers]

Created attachment 68279 [details]
Patch

[Chris Rogers]

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.

[Kenneth Russell]

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.

[Kenneth Russell]

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.

[WebKit Commit Bot]

Comment on attachment 68279 [details]
Patch

Clearing flags on attachment: 68279

Committed r68163: <http://trac.webkit.org/changeset/68163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Olli Pettay (:smaug)]

So the main interface is non-prefixed?
It certainly should be prefixed with webkit, because there
is no specification for Audio yet, just proposals.

[Chris Rogers]

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();

[Olli Pettay (:smaug)]

Ok, great.