Bug 147117 – Exposed method to query device by UID

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Matthew Daiter]

Needed for getting devices from Safari by UID.

[Radar WebKit Bug Importer]

<rdar://problem/21904678>

[Matthew Daiter]

Created attachment 257116 [details]
Patch

[Eric Carlson]

Comment on attachment 257116 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:53
> +    RefPtr<RealtimeMediaSource> sourceWithUID(String&, RealtimeMediaSource::Type, RefPtr<MediaConstraints>&&);

Ref&& seems wrong unless you are passing ownership of the RefPtr (https://www.webkit.org/coding/RefPtr.html). Because this argument is optional (I think), it should be a MediaConstraints*.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:427
> +        AVCaptureDeviceType *device = [AVCaptureDevice deviceWithUniqueID:captureDevice.m_captureDeviceID];
> +        ASSERT(device);

captureDevice.m_captureDeviceID is set from device.uniqueID, so can you short circuit the lookup:

    if (captureDevice.m_captureDeviceID != deviceUID)
        continue;

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:436
> +            if (type == RealtimeMediaSource::Type::Audio && !captureDevice.m_audioSourceId.isEmpty()) {
> +                captureDevice.m_audioSource = AVAudioCaptureSource::create(device, captureDevice.m_audioSourceId, constraints);
> +                return captureDevice.m_audioSource;
> +            }
> +            if (type == RealtimeMediaSource::Type::Video && !captureDevice.m_videoSourceId.isEmpty()) {
> +                captureDevice.m_videoSource = AVVideoCaptureSource::create(device, captureDevice.m_videoSourceId, constraints);
> +                return captureDevice.m_videoSource;
> +            }

Don't you also need to consider constraints:

    String invalidConstraint;
    AVCaptureDeviceManager::singleton().verifyConstraintsForMediaType(type, constraints, invalidConstraint);
    if (!invalidConstraint.isEmpty())
        continue;

[Matthew Daiter]

Created attachment 257145 [details]
Patch

[Matthew Daiter]

Comment on attachment 257116 [details]
Patch

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

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:53
>> +    RefPtr<RealtimeMediaSource> sourceWithUID(String&, RealtimeMediaSource::Type, RefPtr<MediaConstraints>&&);
> 
> Ref&& seems wrong unless you are passing ownership of the RefPtr (https://www.webkit.org/coding/RefPtr.html). Because this argument is optional (I think), it should be a MediaConstraints*.

The RefPtr.html page said that the methods should be implemented with a RefPtr or Ref reference reference in the place of a PassRefPtr. However, the argument could be a raw pointer instead.
Fixed.

[Eric Carlson]

(In reply to comment #5)
> Comment on attachment 257116 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257116&action=review
> 
> >> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:53
> >> +    RefPtr<RealtimeMediaSource> sourceWithUID(String&, RealtimeMediaSource::Type, RefPtr<MediaConstraints>&&);
> > 
> > Ref&& seems wrong unless you are passing ownership of the RefPtr (https://www.webkit.org/coding/RefPtr.html). Because this argument is optional (I think), it should be a MediaConstraints*.
> 
> The RefPtr.html page said that the methods should be implemented with a
> RefPtr or Ref reference reference in the place of a PassRefPtr. However, the
> argument could be a raw pointer instead.
> Fixed.

Yes, but Ref&& should be used when passing ownership of an object:

    If a function does take ownership of an object, the argument should be 
    a Ref&& or a RefPtr&&. This includes many setter functions.

[Eric Carlson]

Comment on attachment 257145 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:429
> +        String invalidConstraints;
> +        AVCaptureDeviceManager::singleton().verifyConstraintsForMediaType(type, constraints, invalidConstraints);
> +        if (!invalidConstraints.isEmpty())
> +            continue;

This should be wrapped with a "if (constraints) { ... }"

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:434
> +        if (captureDevice.m_captureDeviceID != deviceUID)
> +            continue;

This is cheap, it should be done before the verifyConstraintsForMediaType() call

[Matthew Daiter]

Comment on attachment 257145 [details]
Patch

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

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:429
>> +            continue;
> 
> This should be wrapped with a "if (constraints) { ... }"

Fixed.

>> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:434
>> +            continue;
> 
> This is cheap, it should be done before the verifyConstraintsForMediaType() call

Fixed.

[Matthew Daiter]

Created attachment 257152 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 257152 [details]
Patch

Clearing flags on attachment: 257152

Committed r187140: <http://trac.webkit.org/changeset/187140>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.