Limit target encoder bitrate change frequency in the frame scheduler.

Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.

Also fixed WebrtcConnectionToHost::Connect() to avoid crash when
running perftests.

Committed: https://crrev.com/88e024f452e7ad64a207e12366c9352ee79a6797
Cr-Commit-Position: refs/heads/master@{#437747}

[Sergey Ulanov, 2016-12-09 23:19:50]

Description was changed from

==========
Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.
==========

to

==========
Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.

Also fixed WebrtcConnectionToHost::Connect() to avoid crash when
running perftests.
==========

[Sergey Ulanov, 2016-12-09 23:21:30]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2016-12-09 23:39:18]

LGTM.

I think this is fine as an incremental improvement, but it would be nice if we
could make use of even a relatively small increase in available bandwidth once
it has been determined to be consistent. For example, if a download was eating
up 25% of my bandwidth when I started Chromoting, then when it stops, this
change will prevent us taking advantage of the extra bandwidth.

A better fix might be to react to changes in bandwidth in time proportional to
their magnitude. For example, >33% would switch immediately, <5% would be
ignored but, say, 15% would cause a reconfiguration after being sustained for
10s or so. I think a momentary quality hit while the codec reconfigured is
acceptable if we have reason to believe that the longer-term quality will
improve.

Obviously something like this would need more testing, so I'm fine with landing
this as-is if you believe that 33% is the right threshold.

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:72: while
(bandwidth_samples_.size() > 1 &&
It took me a while to work out why you needed at least two elements for this
loop before I realized that you don't want to remove the one you just added. I
think it might be clearer to clean up the old entries (using !empty() as a
conditional, which is a lot more self-documenting) and then adding the new one
afterwards.

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:93: // necessary because the
encoder resets internal state it's reconfigured and
s/it's/when it's/

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:97: current_target_bitrate_ /
3) {
Add a named constant with rationale to explain why 33% was chosen.

[Sergey Ulanov, 2016-12-10 01:22:57]

> I think this is fine as an incremental improvement, but it would be nice if we
> could make use of even a relatively small increase in available bandwidth once
> it has been determined to be consistent. For example, if a download was eating
> up 25% of my bandwidth when I started Chromoting, then when it stops, this
> change will prevent us taking advantage of the extra bandwidth.

This change only changes the target bitrate we pass to the encoder. Exact BWE
numbers are still used to pace the capturer, so in your scenario we will still
be able to use all available BW by sending higher frame-rate.
VP8 responds very poorly to target bitrate changes. Even if we adjust target
bitrate by 25%, VP8 may not be able to use that bandwidth.


> 
> A better fix might be to react to changes in bandwidth in time proportional to
> their magnitude. For example, >33% would switch immediately, <5% would be
> ignored but, say, 15% would cause a reconfiguration after being sustained for
> 10s or so. I think a momentary quality hit while the codec reconfigured is
> acceptable if we have reason to believe that the longer-term quality will
> improve.
> 
> Obviously something like this would need more testing, so I'm fine with
landing
> this as-is if you believe that 33% is the right threshold.

This all makes sense, but I think a better solution would be switch to VP9. VP9
does much better job adjusting to target bitrate, so it may be that we won't
need any of this logic with VP9.

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:72: while
(bandwidth_samples_.size() > 1 &&
On 2016/12/09 23:39:18, Jamie wrote:
> It took me a while to work out why you needed at least two elements for this
> loop before I realized that you don't want to remove the one you just added. I
> think it might be clearer to clean up the old entries (using !empty() as a
> conditional, which is a lot more self-documenting) and then adding the new one
> afterwards.

Done.

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:93: // necessary because the
encoder resets internal state it's reconfigured and
On 2016/12/09 23:39:18, Jamie wrote:
> s/it's/when it's/

Done.

https://codereview.chromium.org/2562893003/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:97: current_target_bitrate_ /
3) {
On 2016/12/09 23:39:18, Jamie wrote:
> Add a named constant with rationale to explain why 33% was chosen.

Done.

[Sergey Ulanov, 2016-12-10 01:24:48]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-12-10 01:24:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2562893003/#ps20001
(title: ".")

[commit-bot: I haz the power, 2016-12-10 01:25:22]

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

[commit-bot: I haz the power, 2016-12-10 04:03:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481333088160730,
"parent_rev": "35f9d162e91e1a29d2ec20fcefc544da75b8e072", "commit_rev":
"d6678ecfcbf04645fc54456137238ad655f1ce93"}

[commit-bot: I haz the power, 2016-12-10 04:04:14]

Description was changed from

==========
Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.

Also fixed WebrtcConnectionToHost::Connect() to avoid crash when
running perftests.
==========

to

==========
Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.

Also fixed WebrtcConnectionToHost::Connect() to avoid crash when
running perftests.

Review-Url: https://codereview.chromium.org/2562893003
==========

[commit-bot: I haz the power, 2016-12-10 04:04:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-12 15:06:13]

Description was changed from

==========
Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.

Also fixed WebrtcConnectionToHost::Connect() to avoid crash when
running perftests.

Review-Url: https://codereview.chromium.org/2562893003
==========

to

==========
Limit target encoder bitrate change frequency in the frame scheduler.

Previously the frame scheduler was setting target bitrate whenever
bandwidth estimate changes. VP8 encoder doesn't handle this very well:
every time bitrate is update quality drops significanlty for the
following frames. It happens because every time biutrate is updated the
codec needs to be reconfigured, resulting in encoder state reset.

Fixed it by adding a filter that calculates average BW estimate over
last second and updates target bitrate only when the average BW
estimate differs from the current target bitrate by more than 33%.

Also fixed WebrtcConnectionToHost::Connect() to avoid crash when
running perftests.

Committed: https://crrev.com/88e024f452e7ad64a207e12366c9352ee79a6797
Cr-Commit-Position: refs/heads/master@{#437747}
==========

[commit-bot: I haz the power, 2016-12-12 15:06:14]

Patchset 2 (id:??) landed as
https://crrev.com/88e024f452e7ad64a207e12366c9352ee79a6797
Cr-Commit-Position: refs/heads/master@{#437747}