Add video frame refresh to MediaStream and VideoCapture stacks.

content/public/renderer/media_stream_video_sink.h

Index: content/public/renderer/media_stream_video_sink.h
diff --git a/content/public/renderer/media_stream_video_sink.h b/content/public/renderer/media_stream_video_sink.h
index b1dda9c46fa42c59f371efc1e966906642c5a68f..af3b33a5933931659286813127c1978bd0095c68 100644
--- a/content/public/renderer/media_stream_video_sink.h
+++ b/content/public/renderer/media_stream_video_sink.h
@@ -13,37 +13,51 @@
 #include "content/common/media/video_capture.h"
 #include "content/public/renderer/media_stream_sink.h"
 #include "media/base/video_capturer_source.h"
-
-namespace blink {
-class WebMediaStreamTrack;
-}
+#include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
 
 namespace content {
 
+class MediaStreamVideoTrack;
+
 // MediaStreamVideoSink is an interface used for receiving video frames from a
 // Video Stream Track or a Video Source.
 // http://dev.w3.org/2011/webrtc/editor/getusermedia.html
 // All methods calls will be done from the main render thread.

[ncarter (slow), 2016/04/06 17:18:26]

Can you add a comment here saying that this class is designed to be extended by
content embedders?

[miu, 2016/04/07 20:07:43]

Done.

 class CONTENT_EXPORT MediaStreamVideoSink : public MediaStreamSink {
- public:
-  // An implementation of MediaStreamVideoSink should call AddToVideoTrack when
+ protected:
+  MediaStreamVideoSink();
+  ~MediaStreamVideoSink() override;
+
+  // An implementation of MediaStreamVideoSink should call ConnectToTrack when
   // it is ready to receive data from a video track. Before the implementation
-  // is destroyed, RemoveFromVideoTrack must be called.
+  // is destroyed, DisconnectFromTrack must be called. This MediaStreamVideoSink
+  // base class holds a reference to the WebMediaStreamTrack until
+  // DisconnectFromTrack is called.
   //
   // Calls to these methods must be done on the main render thread.
   // Note that |callback| for frame delivery happens on the IO thread.
   //
-  // Calling RemoveFromVideoTrack also not stop frame delivery through the
-  // callback immediately because it may happen on another thread.
-  // The added callback will be reset on the render thread.
-  static void AddToVideoTrack(MediaStreamVideoSink* sink,
-                              const VideoCaptureDeliverFrameCB& callback,
-                              const blink::WebMediaStreamTrack& track);
-  static void RemoveFromVideoTrack(MediaStreamVideoSink* sink,
-                                   const blink::WebMediaStreamTrack& track);
+  // Warning: Calling DisconnectFromTrack does not immediately stop frame
+  // delivery through the |callback|, since frames are being delivered on a
+  // different thread.
+  void ConnectToTrack(const blink::WebMediaStreamTrack& track,
+                      const VideoCaptureDeliverFrameCB& callback);
+  void DisconnectFromTrack();

[ncarter (slow), 2016/04/06 17:18:26]

Though I understand what you're trying to do here, concrete methods and data
members in a content/public class are pretty unusual. (
https://www.chromium.org/developers/content-module/content-api )

That may be why the original class was structured as it was, with static
methods, though in this case, the concrete methods vs static methods that accept
a member as the first argument are are distinction without a difference.

But I see this as basically an observer interface, and it seems natural for
observers (e.g. WebContentsObserver) to have a data member that tracks the
observed object, and ConnectToTrack functions much like.

So I think this is actually okay.

[miu, 2016/04/07 20:07:43]

Ah, yes.  I reviewed the sites page, and I realized I was starting to put too
much implementation in the public class here, mostly for convenience.  I've
fixed the things mentioned in your other comments.

 
- protected:
-  ~MediaStreamVideoSink() override {}
+  // Returns true after ConnectToTrack() is called, and false after
+  // DisconnectFromTrack().
+  bool is_connected() const { return !!connected_track_; }

[ncarter (slow), 2016/04/06 17:18:26]

Could we expose this as connected_track(), returning a const
WebMediaStreamTrack&. The caller can then look at isNull() to determine the
connected status?

[miu, 2016/04/07 20:07:43]

Done.

+
+  // Requests the source send a frame "soon" (e.g., to resolve picture loss or
+  // quality issues).
+  void RequestRefreshFrame();

[ncarter (slow), 2016/04/06 17:18:26]

This is an operation on the track/source, not on the sink, so I don't think it
belongs in this class.

Our options for this would be:
 - Add it to WebMediaStreamTrack (not sure if that makes sense or not,
blink/content layering wise, but it feels tempting).
 - Add a content/public interface and Impl class for MediaStreamVideoTrack
(feels like overkill)
 - Move this to a static method in content/public/renderer/media_stream_utils.h
(feels like cheating, but it matches the precedent for
GetCurrentVideoTrackFormat())

[miu, 2016/04/07 20:07:43]

Done.  Went with the 3rd option.

+
+ private:
+  // Set by ConnectToTrack() and cleared by DisconnectFromTrack().  The object
+  // is not owned by MediaStreamVideoSink, but is guaranteed to remain valid
+  // until the disconnect because MediaStreamVideoTrack disallows its own
+  // destruction while sinks are still connected.
+  MediaStreamVideoTrack* connected_track_;

[ncarter (slow), 2016/04/06 17:18:26]

Could this be a WebMediaStreamTrack? It is preferred to avoid using
content-private types in content/public, even as forward declarations, since the
#include to pick up the full declaration would actually be illegal from this
header file.

[miu, 2016/04/07 20:07:43]

Done.

 };