Include supported frame rates in video capture presets.
<rdar://problem/44188917>
Created attachment 349060 [details] Patch
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 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.
Created attachment 349075 [details] Patch for landing
Comment on attachment 349075 [details] Patch for landing Clearing flags on attachment: 349075 Committed r235760: <https://trac.webkit.org/changeset/235760>
All reviewed patches have been landed. Closing bug.