Bug 34716 – audio engine: audio output classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Rogers]

AudioOutput is an abstract base-class for providing a rendered audio stream to the audio output hardware.
AudioOutputMac is an implementation for the mac port

[Chris Rogers]

Created attachment 48353 [details]
patch file for audio output classes

[Darin Adler]

Comment on attachment 48353 [details]
patch file for audio output classes

Not reviewing this all at this time, but a first thought I had is that we don't normally print errors to stderr, so that does not seem the right thing to do here.

[Chris Rogers]

I understand.  These OS calls rarely fail, the fprintf statements were really there for debugging.  I could just remove them...

[Adam Barth]

Comment on attachment 48353 [details]
patch file for audio output classes

Sorry that this patch has been up for review for so long.  I'm not that familiar with audio, so I'm going to give you a mostly syntactic / cultural review of your patch.  Hopefully you'll find it helpful nonetheless.

+#define CheckError(error, operation)                \

I don't think we should have this macro.  If we decide we should have it, then it should be defined somewhere more generic so that i can be used beyond the audio code.

+        fprintf(stderr, "can't find audio output unit\n");

This isn't the way we usually handle errors liek this in WebCore.  If this can't happen, I'd recommend an ASSERT.  If it can happen, we need to do something more than printing to stderror.

+     AudioOutputMac* This = static_cast<AudioOutputMac*>(inRefCon);

Please use a different variable name than |This|.  The capitalization is a bit too subtle.

+ static OSStatus inputProc(void* inRefCon,

WebCore doesn't have an 80 character line limit.  Although I think it's prettier this way, we usually put all this code on the same line.

+ AudioUnitRenderActionFlags* ioActionFlags

We also omit formal parameter names when the names don't provide any additional information.

[Chris Rogers]

Created attachment 50639 [details]
Patch

[Chris Rogers]

Comment on attachment 50639 [details]
Patch

I believe I've addressed Adam's comments.

[Adam Barth]

Comment on attachment 50639 [details]
Patch

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

[Chris Rogers]

Created attachment 65748 [details]
Patch

[Chris Rogers]

Here are some links to the relevant API documentation:

AudioHardware:
http://developer.apple.com/mac/library/documentation/MusicAudio/Reference/CACoreAudioReference/AudioHardware/CompositePage.html

Component Manager:
http://developer.apple.com/mac/library/documentation/Carbon/Reference/Component_Manager/Reference/reference.html

AudioUnits:
http://developer.apple.com/iphone/library/documentation/AudioUnit/Reference/AUComponentServicesReference/Reference/reference.html

[Kenneth Russell]

Comment on attachment 65748 [details]
Patch

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

This generally looks fine overall, but I have a couple of questions before adjusting the r? status.

> WebCore/platform/audio/AudioDestination.h:34
> +#include <wtf/RefCounted.h>
Why are you including RefCounted here if not using the type in this header?

> WebCore/platform/audio/AudioDestination.h:45
> +    static PassOwnPtr<AudioDestination> create(AudioSourceProvider&, double sampleRate);
Should AudioSourceProvider be made RefCounted and this use PassRefPtr<AudioSourceProvider>?

> WebCore/platform/audio/AudioDestination.h:47
> +    virtual ~AudioDestination() { };
No semicolon.

> WebCore/platform/audio/mac/AudioDestinationMac.cpp:56
> +    if (!result) {
Prefer early return; e.g. here, "if (result) return 0.0;".

> WebCore/platform/audio/mac/AudioDestinationMac.cpp:148
> +    usleep(4000); // Allow device to actually stop.
This looks like a hack. Is it really necessary?

[Chris Rogers]

Created attachment 67594 [details]
Patch

[Chris Rogers]

> Why are you including RefCounted here if not using the type in this header?

Turns out this wasn't necessary - I removed this line.


>Should AudioSourceProvider be made RefCounted and this use PassRefPtr<AudioSourceProvider>?

The ownership is in the other direction.  In actual usage, the AudioSourceProvider subclass  creates and owns the AudioDestination.  Also,  AudioSourceProvider is an abstract base class and it adds complications to inherit from RefCounted, since other subclasses define their own versions of ref() / deref()...


>No semicolon.
fixed

> Prefer early return; e.g. here, "if (result) return 0.0;".
fixed

> This looks like a hack. Is it really necessary?
Yes, you're right.  It turns out the usleep() is not necessary since AudioOutputUnitStop() waits for the audio thread to complete before returning.  I've removed this.

[Kenneth Russell]

Comment on attachment 67594 [details]
Patch

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

Looks good.

> WebCore/platform/audio/mac/AudioDestinationMac.cpp:40
> +const int kBufferSize = 128;

FYI, naming convention for these constants is "BufferSize".

[WebKit Commit Bot]

Comment on attachment 67594 [details]
Patch

Clearing flags on attachment: 67594

Committed r68539: <http://trac.webkit.org/changeset/68539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.