Bug 51424 – Add WebKitClient::createAudioDevice() for Chromium port of web audio API

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

[Chris Rogers]

Add WebKitClient::createAudioDevice() for Chromium port of web audio API

[Chris Rogers]

Created attachment 77154 [details]
Patch

[Chris Rogers]

Darin, does the callback typedef in WebAudioDevice need to have the WEBKIT_API prefix?

[WebKit Review Bot]

Attachment 77154 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7252090

[Chris Rogers]

The Chromium build error I'll fix by landing AudioBusChromium in another patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 77154 [details]
Patch

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

> WebKit/chromium/public/WebAudioDevice.h:45
> +    typedef void (*RenderCallback)(std::vector<float*>& audioData, size_t numberOfFrames, void* userData);

note: we do not use STL types in the WebKit API.  please use WebVector, or just use a raw float* here.  do you need the callee to allocate the audioData buffer, or will it be preallocated to the maximum size by the caller?  if it will be preallocated, then passing a raw float* would seem fine.

> WebKit/chromium/public/WebKitClient.h:282
> +    // Audio --------------------------------------------------------------
> +    virtual WebAudioDevice* createAudioDevice(size_t bufferSize, unsigned numberOfChannels, double sampleRate, WebAudioDevice::RenderCallback, void* userData) { return 0; }

nit: add a new line below the section header.

The C++ way of passing user data to a function pointer is to define an interface
that folks can subclass.  Why not do that here?

class WebAudioRenderCallback {
public:
    virtual void renderFrames(float* audioData, size_t numberOfFrames) = 0;
protected:
    virtual ~WebAudioRenderCallback() {}
};

or you could also do this as an inner class:

class WebAudioDevice {
public:
    class RenderCallback {
    public:
        ...
    };
};

however the downside to it being an inner class is that you force WebKitClient.h
to also #include WebAudioDevice.h.  it would be nice to avoid that.

> WebKit/chromium/src/AudioDestinationChromium.cpp:40
> +using namespace std;

please avoid using the "std" namespace for container types in webkit code.

> WebKit/chromium/src/AudioDestinationChromium.cpp:41
> +using WebKit::webKitClient;

it is more commonplace to just do "using namespace WebKit" in WebKit code.

> WebKit/chromium/src/AudioDestinationChromium.cpp:102
> +void AudioDestinationChromium::renderDispatch(vector<float*>& audioData, size_t numberOfFrames, void* userData)

if you define an interface for the render callback function, then you would
not need to implement this thunk function yourself.

[WebKit Review Bot]

Attachment 77154 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7275108

[Chris Rogers]

Comment on attachment 77154 [details]
Patch

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

>> WebKit/chromium/public/WebAudioDevice.h:45
>> +    typedef void (*RenderCallback)(std::vector<float*>& audioData, size_t numberOfFrames, void* userData);
> 
> note: we do not use STL types in the WebKit API.  please use WebVector, or just use a raw float* here.  do you need the callee to allocate the audioData buffer, or will it be preallocated to the maximum size by the caller?  if it will be preallocated, then passing a raw float* would seem fine.

FIXED: I switched over to WebVector.  I can't use raw float* since it's an array of pointers to float (planar non-interleaved)

>> WebKit/chromium/public/WebKitClient.h:282
>> +    virtual WebAudioDevice* createAudioDevice(size_t bufferSize, unsigned numberOfChannels, double sampleRate, WebAudioDevice::RenderCallback, void* userData) { return 0; }
> 
> nit: add a new line below the section header.
> 
> The C++ way of passing user data to a function pointer is to define an interface
> that folks can subclass.  Why not do that here?
> 
> class WebAudioRenderCallback {
> public:
>     virtual void renderFrames(float* audioData, size_t numberOfFrames) = 0;
> protected:
>     virtual ~WebAudioRenderCallback() {}
> };
> 
> or you could also do this as an inner class:
> 
> class WebAudioDevice {
> public:
>     class RenderCallback {
>     public:
>         ...
>     };
> };
> 
> however the downside to it being an inner class is that you force WebKitClient.h
> to also #include WebAudioDevice.h.  it would be nice to avoid that.

FIXED: added the new line and changed this to use an inner class as you suggest instead of old-fashioned callback

>> WebKit/chromium/src/AudioDestinationChromium.cpp:40
>> +using namespace std;
> 
> please avoid using the "std" namespace for container types in webkit code.

FIXED

>> WebKit/chromium/src/AudioDestinationChromium.cpp:41
>> +using WebKit::webKitClient;
> 
> it is more commonplace to just do "using namespace WebKit" in WebKit code.

FIXED

>> WebKit/chromium/src/AudioDestinationChromium.cpp:102
>> +void AudioDestinationChromium::renderDispatch(vector<float*>& audioData, size_t numberOfFrames, void* userData)
> 
> if you define an interface for the render callback function, then you would
> not need to implement this thunk function yourself.

FIXED

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> >> WebKit/chromium/public/WebAudioDevice.h:45
> >> +    typedef void (*RenderCallback)(std::vector<float*>& audioData, size_t numberOfFrames, void* userData);
> > 
> > note: we do not use STL types in the WebKit API.  please use WebVector, or just use a raw float* here.  do you need the callee to allocate the audioData buffer, or will it be preallocated to the maximum size by the caller?  if it will be preallocated, then passing a raw float* would seem fine.
> 
> FIXED: I switched over to WebVector.  I can't use raw float* since it's an array of pointers to float (planar non-interleaved)

oh, right!  what about float** then?  aren't you concerned about the overhead of copying the buffers that a WebVector imposes?


> > however the downside to it being an inner class is that you force WebKitClient.h
> > to also #include WebAudioDevice.h.  it would be nice to avoid that.
> 
> FIXED: added the new line and changed this to use an inner class as you suggest instead of old-fashioned callback

i also suggested not using inner classes since it forces a #include.  any thoughts
on that?

[Chris Rogers]

Created attachment 77165 [details]
Patch

[Chris Rogers]

(In reply to comment #8)
> (In reply to comment #7)
> > >> WebKit/chromium/public/WebAudioDevice.h:45
> > >> +    typedef void (*RenderCallback)(std::vector<float*>& audioData, size_t numberOfFrames, void* userData);
> > > 
> > > note: we do not use STL types in the WebKit API.  please use WebVector, or just use a raw float* here.  do you need the callee to allocate the audioData buffer, or will it be preallocated to the maximum size by the caller?  if it will be preallocated, then passing a raw float* would seem fine.
> > 
> > FIXED: I switched over to WebVector.  I can't use raw float* since it's an array of pointers to float (planar non-interleaved)
> 
> oh, right!  what about float** then?  aren't you concerned about the overhead of copying the buffers that a WebVector imposes?

The only overhead is copying the pointers, but not the actual PCM data.  But, if you really don't like WebVector, then we can switch to float**.
WebVector just seems a little safer, since we can check its size...



> 
> > > however the downside to it being an inner class is that you force WebKitClient.h
> > > to also #include WebAudioDevice.h.  it would be nice to avoid that.
> > 
> > FIXED: added the new line and changed this to use an inner class as you suggest instead of old-fashioned callback
> 
> i also suggested not using inner classes since it forces a #include.  any thoughts
> on that?

It's up to you.  I like the inner class since it seems cleaner, but if you feel the extra include is too big a build-time penalty then I can change it...

[WebKit Review Bot]

Attachment 77165 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7255096

[Darin Fisher (:fishd, Google)]

Comment on attachment 77165 [details]
Patch

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

> WebKit/chromium/src/AudioDestinationChromium.cpp:47
> +const unsigned CallbackBufferSize = 2048;

I'm pretty sure the preferred style for constants is to use variableNaming style.

> WebKit/chromium/src/AudioDestinationChromium.cpp:105
> +    if (!isNumberOfChannelsGood)

nit: instead of repeating the expression, use ASSERT_NOT_REACHED if the branch is taken.

> WebKit/chromium/src/AudioDestinationChromium.cpp:110
> +    if (!isBufferSizeGood)

ditto

[Chris Rogers]

Committed r75118: <http://trac.webkit.org/changeset/75118>