Wire up rtcp xr target bitrate on receive side.

Wire up rtcp xr target bitrate on receive side.

BUG=webrtc:6301
Committed: https://crrev.com/a790d834c90bf7d29d67717ce9f0a2ad479974a6
Cr-Commit-Position: refs/heads/master@{#15388}

[sprang_webrtc, 2016-12-01 16:18:11]

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

[danilchap, 2016-12-01 17:05:41]

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:694: // TODO(sprang): Add VoIP
metrics?
do we have any part of code that might be interested in it?
probably no point to plan readding it until we do.

May it would make more sense to also remove voip metric from sending side, since
it is unused there too. (like non-implemented blocks of extended reports)

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:698:
packet_information->packet_type_flags |= kRtcpXrReceiverReferenceTime;
kRtcpXrReceiverReferenceTime is not used in the packet_type_flags, so no need to
set it.
packet_information use private class (i.e. implementation detail of the
RTCPReceiver), so no need to worry about public use.

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:704:
packet_information->packet_type_flags |= kRtcpXrDlrrReportBlock;
ditto

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:709:
packet_information->packet_type_flags |= kRtcpXrTargetBitrate;
ditto

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:750:
bitrate_allocation_.emplace(std::move(bitrate_allocation));
copy constructor is as expensive as move constructor for BitrateAllocation, so
no need to move it.

if you want to avoid copy you can do other way around:
bitrate_allocate_.emplace();
for (const auto& item : ) {
  bitrate_allocation_->SetBitrate(item.sl, item.tl, item.bitrate);
}

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1034:
bitrate_allocation_observer_->OnBitrateAllocationUpdated(
Do you want to call OnBitrateAllocationUpdated every time some rtcp packet
received, or only on rtcp packet with (new) bitrate_allocation?

In second case you likely want to move bitrate_allocation_ member from
RtcpReceiver to RtcpReceiver::PacketInformation

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.h (right):

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.h:219: rtc::CriticalSection
feedbacks_lock_;
Thank you!

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc (right):

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:1297:
InjectRtcpPacket(xr);
May be inject one more rtcp packet after this one, without TargetBitrate.
Likely, as mentioned in comment above, you wouldn't expect a second call to
bitrate_allocation_observer.

[sprang_webrtc, 2016-12-02 09:28:45]

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:694: // TODO(sprang): Add VoIP
metrics?
On 2016/12/01 17:05:40, danilchap wrote:
> do we have any part of code that might be interested in it?
> probably no point to plan readding it until we do.
> 
> May it would make more sense to also remove voip metric from sending side,
since
> it is unused there too. (like non-implemented blocks of extended reports)

Hm, ok. Shall I add a todo in the send side about removing instead?

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:698:
packet_information->packet_type_flags |= kRtcpXrReceiverReferenceTime;
On 2016/12/01 17:05:41, danilchap wrote:
> kRtcpXrReceiverReferenceTime is not used in the packet_type_flags, so no need
to
> set it.
> packet_information use private class (i.e. implementation detail of the
> RTCPReceiver), so no need to worry about public use.

Done.

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:704:
packet_information->packet_type_flags |= kRtcpXrDlrrReportBlock;
On 2016/12/01 17:05:40, danilchap wrote:
> ditto

Done.

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:709:
packet_information->packet_type_flags |= kRtcpXrTargetBitrate;
On 2016/12/01 17:05:40, danilchap wrote:
> ditto

Done.

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:750:
bitrate_allocation_.emplace(std::move(bitrate_allocation));
On 2016/12/01 17:05:40, danilchap wrote:
> copy constructor is as expensive as move constructor for BitrateAllocation, so
> no need to move it.
> 
> if you want to avoid copy you can do other way around:
> bitrate_allocate_.emplace();
> for (const auto& item : ) {
>   bitrate_allocation_->SetBitrate(item.sl, item.tl, item.bitrate);
> }

I forgot to remove the move when I changed from an earlier impl, will just
remove that. I think it's more readable this way.

https://codereview.webrtc.org/2540363003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1034:
bitrate_allocation_observer_->OnBitrateAllocationUpdated(
On 2016/12/01 17:05:40, danilchap wrote:
> Do you want to call OnBitrateAllocationUpdated every time some rtcp packet
> received, or only on rtcp packet with (new) bitrate_allocation?
> 
> In second case you likely want to move bitrate_allocation_ member from
> RtcpReceiver to RtcpReceiver::PacketInformation

Whoops. I was debating with myself whether to have it as an optional in the main
class (next to the other xr members) and reset it after the callback, or if I
should move it to packet information. I forget the reset and now have the worst
of both worlds. Thanks for catching!

[danilchap, 2016-12-02 11:01:14]

lgtm with a nit.

https://codereview.webrtc.org/2540363003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2540363003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:698: if
(!xr.dlrr().sub_blocks().empty()) {
can remove back if (!empty()) block too

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

https://codereview.webrtc.org/2540363003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:967: // TODO(sprang): Remove
support for VoIP metrics? (Not supported in receiver.)
Probably better reason: 'not used'.
(e.g. there is an rtcp message we support in receiver but not in sender. In case
other implementation of webrtc sends it.)

[sprang_webrtc, 2016-12-02 12:20:11]

https://codereview.webrtc.org/2540363003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2540363003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:698: if
(!xr.dlrr().sub_blocks().empty()) {
On 2016/12/02 11:01:14, danilchap wrote:
> can remove back if (!empty()) block too

Done.

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

https://codereview.webrtc.org/2540363003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:967: // TODO(sprang): Remove
support for VoIP metrics? (Not supported in receiver.)
On 2016/12/02 11:01:14, danilchap wrote:
> Probably better reason: 'not used'.
> (e.g. there is an rtcp message we support in receiver but not in sender. In
case
> other implementation of webrtc sends it.)

Done.

[sprang_webrtc, 2016-12-02 12:20:16]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-12-02 12:20:16]

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/2540363003/#ps40001 (title:
"Nits")

[commit-bot: I haz the power, 2016-12-02 12:20:22]

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-02 12:26:36]

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

[commit-bot: I haz the power, 2016-12-02 12:26:36]

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

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

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

[commit-bot: I haz the power, 2016-12-02 13:43:36]

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-02 14:00:26]

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

[commit-bot: I haz the power, 2016-12-02 14:00:26]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2016-12-02 14:07:15]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-12-02 14:07:15]

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/2540363003/#ps80001 (title:
"Actual fuzzer fix")

[commit-bot: I haz the power, 2016-12-02 14:07:23]

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-02 14:12:09]

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

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

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

[sprang_webrtc, 2016-12-02 14:14:27]

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

[sprang_webrtc, 2016-12-02 14:14:27]

+stefan as owner for 	webrtc/test/fuzzers/rtcp_receiver_fuzzer.cc

[stefan-webrtc, 2016-12-02 15:27:08]

lgtm

[sprang_webrtc, 2016-12-02 15:28:00]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2016-12-02 15:28:09]

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-02 15:29:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480692480895330,
"parent_rev": "6cda6e4744217678ba951e26fe8f2ccc17dde91e", "commit_rev":
"02aa2292dd44e417cf16146860d19bafdfa19fe1"}

[commit-bot: I haz the power, 2016-12-02 15:29:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-02 15:29:56]

Description was changed from

==========
Wire up rtcp xr target bitrate on receive side.

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

to

==========
Wire up rtcp xr target bitrate on receive side.

BUG=webrtc:6301

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

[commit-bot: I haz the power, 2016-12-02 15:29:58]

Patchset 5 (id:??) landed as
https://crrev.com/a790d834c90bf7d29d67717ce9f0a2ad479974a6
Cr-Commit-Position: refs/heads/master@{#15388}