From db25d2e8c57b14b14111ee6ab4a5cb6372142155 Mon Sep 17 00:00:00 2001 From: nisse Date: Fri, 26 Feb 2016 01:24:58 -0800 Subject: [PATCH] Make VideoTrack and VideoTrackRenderers implement rtc::VideoSourceInterface. This patch tries to only change the interface to VideoTrack, with minimal changes to the implementation. Some points worth noting: VideoTrackRenderers should ultimately be deleted, but it is kept for now since we need an object implementing webrtc::VideoRenderer, and that shouldn't be VideoTrack. BUG=webrtc:5426 TBR=glaznev@webrtc.org // please look at examples Review URL: https://codereview.webrtc.org/1684423002 Cr-Commit-Position: refs/heads/master@{#11775} --- webrtc/api/mediastreaminterface.h | 29 ++++++++++--- webrtc/api/mediastreamtrackproxy.h | 5 +++ webrtc/api/test/fakevideotrackrenderer.h | 41 +++++++++++++----- webrtc/api/videotrack.cc | 14 ++++--- webrtc/api/videotrack.h | 10 +++-- webrtc/api/videotrack_unittest.cc | 42 +++++++++++++++++++ webrtc/api/videotrackrenderers.cc | 29 +++++++------ webrtc/api/videotrackrenderers.h | 14 ++++--- .../peerconnection/client/linux/main_wnd.cc | 12 +++--- .../peerconnection/client/linux/main_wnd.h | 4 +- 10 files changed, 148 insertions(+), 52 deletions(-) diff --git a/webrtc/api/mediastreaminterface.h b/webrtc/api/mediastreaminterface.h index d4fe2bf0eb..100db08d04 100644 --- a/webrtc/api/mediastreaminterface.h +++ b/webrtc/api/mediastreaminterface.h @@ -24,6 +24,7 @@ #include "webrtc/base/refcount.h" #include "webrtc/base/scoped_ref_ptr.h" #include "webrtc/media/base/videosinkinterface.h" +#include "webrtc/media/base/videosourceinterface.h" namespace cricket { @@ -121,12 +122,30 @@ class VideoRendererInterface class VideoSourceInterface; -class VideoTrackInterface : public MediaStreamTrackInterface { +class VideoTrackInterface + : public MediaStreamTrackInterface, + public rtc::VideoSourceInterface { public: - // Register a renderer that will render all frames received on this track. - virtual void AddRenderer(VideoRendererInterface* renderer) = 0; - // Deregister a renderer. - virtual void RemoveRenderer(VideoRendererInterface* renderer) = 0; + // Make an unqualified VideoSourceInterface resolve to + // webrtc::VideoSourceInterface, not our base class + // rtc::VideoSourceInterface. + using VideoSourceInterface = webrtc::VideoSourceInterface; + + // AddRenderer and RemoveRenderer are for backwards compatibility + // only. They are obsoleted by the methods of + // rtc::VideoSourceInterface. + virtual void AddRenderer(VideoRendererInterface* renderer) { + AddOrUpdateSink(renderer, rtc::VideoSinkWants()); + } + virtual void RemoveRenderer(VideoRendererInterface* renderer) { + RemoveSink(renderer); + } + + // Register a video sink for this track. + void AddOrUpdateSink(rtc::VideoSinkInterface* sink, + const rtc::VideoSinkWants& wants) override{}; + void RemoveSink( + rtc::VideoSinkInterface* sink) override{}; virtual VideoSourceInterface* GetSource() const = 0; diff --git a/webrtc/api/mediastreamtrackproxy.h b/webrtc/api/mediastreamtrackproxy.h index 5b29a0efa0..dca2d15561 100644 --- a/webrtc/api/mediastreamtrackproxy.h +++ b/webrtc/api/mediastreamtrackproxy.h @@ -48,6 +48,11 @@ BEGIN_PROXY_MAP(VideoTrack) PROXY_METHOD1(void, AddRenderer, VideoRendererInterface*) PROXY_METHOD1(void, RemoveRenderer, VideoRendererInterface*) + PROXY_METHOD2(void, + AddOrUpdateSink, + rtc::VideoSinkInterface*, + const rtc::VideoSinkWants&) + PROXY_METHOD1(void, RemoveSink, rtc::VideoSinkInterface*) PROXY_CONSTMETHOD0(VideoSourceInterface*, GetSource) PROXY_METHOD0(rtc::VideoSinkInterface*, GetSink) diff --git a/webrtc/api/test/fakevideotrackrenderer.h b/webrtc/api/test/fakevideotrackrenderer.h index 816b44744f..387ce51b42 100644 --- a/webrtc/api/test/fakevideotrackrenderer.h +++ b/webrtc/api/test/fakevideotrackrenderer.h @@ -16,18 +16,43 @@ namespace webrtc { -class FakeVideoTrackRenderer : public VideoRendererInterface { +class FakeVideoTrackRenderer + : public rtc::VideoSinkInterface { public: FakeVideoTrackRenderer(VideoTrackInterface* video_track) - : video_track_(video_track), last_frame_(NULL) { - video_track_->AddRenderer(this); + : video_track_(video_track) { + video_track_->AddOrUpdateSink(this, rtc::VideoSinkWants()); } - ~FakeVideoTrackRenderer() { - video_track_->RemoveRenderer(this); + ~FakeVideoTrackRenderer() { video_track_->RemoveSink(this); } + + virtual void OnFrame(const cricket::VideoFrame& video_frame) override { + fake_renderer_.RenderFrame(&video_frame); } + int errors() const { return fake_renderer_.errors(); } + int width() const { return fake_renderer_.width(); } + int height() const { return fake_renderer_.height(); } + bool black_frame() const { return fake_renderer_.black_frame(); } + + int num_rendered_frames() const { + return fake_renderer_.num_rendered_frames(); + } + + private: + cricket::FakeVideoRenderer fake_renderer_; + rtc::scoped_refptr video_track_; +}; + +// Similar class, testing the deprecated AddRenderer/RemoveRenderer methods. +class FakeVideoTrackRendererOld : public VideoRendererInterface { + public: + FakeVideoTrackRendererOld(VideoTrackInterface* video_track) + : video_track_(video_track) { + video_track_->AddRenderer(this); + } + ~FakeVideoTrackRendererOld() { video_track_->RemoveRenderer(this); } + virtual void RenderFrame(const cricket::VideoFrame* video_frame) override { - last_frame_ = const_cast(video_frame); fake_renderer_.RenderFrame(video_frame); } @@ -39,14 +64,10 @@ class FakeVideoTrackRenderer : public VideoRendererInterface { int num_rendered_frames() const { return fake_renderer_.num_rendered_frames(); } - const cricket::VideoFrame* last_frame() const { return last_frame_; } private: cricket::FakeVideoRenderer fake_renderer_; rtc::scoped_refptr video_track_; - - // Weak reference for frame pointer comparison only. - cricket::VideoFrame* last_frame_; }; } // namespace webrtc diff --git a/webrtc/api/videotrack.cc b/webrtc/api/videotrack.cc index 577ac68498..11a0177c77 100644 --- a/webrtc/api/videotrack.cc +++ b/webrtc/api/videotrack.cc @@ -33,12 +33,15 @@ std::string VideoTrack::kind() const { return kVideoKind; } -void VideoTrack::AddRenderer(VideoRendererInterface* renderer) { - renderers_.AddRenderer(renderer); +void VideoTrack::AddOrUpdateSink( + rtc::VideoSinkInterface* sink, + const rtc::VideoSinkWants& wants) { + renderers_.AddOrUpdateSink(sink, wants); } -void VideoTrack::RemoveRenderer(VideoRendererInterface* renderer) { - renderers_.RemoveRenderer(renderer); +void VideoTrack::RemoveSink( + rtc::VideoSinkInterface* sink) { + renderers_.RemoveSink(sink); } rtc::VideoSinkInterface* VideoTrack::GetSink() { @@ -51,7 +54,8 @@ bool VideoTrack::set_enabled(bool enable) { } rtc::scoped_refptr VideoTrack::Create( - const std::string& id, VideoSourceInterface* source) { + const std::string& id, + VideoSourceInterface* source) { rtc::RefCountedObject* track = new rtc::RefCountedObject(id, source); return track; diff --git a/webrtc/api/videotrack.h b/webrtc/api/videotrack.h index f2b8f19c5b..1964cb4898 100644 --- a/webrtc/api/videotrack.h +++ b/webrtc/api/videotrack.h @@ -22,11 +22,13 @@ namespace webrtc { class VideoTrack : public MediaStreamTrack { public: - static rtc::scoped_refptr Create( - const std::string& label, VideoSourceInterface* source); + static rtc::scoped_refptr Create(const std::string& label, + VideoSourceInterface* source); + + void AddOrUpdateSink(rtc::VideoSinkInterface* sink, + const rtc::VideoSinkWants& wants) override; + void RemoveSink(rtc::VideoSinkInterface* sink) override; - virtual void AddRenderer(VideoRendererInterface* renderer); - virtual void RemoveRenderer(VideoRendererInterface* renderer); virtual VideoSourceInterface* GetSource() const { return video_source_.get(); } diff --git a/webrtc/api/videotrack_unittest.cc b/webrtc/api/videotrack_unittest.cc index 0f82e1830b..df412589c7 100644 --- a/webrtc/api/videotrack_unittest.cc +++ b/webrtc/api/videotrack_unittest.cc @@ -21,6 +21,7 @@ #include "webrtc/pc/channelmanager.h" using webrtc::FakeVideoTrackRenderer; +using webrtc::FakeVideoTrackRendererOld; using webrtc::VideoSource; using webrtc::VideoTrack; using webrtc::VideoTrackInterface; @@ -86,6 +87,47 @@ TEST_F(VideoTrackTest, RenderVideo) { EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(1, renderer_2->num_rendered_frames()); + video_track_->RemoveSink(renderer_1.get()); + renderer_input->OnFrame(frame); + + EXPECT_EQ(2, renderer_1->num_rendered_frames()); + EXPECT_EQ(2, renderer_2->num_rendered_frames()); +} + +// Test adding renderers to a video track and render to them by +// providing frames to the source. Uses the old VideoTrack interface +// with AddRenderer and RemoveRenderer. +TEST_F(VideoTrackTest, RenderVideoOld) { + // FakeVideoTrackRenderer register itself to |video_track_| + rtc::scoped_ptr renderer_1( + new FakeVideoTrackRendererOld(video_track_.get())); + + rtc::VideoSinkInterface* renderer_input = + video_track_->GetSink(); + ASSERT_FALSE(renderer_input == NULL); + + cricket::WebRtcVideoFrame frame; + frame.InitToBlack(123, 123, 0); + renderer_input->OnFrame(frame); + EXPECT_EQ(1, renderer_1->num_rendered_frames()); + + EXPECT_EQ(123, renderer_1->width()); + EXPECT_EQ(123, renderer_1->height()); + + // FakeVideoTrackRenderer register itself to |video_track_| + rtc::scoped_ptr renderer_2( + new FakeVideoTrackRenderer(video_track_.get())); + + renderer_input->OnFrame(frame); + + EXPECT_EQ(123, renderer_1->width()); + EXPECT_EQ(123, renderer_1->height()); + EXPECT_EQ(123, renderer_2->width()); + EXPECT_EQ(123, renderer_2->height()); + + EXPECT_EQ(2, renderer_1->num_rendered_frames()); + EXPECT_EQ(1, renderer_2->num_rendered_frames()); + video_track_->RemoveRenderer(renderer_1.get()); renderer_input->OnFrame(frame); diff --git a/webrtc/api/videotrackrenderers.cc b/webrtc/api/videotrackrenderers.cc index 4800d3b5c8..9e928efbf1 100644 --- a/webrtc/api/videotrackrenderers.cc +++ b/webrtc/api/videotrackrenderers.cc @@ -19,17 +19,21 @@ VideoTrackRenderers::VideoTrackRenderers() : enabled_(true) { VideoTrackRenderers::~VideoTrackRenderers() { } -void VideoTrackRenderers::AddRenderer(VideoRendererInterface* renderer) { - if (!renderer) { - return; - } +void VideoTrackRenderers::AddOrUpdateSink( + VideoSinkInterface* sink, + const rtc::VideoSinkWants& wants) { + // TODO(nisse): Currently ignores wants. We should somehow use + // VideoBroadcaster, but we need to sort out its threading issues + // first. rtc::CritScope cs(&critical_section_); - renderers_.insert(renderer); + if (std::find(sinks_.begin(), sinks_.end(), sink) == sinks_.end()) + sinks_.push_back(sink); } -void VideoTrackRenderers::RemoveRenderer(VideoRendererInterface* renderer) { +void VideoTrackRenderers::RemoveSink( + VideoSinkInterface* sink) { rtc::CritScope cs(&critical_section_); - renderers_.erase(renderer); + sinks_.erase(std::remove(sinks_.begin(), sinks_.end(), sink), sinks_.end()); } void VideoTrackRenderers::SetEnabled(bool enable) { @@ -41,7 +45,7 @@ bool VideoTrackRenderers::RenderFrame(const cricket::VideoFrame* frame) { { rtc::CritScope cs(&critical_section_); if (enabled_) { - RenderFrameToRenderers(frame); + RenderFrameToSinks(*frame); return true; } } @@ -64,17 +68,16 @@ bool VideoTrackRenderers::RenderFrame(const cricket::VideoFrame* frame) { // enabled while we generated the black frame. I think the // enabled-ness ought to be applied at the track output, and hence // an enabled track shouldn't send any blacked out frames. - RenderFrameToRenderers(enabled_ ? frame : &black); + RenderFrameToSinks(enabled_ ? *frame : black); return true; } } // Called with critical_section_ already locked -void VideoTrackRenderers::RenderFrameToRenderers( - const cricket::VideoFrame* frame) { - for (VideoRendererInterface* renderer : renderers_) { - renderer->RenderFrame(frame); +void VideoTrackRenderers::RenderFrameToSinks(const cricket::VideoFrame& frame) { + for (auto sink : sinks_) { + sink->OnFrame(frame); } } diff --git a/webrtc/api/videotrackrenderers.h b/webrtc/api/videotrackrenderers.h index 46f0c2f56e..04e1ad1798 100644 --- a/webrtc/api/videotrackrenderers.h +++ b/webrtc/api/videotrackrenderers.h @@ -25,7 +25,9 @@ namespace webrtc { // Each VideoTrack owns a VideoTrackRenderers instance. // The class is thread safe. Rendering to the added VideoRendererInterfaces is // done on the same thread as the cricket::VideoRenderer. -class VideoTrackRenderers : public cricket::VideoRenderer { +class VideoTrackRenderers + : public cricket::VideoRenderer, + public rtc::VideoSourceInterface { public: VideoTrackRenderers(); ~VideoTrackRenderers(); @@ -34,18 +36,18 @@ class VideoTrackRenderers : public cricket::VideoRenderer { // incoming frames are replaced by black frames. virtual bool RenderFrame(const cricket::VideoFrame* frame); - void AddRenderer(VideoRendererInterface* renderer); - void RemoveRenderer(VideoRendererInterface* renderer); + void AddOrUpdateSink(VideoSinkInterface* sink, + const rtc::VideoSinkWants& wants) override; + void RemoveSink(VideoSinkInterface* sink) override; void SetEnabled(bool enable); private: // Pass the frame on to to each registered renderer. Requires // critical_section_ already locked. - void RenderFrameToRenderers(const cricket::VideoFrame* frame); + void RenderFrameToSinks(const cricket::VideoFrame& frame); bool enabled_; - std::set renderers_; - + std::vector*> sinks_; rtc::CriticalSection critical_section_; // Protects the above variables }; diff --git a/webrtc/examples/peerconnection/client/linux/main_wnd.cc b/webrtc/examples/peerconnection/client/linux/main_wnd.cc index cf98c1cac7..5a9c268175 100644 --- a/webrtc/examples/peerconnection/client/linux/main_wnd.cc +++ b/webrtc/examples/peerconnection/client/linux/main_wnd.cc @@ -460,11 +460,11 @@ GtkMainWnd::VideoRenderer::VideoRenderer( height_(0), main_wnd_(main_wnd), rendered_track_(track_to_render) { - rendered_track_->AddRenderer(this); + rendered_track_->AddOrUpdateSink(this, rtc::VideoSinkWants()); } GtkMainWnd::VideoRenderer::~VideoRenderer() { - rendered_track_->RemoveRenderer(this); + rendered_track_->RemoveSink(this); } void GtkMainWnd::VideoRenderer::SetSize(int width, int height) { @@ -480,11 +480,11 @@ void GtkMainWnd::VideoRenderer::SetSize(int width, int height) { gdk_threads_leave(); } -void GtkMainWnd::VideoRenderer::RenderFrame( - const cricket::VideoFrame* video_frame) { +void GtkMainWnd::VideoRenderer::OnFrame( + const cricket::VideoFrame& video_frame) { gdk_threads_enter(); - const cricket::VideoFrame* frame = video_frame->GetCopyWithRotationApplied(); + const cricket::VideoFrame* frame = video_frame.GetCopyWithRotationApplied(); SetSize(static_cast(frame->GetWidth()), static_cast(frame->GetHeight())); @@ -511,5 +511,3 @@ void GtkMainWnd::VideoRenderer::RenderFrame( g_idle_add(Redraw, main_wnd_); } - - diff --git a/webrtc/examples/peerconnection/client/linux/main_wnd.h b/webrtc/examples/peerconnection/client/linux/main_wnd.h index e35d4dd8fa..a7eb25e9b1 100644 --- a/webrtc/examples/peerconnection/client/linux/main_wnd.h +++ b/webrtc/examples/peerconnection/client/linux/main_wnd.h @@ -71,7 +71,7 @@ class GtkMainWnd : public MainWindow { void OnRedraw(); protected: - class VideoRenderer : public webrtc::VideoRendererInterface { + class VideoRenderer : public rtc::VideoSinkInterface { public: VideoRenderer(GtkMainWnd* main_wnd, webrtc::VideoTrackInterface* track_to_render); @@ -79,7 +79,7 @@ class GtkMainWnd : public MainWindow { // VideoRendererInterface implementation virtual void SetSize(int width, int height); - virtual void RenderFrame(const cricket::VideoFrame* frame); + virtual void OnFrame(const cricket::VideoFrame& frame); const uint8_t* image() const { return image_.get(); }