WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
189284
[MediaStream] Simplify logic when changing RealtimeMediaSource settings
https://bugs.webkit.org/show_bug.cgi?id=189284
Summary
[MediaStream] Simplify logic when changing RealtimeMediaSource settings
Eric Carlson
Reported
2018-09-04 16:38:10 PDT
Simplify logic when changing RealtimeMediaSource settings
Attachments
Patch
(60.92 KB, patch)
2018-09-04 18:21 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch
(65.90 KB, patch)
2018-09-04 19:35 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch
(65.91 KB, patch)
2018-09-04 20:14 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch for landing.
(66.45 KB, patch)
2018-09-05 07:52 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Patch for landing.
(66.54 KB, patch)
2018-09-05 08:04 PDT
,
Eric Carlson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-09-04 16:38:38 PDT
<
rdar://problem/44117948
>
Eric Carlson
Comment 2
2018-09-04 18:21:23 PDT
Created
attachment 348882
[details]
Patch
Eric Carlson
Comment 3
2018-09-04 19:35:07 PDT
Created
attachment 348885
[details]
Patch
Eric Carlson
Comment 4
2018-09-04 20:14:25 PDT
Created
attachment 348888
[details]
Patch
youenn fablet
Comment 5
2018-09-04 22:11:46 PDT
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
Eric Carlson
Comment 6
2018-09-05 07:52:34 PDT
Created
attachment 348921
[details]
Patch for landing.
Eric Carlson
Comment 7
2018-09-05 08:04:58 PDT
Created
attachment 348923
[details]
Patch for landing.
Eric Carlson
Comment 8
2018-09-05 08:10:13 PDT
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.
WebKit Commit Bot
Comment 9
2018-09-05 08:56:51 PDT
Comment on
attachment 348923
[details]
Patch for landing. Clearing flags on attachment: 348923 Committed
r235670
: <
https://trac.webkit.org/changeset/235670
>
WebKit Commit Bot
Comment 10
2018-09-05 08:56:53 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug