Simplify logic when changing RealtimeMediaSource settings
<rdar://problem/44117948>
Created attachment 348882 [details] Patch
Created attachment 348885 [details] Patch
Created attachment 348888 [details] Patch
Comment on attachment 348888 [details] Patch LGTM, some questions below though. I would try to move away from calling base class overriden methods settingsDidchange to handle observers. View in context: https://bugs.webkit.org/attachment.cgi?id=348888&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:204 > + } Might be nice to add something like min_element for Vector. > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:134 > +void RealtimeMediaSource::settingsDidChange(OptionSet<RealtimeMediaSourceSettings::Flag>) I would rename it to notifySettingsDidChange and be non virtual. Then settingsDidChange() would be virtual and do nothing. > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:427 > +static void applyNumericConstraint(const NumericConstraint<ValueType>& constraint, ValueType current, std::optional<Vector<ValueType>> discreteCapabilityValues, ValueType capabilityMin, ValueType capabilityMax, RealtimeMediaSource* source, void (RealtimeMediaSource::*applier)(ValueType)) Should ideally be a Vector<ValueType>* discreteCapabilityValues Can source be a RealtimeMediaSource& or better applier be changed to a lambda? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:877 > + settingsDidChange(RealtimeMediaSourceSettings::Flag::Width); We might be changing height, should we notify it as well? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:889 > + settingsDidChange(RealtimeMediaSourceSettings::Flag::Height); Ditto? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:908 > + settingsDidChange(RealtimeMediaSourceSettings::Flag::AspectRatio); Ditto? > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:187 > + virtual std::optional<Vector<int>> discreteSampleRates() const; Wouldn't it be better to return const Vector<int>*? > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:191 > + virtual std::optional<Vector<int>> discreteSampleSizes() const; Ditto? > Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:72 > +OptionSet<RealtimeMediaSourceSettings::Flag> RealtimeMediaSourceSettings::diff(const RealtimeMediaSourceSettings& that) const s/diff/difference? > Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:74 > + OptionSet<RealtimeMediaSourceSettings::Flag> diff; s/diff/difference? > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:276 > + RealtimeMediaSource::settingsDidChange(settings); There is something I am not sure about here. settingsDidChange() is used by GStreamer to apply settings set by the user of the source. But here, it is used in processNewFrame() so it comes from underneath to the user of the source. Would it be easier to have something like a RealtimeMediaSource method that would for each change call settingsDidChange that would apply the changes on the source and another that would notify observers? In processNewFrame, we would just notify observers. > Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:414 > } Do we need to return a boolean? > Source/WebCore/platform/mediastream/mac/MockRealtimeAudioSourceMac.mm:181 > + MockRealtimeAudioSource::settingsDidChange(settings); This call to
Created attachment 348921 [details] Patch for landing.
Created attachment 348923 [details] Patch for landing.
Comment on attachment 348888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348888&action=review >> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:134 >> +void RealtimeMediaSource::settingsDidChange(OptionSet<RealtimeMediaSourceSettings::Flag>) > > I would rename it to notifySettingsDidChange and be non virtual. > Then settingsDidChange() would be virtual and do nothing. The problem is, in addition to notifying observers from the base class, we sometimes need to react to a settings change in more than one derived class. For example MockRealtimeVideoSourceMac -> MockRealtimeVideoSource -> RealtimeMediaSource. I'll leave this as is for now and we can talk about ways to simplify it further. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:427 >> +static void applyNumericConstraint(const NumericConstraint<ValueType>& constraint, ValueType current, std::optional<Vector<ValueType>> discreteCapabilityValues, ValueType capabilityMin, ValueType capabilityMax, RealtimeMediaSource* source, void (RealtimeMediaSource::*applier)(ValueType)) > > Should ideally be a Vector<ValueType>* discreteCapabilityValues > Can source be a RealtimeMediaSource& or better applier be changed to a lambda? Fixed. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:877 >> + settingsDidChange(RealtimeMediaSourceSettings::Flag::Width); > > We might be changing height, should we notify it as well? Fixed. BTW, this will go away in a future refactoring. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:889 >> + settingsDidChange(RealtimeMediaSourceSettings::Flag::Height); > > Ditto? Ditto. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:908 >> + settingsDidChange(RealtimeMediaSourceSettings::Flag::AspectRatio); > > Ditto? Ditto. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:187 >> + virtual std::optional<Vector<int>> discreteSampleRates() const; > > Wouldn't it be better to return const Vector<int>*? Not all derived classes implement this. >> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:191 >> + virtual std::optional<Vector<int>> discreteSampleSizes() const; > > Ditto? Ditto. >> Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:72 >> +OptionSet<RealtimeMediaSourceSettings::Flag> RealtimeMediaSourceSettings::diff(const RealtimeMediaSourceSettings& that) const > > s/diff/difference? Fixed. >> Source/WebCore/platform/mediastream/RealtimeMediaSourceSettings.cpp:74 >> + OptionSet<RealtimeMediaSourceSettings::Flag> diff; > > s/diff/difference? Ditto. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:276 >> + RealtimeMediaSource::settingsDidChange(settings); > > There is something I am not sure about here. > settingsDidChange() is used by GStreamer to apply settings set by the user of the source. > But here, it is used in processNewFrame() so it comes from underneath to the user of the source. > > Would it be easier to have something like a RealtimeMediaSource method that would for each change call settingsDidChange that would apply the changes on the source and another that would notify observers? > In processNewFrame, we would just notify observers. I'm not sure it would be clearer, so I'm going to leave this for now. We can talk about this when I am back in the office to see if we can come up with something better. >> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:414 >> } > > Do we need to return a boolean? No, fixed.
Comment on attachment 348923 [details] Patch for landing. Clearing flags on attachment: 348923 Committed r235670: <https://trac.webkit.org/changeset/235670>
All reviewed patches have been landed. Closing bug.