[webkit-reviews] review granted: [Bug 189284] [MediaStream] Simplify logic when changing RealtimeMediaSource settings : [Attachment 348888] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 189284: [MediaStream] Simplify logic when changing RealtimeMediaSource
settings
https://bugs.webkit.org/show_bug.cgi?id=189284

Attachment 348888: Patch

https://bugs.webkit.org/attachment.cgi?id=348888&action=review




--- Comment #5 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 348888
  --> https://bugs.webkit.org/attachment.cgi?id=348888
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::F
lag>)

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