StartTimestamp generated randomly in RtpSender constructor

StartTimestamp generated randomly in RtpSender constructor
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.

R=asapersson@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/4466782ae43e1b1125a55ee7e18abd10dd37cede
Cr-Commit-Position: refs/heads/master@{#13796}

[danilchap, 2016-08-15 09:49:57]

Description was changed from

==========
StartTimestamp generated randomly at RtpSender
instead of not-randomly at SetSendingState(true)
Renamed to TimestampOffset to better match meaning of the variable.
==========

to

==========
StartTimestamp generated randomly at RtpSender
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.
==========

[danilchap, 2016-08-15 11:14:04]

Description was changed from

==========
StartTimestamp generated randomly at RtpSender
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.
==========

to

==========
StartTimestamp generated randomly in RtpSender constructor
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.
==========

[danilchap, 2016-08-15 11:14:48]

danilchap@webrtc.org changed reviewers:
+ asapersson@webrtc.org, terelius@webrtc.org

[danilchap, 2016-08-15 11:14:48]

Åsa, do you know any use case where start timestamp need to be (re)generated
when sending status set to true?

[terelius, 2016-08-15 14:56:03]

Looks good. Some comments below.

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

https://codereview.webrtc.org/2241193002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:110:
rtcp_sender_.SetStartTimestamp(rtp_sender_.TimestampOffset());
Could we rename this function too?

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

https://codereview.webrtc.org/2241193002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:129: timestamp_offset_ =
random_.Rand<uint32_t>();
Note that this PRNG is not cryptographic strength. It is also not seeded with
enough entropy for cryptographic purposes. In fact, it has even less entropy
than what is suggested in the RFC 3550.

https://codereview.webrtc.org/2241193002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1718: timestamp_offset_ =
rtp_state.start_timestamp;
Rename start_timestamp in the state struct too?

[danilchap, 2016-08-15 15:23:17]

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

https://codereview.webrtc.org/2241193002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:110:
rtcp_sender_.SetStartTimestamp(rtp_sender_.TimestampOffset());
On 2016/08/15 14:56:03, terelius wrote:
> Could we rename this function too?

Done.

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

https://codereview.webrtc.org/2241193002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:129: timestamp_offset_ =
random_.Rand<uint32_t>();
On 2016/08/15 14:56:03, terelius wrote:
> Note that this PRNG is not cryptographic strength. It is also not seeded with
> enough entropy for cryptographic purposes. In fact, it has even less entropy
> than what is suggested in the RFC 3550.

Done. Simple note added.

https://codereview.webrtc.org/2241193002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1718: timestamp_offset_ =
rtp_state.start_timestamp;
On 2016/08/15 14:56:03, terelius wrote:
> Rename start_timestamp in the state struct too?

Prefer to avoid modifying structure in the /include/ subfolder.

[terelius, 2016-08-16 09:09:37]

lgtm

[terelius, 2016-08-16 10:49:24]

Stefan suggested creating a unit test to verify that the offset doesn't change
when pausing the sender. I'm not sure where to place it though. VideoSendStream?

[stefan-webrtc, 2016-08-16 10:52:36]

On 2016/08/16 10:49:24, terelius wrote:
> Stefan suggested creating a unit test to verify that the offset doesn't change
> when pausing the sender. I'm not sure where to place it though.
VideoSendStream?

I'd suggest video_send_stream_tests.cc

[danilchap, 2016-08-16 13:47:27]

On 2016/08/16 10:52:36, stefan-webrtc (holmer) wrote:
> On 2016/08/16 10:49:24, terelius wrote:
> > Stefan suggested creating a unit test to verify that the offset doesn't
change
> > when pausing the sender. I'm not sure where to place it though.
> VideoSendStream?
> 
> I'd suggest video_send_stream_tests.cc

Can't find reasonable place for this kind of test:
RtpSender/RtpRtcpImpl doesn't look right since this CL remove ability to change
timestamp offset indirectly: doesn't make sense to write a test to check that
only SetStartTimestamp/SetRtpState allowed to change the offset.
for VideoSendStream this offset is an implementation detail: it is not trivial
to retrieve offset value from that class. (webrtc::internal::VideoSendStream has
a function GetRtpState, but namespace internal suggest it is not an interface to
test)
adding case to EndToEnd test that monitors there are no large timestamp gaps
wouldn't fail with current implementation because start_timestamp would be
reinitialized close in time to initial initialization, which would make the
difference between offsets (and thus resulting timestamps) too small to be
alerted on.
Since realtime clock is used in that test, large time between initialization and
re-initialization is not easy to simulate.

[åsapersson, 2016-08-17 10:53:24]

lgtm

[danilchap, 2016-08-17 13:07:54]

Description was changed from

==========
StartTimestamp generated randomly in RtpSender constructor
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.
==========

to

==========
StartTimestamp generated randomly in RtpSender constructor
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.

R=asapersson@webrtc.org, terelius@webrtc.org

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

[danilchap, 2016-08-17 13:07:56]

Committed patchset #4 (id:60001) manually as
4466782ae43e1b1125a55ee7e18abd10dd37cede (presubmit successful).

[commit-bot: I haz the power, 2016-08-17 13:07:58]

Description was changed from

==========
StartTimestamp generated randomly in RtpSender constructor
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.

R=asapersson@webrtc.org, terelius@webrtc.org

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

to

==========
StartTimestamp generated randomly in RtpSender constructor
instead of not-randomly at SetSendingState(true)
Renamed to timestamp_offset_ to better match meaning of the variable.

R=asapersson@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/4466782ae43e1b1125a55ee7e18abd10dd37cede
Cr-Commit-Position: refs/heads/master@{#13796}
==========

[danilchap, 2016-08-17 15:12:09]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/2248413002/ by danilchap@webrtc.org.

The reason for reverting is: Breaks downstream code..