Bug 171442 – Add notifications from CaptureDeviceManager (and subclasses) when device lists change

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

[Jer Noble]

Add notifications from CaptureDeviceManager (and subclasses) when device lists change

[Jer Noble]

Created attachment 308568 [details]
Patch

[Jeremy Jones]

Comment on attachment 308568 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        of devices changes.

Added support for enumerating AVAudioSession capture devices.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:85
> +    return m_devices.value();

If there are no observers, this list will get stale.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:116
> +    }

newAudioDevices.append(WTFMove(audioDevice));

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:135
> +    }]);

Maybe we should always listen for device changes so that captureDevices() returns the correct list even if there are no observers.

[Jer Noble]

Comment on attachment 308568 [details]
Patch

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

>> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:116
>> +    }
> 
> newAudioDevices.append(WTFMove(audioDevice));

Fixed.

>> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:135
>> +    }]);
> 
> Maybe we should always listen for device changes so that captureDevices() returns the correct list even if there are no observers.

Done.

[Jer Noble]

Created attachment 308583 [details]
Patch

[youenn fablet]

Sounds good, just having a question related to the observer model and some nits.

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

> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:138
> +CaptureDeviceManager::ObserverToken CaptureDeviceManager::addCaptureDeviceChangedObserver(CaptureDeviceChangedCallback observer)

Should pass a CaptureDeviceChangedCallback&&.

> Source/WebCore/platform/mediastream/CaptureDeviceManager.h:40
> +    virtual void removeCaptureDeviceChangedObserver(ObserverToken);

What are the pros and cons of this approach compared to a CaptureDeviceManager::Observer class in this particular context?
For instance, a CaptureDeviceManager::Observer destructor could ensure that it gets removed from m_observers. Is it useful in that context?
What about readability/searchability?

> Source/WebCore/platform/mediastream/CaptureDeviceManager.h:42
>      virtual Vector<CaptureDevice>& captureDevices() = 0;

Unrelated, but can we change the signature to const Vector<CaptureDevice>& captureDevices() const = 0;

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.h:39
> +    static AVAudioSessionCaptureDevice create(AVAudioSessionPortDescription*);

Can the port be null? Should it be a ref?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.h:45
> +    RetainPtr<AVAudioSessionPortDescription> m_portDescription;

Wonder whether we could have a UniqueRef equivalent here?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.mm:42
> +AVAudioSessionCaptureDevice::AVAudioSessionCaptureDevice(AVAudioSessionPortDescription* portDescription, const String& persistentID, const String& label)

Nit: use String&& for persistentID and label

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.h:47
> +    Vector<CaptureDevice>& captureDevices() final;

Move to private?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:127
> +    auto token = CaptureDeviceManager::addCaptureDeviceChangedObserver(observer);

Should move observer here.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:134
> +    m_devices = Vector<CaptureDevice>();

Can we use reserveCapacity/uncheckedAppend?
Ditto for AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:139
> +        m_devices.append(captureDevice);

Worth moving captureDevice.

[Jer Noble]

(In reply to youenn fablet from comment #5)
> Sounds good, just having a question related to the observer model and some
> nits.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308568&action=review
> 
> > Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:138
> > +CaptureDeviceManager::ObserverToken CaptureDeviceManager::addCaptureDeviceChangedObserver(CaptureDeviceChangedCallback observer)
> 
> Should pass a CaptureDeviceChangedCallback&&.
> 
> > Source/WebCore/platform/mediastream/CaptureDeviceManager.h:40
> > +    virtual void removeCaptureDeviceChangedObserver(ObserverToken);
> 
> What are the pros and cons of this approach compared to a
> CaptureDeviceManager::Observer class in this particular context?

Doing an ::Observer approach requires clients to include this header in their own header. Doing a Token approach only requires clients to include this header in their implementation file. This, in the long term, will reduce header bloat.

> For instance, a CaptureDeviceManager::Observer destructor could ensure that
> it gets removed from m_observers. Is it useful in that context?

You'd just do the same thing (a destructor-time unregister) for the Token approach.

> What about readability/searchability?

Build-time optimization is more important, IMHO.

> > Source/WebCore/platform/mediastream/CaptureDeviceManager.h:42
> >      virtual Vector<CaptureDevice>& captureDevices() = 0;
> 
> Unrelated, but can we change the signature to const Vector<CaptureDevice>&
> captureDevices() const = 0;

We could, but it would mean making the underlying devices ivar mutable, since it's lazily initialized.  Also, it wouldn't make sense to return a non-const reference from a const getter.

> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.h:39
> > +    static AVAudioSessionCaptureDevice create(AVAudioSessionPortDescription*);
> 
> Can the port be null? Should it be a ref?

You can't really have a reference to an Obj-C object.

> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.h:45
> > +    RetainPtr<AVAudioSessionPortDescription> m_portDescription;
> 
> Wonder whether we could have a UniqueRef equivalent here?

You can't have a UniqueRef to a Obj-C object.

> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.mm:42
> > +AVAudioSessionCaptureDevice::AVAudioSessionCaptureDevice(AVAudioSessionPortDescription* portDescription, const String& persistentID, const String& label)
> 
> Nit: use String&& for persistentID and label

No, we don't need to pass string by r-value ref.

> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.h:47
> > +    Vector<CaptureDevice>& captureDevices() final;
> 
> Move to private?

Sure.

> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:127
> > +    auto token = CaptureDeviceManager::addCaptureDeviceChangedObserver(observer);
> 
> Should move observer here.

Sure.

> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:134
> > +    m_devices = Vector<CaptureDevice>();
> 
> Can we use reserveCapacity/uncheckedAppend?
> Ditto for AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices

Sure.

> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:139
> > +        m_devices.append(captureDevice);
> 
> Worth moving captureDevice.

Ok.

[youenn fablet]

> > What are the pros and cons of this approach compared to a
> > CaptureDeviceManager::Observer class in this particular context?
> 
> Doing an ::Observer approach requires clients to include this header in
> their own header. Doing a Token approach only requires clients to include
> this header in their implementation file. This, in the long term, will
> reduce header bloat.

You could encapsulate the Observer in a std::function in the cpp file so as to reduce header bloat if that is important. 
Not sure how much this is a gain in that particular case.
Not sure about performance either.

> > For instance, a CaptureDeviceManager::Observer destructor could ensure that
> > it gets removed from m_observers. Is it useful in that context?
> 
> You'd just do the same thing (a destructor-time unregister) for the Token
> approach.

You would need to do that on each capturer class, while it would be isolated in the Observer destructor here.

> > > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.h:45
> > > +    RetainPtr<AVAudioSessionPortDescription> m_portDescription;
> > 
> > Wonder whether we could have a UniqueRef equivalent here?
> 
> You can't have a UniqueRef to a Obj-C object.

Wouldn't it be possible to assert at UniqueRef<ObjC> creation time that the ObjC pointer is not null?

> 
> > > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.mm:42
> > > +AVAudioSessionCaptureDevice::AVAudioSessionCaptureDevice(AVAudioSessionPortDescription* portDescription, const String& persistentID, const String& label)
> > 
> > Nit: use String&& for persistentID and label
> 
> No, we don't need to pass string by r-value ref.

Will it not cause unnecessary count churning like passing const RefPtr<>& instead of RefPtr<>&&?

[Jer Noble]

(In reply to youenn fablet from comment #7)
> > > What are the pros and cons of this approach compared to a
> > > CaptureDeviceManager::Observer class in this particular context?
> > 
> > Doing an ::Observer approach requires clients to include this header in
> > their own header. Doing a Token approach only requires clients to include
> > this header in their implementation file. This, in the long term, will
> > reduce header bloat.
> 
> You could encapsulate the Observer in a std::function in the cpp file so as
> to reduce header bloat if that is important. 
> Not sure how much this is a gain in that particular case.
> Not sure about performance either.

I don't know what that buys you.

> > > For instance, a CaptureDeviceManager::Observer destructor could ensure that
> > > it gets removed from m_observers. Is it useful in that context?
> > 
> > You'd just do the same thing (a destructor-time unregister) for the Token
> > approach.
> 
> You would need to do that on each capturer class, while it would be isolated
> in the Observer destructor here.

The alternative would be for the Observer to have to have a back pointer to the capturer and to somehow track the capturer's lifetime.  So you're back to a tightly coupled notification system.

> > > > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.h:45
> > > > +    RetainPtr<AVAudioSessionPortDescription> m_portDescription;
> > > 
> > > Wonder whether we could have a UniqueRef equivalent here?
> > 
> > You can't have a UniqueRef to a Obj-C object.
> 
> Wouldn't it be possible to assert at UniqueRef<ObjC> creation time that the
> ObjC pointer is not null?

There's no such thing as a UniqueRef<ObjC>, because you can't pass in an ObjC object by reference.  UniqueRef<> also doesn't do retain-release, so it's semantics don't make sense for something that's reference counted, like ObjC objects are.

> > 
> > > > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDevice.mm:42
> > > > +AVAudioSessionCaptureDevice::AVAudioSessionCaptureDevice(AVAudioSessionPortDescription* portDescription, const String& persistentID, const String& label)
> > > 
> > > Nit: use String&& for persistentID and label
> > 
> > No, we don't need to pass string by r-value ref.
> 
> Will it not cause unnecessary count churning like passing const RefPtr<>&
> instead of RefPtr<>&&?

No.  And by mandating r-value input, you would create a bunch of String objects just for the sake of passing them in.  (E.g., you'd need to copy a static constant string just so that you could pass it in as an r-value.)

[WebKit Commit Bot]

Comment on attachment 308583 [details]
Patch

Clearing flags on attachment: 308583

Committed r216016: <http://trac.webkit.org/changeset/216016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.