Bug 189351 – [MediaStream] Include supported frame rates in video capture presets

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Include supported frame rates in video capture presets.

[Radar WebKit Bug Importer]

<rdar://problem/44188917>

[Eric Carlson]

Created attachment 349060 [details]
Patch

[youenn fablet]

Comment on attachment 349060 [details]
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/

[Eric Carlson]

Comment on attachment 349060 [details]
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//

OK

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

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:65
>> +        m_presets.begin(), m_presets.end(),
> 
> How about merging these two as one line?

Sure.

>> 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/?

OK, I defined a struct.

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

OK

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:106
>> +        for (auto& rate : rates) {
> 
> const auto& I guess.
> s/rates/preset.second/

Sure.

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

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:133
>> +        const auto& rates = preset.second;
> 
> Moving below closer to the first use?

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:163
>> +    if (!width && !height && !size.isEmpty())
> 
> Maybe this check should be included in bestSupportedSizeAndFrameRate?

I think it is better to have bestSupportedSizeAndFrameRate only return a matching preset.

[Eric Carlson]

Created attachment 349075 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 349075 [details]
Patch for landing

Clearing flags on attachment: 349075

Committed r235760: <https://trac.webkit.org/changeset/235760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.