[Webkit-unassigned] [Bug 67952] Ask for audio hardware buffer size instead of using hardwired constants.

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Chris Rogers <crogers at google.com>  2011-09-12 15:02:10 PST ---
(From update of attachment 107084)
View in context: https://bugs.webkit.org/attachment.cgi?id=107084&action=review

>> Source/WebCore/ChangeLog:11
>> +	Add declaration for hardwareAudioBufferSize().
> 
> Line contains tab character.  [whitespace/tab] [5]

It looks like you're using tabs in this file.  I'd recommend configuring your text editor to not use tabs, but instead use "spaces for tabs".  In WebKit's case one tab should equal four spaces.

> Source/WebCore/platform/audio/AudioDestination.h:55
> +    static int hardwareAudioBufferSize();

I would remove this as a public method and instead simply get the buffer size in the .cpp file (see below)
This is just an implementation detail.

>> Source/WebKit/chromium/ChangeLog:10
>> +	Define default audioHardwareBufferSize().
> 
> Line contains tab character.  [whitespace/tab] [5]

As with the other ChangeLog it looks like you are using tab characters several places in this file.  Please use "soft tabs" with four spaces per tab

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:46
> +unsigned callbackBufferSize = 128;

Because this is no a longer hard-coded constant, we should remove this static global variable.  Instead, use local variables in the constructor as necessary.

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:51
> +unsigned renderCountPerCallback = callbackBufferSize / renderBufferSize;

renderCountPerCallback should no longer be a static global constant, but can be changed to a local variable in the constructor since its value is calculated there...

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:69
> +    callbackBufferSize = hardwareAudioBufferSize();

callbackBufferSize should be a local variable here.

Probably overkill to have a separate function "hardwareAudioBufferSize()".  Instead it would be more direct to simply call:
webKitPlatformSupport()->audioHardwareBufferSize();

directly...

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:70
> +    renderCountPerCallback = callbackBufferSize / renderBufferSize;

Here we should probably check for exact divisibility by "renderBufferSize".  We'll need to round up to the nearest multiple of renderBufferSize >= callbackBufferSize

This is because in the chromium back-end, we're no longer guaranteeing that the buffer size value will be a multiple of renderBufferSize.

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:102
> +int AudioDestination::hardwareAudioBufferSize()

Probably best to remove this and directly call webKitPlatformSupport()->audioHardwareBufferSize() in the constructor...

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