Auto pause video streams based on encoder target bitrate.

Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility
to run with suspension in video loopback test.

BUG=webrtc:5868
R=stefan@webrtc.org

Committed: https://crrev.com/48a4beb7a406588419d0464154c0afdc5bd41531
Cr-Commit-Position: refs/heads/master@{#13360}

[mflodman, 2016-06-30 15:18:15]

Description was changed from

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

BUG=5868
==========

to

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and
possibility to run with suspension in video loopback test.


BUG=5868
==========

[mflodman, 2016-06-30 15:18:31]

Description was changed from

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and
possibility to run with suspension in video loopback test.


BUG=5868
==========

to

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility to run
with suspension in video loopback test.


BUG=5868
==========

[mflodman, 2016-06-30 15:18:52]

Description was changed from

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility to run
with suspension in video loopback test.


BUG=5868
==========

to

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility to run
with suspension in video loopback test.

BUG=webrtc:5868
==========

[mflodman, 2016-06-30 15:19:47]

Description was changed from

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility to run
with suspension in video loopback test.

BUG=webrtc:5868
==========

to

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility
to run with suspension in video loopback test.

BUG=webrtc:5868
==========

[mflodman, 2016-06-30 15:20:17]

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

[mflodman, 2016-06-30 15:20:17]

Stefan,
Please take a look.

[stefan-webrtc, 2016-06-30 16:17:38]

lg, just a few comments.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:49: num_pause_events_);
Good to have! Remember to add it to the uma configs too.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:79: uint32_t predicted_protection =
_bps

Add a comment describing that this assumes the same amount of protection as was
observed before the stream was paused.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:83: << " and current estimate of " <<
target_bitrate_bps
Should you log the allocation here too?

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:96: config.media_ratio = (allocated_bitrate -
protection_bitrate) /
Use a helper variable media_bitrate = allocated_bitrate - protection_bitrate to
simplify and make it clear that we assume allocated = media + protection

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:131: config.allocated_bitrate_bps =
allocated_bitrate;
Is there no need to update the media ratio here? I assume it could be the case
that some protection is used if last_fraction_loss_ > 0.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:176: // This observer hasn't been added yet,
just give it its fair share.
Not clear to me why this can happen? Is it correct usage?

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:340: // avoid too much toggling.
I wonder if this should be moved up to where the ratio is updated instead? Not
sure, so feel free to disagree.

That said, I do think this is the right trade-off.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/video/video_send_stream.h
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.h:90: uint32_t OnBitrateUpdated(uint32_t
bitrate_bps,
Comment on what this returns. Should we have a test which verifies that it
returns what it should?

[mflodman, 2016-07-01 09:40:15]

Stefan,
Please take another look.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:49: num_pause_events_);
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> Good to have! Remember to add it to the uma configs too.

Thanks for the reminder, I'll make sure this happens.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:79: uint32_t predicted_protection =
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> _bps
> 
> Add a comment describing that this assumes the same amount of protection as
was
> observed before the stream was paused.

Done.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:83: << " and current estimate of " <<
target_bitrate_bps
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> Should you log the allocation here too?

Good point, added.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:96: config.media_ratio = (allocated_bitrate -
protection_bitrate) /
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> Use a helper variable media_bitrate = allocated_bitrate - protection_bitrate
to
> simplify and make it clear that we assume allocated = media + protection

Done.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:131: config.allocated_bitrate_bps =
allocated_bitrate;
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> Is there no need to update the media ratio here? I assume it could be the case
> that some protection is used if last_fraction_loss_ > 0.

That is a good question and I assumed it would be ok to wait for the first
"real" allocation. I added that now though.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:176: // This observer hasn't been added yet,
just give it its fair share.
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> Not clear to me why this can happen? Is it correct usage?

AS of now it is, the encoder is configured before the observer is added here and
we need the start bitrate before that happens.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:340: // avoid too much toggling.
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> I wonder if this should be moved up to where the ratio is updated instead? Not
> sure, so feel free to disagree.
> 
> That said, I do think this is the right trade-off.

As of now I prefer it this way to have it separate. Let's discuss for the future
though, if this can be simplified and done differently.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/video/video_send_stream.h
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.h:90: uint32_t OnBitrateUpdated(uint32_t
bitrate_bps,
On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> Comment on what this returns. Should we have a test which verifies that it
> returns what it should?

I adde a comment in BitrateAllocatorObserver::OnBitrateUpdated instead.

I'll see if I can find a good way of testing this, but that will be hard given
the structure of the classes. I think, but I'l check.

[stefan-webrtc, 2016-07-01 10:42:49]

lgtm

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:83: << " and current estimate of " <<
target_bitrate_bps
On 2016/07/01 09:40:15, mflodman wrote:
> On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> > Should you log the allocation here too?
> 
> Good point, added.

Can't see that it was added?

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:131: config.allocated_bitrate_bps =
allocated_bitrate;
On 2016/07/01 09:40:15, mflodman wrote:
> On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> > Is there no need to update the media ratio here? I assume it could be the
case
> > that some protection is used if last_fraction_loss_ > 0.
> 
> That is a good question and I assumed it would be ok to wait for the first
> "real" allocation. I added that now though.

Thanks. Better to do it and avoid others asking themselves the same question, I
guess.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:176: // This observer hasn't been added yet,
just give it its fair share.
On 2016/07/01 09:40:15, mflodman wrote:
> On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> > Not clear to me why this can happen? Is it correct usage?
> 
> AS of now it is, the encoder is configured before the observer is added here
and
> we need the start bitrate before that happens.

Acknowledged.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:340: // avoid too much toggling.
On 2016/07/01 09:40:15, mflodman wrote:
> On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> > I wonder if this should be moved up to where the ratio is updated instead?
Not
> > sure, so feel free to disagree.
> > 
> > That said, I do think this is the right trade-off.
> 
> As of now I prefer it this way to have it separate. Let's discuss for the
future
> though, if this can be simplified and done differently.

Acknowledged.

https://codereview.webrtc.org/2117493002/diff/1/webrtc/video/video_send_stream.h
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.h:90: uint32_t OnBitrateUpdated(uint32_t
bitrate_bps,
On 2016/07/01 09:40:15, mflodman wrote:
> On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> > Comment on what this returns. Should we have a test which verifies that it
> > returns what it should?
> 
> I adde a comment in BitrateAllocatorObserver::OnBitrateUpdated instead.
> 
> I'll see if I can find a good way of testing this, but that will be hard given
> the structure of the classes. I think, but I'l check.

Ok, I was thinking a simple unittest, but I won't block on this.

[stefan-webrtc, 2016-07-01 10:43:14]

But please make sure the allocation is logged as agreed.

[mflodman, 2016-07-01 10:52:29]

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2117493002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:83: << " and current estimate of " <<
target_bitrate_bps
On 2016/07/01 10:42:49, stefan-webrtc (holmer) wrote:
> On 2016/07/01 09:40:15, mflodman wrote:
> > On 2016/06/30 16:17:38, stefan-webrtc (holmer) wrote:
> > > Should you log the allocation here too?
> > 
> > Good point, added.
> 
> Can't see that it was added?

Sorry, bad reply from me. The allocation will be 0 for this observer, but I
added it for the case below when we have an allocation.

[mflodman, 2016-07-01 10:57:13]

And removing the double logging. I'll submit this patch set manually since this
is only a logging change.

[stefan-webrtc, 2016-07-01 10:59:42]

lgtm

[commit-bot: I haz the power, 2016-07-01 11:04:15]

Description was changed from

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility
to run with suspension in video loopback test.

BUG=webrtc:5868
==========

to

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility
to run with suspension in video loopback test.

BUG=webrtc:5868
R=stefan@webrtc.org

Committed: https://crrev.com/48a4beb7a406588419d0464154c0afdc5bd41531
Cr-Commit-Position: refs/heads/master@{#13360}
==========

[mflodman, 2016-07-01 11:04:15]

Description was changed from

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility
to run with suspension in video loopback test.

BUG=webrtc:5868
R=stefan@webrtc.org

Committed: https://crrev.com/48a4beb7a406588419d0464154c0afdc5bd41531
Cr-Commit-Position: refs/heads/master@{#13360}
==========

to

==========
Auto pause video streams based on encoder target bitrate.

This CL changes the auto-pause logic to suspend a stream based on the
encoder target bitrate instead of the allocated bitrate for a stream,
to account for possible protection, e.g. FEC and NACK.

This CL also adds periodic logging of the current BWE and possibility
to run with suspension in video loopback test.

BUG=webrtc:5868
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/48a4beb7a406588419d046415...
==========

[commit-bot: I haz the power, 2016-07-01 11:04:16]

Patchset 4 (id:??) landed as
https://crrev.com/48a4beb7a406588419d0464154c0afdc5bd41531
Cr-Commit-Position: refs/heads/master@{#13360}

[mflodman, 2016-07-01 11:04:17]

Committed patchset #4 (id:60001) manually as
48a4beb7a406588419d0464154c0afdc5bd41531 (presubmit successful).