Summary: | Safari doesn't apply frameRate limit when request stream from Camera | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | olsviate <olsviate> | ||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Safari 13 | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | macOS 10.15 | ||||||||||||
Attachments: |
|
Description
olsviate
2020-04-08 06:50:19 PDT
Created attachment 396636 [details]
Patch
Created attachment 396642 [details]
Patch
Comment on attachment 396642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396642&action=review r=me once the bots are happy > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:153 > + auto size = this->size(); Nit: this local variable is only used once now. > LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-decreasing.html:20 > + if (!stream1.getVideoTracks()[0].ended) { Is this check necessary? If so, doesn't it mean we're testing different things on different runs? > LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-decreasing.html:26 > + let frameRate = await computeFrameRate(stream2, video); Nit: this can be a const. > LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-increasing.html:26 > + if (!stream1.getVideoTracks()[0].ended) { > + const frameRate = await computeFrameRate(stream1, video); > + assert_greater_than(frameRate, 1, "stream1 frame rate above 1"); > + assert_less_than(frameRate, 20, "stream1 frame rate below 20"); > + } > + > + let frameRate = await computeFrameRate(stream2, video); Ditto the comments above. Thanks for the review. (In reply to Eric Carlson from comment #4) > Comment on attachment 396642 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396642&action=review > > r=me once the bots are happy > > > Source/WebCore/platform/mediastream/RealtimeVideoSource.cpp:153 > > + auto size = this->size(); > > Nit: this local variable is only used once now. Will fix. > > LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-decreasing.html:20 > > + if (!stream1.getVideoTracks()[0].ended) { > > Is this check necessary? If so, doesn't it mean we're testing different > things on different runs? This is to cover the case of iOS which might mute the previous track. I should replace ended by muted though. > > LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-decreasing.html:26 > > + let frameRate = await computeFrameRate(stream2, video); > > Nit: this can be a const. > > > LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-increasing.html:26 > > + if (!stream1.getVideoTracks()[0].ended) { > > + const frameRate = await computeFrameRate(stream1, video); > > + assert_greater_than(frameRate, 1, "stream1 frame rate above 1"); > > + assert_less_than(frameRate, 20, "stream1 frame rate below 20"); > > + } > > + > > + let frameRate = await computeFrameRate(stream2, video); > > Ditto the comments above. OK Created attachment 396758 [details]
Patch for landing
Committed r260245: <https://trac.webkit.org/changeset/260245> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396758 [details]. It looks like all the tests added with this change are failing more often than they are passing on macOS release bots: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=fast%2Fmediastream%2Fmediastreamtrack-video-frameRate-clone-decreasing.html&test=fast%2Fmediastream%2Fmediastreamtrack-video-frameRate-clone-increasing.html&test=fast%2Fmediastream%2Fmediastreamtrack-video-frameRate-decreasing.html&test=fast%2Fmediastream%2Fmediastreamtrack-video-frameRate-increasing.html Reverted r260245 for reason: The tests added with this change are frequently failing on macOS bots. Committed r260274: <https://trac.webkit.org/changeset/260274> It seems that the release bots have issues in producing more than 10fps frame rate for capture video tracks. Created attachment 396970 [details]
Patch
Comment on attachment 396970 [details]
Patch
Relanding the patch with checking whether the source is producing 30fps.
Committed r260364: <https://trac.webkit.org/changeset/260364> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396970 [details]. |