Let FlexfecReceiveStreamImpl send RTCP RRs.

Let FlexfecReceiveStreamImpl send RTCP RRs.

This CL adds an RTP module to FlexfecReceiveStreamImpl, and wires it up
to send RTCP RRs. It further makes some methods take const refs instead
of values, to make it more clear where packet copies are made. This
change reduces the number of copies by one, for the case when media
packets are added to the FlexFEC receiver.

The end-to-end test is modified to check for RTCP RRs being sent.
Part of this modification involves some indentation changes, and the
diff thus looks bigger than it logically is.

BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2625633003
Cr-Commit-Position: refs/heads/master@{#16106}
Committed: https://chromium.googlesource.com/external/webrtc/+/fa5a368b3cc9625153c6f10c30dbccd70e56357c

[brandtr, 2017-01-10 11:43:39]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-01-10 11:57:51]

brandtr@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[brandtr, 2017-01-10 11:57:52]

Please take a look.

holmer: General.
danilchap: Usage of RTP module.

Note that the wire-up in WebRtcVideoEngine2 sets the local_ssrc for FlexFEC to
be the same as for media. (*) IIUC, that means that the RTCP receiver at the
sender side will happily receive the RRs for FlexFEC as well. Is this correct
thinking?

(*) This is intentionally not the case in the E2E test.

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.h (right):

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.h:21: class FlexfecReceiver;
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

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

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:761: Action OnReceiveRtcp(const uint8_t* data,
size_t length) override {
This method is new.

[danilchap, 2017-01-10 13:48:17]

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.cc:111: configuration.audio = false;
add configuration.clock = Clock::GetRealTimeClock();
though CreateRtpRtcp would add that line itself, it is better to control clock
you are using, to ensure it is same clock you pass to ReceiveStatistics::Create

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

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:718: static constexpr uint32_t kVideoLocalSsrc
= 123;
constants above constructors

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:767: uint32_t rtcp_sender_ssrc =
ByteReader<uint32_t>::ReadBigEndian(&data[4]);
avoid reimplmenting (simplifed) rtcp parser:
in test code you can use RtcpPacketParser from test/rtcp_packet_parser.h

it has sender_ssrc() accessor

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:773: uint32_t reported_ssrc =
ByteReader<uint32_t>::ReadBigEndian(&data[8]);
test_parser->receiver_report()->report_blocks().front().source_ssrc()
with some natural expectation above to have nice error messages if something
will be wrong.

[brandtr, 2017-01-11 09:52:47]

Patchset #2 (id:40001) has been deleted

[brandtr, 2017-01-11 09:55:24]

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/call/flexfec_recei...
File webrtc/call/flexfec_receive_stream_impl.cc (right):

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/call/flexfec_recei...
webrtc/call/flexfec_receive_stream_impl.cc:111: configuration.audio = false;
On 2017/01/10 13:48:17, danilchap wrote:
> add configuration.clock = Clock::GetRealTimeClock();
> though CreateRtpRtcp would add that line itself, it is better to control clock
> you are using, to ensure it is same clock you pass to
ReceiveStatistics::Create

Done.

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

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:718: static constexpr uint32_t kVideoLocalSsrc
= 123;
On 2017/01/10 13:48:17, danilchap wrote:
> constants above constructors

Done.

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:767: uint32_t rtcp_sender_ssrc =
ByteReader<uint32_t>::ReadBigEndian(&data[4]);
On 2017/01/10 13:48:17, danilchap wrote:
> avoid reimplmenting (simplifed) rtcp parser:
> in test code you can use RtcpPacketParser from test/rtcp_packet_parser.h
> 
> it has sender_ssrc() accessor

Done. See below.

https://codereview.webrtc.org/2625633003/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:773: uint32_t reported_ssrc =
ByteReader<uint32_t>::ReadBigEndian(&data[8]);
On 2017/01/10 13:48:17, danilchap wrote:
> test_parser->receiver_report()->report_blocks().front().source_ssrc()
> with some natural expectation above to have nice error messages if something
> will be wrong.

Hmm, OK. I did have a look at that class before writing this code, but it looked
to me that it was only about counting stuff. But now when I look more carefully,
I see how the inheritance is set up.

Done.

[brandtr, 2017-01-11 10:47:52]

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

https://codereview.webrtc.org/2625633003/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:771: if (!report_blocks.empty()) {
This check is needed because the newly created RTP module spits out one or two
empty RTCP packets, before sending proper RTCP RRs for FlexFEC.

These empty RTCP packets seem to be sent even if no RTP statistics are provided
to the RTP module. Is this correct/reasonable behaviour of the RTP module?

[danilchap, 2017-01-11 12:13:07]

usage of rtp module lgtm

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

https://codereview.webrtc.org/2625633003/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:766: received_flexfec_rtcp_ = true;
probably set to true only if (!report_blocks.empty())

https://codereview.webrtc.org/2625633003/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:771: if (!report_blocks.empty()) {
On 2017/01/11 10:47:52, brandtr wrote:
> This check is needed because the newly created RTP module spits out one or two
> empty RTCP packets, before sending proper RTCP RRs for FlexFEC.
> 
> These empty RTCP packets seem to be sent even if no RTP statistics are
provided
> to the RTP module. Is this correct/reasonable behaviour of the RTP module?

should be correct, not so sure if reasonable.

quote from rfc3550 section 6.4.2: "An empty RR packet (RC = 0) MUST be put at
the head of a compound RTCP packet when there is no data transmission or
reception to report."

[brandtr, 2017-01-11 12:25:36]

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

https://codereview.webrtc.org/2625633003/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:766: received_flexfec_rtcp_ = true;
On 2017/01/11 12:13:07, danilchap wrote:
> probably set to true only if (!report_blocks.empty())

Yes, this is better, as it requires the EXPECTs below to always run at least
once.

https://codereview.webrtc.org/2625633003/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:771: if (!report_blocks.empty()) {
On 2017/01/11 12:13:07, danilchap wrote:
> On 2017/01/11 10:47:52, brandtr wrote:
> > This check is needed because the newly created RTP module spits out one or
two
> > empty RTCP packets, before sending proper RTCP RRs for FlexFEC.
> > 
> > These empty RTCP packets seem to be sent even if no RTP statistics are
> provided
> > to the RTP module. Is this correct/reasonable behaviour of the RTP module?
> 
> should be correct, not so sure if reasonable.
> 
> quote from rfc3550 section 6.4.2: "An empty RR packet (RC = 0) MUST be put at
> the head of a compound RTCP packet when there is no data transmission or
> reception to report."

Acknowledged.

[brandtr, 2017-01-12 15:04:12]

Rebase.

[brandtr, 2017-01-16 10:26:17]

Rebase.

[stefan-webrtc, 2017-01-16 13:20:55]

lgtm

[brandtr, 2017-01-16 15:17:52]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-01-16 15:17:52]

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/2625633003/#ps120001 (title:
"Rebase.")

[commit-bot: I haz the power, 2017-01-16 15:17:58]

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-01-17 07:56:24]

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

[commit-bot: I haz the power, 2017-01-17 07:56:27]

Failed to apply patch for webrtc/video/video_quality_test.cc:
While running git apply --index -p1;
  error: patch failed: webrtc/video/video_quality_test.cc:1122
  error: webrtc/video/video_quality_test.cc: patch does not apply

Patch:       webrtc/video/video_quality_test.cc
Index: webrtc/video/video_quality_test.cc
diff --git a/webrtc/video/video_quality_test.cc
b/webrtc/video/video_quality_test.cc
index
e37b555f7e35bc1eb717f90dea3ce4b658ab8d18..6962ca1520656b5e7e9f3dc1bbcccae40272b65c
100644
--- a/webrtc/video/video_quality_test.cc
+++ b/webrtc/video/video_quality_test.cc
@@ -1122,6 +1122,7 @@ void VideoQualityTest::SetupVideo(Transport*
send_transport,
         video_send_config_.rtp.flexfec.flexfec_ssrc;
     flexfec_receive_config.protected_media_ssrcs =
         video_send_config_.rtp.flexfec.protected_media_ssrcs;
+    flexfec_receive_config.local_ssrc = kReceiverLocalVideoSsrc;
     flexfec_receive_config.transport_cc = params_.call.send_side_bwe;
     if (params_.call.send_side_bwe) {
       flexfec_receive_config.rtp_header_extensions.push_back(

[brandtr, 2017-01-17 08:21:58]

Rebase.

[brandtr, 2017-01-17 08:25:43]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-01-17 08:25:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2625633003/#ps140001 (title:
"Rebase.")

[commit-bot: I haz the power, 2017-01-17 08:25:54]

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-01-17 09:33:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484641543345760,
"parent_rev": "9506e12eba15c18b013cd9f6c28aba10ad5ace63", "commit_rev":
"fa5a368b3cc9625153c6f10c30dbccd70e56357c"}

[commit-bot: I haz the power, 2017-01-17 09:33:58]

Description was changed from

==========
Let FlexfecReceiveStreamImpl send RTCP RRs.

This CL adds an RTP module to FlexfecReceiveStreamImpl, and wires it up
to send RTCP RRs. It further makes some methods take const refs instead
of values, to make it more clear where packet copies are made. This
change reduces the number of copies by one, for the case when media
packets are added to the FlexFEC receiver.

The end-to-end test is modified to check for RTCP RRs being sent.
Part of this modification involves some indentation changes, and the
diff thus looks bigger than it logically is.

BUG=webrtc:5654
==========

to

==========
Let FlexfecReceiveStreamImpl send RTCP RRs.

This CL adds an RTP module to FlexfecReceiveStreamImpl, and wires it up
to send RTCP RRs. It further makes some methods take const refs instead
of values, to make it more clear where packet copies are made. This
change reduces the number of copies by one, for the case when media
packets are added to the FlexFEC receiver.

The end-to-end test is modified to check for RTCP RRs being sent.
Part of this modification involves some indentation changes, and the
diff thus looks bigger than it logically is.

BUG=webrtc:5654

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

[commit-bot: I haz the power, 2017-01-17 09:33:59]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/fa5a368b3cc9625153c6f10c3...