Bug 45003 - Add AudioBuffer files
Summary: Add AudioBuffer files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-31 16:26 PDT by Chris Rogers
Modified: 2010-09-09 18:12 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.05 KB, patch)
2010-08-31 16:27 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2010-09-08 16:53 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-08-31 16:26:08 PDT
Add AudioBuffer files
Comment 1 Chris Rogers 2010-08-31 16:27:27 PDT
Created attachment 66136 [details]
Patch
Comment 2 Chris Rogers 2010-08-31 16:28:35 PDT
Implements AudioBuffer as described in the web audio specification:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioBuffer-section
Comment 3 Kenneth Russell 2010-09-07 17:55:36 PDT
Comment on attachment 66136 [details]
Patch

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

Looks basically fine overall but I'd like to avoid memsets and memcpys strewn throughout the code, so suggesting a couple of refactorings. Also two tiny grammatical fixes.

> WebCore/webaudio/AudioBuffer.cpp:73
> +    // Copy audio data from the bus to the Float32Array's we manage.
Float32Array's -> Float32Arrays

> WebCore/webaudio/AudioBuffer.cpp:78
> +        memcpy(channelDataArray->data(), bus->channel(i)->data(), sizeof(float) * m_length);
Please go ahead and add the appropriate setRange method taking const T* as the source data to TypedArrayBase.h, with the paired setRangeImpl in ArrayBufferView.cpp. See the existing TypedArrayBase::set and ArrayBufferView::setImpl.

> WebCore/webaudio/AudioBuffer.cpp:100
> +            memset(getChannelData(i)->data(), 0, sizeof(float) * length());
Please go ahead and add the appropriate zeroRange call to TypedArrayBase and implementation in ArrayBufferView.

> WebCore/webaudio/AudioBuffer.h:46
> +    // Returns 0 if data is not valid audio file.
not valid -> not a valid
Comment 4 Chris Rogers 2010-09-08 16:53:50 PDT
Created attachment 66963 [details]
Patch
Comment 5 Chris Rogers 2010-09-08 16:55:30 PDT
Hi Ken, I've addressed your comments including adding setRange() and zeroRange() to TypedArrayBase which I've added in a separate patch:

https://bugs.webkit.org/show_bug.cgi?id=45419
Comment 6 Kenneth Russell 2010-09-09 14:18:28 PDT
Comment on attachment 66963 [details]
Patch

Thanks for making these changes; this looks good.
Comment 7 WebKit Commit Bot 2010-09-09 16:59:12 PDT
Comment on attachment 66963 [details]
Patch

Clearing flags on attachment: 66963

Committed r67128: <http://trac.webkit.org/changeset/67128>
Comment 8 WebKit Commit Bot 2010-09-09 16:59:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2010-09-09 18:12:27 PDT
http://trac.webkit.org/changeset/67128 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67128
http://trac.webkit.org/changeset/67129
http://trac.webkit.org/changeset/67126
http://trac.webkit.org/changeset/67127