Bug 67952 – Ask for audio hardware buffer size instead of using hardwired constants.

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

Ask for audio hardware buffer size instead of using hardwired constants.

[Raymond Toy]

Created attachment 107084 [details]
Patch

[WebKit Review Bot]

Attachment 107084 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Rogers]

Comment on attachment 107084 [details]
Patch

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

[Raymond Toy]

Created attachment 107096 [details]
Patch

[Chris Rogers]

Comment on attachment 107096 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:13
> +        Call hardwareAudioBufferSize to get buffer size; update 

minor nit: this function name should be: audioHardwareBufferSize()

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:62
> +    m_callbackBufferSize = webKitPlatformSupport()->audioHardwareBufferSize();

We need to round m_callbackBufferSize to an even multiple of renderBufferSize.  It's not sufficient to make the calculation only involve m_renderCountPerCallback below.  This is because when we call createAudioDevice() we need to pass in a buffer size which is of the proper multiple, otherwise the rendering will not work correctly in render()

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:65
> +    // renderBufferSize.

very minor nit: I'd not wrap this comment and instead keep it on a single line

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:68
> +    m_audioDevice = adoptPtr(webKitPlatformSupport()->createAudioDevice(m_callbackBufferSize, numberOfChannels, sampleRate, this));

But, m_callbackBufferSize *must* be evenly divisible by renderBufferSize since otherwise we won't render properly each time the render() method is called.

> Source/WebKit/chromium/src/AudioDestinationChromium.h:64
> +    unsigned m_renderCountPerCallback;

To be consistent with what audioHardwareBufferSize() returns we should change "unsigned" -> "size_t"

[Raymond Toy]

Created attachment 107201 [details]
Patch

[Chris Rogers]

Comment on attachment 107201 [details]
Patch

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

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:61
> +    m_callbackBufferSize = webKitPlatformSupport()->audioHardwareBufferSize();

I would add a comment here saying something like "Get the minimum usable buffer size. We'll round this value up to a multiple of our render size.
Just to be extra careful, it would be a good idea to compare this value and make sure it's within a certain reasonable range.

I would propose creating a "const size_t maxCallbackBufferSize = 16384;" near the top of the file then check with an ASSERT:
ASSERT(m_callbackBufferSize <= maxCallbackBufferSize);

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:64
> +    // needed.

nit: no need to wrap this comment on two lines.

[Raymond Toy]

Created attachment 107216 [details]
Patch

[Chris Rogers]

Thanks Ray, this looks fine to me.  I'm adding Ken and Darin (for WebKit API) as reviewers.

[Darin Fisher (:fishd, Google)]

Comment on attachment 107216 [details]
Patch

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

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:310
>      virtual double audioHardwareSampleRate() { return 0; }

It seems like we are growing enough audio specific functions here to perhaps
justify defining a WebAudioSupport interface.  This is a larger refactoring
though, and it doesn't have to be a pre-requisite for this patch.

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:66
> +    m_callbackBufferSize = webKitPlatformSupport()->audioHardwareBufferSize();

nit: I think it is a bit awkward to use this member variable to store the audioHardwareBufferSize result.  It seems like it would be better to create a local variable for this.

[Raymond Toy]

Comment on attachment 107216 [details]
Patch

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

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:66
>> +    m_callbackBufferSize = webKitPlatformSupport()->audioHardwareBufferSize();
> 
> nit: I think it is a bit awkward to use this member variable to store the audioHardwareBufferSize result.  It seems like it would be better to create a local variable for this.

m_callbackBufferSize is also used in AudioDestinationChromium::render below.

[Raymond Toy]

Comment on attachment 107216 [details]
Patch

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

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:310
>      virtual double audioHardwareSampleRate() { return 0; }

Yes, this should probably be refactored.  I will look into it.

[Raymond Toy]

Created attachment 107400 [details]
Patch

[Raymond Toy]

Forgot to mention that, as requested, I've added the local variable callbackSize.

[Kenneth Russell]

Comment on attachment 107400 [details]
Patch

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

This looks fine overall, but it has to wait to land until the downstream Chromium code implementing the new entry point has been checked in. At that point, Source/WebKit/chromium/DEPS will presumably need to be rolled forward. If so, then I'd suggest incorporating that roll into this patch so the dependencies are clearer.

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:72
> +    ASSERT(m_callbackBufferSize <= maximumCallbackBufferSize);

Is asserting really the best behavior in this case, rather than for example clamping to the maximum callback buffer size?

[Julien Chaffraix]

*** Bug 67951 has been marked as a duplicate of this bug. ***

[Raymond Toy]

Comment on attachment 107400 [details]
Patch

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

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:72
>> +    ASSERT(m_callbackBufferSize <= maximumCallbackBufferSize);
> 
> Is asserting really the best behavior in this case, rather than for example clamping to the maximum callback buffer size?

Good question.  This was suggested by Chris in an earlier comment (#7).

This also begs the question if we should check for a minimum too.  I think ALSA can actually return a value as low as 1, which would be way too small for us.

[Chris Rogers]

I would just go ahead and add the complete bounds checking to keep the callback size sane.  If audioHardwareBufferSize() returns 0, for example...

[Raymond Toy]

Created attachment 107710 [details]
Patch

[Raymond Toy]

Lower and upper bounds check added for m_callbackBufferSize.  I did not address the comment about the ASSERT, but I can change the ASSERT to clip the value to either the min or max value, if that makes more sense.

[Chris Rogers]

Comment on attachment 107710 [details]
Patch

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

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:47
> +const size_t minimumCallbackBufferSize = 128;

I wouldn't create a new constant here, but instead use the already existing constant "renderBufferSize"

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:74
> +    ASSERT((minimumCallbackBuffersize <= m_callbackBuffersize)

I would suggest something like this:

bool isSizeGood = m_callbackBuffersize >= renderBufferSize && m_callbackBuffersize <= maximumCallbackBufferSize;
ASSERT(isSizeGood);
if (!isSizeGood)
    return;

[Raymond Toy]

Comment on attachment 107710 [details]
Patch

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

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:47
>> +const size_t minimumCallbackBufferSize = 128;
> 
> I wouldn't create a new constant here, but instead use the already existing constant "renderBufferSize"

Although they have the same value, they're conceptually different things.  Do you think overloading the meaning is appropriate?

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:74
>> +    ASSERT((minimumCallbackBuffersize <= m_callbackBuffersize)
> 
> I would suggest something like this:
> 
> bool isSizeGood = m_callbackBuffersize >= renderBufferSize && m_callbackBuffersize <= maximumCallbackBufferSize;
> ASSERT(isSizeGood);
> if (!isSizeGood)
>     return;

Yes, I'll do this in the next patch.

[Chris Rogers]

(In reply to comment #22)
> (From update of attachment 107710 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107710&action=review
> 
> >> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:47
> >> +const size_t minimumCallbackBufferSize = 128;
> > 
> > I wouldn't create a new constant here, but instead use the already existing constant "renderBufferSize"
> 
> Although they have the same value, they're conceptually different things.  Do you think overloading the meaning is appropriate?

The problem with having two different constants is that one could be changed, and the other value would no longer be consistent.  I think two values would be more bug prone.  If you really wanted two constants then you could define one in terms of the other (equal to the other).  But this really seems overkill.


> 
> >> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:74
> >> +    ASSERT((minimumCallbackBuffersize <= m_callbackBuffersize)
> > 
> > I would suggest something like this:
> > 
> > bool isSizeGood = m_callbackBuffersize >= renderBufferSize && m_callbackBuffersize <= maximumCallbackBufferSize;
> > ASSERT(isSizeGood);
> > if (!isSizeGood)
> >     return;
> 
> Yes, I'll do this in the next patch.

[Raymond Toy]

Created attachment 107888 [details]
Patch

[Raymond Toy]

Comment on attachment 107710 [details]
Patch

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

>>>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:47
>>>> +const size_t minimumCallbackBufferSize = 128;
>>> 
>>> I wouldn't create a new constant here, but instead use the already existing constant "renderBufferSize"
>> 
>> Although they have the same value, they're conceptually different things.  Do you think overloading the meaning is appropriate?
> 
> The problem with having two different constants is that one could be changed, and the other value would no longer be consistent.  I think two values would be more bug prone.  If you really wanted two constants then you could define one in terms of the other (equal to the other).  But this really seems overkill.

Fair enough.  I'll make the change.  (Actually, as long as  callbackSize is not 0, it is rounded up to a multiple of renderBufferSize anyway since we're testing m_callbackBufferSize, not callbackSize.  Maybe just test against 1 instead of renderBufferSize?)

>>>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:74
>>>> +    ASSERT((minimumCallbackBuffersize <= m_callbackBuffersize)
>>> 
>>> I would suggest something like this:
>>> 
>>> bool isSizeGood = m_callbackBuffersize >= renderBufferSize && m_callbackBuffersize <= maximumCallbackBufferSize;
>>> ASSERT(isSizeGood);
>>> if (!isSizeGood)
>>>     return;
>> 
>> Yes, I'll do this in the next patch.
> 
> 

Done.

[Chris Rogers]

Thanks Ray, looks fine to me.

[Kenneth Russell]

Comment on attachment 107888 [details]
Patch

Looks good to me too.

[Darin Fisher (:fishd, Google)]

Comment on attachment 107888 [details]
Patch

R=me too

[WebKit Review Bot]

Comment on attachment 107888 [details]
Patch

Clearing flags on attachment: 107888

Committed r95508: <http://trac.webkit.org/changeset/95508>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.