Add WebKitClient::createAudioDevice() for Chromium port of web audio API
Created attachment 77154 [details] Patch
Darin, does the callback typedef in WebAudioDevice need to have the WEBKIT_API prefix?
Attachment 77154 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7252090
The Chromium build error I'll fix by landing AudioBusChromium in another patch.
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.
Attachment 77154 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7275108
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
(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?
Created attachment 77165 [details] Patch
(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...
Attachment 77165 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7255096
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
Committed r75118: <http://trac.webkit.org/changeset/75118>