Make cricket::VideoCapturer implement VideoSourceInterface

talk/media/base/videosourcebase.h

 /*
  * libjingle
- * Copyright 2004 Google Inc.
+ * Copyright 2016 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
  *     this list of conditions and the following disclaimer in the documentation
  *     and/or other materials provided with the distribution.
  *  3. The name of the author may not be used to endorse or promote products
  *     derived from this software without specific prior written permission.
  *
  * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED
  * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
  * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO
  * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
  * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS;
  * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
  * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
  * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef TALK_MEDIA_BASE_VIDEORENDERER_H_
-#define TALK_MEDIA_BASE_VIDEORENDERER_H_
+#ifndef TALK_MEDIA_BASE_VIDEOSOURCEBASE_H_
+#define TALK_MEDIA_BASE_VIDEOSOURCEBASE_H_
 
+#include <unordered_set>
+
+#include "talk/media/base/videoframe.h"
 #include "webrtc/media/base/videosinkinterface.h"
+#include "webrtc/media/base/videosourceinterface.h"
 
-namespace cricket {
+namespace rtc {
 
-class VideoFrame;
+class VideoSourceBase : public VideoSourceInterface<cricket::VideoFrame> {

[pthatcher1, 2016/02/03 15:38:36]

Can we call this thing something more explicit, like VideoBroadcaster?  

[perkj_webrtc, 2016/02/08 14:32:01]

Done.

+ public:
+  void AddSink(VideoSinkInterface<cricket::VideoFrame>* sink,
+               const VideoSinkCapabilities& capabilities) override;

[pthatcher1, 2016/02/03 15:38:36]

Why does AddSink take the capabilities instead of calling sink->capabilities()?

[perkj_webrtc, 2016/02/08 14:32:01]

Acknowledged.

+  void RemoveSink(VideoSinkInterface<cricket::VideoFrame>* sink) override;
 
-// Abstract interface for rendering VideoFrames.
-class VideoRenderer : public rtc::VideoSinkInterface<VideoFrame> {
- public:
-  virtual ~VideoRenderer() {}
-  // Called when a new frame is available for display.
-  virtual bool RenderFrame(const VideoFrame *frame) = 0;
-  // Intended to replace RenderFrame.
-  void OnFrame(const cricket::VideoFrame& frame) override {
-    // Unused return value
-    RenderFrame(&frame);
-  }
+  // TODO(perkj): Make DeliverFrameToSinks protected when the hack in
+  // FrameInputWrapper->DeliverFrameToSinks (videosource.cc) has been removed.
+  void DeliverFrameToSinks(const cricket::VideoFrame& frame);

[nisse-webrtc, 2016/02/03 09:16:34]

Is there any obvious drawback to rename this OnFrame, and make the class also
inherit VideoSinkInterface?

[pthatcher1, 2016/02/03 15:38:36]

Yes, I was thinking the same thing.  I think that makes sense.  

Of course, then you have to implement capability merging (assuming we stick with
that design), but I think that makes sense as well.

[perkj_webrtc, 2016/02/08 14:32:01]

Done.

+
+ protected:
+  bool HasSinks() const { return !sinks_.empty(); }
+
+  virtual void OnSinkCapabilitiesChanged(
+      const VideoSinkCapabilities& capabilities) = 0;
+
+ private:
+  CriticalSection sink_lock_;

[nisse-webrtc, 2016/02/03 09:16:34]

I guess this makes sense, if used as a base class. If instead used as a member,
I think it would make more sense to have the using class be responsible for
locking.

[pthatcher1, 2016/02/03 15:38:36]

As mentioned earlier, I'd like to see us try making it a member instead of a
base class.

[perkj_webrtc, 2016/02/08 14:32:01]

Done.

+  VideoSinkCapabilities capabilities_;
+  std::unordered_set<VideoSinkInterface<cricket::VideoFrame>*> sinks_;

[nisse-webrtc, 2016/02/03 09:16:34]

I think Tommi objected to using std::set earlier, since it implies unneeded
overhead. If we're not expecting more than a dozen sinks, std::vector or
std::list should be good enough, right?

[pthatcher1, 2016/02/03 15:38:36]

Throughout our code base, we almost alway use std::vector.  That's what I'd
prefer here.

[perkj_webrtc, 2016/02/08 14:32:01]

Done.

 };
 
-}  // namespace cricket
+}  // namespace rtc
 
-#endif  // TALK_MEDIA_BASE_VIDEORENDERER_H_
+#endif  // TALK_MEDIA_BASE_VIDEOSOURCEBASE_H_