[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

Committed: https://crrev.com/d673b0fa5d3fc319a4f41625db89c07e8a21794f
Cr-Commit-Position: refs/heads/master@{#11393}

[danilchap, 2016-01-25 15:04:09]

Description was changed from

==========
[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

to

==========
[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

[stefan-webrtc, 2016-01-25 15:18:11]

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

[stefan-webrtc, 2016-01-25 15:18:11]

Nice find! These definitely looks broken. It would be nice to have a test
somewhere, either in rtp_rtcp_impl_unittest.cc, or in
video_send_stream_tests.cc.

Peter, could you point Danil to some existing test for the rtp state stuff?

[pbos-webrtc, 2016-01-26 10:30:17]

On 2016/01/25 15:18:11, stefan-webrtc (holmer) wrote:
> Nice find! These definitely looks broken. It would be nice to have a test
> somewhere, either in rtp_rtcp_impl_unittest.cc, or in
> video_send_stream_tests.cc.
> 
> Peter, could you point Danil to some existing test for the rtp state stuff?

Can you take a look at TestRtpStatePreservation in end_to_end_tests.cc to see if
you can add a check for the RTCP timestamps? (Make sure the test fails before
your fix.)

[danilchap, 2016-01-26 12:28:09]

RtcpSender::start_timestamp_ is not easily accessible from EndToEnd tests (and
should not be), so to check that it does not match RtpSender::start_timestamp
had to add a chain of debug methods.

Also, added a more straightforward unittest to check the same thing directly on
RtpRtcp module.
Ensured they both fail before the change.

[pbos-webrtc, 2016-01-26 14:49:50]

as discussed offline please ping this back when parsing rtcp timestamps and
checking that they're roughly equivalent instead of exposing start_timestamp_.

[danilchap, 2016-01-26 17:23:10]

On 2016/01/26 14:49:50, pbos-webrtc wrote:
> as discussed offline please ping this back when parsing rtcp timestamps and
> checking that they're roughly equivalent instead of exposing start_timestamp_.

Turned out the issue fixed by this CL is too minor to be caught by described
real-life scenario: before first packet is sent, ReconfigureVideoEncoder
function is called and set RtcpSender::start_timestamp_ to same value as
RtpSender::start_timestamp_ on active rtp_rtcp modules.
EndToEnd test from patchset#2 fails because it check start_timestamp_ inequality
on disabled rtp_rtcp modules too.
RtpRtcpImpl test from patchset#2 fails because it use RtpRtcp module directly in
an odd way.

[pbos-webrtc, 2016-01-26 17:28:54]

On 2016/01/26 17:23:10, danilchap wrote:
> On 2016/01/26 14:49:50, pbos-webrtc wrote:
> > as discussed offline please ping this back when parsing rtcp timestamps and
> > checking that they're roughly equivalent instead of exposing
start_timestamp_.
> 
> Turned out the issue fixed by this CL is too minor to be caught by described
> real-life scenario: before first packet is sent, ReconfigureVideoEncoder
> function is called and set RtcpSender::start_timestamp_ to same value as
> RtpSender::start_timestamp_ on active rtp_rtcp modules.
> EndToEnd test from patchset#2 fails because it check start_timestamp_
inequality
> on disabled rtp_rtcp modules too.
> RtpRtcpImpl test from patchset#2 fails because it use RtpRtcp module directly
in
> an odd way.

Ok, maybe a test that verifies similar timestamps in RTP and RTCP is still good
to have (even if it's not failing right now), but I'd drop patchset 2 because I
think it's too intrusive to expose all of this in the APIs.

[danilchap, 2016-01-26 17:56:05]

Patchset #2 (id:20001) has been deleted

[danilchap, 2016-01-26 18:08:09]

On 2016/01/26 17:28:54, pbos-webrtc wrote:
> On 2016/01/26 17:23:10, danilchap wrote:
> > On 2016/01/26 14:49:50, pbos-webrtc wrote:
> > > as discussed offline please ping this back when parsing rtcp timestamps
and
> > > checking that they're roughly equivalent instead of exposing
> start_timestamp_.
> > 
> > Turned out the issue fixed by this CL is too minor to be caught by described
> > real-life scenario: before first packet is sent, ReconfigureVideoEncoder
> > function is called and set RtcpSender::start_timestamp_ to same value as
> > RtpSender::start_timestamp_ on active rtp_rtcp modules.
> > EndToEnd test from patchset#2 fails because it check start_timestamp_
> inequality
> > on disabled rtp_rtcp modules too.
> > RtpRtcpImpl test from patchset#2 fails because it use RtpRtcp module
directly
> in
> > an odd way.
> 
> Ok, maybe a test that verifies similar timestamps in RTP and RTCP is still
good
> to have (even if it's not failing right now), but I'd drop patchset 2 because
I
> think it's too intrusive to expose all of this in the APIs.

Completely agree.
Deleted patchset2.
Adding check that timestamps in RTP and RTCP are similiar better do in a
different CL: https://codereview.webrtc.org/1633843003/

[stefan-webrtc, 2016-01-27 08:11:05]

lgtm

[danilchap, 2016-01-27 09:03:46]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-01-27 09:03:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1628323003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1628323003/1

[commit-bot: I haz the power, 2016-01-27 09:49:56]

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

[commit-bot: I haz the power, 2016-01-27 09:49:57]

Try jobs failed on following builders:
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[commit-bot: I haz the power, 2016-01-27 11:55:10]

Description was changed from

==========
[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

to

==========
[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

Committed: https://crrev.com/d673b0fa5d3fc319a4f41625db89c07e8a21794f
Cr-Commit-Position: refs/heads/master@{#11393}
==========

[danilchap, 2016-01-27 11:55:11]

Description was changed from

==========
[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

Committed: https://crrev.com/d673b0fa5d3fc319a4f41625db89c07e8a21794f
Cr-Commit-Position: refs/heads/master@{#11393}
==========

to

==========
[rtp_rtcp] Fix potentional time difference between rtp and rtcp packets.

SetRtpState function was updating only rtp_sender start timestamp.
Now it updates both rtp_sender and rtcp_sender start timestamps.

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

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

[commit-bot: I haz the power, 2016-01-27 11:55:11]

Patchset 1 (id:??) landed as
https://crrev.com/d673b0fa5d3fc319a4f41625db89c07e8a21794f
Cr-Commit-Position: refs/heads/master@{#11393}

[danilchap, 2016-01-27 11:55:12]

Committed patchset #1 (id:1) manually as
d673b0fa5d3fc319a4f41625db89c07e8a21794f (presubmit successful).