[Remoting Host] Supporting WebRTC VP9 streaming

[Remoting Host] Supporting WebRTC VP9 streaming

This CL makes the host able to stream both VP8 and VP9 video through WebRTC. In
runtime codec to be used will be negotiated with the client.

BUG=706220
Review-Url: https://codereview.chromium.org/2782523003
Cr-Commit-Position: refs/heads/master@{#461607}
Committed: https://chromium.googlesource.com/chromium/src/+/1481d8d05554a3eaad097ba80ad38ca860b1a674

[Yuwei, 2017-03-29 00:57:20]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 00:58:28]

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

[Yuwei, 2017-03-29 01:08:32]

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

[Yuwei, 2017-03-29 01:08:33]

PTAL

Looks like the performance is not quite on-par with VP8 so I guess more
optimization is needed :)

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_dummy_video_encoder.h (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_dummy_video_encoder.h:94: void
SetEncoderCreatedCallback(
Alternatively we can make the factory own the VPX encoder and make the video
stream directly post the frame into something like Factory::EncodeFrame().

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:171: LOG(WARNING) << "Encoder has not
been created yet.";
I'm not sure whether this is very safe but IIUC the scheduler will always
capture new frames as long as the last frame has captured and it's the
|encoder_| itself deciding how to do incremental updates.

Let me know if I got anything wrong O_o

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

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

[commit-bot: I haz the power, 2017-03-29 03:00:06]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[Sergey Ulanov, 2017-03-31 19:23:11]

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_dummy_video_encoder.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_dummy_video_encoder.cc:143: LOG(FATAL) << "Unsupported
codec: " << codec_type_;
NOTREACHED().

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_transport.cc:87: LOG(WARNING) << "VP9 not found in an
incoming SDP.";
I think we should log this message only of neither VP8 nor VP9 is found. I.e.
the client should be allowed to remove VP8 or VP9.

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:80:
webrtc_transport_->video_encoder_factory()->SetEncoderCreatedCallback(
Maybe call it "EncoderSelectedCallback"? That would help to avoid confusion as
the encoder is actually created by the handler of this calblack.

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:171: LOG(WARNING) << "Encoder has not
been created yet.";
On 2017/03/29 01:08:33, Yuwei wrote:
> I'm not sure whether this is very safe but IIUC the scheduler will always
> capture new frames as long as the last frame has captured and it's the
> |encoder_| itself deciding how to do incremental updates.
> 
> Let me know if I got anything wrong O_o

I think we need to fix this in WebrtcFrameSchedulerSimple. It shouldn't request
a frame until OnKeyFrameRequested() is called for the first time (by
WebrtcDummyVideoEncoder::Encode()). The first key frame request indicates that
the capturer has been created successfully.
WebrtcFrameSchedulerSimple::ScheduleNextFrame() should only schedule a frame if
a key frame request has been received.

[Yuwei, 2017-04-03 22:29:10]

PTAL

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_dummy_video_encoder.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_dummy_video_encoder.cc:143: LOG(FATAL) << "Unsupported
codec: " << codec_type_;
On 2017/03/31 19:23:11, Sergey Ulanov wrote:
> NOTREACHED().

Done.

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_transport.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_transport.cc:87: LOG(WARNING) << "VP9 not found in an
incoming SDP.";
On 2017/03/31 19:23:11, Sergey Ulanov wrote:
> I think we should log this message only of neither VP8 nor VP9 is found. I.e.
> the client should be allowed to remove VP8 or VP9.

Done.

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:80:
webrtc_transport_->video_encoder_factory()->SetEncoderCreatedCallback(
On 2017/03/31 19:23:11, Sergey Ulanov wrote:
> Maybe call it "EncoderSelectedCallback"? That would help to avoid confusion as
> the encoder is actually created by the handler of this calblack.

Done.

https://codereview.chromium.org/2782523003/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:171: LOG(WARNING) << "Encoder has not
been created yet.";
On 2017/03/31 19:23:11, Sergey Ulanov wrote:
> On 2017/03/29 01:08:33, Yuwei wrote:
> > I'm not sure whether this is very safe but IIUC the scheduler will always
> > capture new frames as long as the last frame has captured and it's the
> > |encoder_| itself deciding how to do incremental updates.
> > 
> > Let me know if I got anything wrong O_o
> 
> I think we need to fix this in WebrtcFrameSchedulerSimple. It shouldn't
request
> a frame until OnKeyFrameRequested() is called for the first time (by
> WebrtcDummyVideoEncoder::Encode()). The first key frame request indicates that
> the capturer has been created successfully.
> WebrtcFrameSchedulerSimple::ScheduleNextFrame() should only schedule a frame
if
> a key frame request has been received.

Done.

[Sergey Ulanov, 2017-04-04 00:21:25]

lgtm

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

https://codereview.chromium.org/2782523003/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler_simple.h:78: // Set to true when the
encoder is ready to accept frames.
Mention that it's set to true after the first key frame request. 
Also I think it's better to put it between paused_ and captured_first_frame_.

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

https://codereview.chromium.org/2782523003/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:172: if (!encoder_) {
Don't need this anymore. Replace with a DCHECK()?

[Yuwei, 2017-04-04 00:50:48]

Thanks!

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

https://codereview.chromium.org/2782523003/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler_simple.h:78: // Set to true when the
encoder is ready to accept frames.
On 2017/04/04 00:21:25, Sergey Ulanov wrote:
> Mention that it's set to true after the first key frame request. 
> Also I think it's better to put it between paused_ and captured_first_frame_.

Done.

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

https://codereview.chromium.org/2782523003/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:172: if (!encoder_) {
On 2017/04/04 00:21:25, Sergey Ulanov wrote:
> Don't need this anymore. Replace with a DCHECK()?

Done.

[Yuwei, 2017-04-04 00:50:53]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2017-04-04 00:50:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2782523003/#ps80001
(title: "Fix Feedback")

[commit-bot: I haz the power, 2017-04-04 00:51:28]

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-04-04 01:43:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491267053382650,
"parent_rev": "dec27332d0e99f15b1825c95a28c3bb169d567ff", "commit_rev":
"1481d8d05554a3eaad097ba80ad38ca860b1a674"}

[commit-bot: I haz the power, 2017-04-04 01:44:42]

Description was changed from

==========
[Remoting Host] Supporting WebRTC VP9 streaming

This CL makes the host able to stream both VP8 and VP9 video through WebRTC. In
runtime codec to be used will be negotiated with the client.

BUG=706220
==========

to

==========
[Remoting Host] Supporting WebRTC VP9 streaming

This CL makes the host able to stream both VP8 and VP9 video through WebRTC. In
runtime codec to be used will be negotiated with the client.

BUG=706220

Review-Url: https://codereview.chromium.org/2782523003
Cr-Commit-Position: refs/heads/master@{#461607}
Committed:
https://chromium.googlesource.com/chromium/src/+/1481d8d05554a3eaad097ba80ad3...
==========

[commit-bot: I haz the power, 2017-04-04 01:44:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1481d8d05554a3eaad097ba80ad3...