Add VideoSendStream::Stats::prefered_media_bitrate_bps

Add VideoSendStream::Stats::prefered_media_bitrate_bps

This cl move calculation of stats for prefered_media_bitrate_bps from webrtcvideoengine2.GetStats to SendStatisticsProxy::OnEncoderReconfigured.
This aligns better with how other send stats are reported and is needed as a prerequisite for moving video encoder configuration due to video resolution change
from WebRtcVideoEngine2 to ViEEncoder.

BUG=webrtc:6371
R=mflodman@webrtc.org, sprang@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/a48ddb763695dd6742fdcb6f0e5fa4f9818595e7

[perkj_webrtc, 2016-09-26 11:27:19]

perkj@webrtc.org changed reviewers:
+ asapersson@webrtc.org, sprang@webrtc.org

[perkj_webrtc, 2016-09-26 11:27:20]

Please?

[sprang_webrtc, 2016-09-26 11:50:02]

https://codereview.webrtc.org/2368223002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2368223002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:440: static_cast<uint32_t>(max_data_payload_length))
== VCM_OK;
Can we add getter to VideoSender instead, and calculate the preferred media
bitrate based on the same implementation as the codec actually uses?
eg, for VP8: create a SimulcastRateAllocator for the current codec, call
GetAllocation(codec.maxBitrate), and then sum the vector?
For other codecs I guess we just return the max bitrate, for now. I'll be
extending rate allocation logic in VideoSender soon anyway.

wdyt?

[perkj_webrtc, 2016-09-27 10:48:40]

Patchset #5 (id:80001) has been deleted

[perkj_webrtc, 2016-09-27 10:49:29]

ptal

https://codereview.webrtc.org/2368223002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2368223002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:440: static_cast<uint32_t>(max_data_payload_length))
== VCM_OK;
On 2016/09/26 11:50:02, språng wrote:
> Can we add getter to VideoSender instead, and calculate the preferred media
> bitrate based on the same implementation as the codec actually uses?
> eg, for VP8: create a SimulcastRateAllocator for the current codec, call
> GetAllocation(codec.maxBitrate), and then sum the vector?
> For other codecs I guess we just return the max bitrate, for now. I'll be
> extending rate allocation logic in VideoSender soon anyway.
> 
> wdyt?

I added SimulcastRateAlloctor directly in ViEEncoder as we discussed.

[sprang_webrtc, 2016-09-27 10:55:07]

lgtm with nits

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:71: for (auto&
rate : rates) {
Prefer not using auto unless type is very verbose.

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:298: int
preferred_media_bitrate_bps = 50;
Maybe have this as named constant at the top, and use everywhere in the test
instead of plain 50?

[perkj_webrtc, 2016-09-27 11:33:11]

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

[perkj_webrtc, 2016-09-27 11:33:12]

Adding mflodman as well since Åsa is ooo.

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:71: for (auto&
rate : rates) {
On 2016/09/27 10:55:07, språng wrote:
> Prefer not using auto unless type is very verbose.

Done.

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2368223002/diff/100001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:298: int
preferred_media_bitrate_bps = 50;
On 2016/09/27 10:55:07, språng wrote:
> Maybe have this as named constant at the top, and use everywhere in the test
> instead of plain 50?

Done.

[mflodman, 2016-09-28 16:33:10]

One super tiny nit, then LGTM.

https://codereview.webrtc.org/2368223002/diff/140001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2368223002/diff/140001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:103: class ConfigureEncoderTask;
Alphabetic order.

[perkj_webrtc, 2016-09-29 09:49:04]

Description was changed from

==========
Add VideoSendStream::Stats::prefered_media_bitrate_bps

This cl move calculation of stats for prefered_media_bitrate_bps from
webrtcvideoengine2.GetStats to SendStatisticsProxy::OnEncoderReconfigured.
This aligns better with how other send stats are reported and is needed as a
prerequisite for moving video encoder configuration due to video resolution
change
from WebRtcVideoEngine2 to ViEEncoder.

BUG=webrtc:6371
==========

to

==========
Add VideoSendStream::Stats::prefered_media_bitrate_bps

This cl move calculation of stats for prefered_media_bitrate_bps from
webrtcvideoengine2.GetStats to SendStatisticsProxy::OnEncoderReconfigured.
This aligns better with how other send stats are reported and is needed as a
prerequisite for moving video encoder configuration due to video resolution
change
from WebRtcVideoEngine2 to ViEEncoder.

BUG=webrtc:6371
R=mflodman@webrtc.org, sprang@webrtc.org

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

[commit-bot: I haz the power, 2016-09-29 09:49:06]

Description was changed from

==========
Add VideoSendStream::Stats::prefered_media_bitrate_bps

This cl move calculation of stats for prefered_media_bitrate_bps from
webrtcvideoengine2.GetStats to SendStatisticsProxy::OnEncoderReconfigured.
This aligns better with how other send stats are reported and is needed as a
prerequisite for moving video encoder configuration due to video resolution
change
from WebRtcVideoEngine2 to ViEEncoder.

BUG=webrtc:6371
R=mflodman@webrtc.org, sprang@webrtc.org

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

to

==========
Add VideoSendStream::Stats::prefered_media_bitrate_bps

This cl move calculation of stats for prefered_media_bitrate_bps from
webrtcvideoengine2.GetStats to SendStatisticsProxy::OnEncoderReconfigured.
This aligns better with how other send stats are reported and is needed as a
prerequisite for moving video encoder configuration due to video resolution
change
from WebRtcVideoEngine2 to ViEEncoder.

BUG=webrtc:6371
R=mflodman@webrtc.org, sprang@webrtc.org

Committed: https://crrev.com/a48ddb763695dd6742fdcb6f0e5fa4f9818595e7
Cr-Commit-Position: refs/heads/master@{#14431}
==========

[perkj_webrtc, 2016-09-29 09:49:07]

Committed patchset #8 (id:160001) manually as
a48ddb763695dd6742fdcb6f0e5fa4f9818595e7 (presubmit successful).