Prevent RTCP SR to be sent with bogus timestamp.

Prevent RTCP SR to be sent with bogus timestamp.

This CL makes sure no RTCP SR is sent before there is a valid timestamp
to set in the SR, based on the first sent media packet.

BUG=webrtc:1600
R=stefan@webrtc.org

Committed: https://crrev.com/0b3d7eec07100a9df006e679408a8e015af643d6
Cr-Commit-Position: refs/heads/master@{#10964}

[mflodman, 2015-12-10 06:08:19]

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

[mflodman, 2015-12-10 06:08:20]

Stefan,
WDYT?

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

https://codereview.webrtc.org/1506103006/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:410: if
(rtcp_sender_.TimeToSendRTCPReport(kVideoFrameKey == frame_type)) {
This will trigger the first SR, but with a correct timestamp. Do we want to
avoid sending here too before the first media frame is sent?

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

https://codereview.webrtc.org/1506103006/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:357: //
sender_.impl_->Process();
Depending on the answer to the other comment, I'll remove this or add an extra
check that no SR is sent with SendFrame().

[stefan-webrtc, 2015-12-10 08:05:40]

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

https://codereview.webrtc.org/1506103006/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:410: if
(rtcp_sender_.TimeToSendRTCPReport(kVideoFrameKey == frame_type)) {
On 2015/12/10 06:08:20, mflodman wrote:
> This will trigger the first SR, but with a correct timestamp. Do we want to
> avoid sending here too before the first media frame is sent?

I think this should be fine. Could you add a comment here describing why we send
an RTCP right before we send our first media packet?

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

https://codereview.webrtc.org/1506103006/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:349:
TEST_F(RtpRtcpImplTest, NoSrBeforeMedia) {
Do you think there's anything else to test, such as that RR still will be sent
even if we haven't sent rtp packets? Maybe we already have a test that verifies
this?

[mflodman, 2015-12-10 08:59:53]

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

https://codereview.webrtc.org/1506103006/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:410: if
(rtcp_sender_.TimeToSendRTCPReport(kVideoFrameKey == frame_type)) {
On 2015/12/10 08:05:40, stefan-webrtc (holmer) wrote:
> On 2015/12/10 06:08:20, mflodman wrote:
> > This will trigger the first SR, but with a correct timestamp. Do we want to
> > avoid sending here too before the first media frame is sent?
> 
> I think this should be fine. Could you add a comment here describing why we
send
> an RTCP right before we send our first media packet?

I added a general comment why this is done for the old code here, but no
explicit for the first RTCP report.

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

https://codereview.webrtc.org/1506103006/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:349:
TEST_F(RtpRtcpImplTest, NoSrBeforeMedia) {
On 2015/12/10 08:05:40, stefan-webrtc (holmer) wrote:
> Do you think there's anything else to test, such as that RR still will be sent
> even if we haven't sent rtp packets? Maybe we already have a test that
verifies
> this?

I added a test verifying the receive module still is sending RR.

[stefan-webrtc, 2015-12-10 09:03:19]

lgtm

[mflodman, 2015-12-10 09:10:59]

Description was changed from

==========
Prevent RTCP SR to be sent with bogus timestamp.

This CL makes sure no RTCP SR is sent before there is a valid timestamp
to set in the SR, based on the first sent media packet.

BUG=webrtc:1600
==========

to

==========
Prevent RTCP SR to be sent with bogus timestamp.

This CL makes sure no RTCP SR is sent before there is a valid timestamp
to set in the SR, based on the first sent media packet.

BUG=webrtc:1600
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/0b3d7eec07100a9df006e6794...
==========

[mflodman, 2015-12-10 09:11:00]

Committed patchset #2 (id:20001) manually as
0b3d7eec07100a9df006e679408a8e015af643d6 (presubmit successful).

[commit-bot: I haz the power, 2015-12-10 09:11:01]

Description was changed from

==========
Prevent RTCP SR to be sent with bogus timestamp.

This CL makes sure no RTCP SR is sent before there is a valid timestamp
to set in the SR, based on the first sent media packet.

BUG=webrtc:1600
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/0b3d7eec07100a9df006e6794...
==========

to

==========
Prevent RTCP SR to be sent with bogus timestamp.

This CL makes sure no RTCP SR is sent before there is a valid timestamp
to set in the SR, based on the first sent media packet.

BUG=webrtc:1600
R=stefan@webrtc.org

Committed: https://crrev.com/0b3d7eec07100a9df006e679408a8e015af643d6
Cr-Commit-Position: refs/heads/master@{#10964}
==========

[commit-bot: I haz the power, 2015-12-10 09:11:01]

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