Dont request keyframes if the stream is inactive or if we are currently receiving a keyframe.

Dont request keyframes if the stream is inactive or if we are currently receiving a keyframe.

BUG=webrtc:7520
Review-Url: https://codereview.webrtc.org/2853503002
Cr-Commit-Position: refs/heads/master@{#18199}
Committed: https://chromium.googlesource.com/external/webrtc/+/3184f8e3293e8b1e4afc314e7e7ba2a24060a5df

[philipel, 2017-04-28 12:11:28]

philipel@webrtc.org changed reviewers:
+ ilnik@webrtc.org, stefan@webrtc.org

[philipel, 2017-04-28 12:11:29]

https://codereview.webrtc.org/2853503002/diff/1/webrtc/modules/video_coding/v...
File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right):

https://codereview.webrtc.org/2853503002/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_packet_buffer_unittest.cc:62: keyframe ==
kKeyFrame ? kVideoFrameKey : kVideoFrameDelta;
The PacketBuffer has not actually been using this information until now, and I
discovered that the |frameType| was being inverted by this incorrect check.

https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:502: if (stream_is_active &&
!receiving_keyframe) {
I know we talked about this as a potential problem when it comes to
screensharing (spamming keyframe requests and in turn never receive a full
keyframe), but I'm not sure if this is a good idea.

Since we resend packets instead of padding we might end up in a state where
packets belonging to a keyframe is received, which would cause us to not send a
keyframe request even if we might need it.

[ilnik, 2017-04-28 12:40:56]

https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:502: if (stream_is_active &&
!receiving_keyframe) {
On 2017/04/28 12:11:29, philipel wrote:
> I know we talked about this as a potential problem when it comes to
> screensharing (spamming keyframe requests and in turn never receive a full
> keyframe), but I'm not sure if this is a good idea.
> 
> Since we resend packets instead of padding we might end up in a state where
> packets belonging to a keyframe is received, which would cause us to not send
a
> keyframe request even if we might need it.

How about a following workaround: prohibit keyframe packets to be used instead
of padding packets?

[ilnik, 2017-04-28 12:58:15]

On 2017/04/28 12:40:56, ilnik wrote:
>
https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
> File webrtc/video/video_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
> webrtc/video/video_receive_stream.cc:502: if (stream_is_active &&
> !receiving_keyframe) {
> On 2017/04/28 12:11:29, philipel wrote:
> > I know we talked about this as a potential problem when it comes to
> > screensharing (spamming keyframe requests and in turn never receive a full
> > keyframe), but I'm not sure if this is a good idea.
> > 
> > Since we resend packets instead of padding we might end up in a state where
> > packets belonging to a keyframe is received, which would cause us to not
send
> a
> > keyframe request even if we might need it.
> 
> How about a following workaround: prohibit keyframe packets to be used instead
> of padding packets?

LGTM, but we need to remember that this change may break something in very
uncommon situations.

[philipel, 2017-04-28 12:58:24]

https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2853503002/diff/1/webrtc/video/video_receive_st...
webrtc/video/video_receive_stream.cc:502: if (stream_is_active &&
!receiving_keyframe) {
On 2017/04/28 12:40:55, ilnik wrote:
> On 2017/04/28 12:11:29, philipel wrote:
> > I know we talked about this as a potential problem when it comes to
> > screensharing (spamming keyframe requests and in turn never receive a full
> > keyframe), but I'm not sure if this is a good idea.
> > 
> > Since we resend packets instead of padding we might end up in a state where
> > packets belonging to a keyframe is received, which would cause us to not
send
> a
> > keyframe request even if we might need it.
> 
> How about a following workaround: prohibit keyframe packets to be used instead
> of padding packets?

I just realized that we can update the timestamps after that packet has been
actually inserted.

[philipel, 2017-04-28 14:19:47]

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

[commit-bot: I haz the power, 2017-04-28 14:19:57]

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-04-28 15:09:36]

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

[commit-bot: I haz the power, 2017-04-28 15:09:37]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-05-04 12:49:37]

ping

[stefan-webrtc, 2017-05-04 14:00:18]

Can we add an integration test, for instance in end_to_end_tests.cc? Something
simple that just verifies that we don't PLI spam.

[philipel, 2017-05-08 15:21:58]

Added EndToEnd test, PTAL

[philipel, 2017-05-16 11:26:57]

ping

[stefan-webrtc, 2017-05-17 06:32:11]

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:156: rtc::Optional<int64_t>
PacketBuffer::LastReceivedPacketMs() const {
I think it would be good to add thread checks to ensure that the methods which
are called from the decode thread are actually called from there,and vice versa.

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

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2247: for (int i = 0; i < 50; ++i) {
Change this to a while loop checking the time since the method started instead.
More clear then that what you want is a timeout.

[philipel, 2017-05-18 09:23:40]

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:156: rtc::Optional<int64_t>
PacketBuffer::LastReceivedPacketMs() const {
On 2017/05/17 06:32:11, stefan-webrtc wrote:
> I think it would be good to add thread checks to ensure that the methods which
> are called from the decode thread are actually called from there,and vice
versa.

I'm not sure I understand why we should do that. We will need synchronization
since the timestamps are updated by the network thread and read by the decoding
thread.

We could use atomic for these variables of course but I don't think we would
gain anything by doing it.

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

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2247: for (int i = 0; i < 50; ++i) {
On 2017/05/17 06:32:11, stefan-webrtc wrote:
> Change this to a while loop checking the time since the method started
instead.
> More clear then that what you want is a timeout.

Done.

[holmer, 2017-05-18 09:25:52]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-05-18 09:25:52]

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2853503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:156: rtc::Optional<int64_t>
PacketBuffer::LastReceivedPacketMs() const {
On 2017/05/18 09:23:40, philipel wrote:
> On 2017/05/17 06:32:11, stefan-webrtc wrote:
> > I think it would be good to add thread checks to ensure that the methods
which
> > are called from the decode thread are actually called from there,and vice
> versa.
> 
> I'm not sure I understand why we should do that. We will need synchronization
> since the timestamps are updated by the network thread and read by the
decoding
> thread.
> 
> We could use atomic for these variables of course but I don't think we would
> gain anything by doing it.

The main reason would be to ensure we get calls from only the threads we expect,
for documentation.

[stefan-webrtc, 2017-05-18 11:46:39]

lgtm after offline discussion

[philipel, 2017-05-18 14:07:50]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-05-18 14:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from ilnik@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2853503002/#ps60001 (title:
"Feedback")

[commit-bot: I haz the power, 2017-05-18 14:08:00]

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-18 14:09:51]

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

[commit-bot: I haz the power, 2017-05-18 14:09:52]

Try jobs failed on following builders:
  ios_api_framework on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/20191)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25714)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/24734)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25761)

[philipel, 2017-05-18 14:46:50]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-05-18 14:46:50]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
ilnik@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2853503002/#ps80001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-05-18 14:46:57]

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-18 15:08:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495118810180560,
"parent_rev": "5ccdd7285c214506b58abffcf30f723c54cc3127", "commit_rev":
"3184f8e3293e8b1e4afc314e7e7ba2a24060a5df"}

[commit-bot: I haz the power, 2017-05-18 15:09:01]

Description was changed from

==========
Dont request keyframes if the stream is inactive or if we are currently
receiving a keyframe.

BUG=webrtc:7520
==========

to

==========
Dont request keyframes if the stream is inactive or if we are currently
receiving a keyframe.

BUG=webrtc:7520

Review-Url: https://codereview.webrtc.org/2853503002
Cr-Commit-Position: refs/heads/master@{#18199}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3184f8e3293e8b1e4afc314e7...
==========

[commit-bot: I haz the power, 2017-05-18 15:09:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/3184f8e3293e8b1e4afc314e7...