Split platform-independent logic in AVCaptureDeviceManager out into a new class

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

To prepare for creating a MockCaptureDeviceManager to be able to test MediaDevices.getUserMedia, we need to create a platform-independent capture device manager which AVCaptureDeviceManager should extend and add platform logic to.

[Wenson Hsieh]

<rdar://problem/23593980>

[Wenson Hsieh]

Created attachment 265746 [details]
Patch

[Wenson Hsieh]

Created attachment 265747 [details]
Patch

[Eric Carlson]

Comment on attachment 265747 [details]
Patch

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

This is a great start, but it needs a bit more work.

> Source/WebCore/ChangeLog:14
> +        CaptureDeviceManager::captureDeviceList should be overridden by the MockCaptureDeviceManager

This will be done by each platform, so "MockCaptureDeviceManager" should be something like "<Platform>CaptureDeviceManager"

> Source/WebCore/ChangeLog:15
> +        to respectively create MockRealtimeMediaSources and return a list of fake capture devices

Typo: "MockRealtimeMediaSources" -> "a RealtimeMediaSource"

> Source/WebCore/Modules/mediastream/CaptureDevice.h:39
> +enum class CaptureDevicePosition {
> +    Unspecified,
> +    Front,
> +    Back
> +};

This should have "facing mode" in the name, and the values should be the same as the legal facingMode values: "left", "right", "user", "environment".

> Source/WebCore/Modules/mediastream/CaptureDevice.h:47
> +enum CaptureSessionPreset {
> +    CaptureSessionPresetNone = 0,
> +    CaptureSessionPresetLow = 1 << 0,
> +    CaptureSessionPreset352x288 = 1 << 1,
> +    CaptureSessionPreset640x480 = 1 << 2,
> +    CaptureSessionPreset1280x720 = 1 << 3
> +};

I don't think this will work as a bitfield because we can't assume anything about the sizes a video capture device will support. I think you will need to have something like a "supportsVideoSize" method that platforms implement.

> Source/WebCore/Modules/mediastream/CaptureDevice.h:57
> +    String m_captureDeviceID;

If this is the persistent ID, I think the name should include "persistent" to make that obvious.

> Source/WebCore/Modules/mediastream/CaptureDevice.h:62
> +    String m_audioSourceId;
> +    String m_videoSourceId;

I thinks the device manager code will be cleaner and easier to understand if each CaptureDevice only supports a single media type, which will make these fields unnecessary, and you won't need CaptureDeviceMediaTypeMuxed.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:149
> +            if (!captureSource)
> +                continue;

I know that you copied this, but it shouldn't "continue" because it won't consider the video source.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:198
> +    case RealtimeMediaSourceStates::Left:
> +    case RealtimeMediaSourceStates::Right:

These should be implemented.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:220
> +    size_t constraint = validConstraintNames().find(name);
> +    if (constraint == notFound)
> +        return true;

This should use RealtimeMediaSourceCenter::supportedConstraints() once bug 151394 lands.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:238
> +        // It would make sense to use [AVCaptureConnection videoMinFrameDuration] and
> +        // [AVCaptureConnection videoMaxFrameDuration], but they only work with a "live" AVCaptureConnection.

Oops!

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:240
> +        return rate > 0 && rate <= 60;

We can't assume anything about frame rates, make this a call into the platform device manager.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:277
> +    DEPRECATED_DEFINE_STATIC_LOCAL(Vector<AtomicString>, constraints, ());

This won't be necessary if you use RealtimeMediaSourceCenter::supportedConstraints.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.h:40
> +    enum ValidConstraints { Width = 0, Height, FrameRate, FacingMode, Gain };

This isn't used.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.h:66
> +    Vector<CaptureDevice> m_devices;

This should either be private and initialized by a one-time call into platform code, or don't include it and make captureDeviceList() pure virtual and always use it. In either case, we also need a method in the base class that platform code can call when the list of devices changes.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:259
> +    ASSERT(device);

I am not certain this should assert, but it should definitely return nullptr if the device is nil.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:268
> +        // FIXME: To preserve behavior, ensure that a newly initialized CaptureSession

ensure that a newly initialized CaptureSession ... ?

[Wenson Hsieh]

Comment on attachment 265747 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        CaptureDeviceManager::captureDeviceList should be overridden by the MockCaptureDeviceManager
> 
> This will be done by each platform, so "MockCaptureDeviceManager" should be something like "<Platform>CaptureDeviceManager"

Fixed.

>> Source/WebCore/ChangeLog:15
>> +        to respectively create MockRealtimeMediaSources and return a list of fake capture devices
> 
> Typo: "MockRealtimeMediaSources" -> "a RealtimeMediaSource"

Fixed.

>> Source/WebCore/Modules/mediastream/CaptureDevice.h:39
>> +};
> 
> This should have "facing mode" in the name, and the values should be the same as the legal facingMode values: "left", "right", "user", "environment".

Fixed.

>> Source/WebCore/Modules/mediastream/CaptureDevice.h:47
>> +};
> 
> I don't think this will work as a bitfield because we can't assume anything about the sizes a video capture device will support. I think you will need to have something like a "supportsVideoSize" method that platforms implement.

Sounds good -- added a supportsVideoSize method.

>> Source/WebCore/Modules/mediastream/CaptureDevice.h:57
>> +    String m_captureDeviceID;
> 
> If this is the persistent ID, I think the name should include "persistent" to make that obvious.

This is indeed the persistent ID -- I'll rename this to be m_persistentDeviceId.

>> Source/WebCore/Modules/mediastream/CaptureDevice.h:62
>> +    String m_videoSourceId;
> 
> I thinks the device manager code will be cleaner and easier to understand if each CaptureDevice only supports a single media type, which will make these fields unnecessary, and you won't need CaptureDeviceMediaTypeMuxed.

Sounds good. Changed to have a single m_sourceId. This also means m_mediaTypes should be m_mediaType instead and use an enum class instead of a bitfield.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:149
>> +                continue;
> 
> I know that you copied this, but it shouldn't "continue" because it won't consider the video source.

Fixed.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:198
>> +    case RealtimeMediaSourceStates::Right:
> 
> These should be implemented.

Fixed.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:220
>> +        return true;
> 
> This should use RealtimeMediaSourceCenter::supportedConstraints() once bug 151394 lands.

Got it. Changed to use supportedConstraints() instead of validConstraintNames().

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:238
>> +        // [AVCaptureConnection videoMaxFrameDuration], but they only work with a "live" AVCaptureConnection.
> 
> Oops!

We should address this issue in a separate patch.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:240
>> +        return rate > 0 && rate <= 60;
> 
> We can't assume anything about frame rates, make this a call into the platform device manager.

Sounds good. I'll add something like isSupportedFrameRate(float rate);

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:277
>> +    DEPRECATED_DEFINE_STATIC_LOCAL(Vector<AtomicString>, constraints, ());
> 
> This won't be necessary if you use RealtimeMediaSourceCenter::supportedConstraints.

Got it. Removed.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.h:40
>> +    enum ValidConstraints { Width = 0, Height, FrameRate, FacingMode, Gain };
> 
> This isn't used.

Removed.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.h:66
>> +    Vector<CaptureDevice> m_devices;
> 
> This should either be private and initialized by a one-time call into platform code, or don't include it and make captureDeviceList() pure virtual and always use it. In either case, we also need a method in the base class that platform code can call when the list of devices changes.

Good point. I'll remove m_devices from the platform-independent code and make captureDeviceList and refreshCaptureDeviceList pure virtual methods of CaptureDeviceManager.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:259
>> +    ASSERT(device);
> 
> I am not certain this should assert, but it should definitely return nullptr if the device is nil.

Sounds good. I'll change this.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:268
>> +        // FIXME: To preserve behavior, ensure that a newly initialized CaptureSession
> 
> ensure that a newly initialized CaptureSession ... ?

Whoops -- forgot to remove this comment! This comment was intended to reflect the fact that the mock capture device manager should override defaultCaptureSession() to return the same thing as AVCaptureDeviceManager::defaultCaptureSession() to ensure that the tested behaviors match what we ship. I decided to remove the comment and add it later when we implement the mock capture device manager.

[Wenson Hsieh]

Created attachment 266149 [details]
Patch

[Eric Carlson]

Comment on attachment 266149 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        which AVCaptureDeviceManager should extend and add platform logic to.

Nit: AVCaptureDeviceManager -> all platforms, and "add platform logic" -> "add platform-specific logic"

> Source/WebCore/ChangeLog:25
> +        component of a capture device, but not both at once. This means a webcam that supports

Nit: webcam -> capture device.

> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:102
> +    } sortBasedOffFitnessScore;

Nit: sortBasedOffFitnessScore -> sortBasedOnFitnessScore

> Source/WebCore/platform/mediastream/RealtimeMediaSourceSupportedConstraints.h:94
> +    String nameForConstraint(MediaConstraintType) const;

String -> AtomicString.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:46
> +    AVCaptureSessionInfo(AVCaptureSession *platformSession);

Nit: the variable name isn't necessary.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:47
> +    virtual bool supportsVideoSize(const String& videoSize) const override;

Ditto.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:60
> +    virtual RefPtr<RealtimeMediaSource> sourceWithUID(const String& deviceUID, RealtimeMediaSource::Type, MediaConstraints*) override;

Ditto.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:274
> +    if (type == RealtimeMediaSource::Video && session) {

Nit; if session is NULL, this should use defaultCaptureSession().

[Wenson Hsieh]

Comment on attachment 266149 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        which AVCaptureDeviceManager should extend and add platform logic to.
> 
> Nit: AVCaptureDeviceManager -> all platforms, and "add platform logic" -> "add platform-specific logic"

Fixed.

>> Source/WebCore/ChangeLog:25
>> +        component of a capture device, but not both at once. This means a webcam that supports
> 
> Nit: webcam -> capture device.

Fixed.

>> Source/WebCore/Modules/mediastream/CaptureDeviceManager.cpp:102
>> +    } sortBasedOffFitnessScore;
> 
> Nit: sortBasedOffFitnessScore -> sortBasedOnFitnessScore

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceSupportedConstraints.h:94
>> +    String nameForConstraint(MediaConstraintType) const;
> 
> String -> AtomicString.

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:46
>> +    AVCaptureSessionInfo(AVCaptureSession *platformSession);
> 
> Nit: the variable name isn't necessary.

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:47
>> +    virtual bool supportsVideoSize(const String& videoSize) const override;
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:60
>> +    virtual RefPtr<RealtimeMediaSource> sourceWithUID(const String& deviceUID, RealtimeMediaSource::Type, MediaConstraints*) override;
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:274
>> +    if (type == RealtimeMediaSource::Video && session) {
> 
> Nit; if session is NULL, this should use defaultCaptureSession().

Good catch!

[Wenson Hsieh]

Committed r192838: <http://trac.webkit.org/changeset/192838>