Bug 170778 - Refactor enumerateDevices to allow separate CaptureDeviceManagers for audio and video.
Summary: Refactor enumerateDevices to allow separate CaptureDeviceManagers for audio a...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-12 10:44 PDT by Jeremy Jones
Modified: 2017-04-22 14:42 PDT (History)
4 users (show)

See Also:


Attachments
Preliminary Patch. (47.98 KB, patch)
2017-04-12 11:00 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (48.79 KB, patch)
2017-04-13 13:14 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (48.76 KB, patch)
2017-04-13 15:51 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (48.92 KB, patch)
2017-04-14 15:31 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-04-12 10:44:07 PDT
Refactor enumerateDevices to allow dseparate CaptureDeviceManagers for audio and video.
Comment 1 Jeremy Jones 2017-04-12 10:44:41 PDT
rdar://problem/31575382
Comment 2 Jeremy Jones 2017-04-12 11:00:33 PDT
Created attachment 306921 [details]
Preliminary Patch.
Comment 3 Eric Carlson 2017-04-12 11:40:32 PDT
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)
Comment 4 Jeremy Jones 2017-04-12 11:54:24 PDT
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.
Comment 5 Jeremy Jones 2017-04-13 13:14:24 PDT
Created attachment 307017 [details]
Patch
Comment 6 Jeremy Jones 2017-04-13 15:51:07 PDT
Created attachment 307040 [details]
Patch
Comment 7 Eric Carlson 2017-04-14 09:36:22 PDT
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.
Comment 8 Jeremy Jones 2017-04-14 15:25:22 PDT
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.
Comment 9 Jeremy Jones 2017-04-14 15:31:18 PDT
Created attachment 307155 [details]
Patch for landing.
Comment 10 WebKit Commit Bot 2017-04-17 10:00:55 PDT
Comment on attachment 307155 [details]
Patch for landing.

Clearing flags on attachment: 307155

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