Wire up RTCP XR target bitrate in rtp/rtcp module

Wire up RTCP XR target bitrate in rtp/rtcp module

This is breakout of the rtcp parts of
https://codereview.webrtc.org/2531383002/

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

[sprang_webrtc, 2016-12-01 10:46:38]

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

[danilchap, 2016-12-01 11:52:05]

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

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:49: }
add comment // namespace

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:998: if (type &
kRtcpAnyExtendedReports) {
probably safer to move this check into SetFlag instead.

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1030: }
may be SetFlag(kRtcpAnyExtendedReports, true). (similar to SetVoip function)
then it might be sent before it is time to generate a sender report.

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

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:844:
EXPECT_EQ(static_cast<size_t>(kNumSpatialLayers * kNumTemporalLayers),
may be use size_t instead of int for kNumSpatialLayers/kNumTemporalLayers/their
indexes. cast here can be removed then.
(besides, size_t is recommended type for indexes)

[sprang_webrtc, 2016-12-01 12:31:01]

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

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:49: }
On 2016/12/01 11:52:05, danilchap wrote:
> add comment // namespace

Done.

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:998: if (type &
kRtcpAnyExtendedReports) {
On 2016/12/01 11:52:05, danilchap wrote:
> probably safer to move this check into SetFlag instead.

Done.

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:1030: }
On 2016/12/01 11:52:05, danilchap wrote:
> may be SetFlag(kRtcpAnyExtendedReports, true). (similar to SetVoip function)
> then it might be sent before it is time to generate a sender report.

Done.

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

https://codereview.webrtc.org/2546713002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:844:
EXPECT_EQ(static_cast<size_t>(kNumSpatialLayers * kNumTemporalLayers),
On 2016/12/01 11:52:05, danilchap wrote:
> may be use size_t instead of int for
kNumSpatialLayers/kNumTemporalLayers/their
> indexes. cast here can be removed then.
> (besides, size_t is recommended type for indexes)

Done.

[danilchap, 2016-12-01 12:32:54]

lgtm

[sprang_webrtc, 2016-12-01 12:51:45]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2016-12-01 12:51:55]

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 13:18:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480596705742660,
"parent_rev": "69d26f73fa0090b4f63ff08df31e2224fe68eb04", "commit_rev":
"44650fda895dea986637a09f91d161273ca75a86"}

[commit-bot: I haz the power, 2016-12-01 13:18:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-01 13:18:26]

Description was changed from

==========
Wire up RTCP XR target bitrate in rtp/rtcp module

This is breakout of the rtcp parts of
https://codereview.webrtc.org/2531383002/

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

to

==========
Wire up RTCP XR target bitrate in rtp/rtcp module

This is breakout of the rtcp parts of
https://codereview.webrtc.org/2531383002/

BUG=webrtc:6301

Committed: https://crrev.com/5e38c967e0df72129e090a3ec2912b8f89e7a061
Cr-Commit-Position: refs/heads/master@{#15358}
==========

[commit-bot: I haz the power, 2016-12-01 13:18:27]

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

[stefan-webrtc, 2016-12-01 14:28:29]

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

[stefan-webrtc, 2016-12-01 14:28:30]

I just want to finish the discussion, better do it here than in the previous CL.

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

https://codereview.webrtc.org/2546713002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:860: 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.

But previously we had separate functions for each extended report type and just
tied them together in the map. There was not a single BuildExtendedReports, and
therefore only a 1:1 mapping in the map. Does that no longer make sense?

[sprang_webrtc, 2016-12-01 14:38:50]

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

https://codereview.webrtc.org/2546713002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:860: feedback_state.has_last_xr_rr
|| video_bitrate_allocation_) {
On 2016/12/01 14:28:30, stefan-webrtc (holmer) wrote:
> > 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.
> 
> But previously we had separate functions for each extended report type and
just
> tied them together in the map. There was not a single BuildExtendedReports,
and
> therefore only a 1:1 mapping in the map. Does that no longer make sense?

Not really. Since the voip part isn't really used, rrtr and dlrr were basically
the only important ones and they go in opposite directions. With target bitrate
added, we would have the possibility of two different xr in the same direction.
Apart from the packet size inefficiency of sending two separate xr, this caused
problems with testing which relied on capturing the last xr sent/received which
would have necessitated a lot of refactoring in the tests in order to avoid
raciness.

[stefan-webrtc, 2016-12-01 14:43:18]

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

https://codereview.webrtc.org/2546713002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:860: feedback_state.has_last_xr_rr
|| video_bitrate_allocation_) {
On 2016/12/01 14:38:50, språng wrote:
> On 2016/12/01 14:28:30, stefan-webrtc (holmer) wrote:
> > > 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.
> > 
> > But previously we had separate functions for each extended report type and
> just
> > tied them together in the map. There was not a single BuildExtendedReports,
> and
> > therefore only a 1:1 mapping in the map. Does that no longer make sense?
> 
> Not really. Since the voip part isn't really used, rrtr and dlrr were
basically
> the only important ones and they go in opposite directions. With target
bitrate
> added, we would have the possibility of two different xr in the same
direction.
> Apart from the packet size inefficiency of sending two separate xr, this
caused
> problems with testing which relied on capturing the last xr sent/received
which
> would have necessitated a lot of refactoring in the tests in order to avoid
> raciness.

Ok :)