Wire up BitrateAllocation to be sent as RTCP TargetBitrate

Wire up BitrateAllocation to be sent as RTCP TargetBitrate

BUG=webrtc:6301

[sprang_webrtc, 2016-11-28 17:23:49]

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

[commit-bot: I haz the power, 2016-11-28 17:24:00]

Dry run: 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-11-28 17:29:49]

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

[commit-bot: I haz the power, 2016-11-28 17:29:50]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/13537)

[sprang_webrtc, 2016-11-28 17:48:45]

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

[commit-bot: I haz the power, 2016-11-28 17:48:55]

Dry run: 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-11-28 17:54:03]

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

[commit-bot: I haz the power, 2016-11-28 17:54:04]

Dry run: Try jobs failed on following builders:
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19766)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/20041)

[sprang_webrtc, 2016-11-28 18:34:01]

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

[commit-bot: I haz the power, 2016-11-28 18:34:06]

Dry run: 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-11-28 18:45:16]

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

[commit-bot: I haz the power, 2016-11-28 18:45:17]

Dry run: Try jobs failed on following builders:
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17402)

[sprang_webrtc, 2016-11-29 09:26:37]

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

[commit-bot: I haz the power, 2016-11-29 09:26:46]

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-11-29 09:44:27]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[danilchap, 2016-11-29 10:39:32]

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1018:
std::unique_ptr<rtcp::RtcpPacket> RTCPSender::BuildTargetBitrate(
move this methods to other Builders, after BuildDlrr
(function order in .h and .cc mismatch a lot, but better try to have them same)

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1021: 
It became more important now to merge different XR blocks into single XR.
Dlrr and Rrtr shouldn't be in same report since one is used by sender, another
one - by receiver.
but target_bitrate and dlrr can be in same report.

Better not to do it in this CL, but add a TODO.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1036: video_bitrate_allocation_ =
rtc::Optional<BitrateAllocation>();
may be video_bitrate_allocation_.reset();

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1037: return
std::unique_ptr<rtcp::RtcpPacket>(xr.release());
return std::move(xr); should work
alternatively make xr a raw pointer

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1042: video_bitrate_allocation_ =
rtc::Optional<BitrateAllocation>(bitrate);
video_bitrate_allocation_.emplace(bitrate); will work too.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:272:
rtc::Optional<BitrateAllocation> video_bitrate_allocation_
#include "webrtc/base/optional.h"

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocationObserver*
bitrate_observer) {
bitrate_observer used as callback (called during this function, not used after
function is finished)
not like an observer (stored somewhere and called periodically as result of
different function calls)
may be rename the variable to to include callback instead of observer?

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2438: void
EndToEndTest::TestXrReceiverReferenceTimeReport(bool enable_rrtr) {
rename or better fork test since now it is used to track target_bitrate too

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2489: EXPECT_GT(sent_rtcp_rrtr_, 0);
Yep, no need to write expected value first this days. Can now order parameters
of EXPECT_* macros in a nicer way :)

[sprang_webrtc, 2016-11-29 12:24:01]

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1018:
std::unique_ptr<rtcp::RtcpPacket> RTCPSender::BuildTargetBitrate(
On 2016/11/29 10:39:31, danilchap wrote:
> move this methods to other Builders, after BuildDlrr
> (function order in .h and .cc mismatch a lot, but better try to have them
same)

Done.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1021: 
On 2016/11/29 10:39:31, danilchap wrote:
> It became more important now to merge different XR blocks into single XR.
> Dlrr and Rrtr shouldn't be in same report since one is used by sender, another
> one - by receiver.
> but target_bitrate and dlrr can be in same report.
> 
> Better not to do it in this CL, but add a TODO.

Agree. I actually had a todo about that here but removed it for some reason I
can't remember.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1036: video_bitrate_allocation_ =
rtc::Optional<BitrateAllocation>();
On 2016/11/29 10:39:31, danilchap wrote:
> may be video_bitrate_allocation_.reset();

Done.
Nice, haven't seen that you added this and emplace. Did you send anything out
about this? :)

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1037: return
std::unique_ptr<rtcp::RtcpPacket>(xr.release());
On 2016/11/29 10:39:31, danilchap wrote:
> return std::move(xr); should work
> alternatively make xr a raw pointer

Done.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1042: video_bitrate_allocation_ =
rtc::Optional<BitrateAllocation>(bitrate);
On 2016/11/29 10:39:31, danilchap wrote:
> video_bitrate_allocation_.emplace(bitrate); will work too.

Done.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:272:
rtc::Optional<BitrateAllocation> video_bitrate_allocation_
On 2016/11/29 10:39:31, danilchap wrote:
> #include "webrtc/base/optional.h"

Done.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:199: VideoBitrateAllocationObserver*
bitrate_observer) {
On 2016/11/29 10:39:31, danilchap wrote:
> bitrate_observer used as callback (called during this function, not used after
> function is finished)
> not like an observer (stored somewhere and called periodically as result of
> different function calls)
> may be rename the variable to to include callback instead of observer?

Done.

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2438: void
EndToEndTest::TestXrReceiverReferenceTimeReport(bool enable_rrtr) {
On 2016/11/29 10:39:32, danilchap wrote:
> rename or better fork test since now it is used to track target_bitrate too

I briefly explored that but it turned out messier than I would like. Prefer not
to spent too much time on this. Renamed the test though and moved out
RtcpXrObserver way down to where it is actually used.

[danilchap, 2016-11-29 13:13:14]

lgtm

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1036: video_bitrate_allocation_ =
rtc::Optional<BitrateAllocation>();
On 2016/11/29 12:24:01, språng wrote:
> On 2016/11/29 10:39:31, danilchap wrote:
> > may be video_bitrate_allocation_.reset();
> 
> Done.
> Nice, haven't seen that you added this and emplace. Did you send anything out
> about this? :)

nope, nothing except cl https://codereview.webrtc.org/2378113002/ where I used
reset and emplace

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2438: void
EndToEndTest::TestXrReceiverReferenceTimeReport(bool enable_rrtr) {
On 2016/11/29 12:24:01, språng wrote:
> On 2016/11/29 10:39:32, danilchap wrote:
> > rename or better fork test since now it is used to track target_bitrate too
> 
> I briefly explored that but it turned out messier than I would like. Prefer
not
> to spent too much time on this. Renamed the test though and moved out
> RtcpXrObserver way down to where it is actually used.

ok

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:743: return
std::unique_ptr<rtcp::RtcpPacket>(std::move(xr));
return std::move(xr);  should work,
no need for explicit cast to std::unique_ptr<rtcp::RtcpPacket> after the
std::move

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3019: EXPECT_LE(parser.xr()->num_packets(), 2);
 // dlrr and/or target bitrate.
fyi: the check Le(1) was here because test parser is not good with two packets
of the same type: last one overwrite first one (unlike in normal rtcp receiver
where both are properly processed).
Shouldn't be a problem if test pass.

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3068: TEST_P(EndToEndTest,
TestExtendedReportsWithDlrr) {
WithRrtr/WithoutRrtr instead of WithDlrr to match enable_rrtr_ and
receiver_reference_time_report (aka rrtr) flag in the config.

Dlrr is a response to Rrtr, not something you configure.

[sprang_webrtc, 2016-11-29 13:34:55]

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:743: return
std::unique_ptr<rtcp::RtcpPacket>(std::move(xr));
On 2016/11/29 13:13:14, danilchap wrote:
> return std::move(xr);  should work,
> no need for explicit cast to std::unique_ptr<rtcp::RtcpPacket> after the
> std::move

Done.

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3019: EXPECT_LE(parser.xr()->num_packets(), 2);
 // dlrr and/or target bitrate.
On 2016/11/29 13:13:14, danilchap wrote:
> fyi: the check Le(1) was here because test parser is not good with two packets
> of the same type: last one overwrite first one (unlike in normal rtcp receiver
> where both are properly processed).
> Shouldn't be a problem if test pass.

Acknowledged.

https://codereview.webrtc.org/2531383002/diff/80001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3068: TEST_P(EndToEndTest,
TestExtendedReportsWithDlrr) {
On 2016/11/29 13:13:14, danilchap wrote:
> WithRrtr/WithoutRrtr instead of WithDlrr to match enable_rrtr_ and
> receiver_reference_time_report (aka rrtr) flag in the config.
> 
> Dlrr is a response to Rrtr, not something you configure.

Oops, meant rrtr.

[sprang_webrtc, 2016-11-29 13:35:00]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-11-29 13:35:01]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2531383002/#ps100001 (title:
"Comments")

[commit-bot: I haz the power, 2016-11-29 13:35:10]

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-11-29 13:38:40]

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

[commit-bot: I haz the power, 2016-11-29 13:38:41]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10773)

[sprang_webrtc, 2016-11-29 15:14:02]

Seems the test turned flaky, and rewriting the test rtcp parser would be more
work than I cared for, so I refactored rtcp_sender to only send a single
extended report with populated with the appropriate members.

please have another look

[danilchap, 2016-11-29 15:54:06]

Nice!
lgtm

[sprang_webrtc, 2016-11-29 16:03:31]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2016-11-29 16:03:36]

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-11-29 16:07:05]

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

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

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10792)

[sprang_webrtc, 2016-11-30 08:54:55]

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

[sprang_webrtc, 2016-11-30 08:54:56]

+stefan as owner

[sprang_webrtc, 2016-11-30 15:10:04]

Found another issue: when using simulcast, there shall be one target bitrate
message sent per stream.
This is commented on in
webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc
Patch set 8 includes a fix in payload_router and additional test.

[stefan-webrtc, 2016-12-01 08:42:21]

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:108: kRtcpXrTargetBitrate
This looks a bit odd. Should this really belong to RTCPPacketType?

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:854: feedback_state.has_last_xr_rr
|| video_bitrate_allocation_) {
Seems like we now have two checks for this. First here we check if any extended
reports should be added, then in BuildExtendedReports we check each of these
again to see which of them should be added. Why not keep as it was and just
SetFlag on those which are available here?

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:85: uint8_t lossRate,
Please change to loss_rate

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:92: void
UpdateChannelParemeters(
I think it would be good to clarify what the difference is between this method
and SetChannelParameters. When should this be used and how does it differ from
the other one?

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_sender.cc:216:
bitrate_updated_callback->OnBitrateAllocationUpdated(bitrate_allocation);
You are holding params_crit_ when calling out here. Is that necessary? Doesn't
look like it since the crit isn't held when calling UpdateEncoderParameters from
SetChannelParameters.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3076: }
Should we add a test for the new target bitrates extended report too?

[sprang_webrtc, 2016-12-01 10:04:19]

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

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

Dry run: 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-01 10:05:36]

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

[commit-bot: I haz the power, 2016-12-01 10:05:37]

Dry run: Try jobs failed on following builders:
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/13022)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15367)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15227)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19304)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10916)

[danilchap, 2016-12-01 10:18:55]

This CL is growing big.
May be it is time to split it, e.g. changes to rtp_rtcp in one CL, changes to
video - to next one.

[sprang_webrtc, 2016-12-01 10:23:39]

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

[commit-bot: I haz the power, 2016-12-01 10:23:45]

Dry run: 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-01 10:37:56]

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

[commit-bot: I haz the power, 2016-12-01 10:37:57]

Dry run: Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/7432)

[sprang_webrtc, 2016-12-01 10:47:58]

I moved the rtcp parts to a new cl: https://codereview.webrtc.org/2546713002/
Will move the video parts to another one as well, without a rebase in the middle
:/
Let's close this one for now.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:108: kRtcpXrTargetBitrate
On 2016/12/01 08:42:20, stefan-webrtc (holmer) wrote:
> This looks a bit odd. Should this really belong to RTCPPacketType?

The problem is in rtcp sender, it becomes either rather complicated or we'll
have to change the type in a number of places from enum to uint32_t... I would
have liked to rather go the other way (to an enum class) in the not too distant
future. 

I'll change to an uint32_t for now and remove this. Let's keep the  refactorings
to separate cls.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:854: feedback_state.has_last_xr_rr
|| video_bitrate_allocation_) {
On 2016/12/01 08:42:20, stefan-webrtc (holmer) wrote:
> Seems like we now have two checks for this. First here we check if any
extended
> reports should be added, then in BuildExtendedReports we check each of these
> again to see which of them should be added. Why not keep as it was and just
> SetFlag on those which are available here?

Because we don't want to have separate builders for each type, creating
individual extended reports containing a single type. Any of them should trigger
just a single builder. If we set all the flags here, and map each of these types
to the same extended reports builder, we would call that multiple times, and
would then there have to check again if the various xr types have already been
built. Or we would have to do a concurrent modification of the set that's being
iterated over. Neither are very nice options.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:85: uint8_t lossRate,
On 2016/12/01 08:42:20, stefan-webrtc (holmer) wrote:
> Please change to loss_rate

Done.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.h:92: void
UpdateChannelParemeters(
On 2016/12/01 08:42:20, stefan-webrtc (holmer) wrote:
> I think it would be good to clarify what the difference is between this method
> and SetChannelParameters. When should this be used and how does it differ from
> the other one?

Done.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_sender.cc (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_sender.cc:216:
bitrate_updated_callback->OnBitrateAllocationUpdated(bitrate_allocation);
On 2016/12/01 08:42:20, stefan-webrtc (holmer) wrote:
> You are holding params_crit_ when calling out here. Is that necessary? Doesn't
> look like it since the crit isn't held when calling UpdateEncoderParameters
from
> SetChannelParameters.

I changed the one usage that made the crit useful instead, and removed the crit
entierly.

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2531383002/diff/140001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3076: }
On 2016/12/01 08:42:20, stefan-webrtc (holmer) wrote:
> Should we add a test for the new target bitrates extended report too?

sent_rtcp_target_bitrate_ is already part of RtcpXrObserver test?

[sprang_webrtc, 2016-12-01 10:48:07]

Description was changed from

==========
Wire up BitrateAllocation to be sent as RTCP TargetBitrate

BUG=webrtc:6301
==========

to

==========
Wire up BitrateAllocation to be sent as RTCP TargetBitrate

BUG=webrtc:6301
==========