[webkit-reviews] review granted: [Bug 210186] Safari doesn't apply frameRate limit when request stream from Camera : [Attachment 396642] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted  review:
Bug 210186: Safari doesn't apply frameRate limit when request stream from
Camera
https://bugs.webkit.org/show_bug.cgi?id=210186

Attachment 396642: Patch

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




--- Comment #4 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 396642
  --> https://bugs.webkit.org/attachment.cgi?id=396642
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:2
0
> +    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:2
6
> +    let frameRate = await computeFrameRate(stream2, video);

Nit: this can be a const.

>
LayoutTests/fast/mediastream/mediastreamtrack-video-frameRate-increasing.html:2
6
> +    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.