Bug 170112 - Add CoreAudioCaptureSource.
Summary: Add CoreAudioCaptureSource.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-26 23:52 PDT by Jeremy Jones
Modified: 2017-11-02 11:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (51.76 KB, patch)
2017-03-27 00:31 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (54.63 KB, patch)
2017-04-05 09:07 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (52.89 KB, patch)
2017-04-05 09:13 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (53.34 KB, patch)
2017-04-05 21:16 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (53.46 KB, patch)
2017-04-05 21:46 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (53.43 KB, patch)
2017-04-06 12:03 PDT, Jeremy Jones
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing. (53.34 KB, patch)
2017-04-10 13:40 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2017-03-26 23:52:53 PDT
Add CoreAudioCaptureSource.
Comment 1 Jeremy Jones 2017-03-27 00:31:44 PDT
Created attachment 305452 [details]
Patch
Comment 2 Jon Lee 2017-03-31 16:17:47 PDT
Comment on attachment 305452 [details]
Patch

Should this be rebased?
Comment 3 Eric Carlson 2017-04-01 08:04:24 PDT
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.
Comment 4 Jeremy Jones 2017-04-05 09:07:22 PDT
Created attachment 306285 [details]
Patch
Comment 5 Jeremy Jones 2017-04-05 09:08:18 PDT
rdar://problem/30293338
Comment 6 Jeremy Jones 2017-04-05 09:12:02 PDT
(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
Comment 7 Jeremy Jones 2017-04-05 09:13:12 PDT
Created attachment 306286 [details]
Patch
Comment 8 Eric Carlson 2017-04-05 09:29:51 PDT
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.
Comment 9 youenn fablet 2017-04-05 10:07:51 PDT
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?
Comment 10 Jeremy Jones 2017-04-05 14:17:00 PDT
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.
Comment 11 youenn fablet 2017-04-05 16:00:45 PDT
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.
Comment 12 Jeremy Jones 2017-04-05 21:16:19 PDT
Created attachment 306364 [details]
Patch
Comment 13 Jeremy Jones 2017-04-05 21:46:18 PDT
Created attachment 306367 [details]
Patch
Comment 14 Jeremy Jones 2017-04-06 12:03:36 PDT
Created attachment 306407 [details]
Patch
Comment 15 Eric Carlson 2017-04-08 12:04:44 PDT
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.
Comment 16 Jeremy Jones 2017-04-10 11:03:24 PDT
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.
Comment 17 Jeremy Jones 2017-04-10 13:40:13 PDT
Created attachment 306736 [details]
Patch for landing.
Comment 18 WebKit Commit Bot 2017-04-10 14:24:17 PDT
Comment on attachment 306736 [details]
Patch for landing.

Clearing flags on attachment: 306736

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