CastExtension Impl for Chromoting Host

remoting/host/cast_extension_session.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_CAST_EXTENSION_SESSION_H_
+#define REMOTING_HOST_CAST_EXTENSION_SESSION_H_
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/timer/timer.h"
+#include "base/values.h"
+#include "jingle/glue/thread_wrapper.h"
+#include "remoting/base/auto_thread.h"
+#include "remoting/host/host_extension_session.h"
+#include "third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h"

[Wez, 2014/08/07 01:25:35]

Forward define?

[aiguha, 2014/08/12 01:42:39]

Done.

+#include "third_party/libjingle/source/talk/app/webrtc/peerconnectioninterface.h"
+#include "third_party/libjingle/source/talk/base/scoped_ref_ptr.h"
+#include "third_party/libjingle/source/talk/media/base/videocapturer.h"

[Wez, 2014/08/07 01:25:35]

Do you need this or could you forward-declare here?

[aiguha, 2014/08/12 01:42:40]

Left-over include actually. Removed!

+#include "third_party/webrtc/modules/desktop_capture/screen_capturer.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+class WaitableEvent;
+}  // namespace base
+
+namespace net {
+class URLRequestContextGetter;
+}  // namespace net
+
+namespace remoting {
+
+namespace protocol {
+class CursorShapeInfo;
+struct NetworkSettings;
+}  // namespace protocol
+
+// A class that extends HostExtensionSession to enable WebRTC support using
+// the PeerConnection native APIs.

[Wez, 2014/08/07 01:25:34]

Suggest "HostExtensionSession implementation that...."

[aiguha, 2014/08/12 01:42:39]

Done.

+class CastExtensionSession : public HostExtensionSession,
+                             public webrtc::PeerConnectionObserver,
+                             public webrtc::ScreenCapturer::MouseShapeObserver {
+ public:
+  virtual ~CastExtensionSession();
+  // Creates and returns a CastExtensionSession object, after performing

[Wez, 2014/08/07 01:25:35]

nit: Blank line between dtor and comment.

[aiguha, 2014/08/12 01:42:40]

Done.

+  // initialization steps on it. The caller must take ownership of the returned
+  // object.
+  static CastExtensionSession* Create(

[Wez, 2014/08/07 01:25:35]

scoped_ptr<CastExtensionSession>

[aiguha, 2014/08/12 01:42:40]

I thought returning a scoped_ptr would be better too, but I was following some
other factory method examples I'd seen. Anyway, changed it to return scoped_ptr.

[Wez, 2014/08/12 22:14:59]

Those probably just pre-date scoped_ptr<> being supported as a return-value!

[aiguha, 2014/08/13 18:33:27]

Acknowledged.

+      scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,

[Wez, 2014/08/07 01:25:36]

Is this TaskRunner used for WebRTC networking, or is it really the "caller"
TaskRunner?

[aiguha, 2014/08/12 01:42:39]

This TaskRunner is used to send/receive ExtensionMessages to/from the client.
Its thread is also wrapped to an rtc::Thread and used as the signaling thread.

[Wez, 2014/08/12 22:15:00]

That happens via the ClientStub, right? So from this class' point of view, it's
the caller's thread?

[aiguha, 2014/08/13 18:33:27]

I see your point. Renamed this the caller_task_runner_ and the corresponding
thread wrapper is now signaling_thread_wrapper_.

+      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+      const protocol::NetworkSettings& network_settings,
+      ClientSessionControl* client_session_control,
+      protocol::ClientStub* client_stub);
+
+  // HostExtensionSession implementation.

[Wez, 2014/08/07 01:25:35]

nit: Elsewhere in remoting we use "<Foo> interface." not "<Foo> implementation."

[aiguha, 2014/08/12 01:42:39]

Acknowledged.

+  virtual bool OnExtensionMessage(
+      ClientSessionControl* client_session_control,
+      protocol::ClientStub* client_stub,
+      const protocol::ExtensionMessage& message) OVERRIDE;
+  virtual bool ModifiesVideoPipeline() const OVERRIDE;
+  virtual scoped_ptr<webrtc::ScreenCapturer> OnCreateVideoCapturer(
+      scoped_ptr<webrtc::ScreenCapturer> capturer) OVERRIDE;

[Wez, 2014/08/07 01:25:34]

nit: It's usually a good idea to list interface overrides in the same order as
in the interface's declaration.

[aiguha, 2014/08/12 01:42:40]

Done.

+
+  // PeerConnectionObserver implementation.

[Wez, 2014/08/07 01:25:35]

nit: You mean webrtc::PeerConnectionObserver?

[aiguha, 2014/08/12 01:42:41]

Yes, my mistake. Fixed!

+  virtual void OnError() OVERRIDE;
+  virtual void OnSignalingChange(
+      webrtc::PeerConnectionInterface::SignalingState new_state) OVERRIDE;
+  virtual void OnStateChange(
+      webrtc::PeerConnectionObserver::StateType state_changed) OVERRIDE;
+  virtual void OnAddStream(webrtc::MediaStreamInterface* stream) OVERRIDE;
+  virtual void OnRemoveStream(webrtc::MediaStreamInterface* stream) OVERRIDE;
+  virtual void OnDataChannel(
+      webrtc::DataChannelInterface* data_channel) OVERRIDE;
+  virtual void OnRenegotiationNeeded() OVERRIDE;
+  virtual void OnIceConnectionChange(
+      webrtc::PeerConnectionInterface::IceConnectionState new_state) OVERRIDE;
+  virtual void OnIceGatheringChange(
+      webrtc::PeerConnectionInterface::IceGatheringState new_state) OVERRIDE;
+  virtual void OnIceCandidate(
+      const webrtc::IceCandidateInterface* candidate) OVERRIDE;
+  virtual void OnIceComplete() OVERRIDE;
+
+  // MouseShapeObserver implementation.

[Wez, 2014/08/07 01:25:34]

Suggest moving this interface definition up nearer the HostExtensionSession
interface, to keep the remoting:: and talk_base::/webrtc:: interface overrides
grouped together.

[aiguha, 2014/08/12 01:42:39]

Done.

+  virtual void OnCursorShapeChanged(
+      webrtc::MouseCursorShape* cursor_shape) OVERRIDE;
+
+  // Called by webrtc::CreateSessionDescriptionObserver implementation.
+  void OnSuccess(webrtc::SessionDescriptionInterface* desc);
+  void OnFailure(const std::string& error);

[Wez, 2014/08/07 01:25:35]

Suggest renaming these "OnSessionDescription" and "OnSessionDescriptionError".
You might also make them private.

[Wez, 2014/08/07 01:25:35]

So are they part of some specific interface? Or do we Bind() them into callbacks
somewhere? If the latter then I suggest putting them at the top, following the
Create() method, so that all the class-specific methods are grouped ahead of the
interface overrides.

[aiguha, 2014/08/12 01:42:39]

Done.

[aiguha, 2014/08/12 01:42:41]

They're not part of an interface, but callbacks (called directly, not binded)
that the nested class impl of webrtc::CreateSessionDescriptionObserver call. 

I used a nested class because webrtc::CreateSessionDescriptionObserver
implements rtc::RefCountInterface, but CastExtensionSession itself can't be ref
counted.

+
+ private:
+  CastExtensionSession(
+      scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+      const protocol::NetworkSettings& network_settings,
+      ClientSessionControl* client_session_control,
+      protocol::ClientStub* client_stub);
+
+  // Parses |message| for a Session Description and sets the remote
+  // description, returning true if successful.
+  bool ParseAndSetRemoteDescription(base::DictionaryValue* message);

[Wez, 2014/08/07 01:25:35]

Could this be a const base::DictionaryValue&, here and below?

[aiguha, 2014/08/12 01:42:40]

No, because we use GetDictionary() which is not marked const.

+
+  // Parses |message| for a PeerConnection ICE candidate and adds it to the
+  // Peer Connection, returning true if successful.
+  bool ParseAndAddICECandidate(base::DictionaryValue* message);
+
+  // Sends messages to client using the ClientStub through |client_session_|.

[Wez, 2014/08/07 01:25:35]

s/messages/a message?

[Wez, 2014/08/07 01:25:36]

you don't have |client_session_| any more, so I think you mean "to the client
through |client_stub_|"?

[aiguha, 2014/08/12 01:42:40]

Yes, forgot to update the comment, thanks!

[aiguha, 2014/08/12 01:42:41]

Done.

+  // |data| should be json formatted. This method must be called on the
+  // network thread.

[Wez, 2014/08/07 01:25:35]

s/network/caller

[Wez, 2014/08/07 01:25:36]

What are the threading requirements of all the other threads?

Let's discuss.

[aiguha, 2014/08/12 01:42:39]

The only threading requirements are that chromoting-host-facing methods
(SendMessageToClient(), SendCursorShape(), OnExtensionMessage()) need to happen
on the network (i.e., signaling)) thread. Sure, we can talk offline today if
you'd like.

[Wez, 2014/08/12 22:14:59]

This extension session object is an "owned" object, though, so there must be
some expectations and requirements on which methods are accessed from which
threads, surely?

[aiguha, 2014/08/13 18:33:27]

Yes, we've got the following:
network thread (caller): all host-facing methods - ResetVideoPipeline() ,
DeliverHostMessage() and SetCursorShape()(being removed) - are called on this
thread. PeerConnection::Close() should also be called on this thread.

worker thread (self-owned): Used internally by PeerConnection. The
CastVideoCapturerAdapter's Start() and Stop() will be called on this thread
(though that doesn't affect us, as long as it is the same thread).

If I'm misunderstanding your question, let's definitely talk tomorrow :)

+  bool SendMessageToClient(const char* subject, const std::string& data);

[Wez, 2014/08/07 01:25:34]

What does the |subject| parameter mean? Why is it const char* rather than const
std::string&?
Why does |data| need to be JSON formatted? Aren't extension messages opaque
strings? If you want |data| to be formatted from JSON then why not have the
caller pass in a base::Value and serialize it in the Send method, so it's
guaranteed to output JSON?

[aiguha, 2014/08/12 01:42:41]

It's now std::string&, thanks for pointing that out.

The |subject| parameter is just part of a little sub-specification I created for
the Cast-communication. We use the ExtensionMessages to send both control and
WebRTC session negotiation messages. The |subject| specifies the message's
"subject". The host and client use the same list of subjects to specify the
communication. (I'll be documenting this). 

[Wez, 2014/08/12 22:14:59]

OK; I think the comment here should clarify what the |subject| means (beyond
saying it specifies the "subject" ;)

[aiguha, 2014/08/13 18:33:27]

Totally agree! I realized that this is the best place for the documentation of
the communication spec, so I've described it completely in comments in the next
patch.

+
+  // Sends the new |cursor_shape| using the ClientStub through
+  // |client_session_|.

[Wez, 2014/08/07 01:25:34]

Must also be called on the caller thread?

[aiguha, 2014/08/12 01:42:40]

Yes, will update!

+  void SendCursorShape(scoped_ptr<protocol::CursorShapeInfo> cursor_shape);
+
+  // Creates the jingle wrapper for the current thread, sets send to allowed,
+  // and saves a pointer to the relevant thread pointer in ptr. If |event|
+  // is not NULL, signals the event on completion.

[Wez, 2014/08/07 01:25:35]

Why would you ever not want to wait for completion?

[aiguha, 2014/08/12 01:42:40]

We need to EnsureTaskAndSetSend() for both threads. We first call it on the
current thread, so we don't need to be notified because it's synchronous. We
then PostTask this on the other thread, so pass in |event| to be notified when
it completes.

+  void EnsureTaskAndSetSend(talk_base::Thread** ptr,
+                            base::WaitableEvent* event = NULL);
+
+  // Wraps each task runner in JingleThreadWrapper using EnsureTaskAndSetSend(),
+  // returning true if successful. Wrapping the task runners allows them to be
+  // shared with and used by the (about to be created) PeerConnectionFactory.
+  bool WrapTasksAndSave();
+
+  // Initializes PeerConnectionFactory and PeerConnection and sends a "ready"
+  // message to client. Returns true if these steps are performed successfully.
+  bool InitializePeerConnection();
+
+  // Sets |this| as the MouseShapeObserver of |screen_capturer|. Constructs a

[Wez, 2014/08/07 01:25:34]

How is becoming the MouseShapeObserver part of setting up the video stream?

[aiguha, 2014/08/12 01:42:39]

SetupVideoStream() after we successfully grab the ScreenCapturer in
OnCreateVideoCapturer(). But I agree, I'll move the line of code to
OnCreateVideoCapturer() itself.

+  // VideoSource, a VideoTrack and a MediaStream |stream_|, which it adds to
+  // the |peer_connection_|. Returns true if these steps are performed
+  // successfully. This method is called when only when a PeerConnection offer
+  // is received from the client.
+  bool SetupVideoStream(scoped_ptr<webrtc::ScreenCapturer> screen_capturer);
+
+  // Polls a single stats report from the PeerConnection immediately. Called
+  // periodically using |stats_polling_timer_| after a PeerConnection has been
+  // established.
+  void PollPeerConnectionStats();
+
+  // Explicitly sets |peer_conn_factory_|, |peer_connection_| and |stream_| to
+  // NULL.

[Wez, 2014/08/07 01:25:34]

They are all ref-counted, though, so this won't necessarily delete them, just
release our references to them?

[aiguha, 2014/08/12 01:42:40]

Agreed. Rename to CleanupPeerConnection()?

+  void DeletePeerConnection();
+
+  // Check if the connection is active.
+  bool connection_active() const;
+
+  // TaskRunner that will be used to setup the PeerConnectionFactory's
+  // signalling thread.
+  scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;

[Wez, 2014/08/07 01:25:34]

Suggest keeping the network and worker task runners near each other, for
clarity.

[aiguha, 2014/08/12 01:42:40]

Done.

+
+  // Objects related to the WebRTC PeerConnection.
+  talk_base::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_;
+  talk_base::scoped_refptr<webrtc::PeerConnectionFactoryInterface>
+      peer_conn_factory_;
+  talk_base::scoped_refptr<webrtc::MediaStreamInterface> stream_;
+
+  // Parameters passed to ChromiumPortAllocatorFactory on creation.
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
+  const protocol::NetworkSettings& network_settings_;
+
+  // Interface to interact with the ClientSession.
+  ClientSessionControl* client_session_control_;
+
+  // Interface through which messages can be sent to the client.
+  protocol::ClientStub* client_stub_;
+
+  // Used to track webrtc connection statistics.
+  talk_base::scoped_refptr<webrtc::StatsObserver> stats_observer_;
+
+  // Used to repeatedly poll stats from the |peer_connection_|.
+  base::RepeatingTimer<CastExtensionSession> stats_polling_timer_;
+
+  // True if a PeerConnection offer from the client has been received. This
+  // necessarily means that the host is not the caller in this attempted
+  // peer connection.
+  bool received_offer_;
+
+  // True if the webrtc::ScreenCapturer has been grabbed through the
+  // OnCreateVideoCapturer() callback.
+  bool has_grabbed_capturer_;
+
+  // Jingle Thread wrappers for talk_base::Thread, to be used with
+  // PeerConnection API as well as |this| for signalling and worker
+  // threads.
+  // Created by calling jingle_glue::EnsureForCurrentMessageLoop() and thus
+  // deletes itself automatically when the associated MessageLoop is destroyed.
+  talk_base::Thread* network_thread_wrapper_;
+  talk_base::Thread* worker_thread_wrapper_;
+
+  scoped_refptr<AutoThreadTaskRunner> worker_task_runner_;
+
+  scoped_ptr<AutoThread> worker_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(CastExtensionSession);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_CAST_EXTENSION_SESSION_H_