Bug 189351 - [MediaStream] Include supported frame rates in video capture presets
Summary: [MediaStream] Include supported frame rates in video capture presets
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-06 07:54 PDT by Eric Carlson
Modified: 2018-09-06 15:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (28.29 KB, patch)
2018-09-06 13:24 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (29.04 KB, patch)
2018-09-06 14:32 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-06 07:54:10 PDT
Include supported frame rates in video capture presets.
Comment 1 Radar WebKit Bug Importer 2018-09-06 11:27:28 PDT
<rdar://problem/44188917>
Comment 2 Eric Carlson 2018-09-06 13:24:41 PDT
Created attachment 349060 [details]
Patch
Comment 3 youenn fablet 2018-09-06 13:47:14 PDT
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/
Comment 4 Eric Carlson 2018-09-06 14:32:00 PDT
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.
Comment 5 Eric Carlson 2018-09-06 14:32:09 PDT
Created attachment 349075 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2018-09-06 15:10:24 PDT
Comment on attachment 349075 [details]
Patch for landing

Clearing flags on attachment: 349075

Committed r235760: <https://trac.webkit.org/changeset/235760>
Comment 7 WebKit Commit Bot 2018-09-06 15:10:26 PDT
All reviewed patches have been landed.  Closing bug.