Cleanup VideoFramePump.

Cleanup VideoFramePump.

1. Threading logic for the capture thread has been moved to
   ScreenCapturerProxy.
2. Mouse shape handling has been moved to MouseShapePump.

BUG=455818
Committed: https://crrev.com/1afb35a1a63e53497245625b1052be436830a95f
Cr-Commit-Position: refs/heads/master@{#316247}

[Sergey Ulanov, 2015-02-05 23:19:22]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2015-02-05 23:20:22]

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

[Sergey Ulanov, 2015-02-10 19:38:48]

ping

[Wez, 2015-02-11 01:05:08]

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.h:32: scoped_refptr<base::SingleThreadTaskRunner>
network_task_runner,
As for VideoFramePump; there's no concept of "network" in this class, so call
this caller_task_runner, or get it via TTRH.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump_unittest.cc (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump_unittest.cc:117:
EXPECT_TRUE(mouse_monitor_callback_);
nit: Does EXPECT work here, or do you need ASSERT?  (Sorry, can't remember the
restrictions off the top of my head)

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump_unittest.cc:152: // This test mocks
MouseCursorMonitor and network layer to verify that the
What is it that it mocks from the network layer?

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
File remoting/host/screen_capturer_proxy.cc (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:17: class ScreenCapturerProxy::Core :
public webrtc::DesktopCapturer::Callback {
nit: You could make this class purely a DesktopCapturer::Callback
implementation, and have the ScreenCapturerProxy do:

PostTask(..., Bind(desktop_capturer_.get(), DesktopCapturer::Capture, ...))

directly - there's no real reason for the DesktopCapturer to be owned by the
ScreenCapturerProxy::Core; it just adds a bit more boilerplate to do so.
Instead, you could keep the Callback implementation purely responsible for
handling the capture completion.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:33: 
nit: Duplicate blank line?

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:59: ScreenCapturerProxy::Core::~Core() {
nit: This seems to be in the wrong order wrt Start()?

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:81: }
nit: Missing blank line.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
File remoting/host/screen_capturer_proxy.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:29: class ScreenCapturerProxy {
Why ScreenCapturerProxy, since it actually proxies a DesktopCapturer?

Also, could you derive this from DesktopCapturer so that it is a drop-in
substitute for a DesktopCapturer anywhere in the codebase?  (It looks like the
CreateSharedMemory() would break that since it's synchronous, but that could be
resolved pretty easily).

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:35:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
This class doesn't do anything with the network; this should be
"caller_task_runner", or could it even be fetched implicitly in the ctor, via
ThreadTaskRunnerHandle::Get()?

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:54:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
See above.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
File remoting/host/video_frame_pump.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:114: // Task runners used to run |encoder_|.
Looks like there's only one task runner?

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:117: scoped_ptr<ScreenCapturerProxy>
capturer_proxy_;
nit: No comment to explain this?

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:129: CaptureScheduler capture_scheduler_;
nit: Comment this; in particular is it responsible for deciding when to
schedule, or for actually scheduling?

[Sergey Ulanov, 2015-02-11 19:19:55]

Patchset #2 (id:40001) has been deleted

[Sergey Ulanov, 2015-02-11 19:21:36]

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.h:32: scoped_refptr<base::SingleThreadTaskRunner>
network_task_runner,
On 2015/02/11 01:05:07, Wez wrote:
> As for VideoFramePump; there's no concept of "network" in this class, so call
> this caller_task_runner, or get it via TTRH.

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump_unittest.cc (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump_unittest.cc:117:
EXPECT_TRUE(mouse_monitor_callback_);
On 2015/02/11 01:05:07, Wez wrote:
> nit: Does EXPECT work here, or do you need ASSERT?  (Sorry, can't remember the
> restrictions off the top of my head)

replaced with ASSERT. the OnMouseCursor() call below will crash without it (note
that most of this code has been moved from the old VideoSchedulerTest).

https://codereview.chromium.org/883673004/diff/20001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump_unittest.cc:152: // This test mocks
MouseCursorMonitor and network layer to verify that the
On 2015/02/11 01:05:07, Wez wrote:
> What is it that it mocks from the network layer?

ClientStub. updated the comment.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
File remoting/host/screen_capturer_proxy.cc (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:17: class ScreenCapturerProxy::Core :
public webrtc::DesktopCapturer::Callback {
On 2015/02/11 01:05:07, Wez wrote:
> nit: You could make this class purely a DesktopCapturer::Callback
> implementation, and have the ScreenCapturerProxy do:
> 
> PostTask(..., Bind(desktop_capturer_.get(), DesktopCapturer::Capture, ...))
> 
> directly - there's no real reason for the DesktopCapturer to be owned by the
> ScreenCapturerProxy::Core; it just adds a bit more boilerplate to do so.
> Instead, you could keep the Callback implementation purely responsible for
> handling the capture completion.

the capturer and the callback would still need to be deleted on the capturer
thread, which means two DeleteSoon() calls in ~DesktopCapturerProxy(), so I
think it's cleaner to make the capturer owned by the Core. It's also more
consistent with other Proxy classes.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:33: 
On 2015/02/11 01:05:07, Wez wrote:
> nit: Duplicate blank line?

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:59: ScreenCapturerProxy::Core::~Core() {
On 2015/02/11 01:05:07, Wez wrote:
> nit: This seems to be in the wrong order wrt Start()?

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.cc:81: }
On 2015/02/11 01:05:07, Wez wrote:
> nit: Missing blank line.

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
File remoting/host/screen_capturer_proxy.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:29: class ScreenCapturerProxy {
On 2015/02/11 01:05:08, Wez wrote:
> Why ScreenCapturerProxy, since it actually proxies a DesktopCapturer?

Done.

> 
> Also, could you derive this from DesktopCapturer so that it is a drop-in
> substitute for a DesktopCapturer anywhere in the codebase?  (It looks like the
> CreateSharedMemory() would break that since it's synchronous, but that could
be
> resolved pretty easily).

Yes, it's not going to be called anyway. Initially I didn't use DesktopCapturer
interface because initially this class was also responsible for mouse shape, but
I removed it since then.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:35:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
On 2015/02/11 01:05:07, Wez wrote:
> This class doesn't do anything with the network; this should be
> "caller_task_runner", or could it even be fetched implicitly in the ctor, via
> ThreadTaskRunnerHandle::Get()?

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:54:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
On 2015/02/11 01:05:08, Wez wrote:
> See above.

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
File remoting/host/video_frame_pump.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:114: // Task runners used to run |encoder_|.
On 2015/02/11 01:05:08, Wez wrote:
> Looks like there's only one task runner?

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:117: scoped_ptr<ScreenCapturerProxy>
capturer_proxy_;
On 2015/02/11 01:05:08, Wez wrote:
> nit: No comment to explain this?

Done.

https://codereview.chromium.org/883673004/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:129: CaptureScheduler capture_scheduler_;
On 2015/02/11 01:05:08, Wez wrote:
> nit: Comment this; in particular is it responsible for deciding when to
> schedule, or for actually scheduling?

Done.

[Wez, 2015-02-12 21:59:16]

LGTM w/ nits!

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
File remoting/host/screen_capturer_proxy.h (right):

https://codereview.chromium.org/883673004/diff/20001/remoting/host/screen_cap...
remoting/host/screen_capturer_proxy.h:29: class ScreenCapturerProxy {
On 2015/02/11 19:21:35, Sergey Ulanov wrote:
> On 2015/02/11 01:05:08, Wez wrote:
> > Why ScreenCapturerProxy, since it actually proxies a DesktopCapturer?
> 
> Done.
> 
> > 
> > Also, could you derive this from DesktopCapturer so that it is a drop-in
> > substitute for a DesktopCapturer anywhere in the codebase?  (It looks like
the
> > CreateSharedMemory() would break that since it's synchronous, but that could
> be
> > resolved pretty easily).
> 
> Yes, it's not going to be called anyway. Initially I didn't use
DesktopCapturer
> interface because initially this class was also responsible for mouse shape,
but
> I removed it since then.

OK, cool; if you go ahead and derived from DesktopCapturer then that has the
nice property that it becomes a drop-in replacement, which is really how these
things should work ideally. :)

https://codereview.chromium.org/883673004/diff/60001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/client_ses...
remoting/host/client_session.cc:464: // When using IpcDesktopCapturer the
capture thread is not useful.
nit: File a bug for this & related cleanup?

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
File remoting/host/desktop_capturer_proxy.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
remoting/host/desktop_capturer_proxy.cc:66: size_t size) {
nit: thread DCHECK?

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
File remoting/host/desktop_capturer_proxy.h (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
remoting/host/desktop_capturer_proxy.h:26: class DesktopCapturerProxy : public
base::NonThreadSafe,
According to Lukasz, ThreadChecker member is preferred over NonThreadSafe.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
remoting/host/desktop_capturer_proxy.h:27: public webrtc::DesktopCapturer {
nit: Indentation

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.cc:22: class MouseShapePump::Core : public
base::NonThreadSafe,
ThreadChecker?

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.cc:120: core_.reset(new
Core(weak_factory_.GetWeakPtr(), capture_task_runner,
I think you mean ThreadTaskRunnerHandle::Get() - you're passing the capture
task-runner as the caller one right now?

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump.h (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.h:30: class MouseShapePump : public
base::NonThreadSafe {
See other comment re ThreadChecker vs NonThreadSafe

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump_unittest.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump_unittest.cc:184: TEST_F(MouseShapePumpTest,
DeleteOnThreads) {
Given the typo in the MouseShapePump ctor re the task runners, you probably want
a test that actually runs it on both threads?

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_pump.h (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:65: class VideoFramePump : public
base::NonThreadSafe,
ThreadChecker?

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_pump_unittest.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_pump_unittest.cc:222: TEST_F(VideoFramePumpTest,
DeleteOnThreads) {
Add a test for actually capturing across threads?

[Sergey Ulanov, 2015-02-13 08:39:32]

New patchsets have been uploaded after l-g-t-m from wez@chromium.org

[Sergey Ulanov, 2015-02-13 08:39:45]

https://codereview.chromium.org/883673004/diff/60001/remoting/host/client_ses...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/client_ses...
remoting/host/client_session.cc:464: // When using IpcDesktopCapturer the
capture thread is not useful.
On 2015/02/12 21:59:15, Wez wrote:
> nit: File a bug for this & related cleanup?

Filed crbug.com/458437

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
File remoting/host/desktop_capturer_proxy.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
remoting/host/desktop_capturer_proxy.cc:66: size_t size) {
On 2015/02/12 21:59:15, Wez wrote:
> nit: thread DCHECK?

Done.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
File remoting/host/desktop_capturer_proxy.h (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
remoting/host/desktop_capturer_proxy.h:26: class DesktopCapturerProxy : public
base::NonThreadSafe,
On 2015/02/12 21:59:15, Wez wrote:
> According to Lukasz, ThreadChecker member is preferred over NonThreadSafe.

Done, though I think NonThreadSafe is better for readability (but comments for
ThreadChecker say that it should be proffered in this case).

https://codereview.chromium.org/883673004/diff/60001/remoting/host/desktop_ca...
remoting/host/desktop_capturer_proxy.h:27: public webrtc::DesktopCapturer {
On 2015/02/12 21:59:15, Wez wrote:
> nit: Indentation

Done.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.cc:22: class MouseShapePump::Core : public
base::NonThreadSafe,
On 2015/02/12 21:59:15, Wez wrote:
> ThreadChecker?

Done.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.cc:120: core_.reset(new
Core(weak_factory_.GetWeakPtr(), capture_task_runner,
On 2015/02/12 21:59:15, Wez wrote:
> I think you mean ThreadTaskRunnerHandle::Get() - you're passing the capture
> task-runner as the caller one right now?

Yes.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump.h (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump.h:30: class MouseShapePump : public
base::NonThreadSafe {
On 2015/02/12 21:59:15, Wez wrote:
> See other comment re ThreadChecker vs NonThreadSafe

Done.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
File remoting/host/mouse_shape_pump_unittest.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/mouse_shap...
remoting/host/mouse_shape_pump_unittest.cc:184: TEST_F(MouseShapePumpTest,
DeleteOnThreads) {
On 2015/02/12 21:59:15, Wez wrote:
> Given the typo in the MouseShapePump ctor re the task runners, you probably
want
> a test that actually runs it on both threads?

Modified FirstCursor to always run a separate thread, so it can catch this error
now (via DCHECKs).

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_pump.h (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_pump.h:65: class VideoFramePump : public
base::NonThreadSafe,
On 2015/02/12 21:59:15, Wez wrote:
> ThreadChecker?

Done.

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
File remoting/host/video_frame_pump_unittest.cc (right):

https://codereview.chromium.org/883673004/diff/60001/remoting/host/video_fram...
remoting/host/video_frame_pump_unittest.cc:222: TEST_F(VideoFramePumpTest,
DeleteOnThreads) {
On 2015/02/12 21:59:15, Wez wrote:
> Add a test for actually capturing across threads?

Done.

[Sergey Ulanov, 2015-02-13 08:39:55]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-02-13 08:40:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883673004/80001

[commit-bot: I haz the power, 2015-02-13 08:58:50]

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

[commit-bot: I haz the power, 2015-02-13 08:58:51]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2015-02-13 17:28:06]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-02-13 17:28:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883673004/100001

[commit-bot: I haz the power, 2015-02-13 17:48:46]

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

[commit-bot: I haz the power, 2015-02-13 17:48:47]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2015-02-13 17:59:51]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-02-13 18:00:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883673004/120001

[commit-bot: I haz the power, 2015-02-13 18:45:38]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-02-13 18:46:50]

Patchset 5 (id:??) landed as
https://crrev.com/1afb35a1a63e53497245625b1052be436830a95f
Cr-Commit-Position: refs/heads/master@{#316247}