Persist RTP state for FlexFEC.

Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomized after a
recreation of VideoSendStream. That might lead to us sending
a non-compliant RTP stream, which is avoided after the
changes in this CL.

BUG=webrtc:5654
TBR=pbos@webrtc.org  # Trivial change to fuzzer.

Review-Url: https://codereview.webrtc.org/2912713002
Cr-Commit-Position: refs/heads/master@{#18322}
Committed: https://chromium.googlesource.com/external/webrtc/+/48d21a23c6663dc1d07c8a05f2922ad14bd12c8a

[brandtr, 2017-05-29 07:37:16]

Description was changed from

==========
Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomize after a
recreation of VideoSendStream. These recreations can happen
due to renegotiations, and therefore we should persist the RTP
state, so we don't send a non-compliant stream.

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

to

==========
Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomized after a
recreation of VideoSendStream. That might lead to us sending
a non-compliant RTP stream, which is avoided after the
changes in this CL.

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

[brandtr, 2017-05-29 07:42:55]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-05-29 07:45:58]

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

[brandtr, 2017-05-29 07:45:59]

Please take a look.

Danil: rtp_rtcp/
Erik: video/

[brandtr, 2017-05-29 07:46:30]

The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 07:46:37]

Dry run: 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-05-29 07:52:46]

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

[commit-bot: I haz the power, 2017-05-29 07:52:47]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/19271)

[brandtr, 2017-05-29 08:00:15]

The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 08:00:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[danilchap, 2017-05-29 08:09:36]

rtp_rtcp/ lgtm

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right):

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:313:
EXPECT_EQ(initial_rtp_state.start_timestamp + 1 * 90000,
may be kVideoPayloadTypeFrequency instead of 90000

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

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4185: // Verify continuity and monotonicity of
RTP sequence numbers.
what are testing with this expectation?
FakeNetworkPipe allowed (if configured) to reorder packets.

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4193:
static_cast<int>(*last_observed_timestamp_));
what about timestamp wrap?
modules/module_common_types.h have functions to check which timestamp is newer,
base/mod_ops.h have more genertic functions to calculate difference with wrap in
mind

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4194: EXPECT_LT(timestamp_abs_diff, 10 *
90000);
prefer to use named constant instead of 90000.
May be name multiplication too:
k10Seconds = 10 * kVideo...Frequency;

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4258:
flexfec_receive_config.rtp_header_extensions.push_back(
there is shortcut for pattern std::vector<T>::push_back(T(a, b)):
std::vector<T>::emplace(a, b);

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4286: restart_video_send_stream_and_test();
is it a simple loop?
for (size_t i = 0; i < 2; ++i) {
  lambda body.
}

[commit-bot: I haz the power, 2017-05-29 08:21:14]

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

[commit-bot: I haz the power, 2017-05-29 08:21:15]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-05-29 14:26:45]

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

https://codereview.webrtc.org/2912713002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4194: EXPECT_LT(timestamp_abs_diff, 10 *
90000);
Maybe EXPECT_NEAR here?

https://codereview.webrtc.org/2912713002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4287: restart_video_send_stream_and_test();
Maybe comment on why you do this twice?

https://codereview.webrtc.org/2912713002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4300: observer.ResetPacketCount();
Should this be called before start()?

[brandtr, 2017-05-29 14:40:15]

Thanks for quick response from both of you!

Comments addressed.

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right):

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:313:
EXPECT_EQ(initial_rtp_state.start_timestamp + 1 * 90000,
On 2017/05/29 08:09:35, danilchap wrote:
> may be kVideoPayloadTypeFrequency instead of 90000

Done.

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

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4185: // Verify continuity and monotonicity of
RTP sequence numbers.
On 2017/05/29 08:09:35, danilchap wrote:
> what are testing with this expectation?
> FakeNetworkPipe allowed (if configured) to reorder packets.

I'm checking that the sequence number increments by exactly 1 between all sent
packets and that the RTP timestamp is not wildly varying. The observer is not
recreated when the VideoSendStream is, thus checking that the RTP state is
properly stored at the Call layer.

This callback is called before the fake network pipe, so reordering should not
be a problem.

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4193:
static_cast<int>(*last_observed_timestamp_));
On 2017/05/29 08:09:35, danilchap wrote:
> what about timestamp wrap?
> modules/module_common_types.h have functions to check which timestamp is
newer,
> base/mod_ops.h have more genertic functions to calculate difference with wrap
in
> mind

Cool, I'll use module_common_types.h :)

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4194: EXPECT_LT(timestamp_abs_diff, 10 *
90000);
On 2017/05/29 08:09:35, danilchap wrote:
> prefer to use named constant instead of 90000.
> May be name multiplication too:
> k10Seconds = 10 * kVideo...Frequency;

Done.

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4258:
flexfec_receive_config.rtp_header_extensions.push_back(
On 2017/05/29 08:09:35, danilchap wrote:
> there is shortcut for pattern std::vector<T>::push_back(T(a, b)):
> std::vector<T>::emplace(a, b);

Done.

https://codereview.webrtc.org/2912713002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4286: restart_video_send_stream_and_test();
On 2017/05/29 08:09:35, danilchap wrote:
> is it a simple loop?
> for (size_t i = 0; i < 2; ++i) {
>   lambda body.
> }

Yes. I find this a bit easier to read though.

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

https://codereview.webrtc.org/2912713002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4194: EXPECT_LT(timestamp_abs_diff, 10 *
90000);
On 2017/05/29 14:26:45, sprang_webrtc wrote:
> Maybe EXPECT_NEAR here?

Replaced by checks that take wrap-around into account instead.

https://codereview.webrtc.org/2912713002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4287: restart_video_send_stream_and_test();
On 2017/05/29 14:26:45, sprang_webrtc wrote:
> Maybe comment on why you do this twice?

Hmm, I guess there's no big reason to do this >1 times actually. I'll remove the
lambdas, and then there will be less confusion.

https://codereview.webrtc.org/2912713002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4300: observer.ResetPacketCount();
On 2017/05/29 14:26:45, sprang_webrtc wrote:
> Should this be called before start()?

Yes, that makes sense!

[sprang_webrtc, 2017-05-30 08:29:53]

lgtm

[brandtr, 2017-05-30 08:38:40]

The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 08:38:47]

Dry run: 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-05-30 09:14:47]

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

[commit-bot: I haz the power, 2017-05-30 09:14:47]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-05-30 09:16:54]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-05-30 09:16:54]

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/2912713002/#ps80001 (title:
"sprang comments 1.")

[commit-bot: I haz the power, 2017-05-30 09:17:01]

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-05-30 09:20:27]

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

[commit-bot: I haz the power, 2017-05-30 09:20:29]

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

[brandtr, 2017-05-30 09:29:24]

Description was changed from

==========
Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomized after a
recreation of VideoSendStream. That might lead to us sending
a non-compliant RTP stream, which is avoided after the
changes in this CL.

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

to

==========
Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomized after a
recreation of VideoSendStream. That might lead to us sending
a non-compliant RTP stream, which is avoided after the
changes in this CL.

BUG=webrtc:5654
TBR=pbos@webrtc.org  # Trivial change to fuzzer.
==========

[brandtr, 2017-05-30 09:29:47]

The CQ bit was checked by brandtr@webrtc.org

[commit-bot: I haz the power, 2017-05-30 09:29:53]

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-05-30 09:32:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496136587840290,
"parent_rev": "bb6f7524bab7c839f8281e1db1d0e7f6cfaeeba6", "commit_rev":
"48d21a23c6663dc1d07c8a05f2922ad14bd12c8a"}

[commit-bot: I haz the power, 2017-05-30 09:32:16]

Description was changed from

==========
Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomized after a
recreation of VideoSendStream. That might lead to us sending
a non-compliant RTP stream, which is avoided after the
changes in this CL.

BUG=webrtc:5654
TBR=pbos@webrtc.org  # Trivial change to fuzzer.
==========

to

==========
Persist RTP state for FlexFEC.

Before this CL, the RTP state would be re-randomized after a
recreation of VideoSendStream. That might lead to us sending
a non-compliant RTP stream, which is avoided after the
changes in this CL.

BUG=webrtc:5654
TBR=pbos@webrtc.org  # Trivial change to fuzzer.

Review-Url: https://codereview.webrtc.org/2912713002
Cr-Commit-Position: refs/heads/master@{#18322}
Committed:
https://chromium.googlesource.com/external/webrtc/+/48d21a23c6663dc1d07c8a05f...
==========

[commit-bot: I haz the power, 2017-05-30 09:32:17]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/48d21a23c6663dc1d07c8a05f...