[webkit-reviews] review granted: [Bug 189351] [MediaStream] Include supported frame rates in video capture presets : [Attachment 349060] 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 189351: [MediaStream] Include supported frame rates in video capture
presets
https://bugs.webkit.org/show_bug.cgi?id=189351

Attachment 349060: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 349060
  --> https://bugs.webkit.org/attachment.cgi?id=349060
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349060&action=review

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:61
> +void RealtimeVideoSource::setSupportedPresets(const
RealtimeVideoSource::VideoPresets&& presets)

s/const//

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:63
> +    m_presets = presets;

WTFMove(presets);

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:65
> +	   m_presets.begin(), m_presets.end(),

How about merging these two as one line?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:67
> +	       return (a.first.width() * a.first.height()) < (b.first.width() *
b.first.height());

Using a pair makes things a bit more complex to read.
Can we go with s/first/size/ and s/second/frameRates/?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:98
> +    for (auto& preset : m_presets) {

Could be const auto& I guess.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:106
> +	   for (auto& rate : rates) {

const auto& I guess.
s/rates/preset.second/

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:131
> +    for (auto preset : m_presets) {

const auto&

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:133
> +	   const auto& rates = preset.second;

Moving below closer to the first use?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:139
> +	       match.frameRate = rates[rates.size() - 1].maximum;

rates.last().
There is no automatic guarantee that rates is not empty, but this should
obviously always be the case.

> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:163
> +    if (!width && !height && !size.isEmpty())

Maybe this check should be included in bestSupportedSizeAndFrameRate?

> Source/WebCore/platform/mediastream/RealtimeVideoSource.h:50
> +    void setSizeAndFrameRate(std::optional<int> width, std::optional<int>
height, std::optional<double>) final;

Add frameRate name as well?

> Source/WebCore/platform/mock/MockMediaDevice.h:122
>	   return MockDisplayProperties { *defaultFrameRate, Color::lightGray
};

s/Color::lightGray/*fillColor/