Fix WebrtcVideoStream to handle failed capture requests.

Fix WebrtcVideoStream to handle failed capture requests.

DesktopCapturer implementations are allowed to fail Capture() requests.
In that case the caller it supposed to retry later. WebrtcVideoStream
wasn't handling this case properly. On Windows the first Capture()
request often fails because it requires desktop process initialization.
WebrtcFrameScheduler, WebrtcVideoEncoder and their implementations were
also updated to accept nullptr frames, to make it possible to top-off
previous frame, while the capturer keeps failing.

Also updated DesktopSessionProxy to return ERROR_TEMPORARY instead of
ERROR_PERMANENT when the desktop process is not connected.

BUG=678712
Review-Url: https://codereview.chromium.org/2616213002
Cr-Commit-Position: refs/heads/master@{#442391}
Committed: https://chromium.googlesource.com/chromium/src/+/14b918b5e1c3df87e6d509848bce539a8dfef264

[Sergey Ulanov, 2017-01-07 00:32:44]

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

[joedow, 2017-01-09 20:00:28]

lgtm

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/connectio...
File remoting/protocol/connection_unittest.cc (right):

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/connectio...
remoting/protocol/connection_unittest.cc:659: WaitNextVideoFrame();
Would it make sense to wait one more time to make sure the third frame is
received correctly (after receiving a failure on the second)?

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler.h (right):

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler.h:36: // changed), true otherwise.
|frame| may be set to nullptr if the capture
s/changed/changes

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

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:111: // TODO(sergeyu): This
logic is applicable only to VP8. Reconsider it for
Should this TODO move up with the VP8 constant or is the rest of the logic also
VP8 specific?

[Sergey Ulanov, 2017-01-09 22:23:53]

Patchset #3 (id:40001) has been deleted

[Sergey Ulanov, 2017-01-09 22:24:06]

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/connectio...
File remoting/protocol/connection_unittest.cc (right):

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/connectio...
remoting/protocol/connection_unittest.cc:659: WaitNextVideoFrame();
On 2017/01/09 20:00:28, joedow wrote:
> Would it make sense to wait one more time to make sure the third frame is
> received correctly (after receiving a failure on the second)?

WaitNextVideoFrame() waits for a frame on the receiving side. Second Capture()
request fails on the sender, but the next one should succeed, and that's the
frame we get here. Renamed these tests to make it clearer

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/webrtc_fr...
File remoting/protocol/webrtc_frame_scheduler.h (right):

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler.h:36: // changed), true otherwise.
|frame| may be set to nullptr if the capture
On 2017/01/09 20:00:28, joedow wrote:
> s/changed/changes

Done.

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

https://codereview.chromium.org/2616213002/diff/1/remoting/protocol/webrtc_fr...
remoting/protocol/webrtc_frame_scheduler_simple.cc:111: // TODO(sergeyu): This
logic is applicable only to VP8. Reconsider it for
On 2017/01/09 20:00:28, joedow wrote:
> Should this TODO move up with the VP8 constant or is the rest of the logic
also
> VP8 specific?

Done.

[Sergey Ulanov, 2017-01-09 22:24:20]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2017-01-09 22:24:20]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2616213002/#ps20001
(title: ".")

[commit-bot: I haz the power, 2017-01-09 22:25:08]

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-01-09 23:10:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484000660131470,
"parent_rev": "82550d783ee6c19b15f4cf1c7872cd18e34df1ea", "commit_rev":
"14b918b5e1c3df87e6d509848bce539a8dfef264"}

[commit-bot: I haz the power, 2017-01-09 23:10:55]

Description was changed from

==========
Fix WebrtcVideoStream to handle failed capture requests.

DesktopCapturer implementations are allowed to fail Capture() requests.
In that case the caller it supposed to retry later. WebrtcVideoStream
wasn't handling this case properly. On Windows the first Capture()
request often fails because it requires desktop process initialization.
WebrtcFrameScheduler, WebrtcVideoEncoder and their implementations were
also updated to accept nullptr frames, to make it possible to top-off
previous frame, while the capturer keeps failing.

Also updated DesktopSessionProxy to return ERROR_TEMPORARY instead of
ERROR_PERMANENT when the desktop process is not connected.

BUG=678712
==========

to

==========
Fix WebrtcVideoStream to handle failed capture requests.

DesktopCapturer implementations are allowed to fail Capture() requests.
In that case the caller it supposed to retry later. WebrtcVideoStream
wasn't handling this case properly. On Windows the first Capture()
request often fails because it requires desktop process initialization.
WebrtcFrameScheduler, WebrtcVideoEncoder and their implementations were
also updated to accept nullptr frames, to make it possible to top-off
previous frame, while the capturer keeps failing.

Also updated DesktopSessionProxy to return ERROR_TEMPORARY instead of
ERROR_PERMANENT when the desktop process is not connected.

BUG=678712

Review-Url: https://codereview.chromium.org/2616213002
Cr-Commit-Position: refs/heads/master@{#442391}
Committed:
https://chromium.googlesource.com/chromium/src/+/14b918b5e1c3df87e6d509848bce...
==========

[commit-bot: I haz the power, 2017-01-09 23:10:56]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/14b918b5e1c3df87e6d509848bce...