Bug 170778 – Refactor enumerateDevices to allow separate CaptureDeviceManagers for audio and video.

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

[Jeremy Jones]

Refactor enumerateDevices to allow dseparate CaptureDeviceManagers for audio and video.

[Jeremy Jones]

rdar://problem/31575382

[Jeremy Jones]

Created attachment 306921 [details]
Preliminary Patch.

[Eric Carlson]

Comment on attachment 306921 [details]
Preliminary Patch.

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

> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:50
>      for (auto captureDevice : captureDevices()) {

Nit: auto&

> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:63
> +    for (auto captureDevice : captureDevices()) {

Ditto.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:131
> +        CaptureDevice existingCaptureDevice;

Nit: this should be declare just above where it is used to avoid initializing it if !hasMatchingType.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDevice.cpp:42
> +

Nit: extra blank line.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:80
> +    bool haveDeviceChanges = false;

Nit: you can declare this just above the loop where it is used.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:109
> +        for (auto& device : m_coreAudioCaptureDevices) {
> +            if (device->deviceID() == deviceID) {
> +                found = true;
> +                break;
> +            }
> +        }

This will be more succinct if you use std::any_of.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:139
> +

Nit: extra blank.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:89
>  

This should ASSERT(m_captureDeviceID)

[Jeremy Jones]

Comment on attachment 306921 [details]
Preliminary Patch.

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

>> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:50
>>      for (auto captureDevice : captureDevices()) {
> 
> Nit: auto&

done

>> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:63
>> +    for (auto captureDevice : captureDevices()) {
> 
> Ditto.

done

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:131
>> +        CaptureDevice existingCaptureDevice;
> 
> Nit: this should be declare just above where it is used to avoid initializing it if !hasMatchingType.

done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDevice.cpp:42
>> +
> 
> Nit: extra blank line.

Done

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:80
>> +    bool haveDeviceChanges = false;
> 
> Nit: you can declare this just above the loop where it is used.

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:109
>> +        }
> 
> This will be more succinct if you use std::any_of.

Done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:139
>> +
> 
> Nit: extra blank.

done.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:89
>>  
> 
> This should ASSERT(m_captureDeviceID)

Done.

[Jeremy Jones]

Created attachment 307017 [details]
Patch

[Jeremy Jones]

Created attachment 307040 [details]
Patch

[Eric Carlson]

Comment on attachment 307040 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Refactor enumerateDevices to allow dseparate CaptureDeviceManagers for audio and video.

Nit: dseparate -> separate

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:63
> +#if PLATFORM(MAC)
> +        RealtimeMediaSourceCenter::singleton().setAudioCaptureDeviceManager(CoreAudioCaptureDeviceManager::singleton());
> +#else
> +        RealtimeMediaSourceCenter::singleton().setAudioCaptureDeviceManager(AVCaptureDeviceManager::singleton());
> +#endif

Please add a FIXME with a bug number about changing this for iOS.

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:97
> +#if PLATFORM(MAC)
> +    m_audioCaptureDeviceManager = &CoreAudioCaptureDeviceManager::singleton();
> +#else
> +    m_audioCaptureDeviceManager = &AVCaptureDeviceManager::singleton();
> +#endif

Ditto.

[Jeremy Jones]

Comment on attachment 307040 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Refactor enumerateDevices to allow dseparate CaptureDeviceManagers for audio and video.
> 
> Nit: dseparate -> separate

done.

>> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:63
>> +#endif
> 
> Please add a FIXME with a bug number about changing this for iOS.

Done.

>> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:97
>> +#endif
> 
> Ditto.

Done.

[Jeremy Jones]

Created attachment 307155 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 307155 [details]
Patch for landing.

Clearing flags on attachment: 307155

Committed r215418: <http://trac.webkit.org/changeset/215418>