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
Created attachment 48353 [details] patch file for audio output classes
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.
I understand. These OS calls rarely fail, the fprintf statements were really there for debugging. I could just remove them...
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.
Created attachment 50639 [details] Patch
Comment on attachment 50639 [details] Patch I believe I've addressed Adam's comments.
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.
Created attachment 65748 [details] Patch
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
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?
Created attachment 67594 [details] Patch
> 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.
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".
Comment on attachment 67594 [details] Patch Clearing flags on attachment: 67594 Committed r68539: <http://trac.webkit.org/changeset/68539>
All reviewed patches have been landed. Closing bug.