DesktopSessionAgent now hosts the video capturer and provides necessary plumbing to drive it via an…

remoting/host/desktop_session_agent.h

Index: remoting/host/desktop_session_agent.h
diff --git a/remoting/host/desktop_session_agent.h b/remoting/host/desktop_session_agent.h
index 569dfd56bd74040e6f0e0013e7e72347efabdc8e..483c65f51fb57879690fff8437ad25ae49615a00 100644
--- a/remoting/host/desktop_session_agent.h
+++ b/remoting/host/desktop_session_agent.h
@@ -5,6 +5,8 @@
 #ifndef REMOTING_HOST_DESKTOP_SESSION_AGENT_H_
 #define REMOTING_HOST_DESKTOP_SESSION_AGENT_H_
 
+#include <list>
+
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
@@ -12,6 +14,11 @@
 #include "base/memory/scoped_ptr.h"
 #include "ipc/ipc_listener.h"
 #include "ipc/ipc_platform_file.h"
+#include "remoting/base/shared_buffer.h"
+#include "remoting/host/shared_buffer_sender.h"
+#include "remoting/host/video_frame_capturer.h"
+#include "third_party/skia/include/core/SkRect.h"
+#include "third_party/skia/include/core/SkRegion.h"
 
 namespace IPC {
 class ChannelProxy;
@@ -24,29 +31,50 @@ class AutoThreadTaskRunner;
 
 // Provides screen/audio capturing and input injection services for
 // the network process.
-class DesktopSessionAgent : public IPC::Listener {
+class DesktopSessionAgent
+    : public base::RefCountedThreadSafe<DesktopSessionAgent>,
+      public IPC::Listener,
+      public SharedBufferSender,
+      public VideoFrameCapturer::Delegate {
  public:
-  static scoped_ptr<DesktopSessionAgent> Create(
+  static scoped_refptr<DesktopSessionAgent> Create(
       scoped_refptr<AutoThreadTaskRunner> caller_task_runner,
-      scoped_refptr<AutoThreadTaskRunner> io_task_runner);
-
-  virtual ~DesktopSessionAgent();
+      scoped_refptr<AutoThreadTaskRunner> io_task_runner,
+      scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner);
 
   // IPC::Listener implementation.
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
   virtual void OnChannelConnected(int32 peer_pid) OVERRIDE;
   virtual void OnChannelError() OVERRIDE;
 
+  // SharedBufferSender implementation.
+  virtual void RegisterSharedBuffer(
+      scoped_refptr<SharedBuffer> buffer) OVERRIDE;
+  virtual void DropSharedBuffer(intptr_t id) OVERRIDE;
+
+  // VideoFrameCapturer::Delegate implementation.

[Wez, 2012/11/16 23:37:31]

We should follow up to re-name VideoFrameCapturer::Delegate, since it's not a
delegate.

[alexeypa (please no reviews), 2012/11/19 21:46:25]

Ack.

+  virtual void OnCaptureCompleted(
+      scoped_refptr<CaptureData> capture_data) OVERRIDE;
+  virtual void OnCursorShapeChanged(
+      scoped_ptr<protocol::CursorShapeInfo> cursor_shape) OVERRIDE;
+
   // Creates the screen/audio recorders, input stubs and the IPC channel to be
   // used to access them. Returns a handle of the client end of the IPC channel
   // pipe to be forwarder to the corresponding desktop environment.

[Wez, 2012/11/16 23:37:31]

typo: forwarded.

I'd suggest rewording this to make it clearer that this function creates the
desktop-integration components, then creates a pipe over which to receive IPCs
for them, returning the client end to pass to the network process.

Why do we have separate |done_task| and |desktop_pipe_out|, rather than a single
callback that includes |desktop_pipe_out|?

The comment should also explain what |done_task| is and how it relates to the
bool return value.

[alexeypa (please no reviews), 2012/11/19 21:46:25]

Done.
|done_task| is not part of starting/stopping of this object.
Added a comment and renamed |done_task| to |disconnect_task| to avoid this
confusion.

   bool Start(const base::Closure& done_task,
              IPC::PlatformFileForTransit* desktop_pipe_out);
 
+  // Stops the agent asynchronously.

[Wez, 2012/11/16 23:37:31]

How does the caller know when this object is completely destroyed?  Or does it
not care?  Based on the comment on |done_task|, it sounds like this method
should take |done_task|, not Start?

[alexeypa (please no reviews), 2012/11/19 21:46:25]

I renamed |done_task| so it is not getting associated with Start() and Stop().
The caller does not care when |this| is destroyed.

+  void Stop();
+
  protected:
   DesktopSessionAgent(
       scoped_refptr<AutoThreadTaskRunner> caller_task_runner,
-      scoped_refptr<AutoThreadTaskRunner> io_task_runner);
+      scoped_refptr<AutoThreadTaskRunner> io_task_runner,
+      scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner);
+
+  friend class base::RefCountedThreadSafe<DesktopSessionAgent>;
+  virtual ~DesktopSessionAgent();
 
   // Creates a pre-connected IPC channel to be used to access the screen/audio

[Wez, 2012/11/16 23:37:31]

nit: What does "pre-connected" mean?  Do you just mean "connected"?

[alexeypa (please no reviews), 2012/11/19 21:46:25]

Done.

   // recorders and input stubs.
@@ -54,6 +82,24 @@ class DesktopSessionAgent : public IPC::Listener {
       IPC::PlatformFileForTransit* client_out,
       scoped_ptr<IPC::ChannelProxy>* server_out) = 0;
 
+  void InvalidateRegion(scoped_ptr<SkRegion> invalid_region);

[Wez, 2012/11/16 23:37:31]

Why not fold this in to OnInvalidateRegion?

[alexeypa (please no reviews), 2012/11/19 21:46:25]

To avoid creating a copy of a bunch of rects. OnInvalidateRegion converts
std::vector<SkIRect> into SkRegion allocated on heap. If I defer this conversion
until OnInvalidateRegion is running on  video_capture_task_runner() an
additional copy of the vector will be created.

+
+  // Implements VideoFrameCapturer::CaptureInvalidRegion.

[Wez, 2012/11/16 23:37:31]

nit: You mean "Implements the CaptureInvalidRegion IPC." or "Handles
CaptureInvalidRegion requests from the client." I think?

[alexeypa (please no reviews), 2012/11/19 21:46:25]

Done.

+  void OnCaptureFrame();
+
+  // Implements VideoFrameCapturer::InvalidateRegion.
+  void OnInvalidateRegion(const std::vector<SkIRect>& invalid_region);
+
+  // Handles the ChromotingNetworkDesktopMsg_SharedBufferRegistered notification
+  // from the network process.
+  void OnSharedBufferRegistered(intptr_t id);
+
+  // Sends a message to the network process.
+  void SendToNetwork(IPC::Message* message);
+
+  void StartVideoCapturer();
+  void StopVideoCapturer();

[Wez, 2012/11/16 23:37:31]

nit: Add comments to explain the function of these.

[alexeypa (please no reviews), 2012/11/19 21:46:25]

Done. What there methods do is documented by their names, so I'm not sure how
useful these comments are.

[Wez, 2012/11/20 07:05:39]

You only need these methods so you have something to bind to post to the capture
thread, so I'd suggest "Posted to the capture thread to start the capturer", for
example.

[alexeypa (please no reviews), 2012/11/20 20:15:17]

Done.

+
   scoped_refptr<AutoThreadTaskRunner> caller_task_runner() const {

[Wez, 2012/11/16 23:37:31]

Add a comment explaining that these getters provide platform-specific derived
classes access to the task runners.

[alexeypa (please no reviews), 2012/11/19 21:46:25]

I don't see any value in such a comment. There are getters which is obvious from
their naming and inline nature. They are defined in protected section, so it is
fairly obvious that only derived classes can use them.

[Wez, 2012/11/20 07:05:39]

Right; a comment should describe why we need them, not what they are, i.e.
clarify that they're here for derived classes to use, not just something that
got missed in a refactoring. :)

[alexeypa (please no reviews), 2012/11/20 20:15:17]

I added the comment but I still think it is not super useful.

     return caller_task_runner_;
   }
@@ -62,6 +108,10 @@ class DesktopSessionAgent : public IPC::Listener {
     return io_task_runner_;
   }
 
+  scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner() const {
+    return video_capture_task_runner_;
+  }
+
  private:
   // Task runner on which public methods of this class should be called.
   scoped_refptr<AutoThreadTaskRunner> caller_task_runner_;
@@ -69,6 +119,9 @@ class DesktopSessionAgent : public IPC::Listener {
   // Message loop used by the IPC channel.
   scoped_refptr<AutoThreadTaskRunner> io_task_runner_;
 
+  // Task runner on which video public methods of this class should be called.

[Wez, 2012/11/16 23:37:31]

Is this correct? Isn't this the thread on which this object will call out to the
video capturer?

[alexeypa (please no reviews), 2012/11/19 21:46:25]

Done.

+  scoped_refptr<AutoThreadTaskRunner> video_capture_task_runner_;
+
   // Run on |caller_task_runner_| to notify the caller that |this| has been
   // stopped.
   base::Closure done_task_;
@@ -76,6 +129,12 @@ class DesktopSessionAgent : public IPC::Listener {
   // IPC channel connecting the desktop process with the network process.
   scoped_ptr<IPC::ChannelProxy> network_channel_;
 
+  // List of the shared buffers registered via |SharedBufferSender| interface.
+  typedef std::list<scoped_refptr<SharedBuffer> > SharedBuffers;
+  SharedBuffers shared_buffers_;
+
+  scoped_ptr<VideoFrameCapturer> video_capturer_;
+
   DISALLOW_COPY_AND_ASSIGN(DesktopSessionAgent);
 };