Add notifications from CaptureDeviceManager (and subclasses) when device lists change
Created attachment 308568 [details] Patch
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.
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.
Created attachment 308583 [details] Patch
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.
(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.
> > 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<>&&?
(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.)
Comment on attachment 308583 [details] Patch Clearing flags on attachment: 308583 Committed r216016: <http://trac.webkit.org/changeset/216016>
All reviewed patches have been landed. Closing bug.