Bug 189284 - [MediaStream] Simplify logic when changing RealtimeMediaSource settings
Summary: [MediaStream] Simplify logic when changing RealtimeMediaSource settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-04 16:38 PDT by Eric Carlson
Modified: 2018-09-05 08:56 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2018-09-04 16:38:10 PDT
Simplify logic when changing RealtimeMediaSource settings
Comment 1 Radar WebKit Bug Importer 2018-09-04 16:38:38 PDT
<rdar://problem/44117948>
Comment 2 Eric Carlson 2018-09-04 18:21:23 PDT
Created attachment 348882 [details]
Patch
Comment 3 Eric Carlson 2018-09-04 19:35:07 PDT
Created attachment 348885 [details]
Patch
Comment 4 Eric Carlson 2018-09-04 20:14:25 PDT
Created attachment 348888 [details]
Patch
Comment 5 youenn fablet 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
Comment 6 Eric Carlson 2018-09-05 07:52:34 PDT
Created attachment 348921 [details]
Patch for landing.
Comment 7 Eric Carlson 2018-09-05 08:04:58 PDT
Created attachment 348923 [details]
Patch for landing.
Comment 8 Eric Carlson 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.
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2018-09-05 08:56:53 PDT
All reviewed patches have been landed.  Closing bug.