Add frame rate throttling to vp8 screenshare_layers.

Add frame rate throttling to vp8 screenshare_layers.

Drop frames if incoming frame rate is higher than the configured max
framerate.

BUG=webrtc:6897
Review-Url: https://codereview.webrtc.org/2578993002
Cr-Commit-Position: refs/heads/master@{#15819}
Committed: https://chromium.googlesource.com/external/webrtc/+/ac4a90dfdcc77411855a32757f33bc74369bf3b6

[sprang_webrtc, 2016-12-15 11:40:07]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 11:40:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sprang_webrtc, 2016-12-15 11:40:27]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org

[commit-bot: I haz the power, 2016-12-15 11:56:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 11:56:21]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-12-19 10:58:29]

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:159:
kOneSecond90Khz / incoming_framerate_.value_or(*target_framerate_);
Looks like incoming_framerate_ can be zero. I'm probably missing something, but
maybe good to make it explicit with a dcheck?

[sprang_webrtc, 2016-12-19 16:46:34]

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:159:
kOneSecond90Khz / incoming_framerate_.value_or(*target_framerate_);
On 2016/12/19 10:58:29, stefan-webrtc (holmer) wrote:
> Looks like incoming_framerate_ can be zero. I'm probably missing something,
but
> maybe good to make it explicit with a dcheck?

This "shouldn't happen" since we treat 0 fps as invalid in
VideoSender::UpdateEncoderParameters(). But I'll add a protection just in case
this breaks in the future.

[stefan-webrtc, 2016-12-22 09:39:35]

lgtm, but please consider dcheck instead.

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:159:
kOneSecond90Khz / incoming_framerate_.value_or(*target_framerate_);
On 2016/12/19 16:46:34, språng wrote:
> On 2016/12/19 10:58:29, stefan-webrtc (holmer) wrote:
> > Looks like incoming_framerate_ can be zero. I'm probably missing something,
> but
> > maybe good to make it explicit with a dcheck?
> 
> This "shouldn't happen" since we treat 0 fps as invalid in
> VideoSender::UpdateEncoderParameters(). But I'll add a protection just in case
> this breaks in the future.

Yes, sounds like the check is pretty far away from here. But if we assume it
should never happen, maybe it's enough to dcheck on it? Otherwise it looks like
it actually can happen.

[sprang_webrtc, 2016-12-28 13:00:35]

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2578993002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:159:
kOneSecond90Khz / incoming_framerate_.value_or(*target_framerate_);
On 2016/12/22 09:39:34, stefan-webrtc_OOO_Jan9 wrote:
> On 2016/12/19 16:46:34, språng wrote:
> > On 2016/12/19 10:58:29, stefan-webrtc (holmer) wrote:
> > > Looks like incoming_framerate_ can be zero. I'm probably missing
something,
> > but
> > > maybe good to make it explicit with a dcheck?
> > 
> > This "shouldn't happen" since we treat 0 fps as invalid in
> > VideoSender::UpdateEncoderParameters(). But I'll add a protection just in
case
> > this breaks in the future.
> 
> Yes, sounds like the check is pretty far away from here. But if we assume it
> should never happen, maybe it's enough to dcheck on it? Otherwise it looks
like
> it actually can happen.

Sure, added dcheck to OnRatesUpdated.

[sprang_webrtc, 2016-12-28 13:00:42]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-12-28 13:00:43]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2578993002/#ps60001 (title:
"dcheck framerate > 0")

[commit-bot: I haz the power, 2016-12-28 13:00:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-28 13:58:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482930042743820,
"parent_rev": "49f465f4b2eb4c80ac3836129a1b08f7a43665b6", "commit_rev":
"ac4a90dfdcc77411855a32757f33bc74369bf3b6"}

[commit-bot: I haz the power, 2016-12-28 13:58:10]

Description was changed from

==========
Add frame rate throttling to vp8 screenshare_layers.

Drop frames if incoming frame rate is higher than the configured max
framerate.

BUG=webrtc:6897
==========

to

==========
Add frame rate throttling to vp8 screenshare_layers.

Drop frames if incoming frame rate is higher than the configured max
framerate.

BUG=webrtc:6897

Review-Url: https://codereview.webrtc.org/2578993002
Cr-Commit-Position: refs/heads/master@{#15819}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ac4a90dfdcc77411855a32757...
==========

[commit-bot: I haz the power, 2016-12-28 13:58:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/ac4a90dfdcc77411855a32757...

[hta-webrtc, 2017-01-11 08:19:45]

hta@webrtc.org changed reviewers:
+ hta@webrtc.org

[hta-webrtc, 2017-01-11 08:19:46]

Unhappy with the nomenclature chosen here. Using "incoming" for stuff that is
not coming from the remote end is confusing.

https://codereview.webrtc.org/2578993002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2578993002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:330: if
(incoming_framerate_) {
Using the term "incoming" here is not a Good Thing.
It's usually used to refer to "stuff coming from the other end of the
connection".

Please consider renaming to "source_framerate_".

[sprang_webrtc, 2017-01-11 11:42:00]

https://codereview.webrtc.org/2578993002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right):

https://codereview.webrtc.org/2578993002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:330: if
(incoming_framerate_) {
On 2017/01/11 08:19:46, hta-webrtc wrote:
> Using the term "incoming" here is not a Good Thing.
> It's usually used to refer to "stuff coming from the other end of the
> connection".
> 
> Please consider renaming to "source_framerate_".

I create a new cl for that: https://codereview.webrtc.org/2624073003/