Implement RTP keepalive in native stack.

Implement RTP keepalive in native stack.

BUG=webrtc:7907
Review-Url: https://codereview.webrtc.org/2960363002
Cr-Commit-Position: refs/heads/master@{#18912}
Committed: https://chromium.googlesource.com/external/webrtc/+/168794c43c7e6d7d78b2b2286eeb42583347aba6

[sprang_webrtc, 2017-06-28 23:42:25]

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

[commit-bot: I haz the power, 2017-06-28 23:42:49]

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-06-28 23:44:35]

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

[commit-bot: I haz the power, 2017-06-28 23:44:36]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/21551)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/21340)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26868)

[sprang_webrtc, 2017-06-29 19:31:45]

Description was changed from

==========
Implement RTP keepalive in native stack.

BUG=
==========

to

==========
Implement RTP keepalive in native stack.

BUG=webrtc:7907
==========

[sprang_webrtc, 2017-06-29 19:33:38]

sprang@webrtc.org changed reviewers:
+ asapersson@webrtc.org

[sprang_webrtc, 2017-06-29 19:36:13]

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

[commit-bot: I haz the power, 2017-06-29 19:36:20]

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-06-29 19:38:09]

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

[commit-bot: I haz the power, 2017-06-29 19:38:10]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/21374)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26902)

[åsapersson, 2017-06-30 15:09:14]

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

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:155: last_process_time_ = now;
is last_process_time_ still needed?

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:167: int64_t last_send_time_ms =
rtp_sender_->LastTimestampTimeMs();
check if last_send_time_ms is set?

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc (right):

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:595: const int kTimeout
= 1500;
kTimeoutMs

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:613: // Process for the
same timestamp again, no new packet shold be sent.
should

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

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1305:
packet->SetTimestamp(last_rtp_timestamp_);
Could last_rtp_timestamp_ be 0 here and if so ok?

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1306:
packet->set_capture_time_ms(capture_time_ms_);
is locked needed for SendToNetwork?

And AllocatePacket?

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:3415: class CNameObserver : public
test::SendTest {
rename class

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:3425: // The video stream has start.
Stop it now. But only once.
But only once?

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video_send_stream....
webrtc/video_send_stream.h:173: // alive packet. The keep-laive packet is an
empty (no payload) RTP
laive->alive

[sprang_webrtc, 2017-06-30 16:21:00]

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

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:155: last_process_time_ = now;
On 2017/06/30 15:09:13, åsapersson wrote:
> is last_process_time_ still needed?

No, cleaned up.

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:167: int64_t last_send_time_ms =
rtp_sender_->LastTimestampTimeMs();
On 2017/06/30 15:09:13, åsapersson wrote:
> check if last_send_time_ms is set?

There's no need, since if it's not set it will return 0, and so the if-statement
below will still trigger as expected.
I can add a comment about it.

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc (right):

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:595: const int kTimeout
= 1500;
On 2017/06/30 15:09:13, åsapersson wrote:
> kTimeoutMs

Done.

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:613: // Process for the
same timestamp again, no new packet shold be sent.
On 2017/06/30 15:09:13, åsapersson wrote:
> should

Done.

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

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1305:
packet->SetTimestamp(last_rtp_timestamp_);
On 2017/06/30 15:09:13, åsapersson wrote:
> Could last_rtp_timestamp_ be 0 here and if so ok?

Hm, if the stream is started with media being sent, then yes it can be 0.
This can potentially lead to a problem since it seems that the bandwidth
estimator
is triggered for the packet, even though it get's discarded.

However the alternative is to not keep the connection open, which is a
problem...
Maybe we should use the timestamp offset logic from SendPadData() though, for
temporary video mute?

Should we also try to ignore these packets in the bwe?

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1306:
packet->set_capture_time_ms(capture_time_ms_);
On 2017/06/30 15:09:13, åsapersson wrote:
> is locked needed for SendToNetwork?
> 
> And AllocatePacket?

No, in fact it probably shouldn't. Missed cleaning this up.
Only needed for reading timestamps.

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:3415: class CNameObserver : public
test::SendTest {
On 2017/06/30 15:09:13, åsapersson wrote:
> rename class

Done.

https://codereview.webrtc.org/2960363002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream_tests.cc:3425: // The video stream has start.
Stop it now. But only once.
On 2017/06/30 15:09:13, åsapersson wrote:
> But only once?

Missed cleanup. Removed.

[sprang_webrtc, 2017-06-30 16:24:51]

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

[commit-bot: I haz the power, 2017-06-30 16:24:56]

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-06-30 16:34:45]

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

[commit-bot: I haz the power, 2017-06-30 16:34:46]

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

[sprang_webrtc, 2017-06-30 19:43:32]

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

[commit-bot: I haz the power, 2017-06-30 19:43:38]

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-06-30 21:08:13]

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

[commit-bot: I haz the power, 2017-06-30 21:08:14]

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

[åsapersson, 2017-07-03 13:39:18]

https://codereview.webrtc.org/2960363002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2960363002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:25: #include
"webrtc/video_send_stream.h"
avoid include of video_send_stream

[sprang_webrtc, 2017-07-05 13:29:59]

I updated the market bit to false. It is now analogous to plain padding packets.
Only RTX uses timestamp alignment, so this should work.

https://codereview.webrtc.org/2960363002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2960363002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:25: #include
"webrtc/video_send_stream.h"
On 2017/07/03 13:39:17, åsapersson wrote:
> avoid include of video_send_stream

Done. Moved to struct to common_types.h

[åsapersson, 2017-07-05 15:19:55]

lgtm with comments

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/common_types.h#ne...
webrtc/common_types.h:918: // packet. The keep-laive packet is an empty (no
payload) RTP packet with a
alive

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/BUILD.gn (right):

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/BUILD.gn:181: "//webrtc:video_stream_api",
remove?

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right):

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:28: #include
"webrtc/video_send_stream.h"
remove?

[sprang_webrtc, 2017-07-05 15:56:30]

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

[commit-bot: I haz the power, 2017-07-05 15:56:39]

Dry run: CQ is trying da patch.

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

[sprang_webrtc, 2017-07-05 15:56:44]

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

[sprang_webrtc, 2017-07-05 15:56:47]

+stefan for webrtc/common_types.h

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/common_types.h#ne...
webrtc/common_types.h:918: // packet. The keep-laive packet is an empty (no
payload) RTP packet with a
On 2017/07/05 15:19:55, åsapersson wrote:
> alive

Done.

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/BUILD.gn (right):

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/BUILD.gn:181: "//webrtc:video_stream_api",
On 2017/07/05 15:19:55, åsapersson wrote:
> remove?

Done.

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right):

https://codereview.webrtc.org/2960363002/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:28: #include
"webrtc/video_send_stream.h"
On 2017/07/05 15:19:55, åsapersson wrote:
> remove?

Done.

[commit-bot: I haz the power, 2017-07-05 16:22:55]

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

[commit-bot: I haz the power, 2017-07-05 16:22:56]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-07-06 11:34:46]

lgtm

[sprang_webrtc, 2017-07-06 11:35:06]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-07-06 11:35:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2960363002/#ps180001 (title:
"Cleanup")

[commit-bot: I haz the power, 2017-07-06 11:35:15]

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-07-06 11:38:07]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499340906889100,
"parent_rev": "5c0d703382d554accb681efcf6d154905cc1a7c1", "commit_rev":
"168794c43c7e6d7d78b2b2286eeb42583347aba6"}

[commit-bot: I haz the power, 2017-07-06 11:38:12]

Description was changed from

==========
Implement RTP keepalive in native stack.

BUG=webrtc:7907
==========

to

==========
Implement RTP keepalive in native stack.

BUG=webrtc:7907

Review-Url: https://codereview.webrtc.org/2960363002
Cr-Commit-Position: refs/heads/master@{#18912}
Committed:
https://chromium.googlesource.com/external/webrtc/+/168794c43c7e6d7d78b2b2286...
==========

[commit-bot: I haz the power, 2017-07-06 11:38:13]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/168794c43c7e6d7d78b2b2286...