Bug 189284 – [MediaStream] Simplify logic when changing RealtimeMediaSource settings

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

Simplify logic when changing RealtimeMediaSource settings

[Radar WebKit Bug Importer]

<rdar://problem/44117948>

[Eric Carlson]

Created attachment 348882 [details]
Patch

[Eric Carlson]

Created attachment 348885 [details]
Patch

[Eric Carlson]

Created attachment 348888 [details]
Patch

[youenn fablet]

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]

Created attachment 348921 [details]
Patch for landing.

[Eric Carlson]

Created attachment 348923 [details]
Patch for landing.

[Eric Carlson]

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 on attachment 348923 [details]
Patch for landing.

Clearing flags on attachment: 348923

Committed r235670: <https://trac.webkit.org/changeset/235670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.