Validates sending RTCP before RTP.

Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP packet.

Now it check if rtp timestamp can be calculating instead of checking number of rtp packets. This way it works for reconfigured streams too.

It also moved deeper into rtcp_sender class to prevent SR no matter the reason it need to be genereated. This way it prevents creating compound rtcp packets that have to start with Sender Report and Sender Reports as response to (mostly theoretical) sr-request rtcp packet.

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

Committed: https://crrev.com/70ffead25675c4761467755f9be844335dd59dba
Cr-Commit-Position: refs/heads/master@{#13503}

[danilchap, 2016-01-29 18:30:21]

Description was changed from

==========
Validates sending RTCP before RTP.

Added test that check rtp_timestamp synchronisation when rtcp tries to arrive
before rtp after reconfiguration.

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

to

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too. 

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents generating SenderReports as
response to (mostly theoretical) sr-request rtcp packet.

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

[danilchap, 2016-01-29 18:30:22]

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

[danilchap, 2016-01-29 19:22:00]

Description was changed from

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too. 

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents generating SenderReports as
response to (mostly theoretical) sr-request rtcp packet.

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

to

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too. 

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents generating compound rtcp packets
that have to start with Sender Report as well generating Sender Report  as
response to (mostly theoretical) sr-request rtcp packet.

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

[danilchap, 2016-01-29 19:23:02]

Description was changed from

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too. 

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents generating compound rtcp packets
that have to start with Sender Report as well generating Sender Report  as
response to (mostly theoretical) sr-request rtcp packet.

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

to

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too. 

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents creating compound rtcp packets
that have to start with Sender Report and Sender Reports as response to (mostly
theoretical) sr-request rtcp packet.

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

[stefan-webrtc, 2016-02-01 17:32:08]

Seems like a good change to me. Comments are only about the tests

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

https://codereview.webrtc.org/1639253007/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:305: }
Should we add a test which verifies we don't send an SR before sending RTP? Or
at least in some way verify what actually was fixed in this CL.

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

https://codereview.webrtc.org/1639253007/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3109: // Wait for SR rtcp packet before
generating rtp packets.
I'm not sure I understand this test change. How can we even expect RTCP SR to be
sent before we send RTP?

[danilchap, 2016-02-01 18:24:18]

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

https://codereview.webrtc.org/1639253007/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:305: }
On 2016/02/01 17:32:07, stefan-webrtc (holmer) wrote:
> Should we add a test which verifies we don't send an SR before sending RTP? Or
> at least in some way verify what actually was fixed in this CL.

test in video/end_to_end is one way to see this CL changes something.
DCHECK in RtcpSender::BuildSR  is another way to ensure bogus RTP timestamps are
not generated.

Not sure tests in RtcpSenderTest are helpful, specially that this change will
not be needed if we switch calculating rtp timestamps base on ntp time instead
of last frame timestamp. Sender Report before first RTP packet could be possible
and will make sense then (receiver will sooner get 2nd Sender Report and will be
able to convert rtp timestamp into ntp time)

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

https://codereview.webrtc.org/1639253007/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:3109: // Wait for SR rtcp packet before
generating rtp packets.
On 2016/02/01 17:32:07, stefan-webrtc (holmer) wrote:
> I'm not sure I understand this test change. How can we even expect RTCP SR to
be
> sent before we send RTP? 

RTP packets will not be send until FrameGeneratorCapturer will start to work.
I know 2 ways to wait for RTCP before starting FrameGeneratorCapturer:
One, longer way - just wait ~5 sec. Process thread will try to generate an SR
report. 
Another, faster way, the one used in this test - send rapid synchronization
request (transport feedback message with feedback_message_type=5). In response
to this message RtpRtcp module tries to send back SenderReport as fast as it
can. Current implementation doesn't validate Sender/Receiver SSRCs, so I put
zeroes in both these field to make code shorter.

Pathset#1 demonstrates test fails before the change.

[stefan-webrtc, 2016-05-06 11:00:59]

Does this need rebasing? Otherwise I can give it a final look so we can get it
in.

[danilchap, 2016-05-06 15:20:45]

On 2016/05/06 11:00:59, stefan-webrtc (holmer) wrote:
> Does this need rebasing? Otherwise I can give it a final look so we can get it
> in.

Rebased & fixed discovered issues.

[pbos-webrtc, 2016-06-16 11:39:43]

lgtm

.. does this need rebasing_

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:438: // Timestamp shouldn't be
esitmated before frame was received.
was transmitted?

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:438: // Timestamp shouldn't be
esitmated before frame was received.
estimated

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3049: rtcp::RapidResyncRequest rrr;
call rrr something understandable

[danilchap, 2016-06-16 12:05:02]

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:438: // Timestamp shouldn't be
esitmated before frame was received.
On 2016/06/16 11:39:43, pbos-webrtc wrote:
> estimated

type Done.
received by rtp_rtcp module for transmittion (doesn't need to be actually
transmitted).
Changed phrasing by removing word 'received'

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1639253007/diff/120001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3049: rtcp::RapidResyncRequest rrr;
On 2016/06/16 11:39:43, pbos-webrtc wrote:
> call rrr something understandable

Done.

[stefan-webrtc, 2016-07-20 09:13:32]

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right):

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:318: // Sender Report
shouldn't be send not as Sr, nor as Report.
Sender Report shouldn't be sent as an SR nor as a Report

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc (right):

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:300: // Send Frame
before sending a SR.
super nit: it should be "an SR", I think

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3144: // There should be no rtcp packet.
How can we wait for an rtcp before we send rtp if we can't send rtcp sr without
first sending an rtp? :)

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3147: // Alternative approach is to wait
several seconds for SR to be generated.
Can you clarify in the comment if this is part of the test, or if it's here
simply to speed the test up?

Seems a bit odd to me that it would take several seconds before we send an SR.
Don't we send SR every second?

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3188: // This tests are flaky. See:
These tests

[danilchap, 2016-07-20 10:09:50]

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc (right):

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc:318: // Sender Report
shouldn't be send not as Sr, nor as Report.
On 2016/07/20 09:13:31, stefan-webrtc (holmer) wrote:
> Sender Report shouldn't be sent as an SR nor as a Report

Done.

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc (right):

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:300: // Send Frame
before sending a SR.
On 2016/07/20 09:13:31, stefan-webrtc (holmer) wrote:
> super nit: it should be "an SR", I think

Done.

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3144: // There should be no rtcp packet.
On 2016/07/20 09:13:31, stefan-webrtc (holmer) wrote:
> How can we wait for an rtcp before we send rtp if we can't send rtcp sr
without
> first sending an rtp? :)
Variable name adjusted (since no-wait approach is used), this comment seems
unnecessary as the result. (Adjusted comment looked redundant)

Also removed from this block comment that there should be no rtcp packet before
rtp because that is an implementation detail: the test validates that there are
no large gaps between rtp and rtcp timestamps and doesn't care how code ensure
that.

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3147: // Alternative approach is to wait
several seconds for SR to be generated.
On 2016/07/20 09:13:31, stefan-webrtc (holmer) wrote:
> Can you clarify in the comment if this is part of the test, or if it's here
> simply to speed the test up?

Done.

> Seems a bit odd to me that it would take several seconds before we send an SR.
> Don't we send SR every second?
Yes 1s for video stream, 5s for audio stream, comment adjusted.

https://codereview.webrtc.org/1639253007/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:3188: // This tests are flaky. See:
On 2016/07/20 09:13:31, stefan-webrtc (holmer) wrote:
> These tests

Done.

[stefan-webrtc, 2016-07-20 10:20:15]

lgtm

[danilchap, 2016-07-20 10:37:36]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-07-20 10:37:36]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1639253007/#ps240001 (title:
"nit")

[commit-bot: I haz the power, 2016-07-20 10:37:46]

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-07-20 10:52:15]

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

[commit-bot: I haz the power, 2016-07-20 10:52:16]

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

[danilchap, 2016-07-20 13:27:16]

Description was changed from

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too. 

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents creating compound rtcp packets
that have to start with Sender Report and Sender Reports as response to (mostly
theoretical) sr-request rtcp packet.

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

to

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too.

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents creating compound rtcp packets
that have to start with Sender Report and Sender Reports as response to (mostly
theoretical) sr-request rtcp packet.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/70ffead25675c4761467755f9...
==========

[danilchap, 2016-07-20 13:27:18]

Committed patchset #14 (id:260001) manually as
70ffead25675c4761467755f9be844335dd59dba (presubmit successful).

[commit-bot: I haz the power, 2016-07-20 13:27:22]

Description was changed from

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too.

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents creating compound rtcp packets
that have to start with Sender Report and Sender Reports as response to (mostly
theoretical) sr-request rtcp packet.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/70ffead25675c4761467755f9...
==========

to

==========
Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP
packet.

Now it check if rtp timestamp can be calculating instead of checking number of
rtp packets. This way it works for reconfigured streams too.

It also moved deeper into rtcp_sender class to prevent SR no matter the reason
it need to be genereated. This way it prevents creating compound rtcp packets
that have to start with Sender Report and Sender Reports as response to (mostly
theoretical) sr-request rtcp packet.

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

Committed: https://crrev.com/70ffead25675c4761467755f9be844335dd59dba
Cr-Commit-Position: refs/heads/master@{#13503}
==========