Fix failed capture handling in WebrtcVideoStream

Fix failed capture handling in WebrtcVideoStream

Previously, when a capture request fails, WebrtcVideoStream was
saving the corresponding event timestamp and then reusing it for
the following frame. That logic was implemented incorrectly - it
worked properly only when key_frame_request_ is set. This caused
the test to fail under MSAN as the key_frame_request_ is
set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
Review-Url: https://codereview.chromium.org/2757923004
Cr-Commit-Position: refs/heads/master@{#458569}
Committed: https://chromium.googlesource.com/chromium/src/+/5e372bf6de94cb418ab952d9e33d2cfa0269e987

[Sergey Ulanov, 2017-03-18 01:24:10]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2017-03-18 01:53:26]

https://codereview.chromium.org/2757923004/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2757923004/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:184: if (!resend_last_frame)
{
This is pretty complicated logic. I think it would benefit from a comment
explaining what's going on.

[Sergey Ulanov, 2017-03-20 18:23:59]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2017-03-20 18:24:14]

https://codereview.chromium.org/2757923004/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2757923004/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:184: if (!resend_last_frame)
{
On 2017/03/18 01:53:26, Jamie wrote:
> This is pretty complicated logic. I think it would benefit from a comment
> explaining what's going on.

Done.

[Sergey Ulanov, 2017-03-21 00:25:36]

Description was changed from

==========
Fix failed capture handling in WebrtcVideoStream

When a capture request fails the stream is supposed to save event
timestamp for the following frame. It wasn't doing it correctly only
when a keyframe request is pending. This caused the test to fail under
MSAN as the key_frame_request_ frame is set with a delay.

BUG=697178
==========

to

==========
Fix failed capture handling in WebrtcVideoStream

When a capture request fails the stream it was trying to save event
timestamp for the following frame. It was doing it correctly only 
when a keyframe request is pending. This caused the test to fail 
under MSAN as the key_frame_request_ frame is set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

[Sergey Ulanov, 2017-03-21 00:25:54]

Simplified this CL as we discussed. PTAL

[Jamie, 2017-03-21 18:33:36]

Could you please also rephrase the second paragraph of the CL description? It
looks like it's missing a few words, or maybe I'm just not understanding it.

https://codereview.chromium.org/2757923004/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2757923004/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler_simple.cc:187: captured_first_frame_ &&
(top_off_is_active_ || key_frame_request_);
Given the name of this variable, I would expect to see the |frame| replaced by
the previous frame here. In fact, this class doesn't even store the previous
frame, which makes me suspect that this isn't the right variable name. Where is
the resend-last-frame logic?

[Sergey Ulanov, 2017-03-21 19:38:02]

Description was changed from

==========
Fix failed capture handling in WebrtcVideoStream

When a capture request fails the stream it was trying to save event
timestamp for the following frame. It was doing it correctly only 
when a keyframe request is pending. This caused the test to fail 
under MSAN as the key_frame_request_ frame is set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

to

==========
Fix failed capture handling in WebrtcVideoStream

Previously, WebrtcVideoStream was saving event timestamp when
a capture request fails, to be used for the following frame. 
That logic was implemented incorrectly. It worked properly only
when key_frame_request_ is set. This caused the test to fail under 
MSAN as the key_frame_request_ frame is set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

[Sergey Ulanov, 2017-03-21 19:38:14]

Description was changed from

==========
Fix failed capture handling in WebrtcVideoStream

Previously, WebrtcVideoStream was saving event timestamp when
a capture request fails, to be used for the following frame. 
That logic was implemented incorrectly. It worked properly only
when key_frame_request_ is set. This caused the test to fail under 
MSAN as the key_frame_request_ frame is set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

to

==========
Fix failed capture handling in WebrtcVideoStream

Previously WebrtcVideoStream was saving event timestamp when
a capture request fails, to be used for the following frame. 
That logic was implemented incorrectly. It worked properly only
when key_frame_request_ is set. This caused the test to fail under 
MSAN as the key_frame_request_ frame is set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

[Sergey Ulanov, 2017-03-21 19:40:14]

Description was changed from

==========
Fix failed capture handling in WebrtcVideoStream

Previously WebrtcVideoStream was saving event timestamp when
a capture request fails, to be used for the following frame. 
That logic was implemented incorrectly. It worked properly only
when key_frame_request_ is set. This caused the test to fail under 
MSAN as the key_frame_request_ frame is set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

to

==========
Fix failed capture handling in WebrtcVideoStream

Previously, when a capture request fails, WebrtcVideoStream was
saving the corresponding event timestamp and then reusing it for
the following frame. That logic was implemented incorrectly - it
worked properly only when key_frame_request_ is set. This caused
the test to fail under MSAN as the key_frame_request_ is
set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

[Sergey Ulanov, 2017-03-21 19:40:26]

Updated description

https://codereview.chromium.org/2757923004/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_frame_scheduler_simple.cc (right):

https://codereview.chromium.org/2757923004/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler_simple.cc:187: captured_first_frame_ &&
(top_off_is_active_ || key_frame_request_);
On 2017/03/21 18:33:36, Jamie wrote:
> Given the name of this variable, I would expect to see the |frame| replaced by
> the previous frame here. In fact, this class doesn't even store the previous
> frame, which makes me suspect that this isn't the right variable name. Where
is
> the resend-last-frame logic?

The encoder keeps current frame internally (in YUV). For every Encode() call it
updates the content of that buffer from the new frame by copying
updated_region() (while also performing RGB->YUV conversion) and then encodes
that buffer.
So encoder->Encode(nullptr) will return either top-off or key-frame (when the
corresponding flag is set).

[Jamie, 2017-03-21 21:09:41]

lgtm

[Sergey Ulanov, 2017-03-21 21:11:33]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2017-03-21 21:12:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 21:56:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490130693035590,
"parent_rev": "d62ca1238e8c477dfbbbe928febfa67dd19bfe35", "commit_rev":
"5e372bf6de94cb418ab952d9e33d2cfa0269e987"}

[commit-bot: I haz the power, 2017-03-21 21:57:14]

Description was changed from

==========
Fix failed capture handling in WebrtcVideoStream

Previously, when a capture request fails, WebrtcVideoStream was
saving the corresponding event timestamp and then reusing it for
the following frame. That logic was implemented incorrectly - it
worked properly only when key_frame_request_ is set. This caused
the test to fail under MSAN as the key_frame_request_ is
set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178
==========

to

==========
Fix failed capture handling in WebrtcVideoStream

Previously, when a capture request fails, WebrtcVideoStream was
saving the corresponding event timestamp and then reusing it for
the following frame. That logic was implemented incorrectly - it
worked properly only when key_frame_request_ is set. This caused
the test to fail under MSAN as the key_frame_request_ is
set with a delay.

It's questionable if that logic is useful, so this CL removes it 
together with the corresponding test. Also improved handling of
failed captures in WebrtcFrameSchedulerSimple.

BUG=697178

Review-Url: https://codereview.chromium.org/2757923004
Cr-Commit-Position: refs/heads/master@{#458569}
Committed:
https://chromium.googlesource.com/chromium/src/+/5e372bf6de94cb418ab952d9e33d...
==========

[commit-bot: I haz the power, 2017-03-21 21:57:19]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5e372bf6de94cb418ab952d9e33d...