Bug 67952 - Ask for audio hardware buffer size instead of using hardwired constants.
Summary: Ask for audio hardware buffer size instead of using hardwired constants.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 67951 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-12 14:24 PDT by Raymond Toy
Modified: 2011-09-19 19:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.48 KB, patch)
2011-09-12 14:30 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2011-09-12 15:50 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2011-09-13 11:38 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2011-09-13 13:02 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.78 KB, patch)
2011-09-14 15:07 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2011-09-16 13:01 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.89 KB, patch)
2011-09-19 10:58 PDT, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond Toy 2011-09-12 14:24:29 PDT
Ask for audio hardware buffer size instead of using hardwired constants.
Comment 1 Raymond Toy 2011-09-12 14:30:42 PDT
Created attachment 107084 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-12 14:32:48 PDT
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.
Comment 3 Chris Rogers 2011-09-12 15:02:10 PDT
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...
Comment 4 Raymond Toy 2011-09-12 15:50:34 PDT
Created attachment 107096 [details]
Patch
Comment 5 Chris Rogers 2011-09-12 16:19:16 PDT
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"
Comment 6 Raymond Toy 2011-09-13 11:38:44 PDT
Created attachment 107201 [details]
Patch
Comment 7 Chris Rogers 2011-09-13 12:06:04 PDT
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.
Comment 8 Raymond Toy 2011-09-13 13:02:50 PDT
Created attachment 107216 [details]
Patch
Comment 9 Chris Rogers 2011-09-13 13:36:59 PDT
Thanks Ray, this looks fine to me.  I'm adding Ken and Darin (for WebKit API) as reviewers.
Comment 10 Darin Fisher (:fishd, Google) 2011-09-13 20:48:29 PDT
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.
Comment 11 Raymond Toy 2011-09-13 22:03:57 PDT
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.
Comment 12 Raymond Toy 2011-09-14 15:03:40 PDT
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.
Comment 13 Raymond Toy 2011-09-14 15:07:03 PDT
Created attachment 107400 [details]
Patch
Comment 14 Raymond Toy 2011-09-14 15:37:04 PDT
Forgot to mention that, as requested, I've added the local variable callbackSize.
Comment 15 Kenneth Russell 2011-09-14 16:49:31 PDT
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?
Comment 16 Julien Chaffraix 2011-09-14 17:35:45 PDT
*** Bug 67951 has been marked as a duplicate of this bug. ***
Comment 17 Raymond Toy 2011-09-16 10:02:12 PDT
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.
Comment 18 Chris Rogers 2011-09-16 10:38:51 PDT
I would just go ahead and add the complete bounds checking to keep the callback size sane.  If audioHardwareBufferSize() returns 0, for example...
Comment 19 Raymond Toy 2011-09-16 13:01:41 PDT
Created attachment 107710 [details]
Patch
Comment 20 Raymond Toy 2011-09-16 13:03:53 PDT
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.
Comment 21 Chris Rogers 2011-09-16 13:41:00 PDT
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;
Comment 22 Raymond Toy 2011-09-16 15:15:41 PDT
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.
Comment 23 Chris Rogers 2011-09-16 15:22:22 PDT
(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.
Comment 24 Raymond Toy 2011-09-19 10:58:17 PDT
Created attachment 107888 [details]
Patch
Comment 25 Raymond Toy 2011-09-19 10:59:20 PDT
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.
Comment 26 Chris Rogers 2011-09-19 13:39:33 PDT
Thanks Ray, looks fine to me.
Comment 27 Kenneth Russell 2011-09-19 13:51:52 PDT
Comment on attachment 107888 [details]
Patch

Looks good to me too.
Comment 28 Darin Fisher (:fishd, Google) 2011-09-19 14:46:36 PDT
Comment on attachment 107888 [details]
Patch

R=me too
Comment 29 WebKit Review Bot 2011-09-19 19:06:28 PDT
Comment on attachment 107888 [details]
Patch

Clearing flags on attachment: 107888

Committed r95508: <http://trac.webkit.org/changeset/95508>
Comment 30 WebKit Review Bot 2011-09-19 19:06:34 PDT
All reviewed patches have been landed.  Closing bug.