Bug 36475 – audio engine: add AudioFileReader files (Mac implementation)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

audio engine: add AudioFileReader files (Mac implementation)

[Chris Rogers]

Created attachment 51377 [details]
Patch

[Adam Barth]

Comment on attachment 51377 [details]
Patch

My understanding is that the audio engine will occur on a branch.  Please renominate for review if I'm mistaken.

[Chris Rogers]

(In reply to comment #2)
> (From update of attachment 51377 [details])
> My understanding is that the audio engine will occur on a branch.  Please renominate for review if I'm mistaken.

Although I'm working in a branch, the code ultimately needs to end up in trunk.  This particular piece of code is at a very low-level and I don't expect it will subject to any javascript API design changes.  It would be good to get the lower-level more stable parts of code reviewed as I'm iterating in the branch.

[Jeremy Orlow]

The last I heard about it (on webkit-reviewers) I think the plan was to put it all in a branch and optionally clean it up there.  You'd then in parallel work on the JS API.  Once that's designed, you'd then start implementing the JS API in trunk and pull in (hopefully already cleaned up) bits of the engine from the branch.

In other words, stuff would only come out of the branch when there was a frontend ready to hook it up to.  The goal of this was to avoid having "dead code" in the tree.

As far as I know, this original plan still stands.  Am I mistaken?

[Chris Rogers]

(In reply to comment #4)
> The last I heard about it (on webkit-reviewers) I think the plan was to put it all in a branch and optionally clean it up there.  You'd then in parallel work on the JS API.  Once that's designed, you'd then start implementing the JS API in trunk and pull in (hopefully already cleaned up) bits of the engine from the branch.
> 
> In other words, stuff would only come out of the branch when there was a frontend ready to hook it up to.  The goal of this was to avoid having "dead code" in the tree.
> 
> As far as I know, this original plan still stands.  Am I mistaken?

That's fine, this bug and the others can wait until I get the IDL files checked in as you suggest.  I just want to make sure the bugs themselves stay open so the already reviewed ones will be ready.

[Chris Rogers]

Created attachment 65750 [details]
Patch

[Chris Rogers]

This patch is ready to be reviewed and landed in trunk.

[Kenneth Russell]

Comment on attachment 65750 [details]
Patch

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

This basically looks fine to me but I have a couple of questions, plus one relatively tiny requested movement of one line of code.

> WebCore/platform/audio/mac/AudioFileReaderMac.cpp:37
> +#include <Carbon/Carbon.h>
Is it kosher to be adding dependencies on Carbon at this point? Does this code build in 64-bit mode?

> WebCore/platform/audio/mac/AudioFileReaderMac.cpp:44
> +    UInt32 bufferListSize = sizeof(AudioBufferList) - sizeof(AudioBuffer);
Why can't bufferListSize use a more standard type like size_t?

> WebCore/platform/audio/mac/AudioFileReaderMac.cpp:193
> +    AudioFloatArray bufR(numberOfFrames);
It would be nice if there were a way to avoid allocating these if we aren't mixing the input down to mono.

> WebCore/platform/audio/mac/AudioFileReaderMac.cpp:225
> +    float* destL = audioBus->channel(0)->data();
This variable declaration should be enclosed in the following if block.

[Chris Rogers]

Created attachment 67588 [details]
Patch

[Chris Rogers]

Ken, I believe I've addressed all of your comments.  It turned out that Carbon.h was the wrong header file and I didn't need to include it anyway.  CoreServices.h was the correct one and it's already pulled in from the CoreAudio headers.

[Kenneth Russell]

Comment on attachment 67588 [details]
Patch

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

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 67588 [details]
Patch

Clearing flags on attachment: 67588

Committed r67530: <http://trac.webkit.org/changeset/67530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.