Bug 210186 – Safari doesn't apply frameRate limit when request stream from Camera

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

[olsviate]

Safari doesn't apply frameRate limit that is provided in constaraints for getUserMedia().
Track settings shows that provided frame rate is set, but stats shows that limit is not applied.

 

Also it is not possible to change frame rate via MediaStreamTrack::applyConstraints().

 

jsfiddle: https://jsfiddle.net/sj1m6eqg/

[Radar WebKit Bug Importer]

<rdar://problem/61452794>

[youenn fablet]

Created attachment 396636 [details]
Patch

[youenn fablet]

Created attachment 396642 [details]
Patch

[Eric Carlson]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 396758 [details]
Patch for landing

[EWS]

Committed r260245: <https://trac.webkit.org/changeset/260245>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396758 [details].

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r260245 for reason:

The tests added with this change are frequently failing on macOS bots.

Committed r260274: <https://trac.webkit.org/changeset/260274>

[youenn fablet]

It seems that the release bots have issues in producing more than 10fps frame rate for capture video tracks.

[youenn fablet]

Created attachment 396970 [details]
Patch

[youenn fablet]

Comment on attachment 396970 [details]
Patch

Relanding the patch with checking whether the source is producing 30fps.

[EWS]

Committed r260364: <https://trac.webkit.org/changeset/260364>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396970 [details].