Bug 170112 – Add CoreAudioCaptureSource.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jeremy Jones]

Add CoreAudioCaptureSource.

[Jeremy Jones]

Created attachment 305452 [details]
Patch

[Jon Lee]

Comment on attachment 305452 [details]
Patch

Should this be rebased?

[Eric Carlson]

Comment on attachment 305452 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:64
> +    , m_captureDeviceID(0)

This can use a FIXME.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:115
> +    // FIXME: Get the preferred rate dynamically - kAUVoiceIOProperty_PreferredHWBlockSizeInSeconds / [[AVAudioSession sharedInstance] preferredIOBufferDuration]

Nit: preferred *duration*

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:148
> +#if 0
> +    microphoneProcFormat.mSampleRate = preferredSampleRate();
> +    microphoneProcFormat.mChannelsPerFrame = 1;
> +    microphoneProcFormat.mFormatFlags = kAudioFormatFlagsNativeFloatPacked | kAudioFormatFlagIsNonInterleaved;
> +
> +    err = AudioUnitSetProperty(m_ioUnit, kAudioUnitProperty_StreamFormat, kAudioUnitScope_Output, inputBus, &microphoneProcFormat, sizeof(microphoneProcFormat));
> +    if (err) {
> +        LOG(Media, "CoreAudioCaptureSource::configureMicrophoneProc(%p) unable to set output stream format, error %d (%.4s)", this, (int)err, (char*)&err);
> +        return err;
> +    }
> +#endif

Is this needed?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:186
> +#if 0
> +    speakerProcFormat.mSampleRate = preferredSampleRate();
> +    speakerProcFormat.mChannelsPerFrame = 1;
> +    speakerProcFormat.mFormatFlags = kAudioFormatFlagsNativeFloatPacked | kAudioFormatFlagIsNonInterleaved;
> +
> +    err = AudioUnitSetProperty(m_ioUnit, kAudioUnitProperty_StreamFormat, kAudioUnitScope_Input, outputBus, &speakerProcFormat, sizeof(speakerProcFormat));
> +    if (err) {
> +        LOG(Media, "CoreAudioCaptureSource::configureSpeakerProc(%p) unable to set input stream format, error %d (%.4s)", this, (int)err, (char*)&err);
> +        return err;
> +    }
> +#endif

Ditto.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:336
> +#if 0
> +OSStatus CoreAudioCaptureSource::defaultOutputDevice(uint32_t* deviceID)
> +{
> +    AudioObjectPropertyAddress propertyInfo = { kAudioHardwarePropertyDefaultOutputDevice, kAudioObjectPropertyScopeGlobal, kAudioObjectPropertyElementMaster };
> +
> +    *deviceID = 0;
> +    UInt32 propertySize = sizeof(AudioDeviceID);
> +    auto err = AudioObjectGetPropertyData(kAudioObjectSystemObject, &propertyInfo, outputBus, nullptr, &propertySize, deviceID);
> +    if (err)
> +        LOG(Media, "CoreAudioCaptureSource::defaultOutputDevice(%p) unable to get default output device ID, error %d (%.4s)", this, (int)err, (char*)&err);
> +
> +    return err;
> +}
> +#endif

Ditto.

[Jeremy Jones]

Created attachment 306285 [details]
Patch

[Jeremy Jones]

rdar://problem/30293338

[Jeremy Jones]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 305452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305452&action=review
> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:64
> > +    , m_captureDeviceID(0)
> 
> This can use a FIXME.

Added.

> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:115
> > +    // FIXME: Get the preferred rate dynamically - kAUVoiceIOProperty_PreferredHWBlockSizeInSeconds / [[AVAudioSession sharedInstance] preferredIOBufferDuration]
> 
> Nit: preferred *duration*

Done.

> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:148
> > +#if 0
> > +    microphoneProcFormat.mSampleRate = preferredSampleRate();
> > +    microphoneProcFormat.mChannelsPerFrame = 1;
> > +    microphoneProcFormat.mFormatFlags = kAudioFormatFlagsNativeFloatPacked | kAudioFormatFlagIsNonInterleaved;
> > +
> > +    err = AudioUnitSetProperty(m_ioUnit, kAudioUnitProperty_StreamFormat, kAudioUnitScope_Output, inputBus, &microphoneProcFormat, sizeof(microphoneProcFormat));
> > +    if (err) {
> > +        LOG(Media, "CoreAudioCaptureSource::configureMicrophoneProc(%p) unable to set output stream format, error %d (%.4s)", this, (int)err, (char*)&err);
> > +        return err;
> > +    }
> > +#endif
> 
> Is this needed?

Removed

> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:186
> > +#if 0
> > +    speakerProcFormat.mSampleRate = preferredSampleRate();
> > +    speakerProcFormat.mChannelsPerFrame = 1;
> > +    speakerProcFormat.mFormatFlags = kAudioFormatFlagsNativeFloatPacked | kAudioFormatFlagIsNonInterleaved;
> > +
> > +    err = AudioUnitSetProperty(m_ioUnit, kAudioUnitProperty_StreamFormat, kAudioUnitScope_Input, outputBus, &speakerProcFormat, sizeof(speakerProcFormat));
> > +    if (err) {
> > +        LOG(Media, "CoreAudioCaptureSource::configureSpeakerProc(%p) unable to set input stream format, error %d (%.4s)", this, (int)err, (char*)&err);
> > +        return err;
> > +    }
> > +#endif
> 
> Ditto.

Removed

> 
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:336
> > +#if 0
> > +OSStatus CoreAudioCaptureSource::defaultOutputDevice(uint32_t* deviceID)
> > +{
> > +    AudioObjectPropertyAddress propertyInfo = { kAudioHardwarePropertyDefaultOutputDevice, kAudioObjectPropertyScopeGlobal, kAudioObjectPropertyElementMaster };
> > +
> > +    *deviceID = 0;
> > +    UInt32 propertySize = sizeof(AudioDeviceID);
> > +    auto err = AudioObjectGetPropertyData(kAudioObjectSystemObject, &propertyInfo, outputBus, nullptr, &propertySize, deviceID);
> > +    if (err)
> > +        LOG(Media, "CoreAudioCaptureSource::defaultOutputDevice(%p) unable to get default output device ID, error %d (%.4s)", this, (int)err, (char*)&err);
> > +
> > +    return err;
> > +}
> > +#endif
> 
> Ditto.

Removed

[Jeremy Jones]

Created attachment 306286 [details]
Patch

[Eric Carlson]

Comment on attachment 306286 [details]
Patch

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

This looks fine to me once it builds everywhere, but a WK2 reviewer should give it the official r+.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:37
> +#include <CoreAudio/AudioHardware.h>

I believe this is macOS-only.

[youenn fablet]

Comment on attachment 306286 [details]
Patch

Some nits below.

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

> Source/WebCore/page/Settings.h:315
> +    WEBCORE_EXPORT static void setUseAVFoundationAudioCapture(bool);

It is strange that page Settings has both static and non static member fields.
I guess it is fine here but should we try to stick with one approach here?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:64
> +            source = nullptr;

return early?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:122
> +    m_ioUnitStarted  = false;

Why do we need to do all that clean-up there?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:193
> +    m_microphoneDataCallbacks.add(m_nextMicrophoneDataCallbackID, callback);

last two lines as a one liner?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:221
> +void CoreAudioCaptureSource::checkTimestamps(const AudioTimeStamp *inTimeStamp, uint64_t sampleTime, double hostTime)

Can inTimeStamp be a ref?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:227
> +OSStatus CoreAudioCaptureSource::provideSpeakerData(AudioUnitRenderActionFlags* /*ioActionFlags*/, const AudioTimeStamp* inTimeStamp, UInt32 /*inBusNumber*/, UInt32 inNumberFrames, AudioBufferList* ioData)

Can take refs here?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:277
> +    CoreAudioCaptureSource* dataSource = static_cast<CoreAudioCaptureSource*>(inRefCon);

auto*?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:65
> +    uint64_t addMicrophoneDataConsumer(MicrophoneDataCallback);

Should it take a MicrophoneDataCallback&& instead?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:93
> +    OSStatus processMicrophoneSamples(AudioUnitRenderActionFlags*, const AudioTimeStamp*, UInt32, UInt32, AudioBufferList*);

microphoneCallback prototype is probably fixed but we might want to have processMicrophoneSamples take references if we can control it.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:96
> +    OSStatus provideSpeakerData(AudioUnitRenderActionFlags*, const AudioTimeStamp*, UInt32, UInt32, AudioBufferList*);

Ditto.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:105
> +    Vector<RefPtr<AudioSampleDataSource>> m_activeSources;

Can it be a vector of Ref?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:108
> +    Vector<std::pair<QueueAction, RefPtr<AudioSampleDataSource>>> m_pendingSources;

Ditto?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:137
> +

Two lines

> Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:131
> +    RefPtr<RealtimeMediaSource> source = RealtimeMediaSourceCenter::singleton().audioFactory()->createMediaSourceForCaptureDeviceWithConstraints(device, constraints.ptr(), invalidConstraints);

auto?

[Jeremy Jones]

Comment on attachment 306286 [details]
Patch

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

>> Source/WebCore/page/Settings.h:315
>> +    WEBCORE_EXPORT static void setUseAVFoundationAudioCapture(bool);
> 
> It is strange that page Settings has both static and non static member fields.
> I guess it is fine here but should we try to stick with one approach here?

This needs to be static because it affects a singleton. There can't be a different value per page.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:37
>> +#include <CoreAudio/AudioHardware.h>
> 
> I believe this is macOS-only.

I'll see if I can remove it. It doesn't look necessary.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:64
>> +            source = nullptr;
> 
> return early?

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:122
>> +    m_ioUnitStarted  = false;
> 
> Why do we need to do all that clean-up there?

Some things need to happen under the lock. I'll get rid of the unnecessary thing here.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:193
>> +    m_microphoneDataCallbacks.add(m_nextMicrophoneDataCallbackID, callback);
> 
> last two lines as a one liner?

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:221
>> +void CoreAudioCaptureSource::checkTimestamps(const AudioTimeStamp *inTimeStamp, uint64_t sampleTime, double hostTime)
> 
> Can inTimeStamp be a ref?

Done. This will affect a few other places too.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:227
>> +OSStatus CoreAudioCaptureSource::provideSpeakerData(AudioUnitRenderActionFlags* /*ioActionFlags*/, const AudioTimeStamp* inTimeStamp, UInt32 /*inBusNumber*/, UInt32 inNumberFrames, AudioBufferList* ioData)
> 
> Can take refs here?

Sure. The C callback uses pointers, but we can use refs here.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:277
>> +    CoreAudioCaptureSource* dataSource = static_cast<CoreAudioCaptureSource*>(inRefCon);
> 
> auto*?

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:65
>> +    uint64_t addMicrophoneDataConsumer(MicrophoneDataCallback);
> 
> Should it take a MicrophoneDataCallback&& instead?

Good idea.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:93
>> +    OSStatus processMicrophoneSamples(AudioUnitRenderActionFlags*, const AudioTimeStamp*, UInt32, UInt32, AudioBufferList*);
> 
> microphoneCallback prototype is probably fixed but we might want to have processMicrophoneSamples take references if we can control it.

Agreed. Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:96
>> +    OSStatus provideSpeakerData(AudioUnitRenderActionFlags*, const AudioTimeStamp*, UInt32, UInt32, AudioBufferList*);
> 
> Ditto.

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:105
>> +    Vector<RefPtr<AudioSampleDataSource>> m_activeSources;
> 
> Can it be a vector of Ref?

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:108
>> +    Vector<std::pair<QueueAction, RefPtr<AudioSampleDataSource>>> m_pendingSources;
> 
> Ditto?

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.h:137
>> +
> 
> Two lines

Removed extra white space.

>> Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:131
>> +    RefPtr<RealtimeMediaSource> source = RealtimeMediaSourceCenter::singleton().audioFactory()->createMediaSourceForCaptureDeviceWithConstraints(device, constraints.ptr(), invalidConstraints);
> 
> auto?

Done.

[youenn fablet]

Comment on attachment 306286 [details]
Patch

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

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:29
> +#include <WebCore/AVCaptureDeviceManager.h>

This is a platform specific header, gtk bot does not seem to like that.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:335
> +    WebCore::AVCaptureDeviceManager::setUseAVFoundationAudioCapture(useAVFoundationAudioCapture);

This might be COCOA specific stuff also I guess.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3243
> +    settings.setUseAVFoundationAudioCapture(store.getBoolValueForKey(WebPreferencesKey::useAVFoundationAudioCaptureKey()));

Ditto.

[Jeremy Jones]

Created attachment 306364 [details]
Patch

[Jeremy Jones]

Created attachment 306367 [details]
Patch

[Jeremy Jones]

Created attachment 306407 [details]
Patch

[Eric Carlson]

Comment on attachment 306407 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:99
> +    if (active != enabled) {

Nit: an return early would be more idiomatic of WebKit style.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:45
> +
> +

Nit: extra blank line.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:165
> +    // Set the format of the data we supply to the output stream.
> +

Nit: this isn't necessary.

[Jeremy Jones]

Comment on attachment 306407 [details]
Patch

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

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:99
>> +    if (active != enabled) {
> 
> Nit: an return early would be more idiomatic of WebKit style.

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:45
>> +
> 
> Nit: extra blank line.

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:165
>> +
> 
> Nit: this isn't necessary.

Done.

[Jeremy Jones]

Created attachment 306736 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 306736 [details]
Patch for landing.

Clearing flags on attachment: 306736

Committed r215201: <http://trac.webkit.org/changeset/215201>