Interface with webrtc through encoded frames

Interface with webrtc through encoded frames

This change introduces a refactor to provide encoded frames to
webrtc. As a result, frame scheduling for capture and encoding
is taken care of by remoting itself. The current interface to webrtc
should eventually improve than what exists now.

The frame scheduling is a simple poll at 30 FPS in this change. The
improvement over previous change is that we only report frames
that are not changed and avoid all the frame dropping issues through
camera pipeline.

The tests are broken for webrtc prototype due to failure to measure
network bandwidth over the simulated interface and need to be fixed
for proper comparison. We still need to investigate latency issues as
well.

Committed: https://crrev.com/87c2fbdcfdab62ae44f43a171804762f64b276a2
Cr-Commit-Position: refs/heads/master@{#387096}

[Irfan, 2016-03-31 07:02:49]

Description was changed from

==========
Add support for external encoder

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

to

==========
Interface with webrtc through encoded frames

This change introduces a refactor to provide encoded frames to
webrtc. As a result, frame scheduling for capture and encoding
is taken care of by remoting itself. The current interface to webrtc
should eventually improve than what exists now.

The frame scheduling is a simple poll at 30 FPS in this change. The
improvement over previous change is that we only report frames
that are not changed and avoid all the frame dropping issues through
camera pipeline.

The tests are broken for webrtc prototype due to failure to measure
network bandwidth over the simulated interface and need to be fixed
for proper comparison. We still need to investigate latency issues as
well.
==========

[Irfan, 2016-03-31 07:02:49]

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

[Irfan, 2016-03-31 07:11:11]

Patchset #1 (id:1) has been deleted

[Irfan, 2016-03-31 07:12:09]

PTAL. I still need to resolve the perf issues and make tests produce reasonable
results.

[Sergey Ulanov, 2016-03-31 22:06:20]

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
File remoting/codec/webrtc_video_encoder.cc (right):

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:97: base::AutoLock Lock(lock_);
lock with lower-case l

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:103: webrtc::EncodedImage
encodedImage(buffer, buffer_size, buffer_size);
encoded_image

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:112: webrtc::CodecSpecificInfo
codecSpecificInfo;
codec_specific_info

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:152:
WebRtcVideoEncoderFactory::~WebRtcVideoEncoderFactory() {}
DCHECK(encoders_.empty())

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:183:
std::vector<WebRtcVideoEncoder*>::iterator position =
auto position =

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:187: delete encoder;
maybe make change type of encoders_ to std::vector<scoped_ptr<RtcVideoEncoder>>
to make it clear that we own them and then you wouldn't need this delete
statement.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:189: LOG(ERROR) << "Asked to remove
encoder not owned by factory";
replace this with DCHECK().

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
File remoting/codec/webrtc_video_encoder.h (right):

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.h:20: // through the capture pipeline.
So if I understand correctly this class doesn't really encode anything. I don't
think it really needs to be part of the codec directory. Move it to
remoting/protocol?

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.h:40: int SendEncodedFrame(int64_t
capture_timestamp_ms,
Maybe use VideoPacket to pass the frame instead of individual fields?

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.h:50: mutable base::Lock lock_;
I don't think you need to mark in mutable.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.cc:22: : key_frame_request_(false),
initialize boolean values in the header please.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.cc:105:
base::Bind(&WebRtcFrameScheduler::EncodeFrame, base::Unretained(this),
I don't think base::Unretained() is safe here. The scheduler may be deleted
before EncodeFrame() finished executing. As far as I can tell
CancellableTaskTracker doesn't help here (it cannot cancel the task once it
started running). You can make is safe by making EncodeFrame() static, similar
to VideoFramePump.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.cc:132: if (!packet) {
nit: remove brackets for consistency with other single-line if statements in
this file

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:26: class WebRtcFrameScheduler :
public webrtc::DesktopCapturer::Callback {
nit: In other places we use Webrtc prefix with lower-case r.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:72:
base::CancelableTaskTracker::TaskId task_id_;
task_id_ is not used anywhere.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_transport.cc:241: std::string vp8line = "a=rtpmap:100
VP8/90000\n";
I think webrtc generates SDP with '\r\n' at the end of each line. Does this code
work properly for you?

This was causing troubles sometimes as \r gets removed sometimes, landed this
workaround today: https://codereview.chromium.org/1843333002/ . We will be
getting \n here after that CL.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_transport.cc:244: rtc::replace_substrs(vp8line.c_str(),
vp8line.length(),
Please use base::ReplaceSubstringsAfterOffset:
 base::ReplaceSubstringsAfterOffset(&sdp, 0, vp8line, vp8line_with_bitrate);

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:51: if (video_tracks.empty())
I think this can be replaced with a DCHECK()

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:77: NOTIMPLEMENTED();
Can you please add Pause method in the scheduler? I think it will just need to
pause the timer.
The client may request video stream to be paused, e.g. when the screen is not
visible.

https://codereview.chromium.org/1846893002/diff/20001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/1846893002/diff/20001/remoting/remoting.gyp#n...
remoting/remoting.gyp:225: '<@(remoting_codec_nonnacl_sources)',
I think you also want to update remoting/codec/BUILD.gn

[Irfan, 2016-04-05 21:23:28]

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
File remoting/codec/webrtc_video_encoder.cc (right):

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:97: base::AutoLock Lock(lock_);
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> lock with lower-case l

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:103: webrtc::EncodedImage
encodedImage(buffer, buffer_size, buffer_size);
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> encoded_image

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:112: webrtc::CodecSpecificInfo
codecSpecificInfo;
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> codec_specific_info

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:152:
WebRtcVideoEncoderFactory::~WebRtcVideoEncoderFactory() {}
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> DCHECK(encoders_.empty())

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:183:
std::vector<WebRtcVideoEncoder*>::iterator position =
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> auto position =

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:187: delete encoder;
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> maybe make change type of encoders_ to
std::vector<scoped_ptr<RtcVideoEncoder>>
> to make it clear that we own them and then you wouldn't need this delete
> statement.

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.cc:189: LOG(ERROR) << "Asked to remove
encoder not owned by factory";
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> replace this with DCHECK().

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
File remoting/codec/webrtc_video_encoder.h (right):

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.h:20: // through the capture pipeline.
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> So if I understand correctly this class doesn't really encode anything. I
don't
> think it really needs to be part of the codec directory. Move it to
> remoting/protocol?

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.h:40: int SendEncodedFrame(int64_t
capture_timestamp_ms,
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> Maybe use VideoPacket to pass the frame instead of individual fields?

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/codec/webrtc_v...
remoting/codec/webrtc_video_encoder.h:50: mutable base::Lock lock_;
On 2016/03/31 22:06:18, Sergey Ulanov wrote:
> I don't think you need to mark in mutable.

Done.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.cc:22: : key_frame_request_(false),
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> initialize boolean values in the header please.

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.cc:105:
base::Bind(&WebRtcFrameScheduler::EncodeFrame, base::Unretained(this),
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> I don't think base::Unretained() is safe here. The scheduler may be deleted
> before EncodeFrame() finished executing. As far as I can tell
> CancellableTaskTracker doesn't help here (it cannot cancel the task once it
> started running). You can make is safe by making EncodeFrame() static, similar
> to VideoFramePump.

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.cc:132: if (!packet) {
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> nit: remove brackets for consistency with other single-line if statements in
> this file

Done.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:26: class WebRtcFrameScheduler :
public webrtc::DesktopCapturer::Callback {
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> nit: In other places we use Webrtc prefix with lower-case r.

I feel like this is something remoting should fix in other places since
third_party/webrtc/ uses WebRTC... for class names.

In the mean time, if you insist, I could use Webrtc for consistency

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:72:
base::CancelableTaskTracker::TaskId task_id_;
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> task_id_ is not used anywhere.

Done.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_transport.cc:241: std::string vp8line = "a=rtpmap:100
VP8/90000\n";
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> I think webrtc generates SDP with '\r\n' at the end of each line. Does this
code
> work properly for you?
> 
> This was causing troubles sometimes as \r gets removed sometimes, landed this
> workaround today: https://codereview.chromium.org/1843333002/ . We will be
> getting \n here after that CL.

It was working for me.

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_transport.cc:244: rtc::replace_substrs(vp8line.c_str(),
vp8line.length(),
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> Please use base::ReplaceSubstringsAfterOffset:
>  base::ReplaceSubstringsAfterOffset(&sdp, 0, vp8line, vp8line_with_bitrate);

Done.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_renderer_adapter.cc:51: if (video_tracks.empty())
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> I think this can be replaced with a DCHECK()

Done.

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

https://codereview.chromium.org/1846893002/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:77: NOTIMPLEMENTED();
On 2016/03/31 22:06:19, Sergey Ulanov wrote:
> Can you please add Pause method in the scheduler? I think it will just need to
> pause the timer.
> The client may request video stream to be paused, e.g. when the screen is not
> visible.

Done.

https://codereview.chromium.org/1846893002/diff/20001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/1846893002/diff/20001/remoting/remoting.gyp#n...
remoting/remoting.gyp:225: '<@(remoting_codec_nonnacl_sources)',
On 2016/03/31 22:06:20, Sergey Ulanov wrote:
> I think you also want to update remoting/codec/BUILD.gn

This moved to protocol/

[Irfan, 2016-04-05 21:28:26]

Patchset #2 (id:40001) has been deleted

[Sergey Ulanov, 2016-04-07 21:05:39]

lgtm
please wait until next week before landing this.

https://codereview.chromium.org/1846893002/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1846893002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:24: enum Flags { kRequestKeyFrame = 1 << 0 };
Call it REQUEST_KEY_FRAME. See
https://www.chromium.org/developers/coding-style#TOC-Naming

https://codereview.chromium.org/1846893002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:36: uint32_t flags = 0) = 0;
style guide doesn't allow optional arguments.

You can overload this function to avoid changing all callers:
scoped_ptr<VideoPacket> Encode(const webrtc::DesktopFrame& frame) {
  return Encode(frame, 0);
}

But there shouldn't be many call sites, so it should be possible to update all
of thme.

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

https://codereview.chromium.org/1846893002/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.h:72: scoped_ptr<WebrtcTransport>
transport_;
What's the reason you need this change? transport_ has the same lifetime as
WebrtcConnectionToClient, so it can be a member.
If there is something else I'm missing then please change this to
std::unique_ptr<>

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

https://codereview.chromium.org/1846893002/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:41:
scoped_ptr<WebrtcVideoCapturerAdapter> capturer_adapter(
WebrtcVideoCapturerAdapter is not used anywhere beside this class.
Remove it please.

[Sergey Ulanov, 2016-04-12 18:57:31]

Looks like there are merge conflicts that need to be resolved. Also I think you
need to update some test code as it fails to compile. Try compiling remoting_all
- that should build all remoting targets.

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
File remoting/codec/video_encoder_verbatim.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
remoting/codec/video_encoder_verbatim.h:23: uint32_t flags = 0) override;
remove =0

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.h:41: const uint32_t flags = 0) override;
s/ = 0//

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/linux/lin...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:519: args.append("--enable-webrtc")
Don't need this

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/video_fra...
File remoting/host/video_frame_recorder.cc (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/video_fra...
remoting/host/video_frame_recorder.cc:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
I deleted this file recently.

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_dummy_video_capturer.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_dummy_video_capturer.h:25:
scoped_ptr<WebRtcFrameScheduler> frame_scheduler);
change this to std::unique_ptr<> please

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_frame_scheduler.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:10: #include
"base/memory/scoped_ptr.h"
remove this.

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:31:
scoped_ptr<webrtc::DesktopCapturer> capturer,
Use std::unique_ptr<> here and everywhere else in this file please.

[Irfan, 2016-04-12 21:13:12]

PTAL

https://codereview.chromium.org/1846893002/diff/60001/remoting/codec/video_en...
File remoting/codec/video_encoder.h (right):

https://codereview.chromium.org/1846893002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:24: enum Flags { kRequestKeyFrame = 1 << 0 };
On 2016/04/07 21:05:39, Sergey Ulanov wrote:
> Call it REQUEST_KEY_FRAME. See
> https://www.chromium.org/developers/coding-style#TOC-Naming

Done.

https://codereview.chromium.org/1846893002/diff/60001/remoting/codec/video_en...
remoting/codec/video_encoder.h:36: uint32_t flags = 0) = 0;
On 2016/04/07 21:05:39, Sergey Ulanov wrote:
> style guide doesn't allow optional arguments.
> 
> You can overload this function to avoid changing all callers:
> scoped_ptr<VideoPacket> Encode(const webrtc::DesktopFrame& frame) {
>   return Encode(frame, 0);
> }
> 
> But there shouldn't be many call sites, so it should be possible to update all
> of thme.

Done.

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

https://codereview.chromium.org/1846893002/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:41:
scoped_ptr<WebrtcVideoCapturerAdapter> capturer_adapter(
On 2016/04/07 21:05:39, Sergey Ulanov wrote:
> WebrtcVideoCapturerAdapter is not used anywhere beside this class.
> Remove it please.

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
File remoting/codec/video_encoder_verbatim.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
remoting/codec/video_encoder_verbatim.h:23: uint32_t flags = 0) override;
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> remove =0

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
File remoting/codec/video_encoder_vpx.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/codec/video_en...
remoting/codec/video_encoder_vpx.h:41: const uint32_t flags = 0) override;
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> s/ = 0//

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/linux/lin...
File remoting/host/linux/linux_me2me_host.py (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/linux/lin...
remoting/host/linux/linux_me2me_host.py:519: args.append("--enable-webrtc")
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> Don't need this

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/video_fra...
File remoting/host/video_frame_recorder.cc (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/host/video_fra...
remoting/host/video_frame_recorder.cc:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> I deleted this file recently.

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_dummy_video_capturer.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_dummy_video_capturer.h:25:
scoped_ptr<WebRtcFrameScheduler> frame_scheduler);
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> change this to std::unique_ptr<> please

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
File remoting/protocol/webrtc_frame_scheduler.h (right):

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:10: #include
"base/memory/scoped_ptr.h"
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> remove this.

Done.

https://codereview.chromium.org/1846893002/diff/80001/remoting/protocol/webrt...
remoting/protocol/webrtc_frame_scheduler.h:31:
scoped_ptr<webrtc::DesktopCapturer> capturer,
On 2016/04/12 18:57:31, Sergey Ulanov wrote:
> Use std::unique_ptr<> here and everywhere else in this file please.

Done.

[Irfan, 2016-04-12 22:10:27]

Patchset #4 (id:100001) has been deleted

[Irfan, 2016-04-12 23:19:35]

There appears to be a race which is triggered only on some platforms where the
encoded frame request is likely coming in before we have frame scheduler ready.
Looking into it.

[Irfan, 2016-04-13 19:15:46]

Patchset #5 (id:140001) has been deleted

[Irfan, 2016-04-13 19:16:41]

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

[commit-bot: I haz the power, 2016-04-13 19:17:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846893002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846893002/160001

[commit-bot: I haz the power, 2016-04-13 19:27:37]

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

[commit-bot: I haz the power, 2016-04-13 19:27:39]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Irfan, 2016-04-13 19:45:37]

PTAL

[Sergey Ulanov, 2016-04-13 19:52:10]

lgtm, but please see my comments

https://codereview.chromium.org/1846893002/diff/160001/remoting/protocol/webr...
File remoting/protocol/webrtc_video_encoder.cc (right):

https://codereview.chromium.org/1846893002/diff/160001/remoting/protocol/webr...
remoting/protocol/webrtc_video_encoder.cc:147: void
WebRtcVideoEncoder::ClearKeyFrameRequestCallback() {
I don't think you really need this method. Instead just call
SetKeyFrameRequestCallback(base::Closure());

https://codereview.chromium.org/1846893002/diff/160001/remoting/protocol/webr...
remoting/protocol/webrtc_video_encoder.cc:228: void
WebRtcVideoEncoderFactory::ClearKeyFrameRequestCallback() {
Same here: Use SetKeyFrameRequestCallback with null callback.

[Irfan, 2016-04-13 20:30:36]

https://codereview.chromium.org/1846893002/diff/160001/remoting/protocol/webr...
File remoting/protocol/webrtc_video_encoder.cc (right):

https://codereview.chromium.org/1846893002/diff/160001/remoting/protocol/webr...
remoting/protocol/webrtc_video_encoder.cc:147: void
WebRtcVideoEncoder::ClearKeyFrameRequestCallback() {
On 2016/04/13 19:52:10, Sergey Ulanov wrote:
> I don't think you really need this method. Instead just call
> SetKeyFrameRequestCallback(base::Closure());

Done.

[Irfan, 2016-04-13 20:30:43]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-04-13 20:30:43]

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/1846893002/#ps180001
(title: "Addressed sergey comment")

[commit-bot: I haz the power, 2016-04-13 20:30:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846893002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846893002/180001

[commit-bot: I haz the power, 2016-04-13 21:21:56]

Description was changed from

==========
Interface with webrtc through encoded frames

This change introduces a refactor to provide encoded frames to
webrtc. As a result, frame scheduling for capture and encoding
is taken care of by remoting itself. The current interface to webrtc
should eventually improve than what exists now.

The frame scheduling is a simple poll at 30 FPS in this change. The
improvement over previous change is that we only report frames
that are not changed and avoid all the frame dropping issues through
camera pipeline.

The tests are broken for webrtc prototype due to failure to measure
network bandwidth over the simulated interface and need to be fixed
for proper comparison. We still need to investigate latency issues as
well.
==========

to

==========
Interface with webrtc through encoded frames

This change introduces a refactor to provide encoded frames to
webrtc. As a result, frame scheduling for capture and encoding
is taken care of by remoting itself. The current interface to webrtc
should eventually improve than what exists now.

The frame scheduling is a simple poll at 30 FPS in this change. The
improvement over previous change is that we only report frames
that are not changed and avoid all the frame dropping issues through
camera pipeline.

The tests are broken for webrtc prototype due to failure to measure
network bandwidth over the simulated interface and need to be fixed
for proper comparison. We still need to investigate latency issues as
well.
==========

[commit-bot: I haz the power, 2016-04-13 21:21:57]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-04-13 21:24:00]

Description was changed from

==========
Interface with webrtc through encoded frames

This change introduces a refactor to provide encoded frames to
webrtc. As a result, frame scheduling for capture and encoding
is taken care of by remoting itself. The current interface to webrtc
should eventually improve than what exists now.

The frame scheduling is a simple poll at 30 FPS in this change. The
improvement over previous change is that we only report frames
that are not changed and avoid all the frame dropping issues through
camera pipeline.

The tests are broken for webrtc prototype due to failure to measure
network bandwidth over the simulated interface and need to be fixed
for proper comparison. We still need to investigate latency issues as
well.
==========

to

==========
Interface with webrtc through encoded frames

This change introduces a refactor to provide encoded frames to
webrtc. As a result, frame scheduling for capture and encoding
is taken care of by remoting itself. The current interface to webrtc
should eventually improve than what exists now.

The frame scheduling is a simple poll at 30 FPS in this change. The
improvement over previous change is that we only report frames
that are not changed and avoid all the frame dropping issues through
camera pipeline.

The tests are broken for webrtc prototype due to failure to measure
network bandwidth over the simulated interface and need to be fixed
for proper comparison. We still need to investigate latency issues as
well.

Committed: https://crrev.com/87c2fbdcfdab62ae44f43a171804762f64b276a2
Cr-Commit-Position: refs/heads/master@{#387096}
==========

[commit-bot: I haz the power, 2016-04-13 21:24:02]

Patchset 6 (id:??) landed as
https://crrev.com/87c2fbdcfdab62ae44f43a171804762f64b276a2
Cr-Commit-Position: refs/heads/master@{#387096}