Take smaller interface for RtpRtcp::Configuration::receive_statistics

Take smaller interface for RtpRtcp::Configuration::receive_statistics

BUG=webrtc:8016
Review-Url: https://codereview.webrtc.org/2988763002
Cr-Commit-Position: refs/heads/master@{#19167}
Committed: https://chromium.googlesource.com/external/webrtc/+/f5f793c2ed4e85253ca5c6171a3988d2ba587800

[danilchap, 2017-07-25 16:36:04]

Description was changed from

==========
Take smaller interface for RtpRtcp::Configuration::receive_statistics

BUG=webrtc:8601
==========

to

==========
Take smaller interface for RtpRtcp::Configuration::receive_statistics

BUG=webrtc:8016
==========

[danilchap, 2017-07-26 11:00:48]

Patchset #1 (id:1) has been deleted

[danilchap, 2017-07-26 12:14:15]

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

[eladalon, 2017-07-27 09:07:31]

lgtm

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc (right):

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:518: LOG(LS_WARNING)
<< "Cumulative lost is oversized.";
Would logging the value be useful?

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

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: result =
receive_statistics_->RtcpReportBlocks(RTCP_MAX_REPORT_BLOCKS);
nit: Consider not passing this RFC-defined constant, but rather having the
function aware of it itself.

[danilchap, 2017-07-27 10:04:32]

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc (right):

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:518: LOG(LS_WARNING)
<< "Cumulative lost is oversized.";
On 2017/07/27 09:07:31, eladalon wrote:
> Would logging the value be useful?

may be. I have a TODO in ReceiveStatistics to handle oversized values better,
but since it is theoretical issue so far, didn't put much thoughts into it.
(i.e. it is out of scope of this CL)

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

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: result =
receive_statistics_->RtcpReportBlocks(RTCP_MAX_REPORT_BLOCKS);
On 2017/07/27 09:07:31, eladalon wrote:
> nit: Consider not passing this RFC-defined constant, but rather having the
> function aware of it itself.

I did that initially, but then introduce this parameter instead for two reasons.
a) One of the shortly-coming CLs plan to call RtcpReportBlocks function passing
1 instead of the constant (it is only test code, but still).
b) In the future, while resolving TODO just above the line, RtcpSender will pass
different values. Those might depend on mtu, presense of other rtcp message,
etc, i.e. responsibility of rtcp sender, but not statistic calculator.

[danilchap, 2017-07-27 10:04:37]

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

[commit-bot: I haz the power, 2017-07-27 10:04:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[eladalon, 2017-07-27 10:40:18]

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

https://codereview.webrtc.org/2988763002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: result =
receive_statistics_->RtcpReportBlocks(RTCP_MAX_REPORT_BLOCKS);
On 2017/07/27 10:04:32, danilchap wrote:
> On 2017/07/27 09:07:31, eladalon wrote:
> > nit: Consider not passing this RFC-defined constant, but rather having the
> > function aware of it itself.
> 
> I did that initially, but then introduce this parameter instead for two
reasons.
> a) One of the shortly-coming CLs plan to call RtcpReportBlocks function
passing
> 1 instead of the constant (it is only test code, but still).
> b) In the future, while resolving TODO just above the line, RtcpSender will
pass
> different values. Those might depend on mtu, presense of other rtcp message,
> etc, i.e. responsibility of rtcp sender, but not statistic calculator.

Acknowledged.

[commit-bot: I haz the power, 2017-07-27 11:09:46]

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

[commit-bot: I haz the power, 2017-07-27 11:09:47]

Dry run: This issue passed the CQ dry run.

[danilchap, 2017-07-27 11:41:47]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2017-07-27 11:41:57]

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, 2017-07-27 11:44:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1501155707296960,
"parent_rev": "77415f561d9fe96c2c1149e9f59735d1ebe8f34e", "commit_rev":
"f5f793c2ed4e85253ca5c6171a3988d2ba587800"}

[commit-bot: I haz the power, 2017-07-27 11:44:24]

Description was changed from

==========
Take smaller interface for RtpRtcp::Configuration::receive_statistics

BUG=webrtc:8016
==========

to

==========
Take smaller interface for RtpRtcp::Configuration::receive_statistics

BUG=webrtc:8016

Review-Url: https://codereview.webrtc.org/2988763002
Cr-Commit-Position: refs/heads/master@{#19167}
Committed:
https://chromium.googlesource.com/external/webrtc/+/f5f793c2ed4e85253ca5c6171...
==========

[commit-bot: I haz the power, 2017-07-27 11:44:25]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/f5f793c2ed4e85253ca5c6171...