From 81354f5ef6b8250c6b370bb17d8447fdf24da00b Mon Sep 17 00:00:00 2001 From: nisse Date: Tue, 19 Jan 2016 00:23:24 -0800 Subject: [PATCH] Added mute logic to VideoTrackRenderers. If the track is disabled, replace incoming frames by black frames. Affects local rendering of disabled tracks. Also intended to replace the similar logic in WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame, once VideoRtpSender is hooked up as a renderer for the track. BUG=webrtc:5426 Review URL: https://codereview.webrtc.org/1575223003 Cr-Commit-Position: refs/heads/master@{#11297} --- .../webrtc/peerconnectionfactory_unittest.cc | 8 ++-- talk/app/webrtc/test/fakevideotrackrenderer.h | 2 + talk/app/webrtc/videotrack_unittest.cc | 35 +++++++++++++++++ talk/app/webrtc/videotrackrenderers.cc | 38 +++++++++++++++++-- talk/app/webrtc/videotrackrenderers.h | 7 +++- 5 files changed, 82 insertions(+), 8 deletions(-) diff --git a/talk/app/webrtc/peerconnectionfactory_unittest.cc b/talk/app/webrtc/peerconnectionfactory_unittest.cc index 9fb013b54f..1fdf2ebf14 100644 --- a/talk/app/webrtc/peerconnectionfactory_unittest.cc +++ b/talk/app/webrtc/peerconnectionfactory_unittest.cc @@ -360,13 +360,15 @@ TEST_F(PeerConnectionFactoryTest, LocalRendering) { EXPECT_EQ(0, local_renderer.num_rendered_frames()); EXPECT_TRUE(capturer->CaptureFrame()); EXPECT_EQ(1, local_renderer.num_rendered_frames()); + EXPECT_FALSE(local_renderer.black_frame()); track->set_enabled(false); EXPECT_TRUE(capturer->CaptureFrame()); - EXPECT_EQ(1, local_renderer.num_rendered_frames()); + EXPECT_EQ(2, local_renderer.num_rendered_frames()); + EXPECT_TRUE(local_renderer.black_frame()); track->set_enabled(true); EXPECT_TRUE(capturer->CaptureFrame()); - EXPECT_EQ(2, local_renderer.num_rendered_frames()); + EXPECT_EQ(3, local_renderer.num_rendered_frames()); + EXPECT_FALSE(local_renderer.black_frame()); } - diff --git a/talk/app/webrtc/test/fakevideotrackrenderer.h b/talk/app/webrtc/test/fakevideotrackrenderer.h index 38b84a6aff..747ba760fd 100644 --- a/talk/app/webrtc/test/fakevideotrackrenderer.h +++ b/talk/app/webrtc/test/fakevideotrackrenderer.h @@ -57,6 +57,8 @@ class FakeVideoTrackRenderer : public VideoRendererInterface { 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(); } diff --git a/talk/app/webrtc/videotrack_unittest.cc b/talk/app/webrtc/videotrack_unittest.cc index c30a98b5e0..fd7b87d30e 100644 --- a/talk/app/webrtc/videotrack_unittest.cc +++ b/talk/app/webrtc/videotrack_unittest.cc @@ -109,3 +109,38 @@ TEST_F(VideoTrackTest, RenderVideo) { EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(2, renderer_2->num_rendered_frames()); } + +// Test that disabling the track results in blacked out frames. +TEST_F(VideoTrackTest, DisableTrackBlackout) { + rtc::scoped_ptr renderer( + new FakeVideoTrackRenderer(video_track_.get())); + + cricket::VideoRenderer* renderer_input = + video_track_->GetSource()->FrameInput(); + ASSERT_FALSE(renderer_input == NULL); + + cricket::WebRtcVideoFrame frame; + frame.InitToBlack(100, 200, 1, 1, 0); + // Make it not all-black + frame.GetUPlane()[0] = 0; + + renderer_input->RenderFrame(&frame); + EXPECT_EQ(1, renderer->num_rendered_frames()); + EXPECT_FALSE(renderer->black_frame()); + EXPECT_EQ(100, renderer->width()); + EXPECT_EQ(200, renderer->height()); + + video_track_->set_enabled(false); + renderer_input->RenderFrame(&frame); + EXPECT_EQ(2, renderer->num_rendered_frames()); + EXPECT_TRUE(renderer->black_frame()); + EXPECT_EQ(100, renderer->width()); + EXPECT_EQ(200, renderer->height()); + + video_track_->set_enabled(true); + renderer_input->RenderFrame(&frame); + EXPECT_EQ(3, renderer->num_rendered_frames()); + EXPECT_FALSE(renderer->black_frame()); + EXPECT_EQ(100, renderer->width()); + EXPECT_EQ(200, renderer->height()); +} diff --git a/talk/app/webrtc/videotrackrenderers.cc b/talk/app/webrtc/videotrackrenderers.cc index 3f9301b718..5b890371a5 100644 --- a/talk/app/webrtc/videotrackrenderers.cc +++ b/talk/app/webrtc/videotrackrenderers.cc @@ -26,7 +26,7 @@ */ #include "talk/app/webrtc/videotrackrenderers.h" -#include "talk/media/base/videoframe.h" +#include "talk/media/webrtc/webrtcvideoframe.h" namespace webrtc { @@ -55,14 +55,44 @@ void VideoTrackRenderers::SetEnabled(bool enable) { } bool VideoTrackRenderers::RenderFrame(const cricket::VideoFrame* frame) { - rtc::CritScope cs(&critical_section_); - if (!enabled_) { + { + rtc::CritScope cs(&critical_section_); + if (enabled_) { + RenderFrameToRenderers(frame); + return true; + } + } + + // Generate the black frame outside of the critical section. Note + // that this may result in unexpected frame order, in the unlikely + // case that RenderFrame is called from multiple threads without + // proper serialization, and the track is switched from disabled to + // enabled in the middle of the first call. + cricket::WebRtcVideoFrame black(new rtc::RefCountedObject( + static_cast(frame->GetWidth()), + static_cast(frame->GetHeight())), + frame->GetTimeStamp(), + frame->GetVideoRotation()); + black.SetToBlack(); + + { + rtc::CritScope cs(&critical_section_); + // Check enabled_ flag again, since the track might have been + // 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); + return true; } +} + +// Called with critical_section_ already locked +void VideoTrackRenderers::RenderFrameToRenderers( + const cricket::VideoFrame* frame) { for (VideoRendererInterface* renderer : renderers_) { renderer->RenderFrame(frame); } - return true; } } // namespace webrtc diff --git a/talk/app/webrtc/videotrackrenderers.h b/talk/app/webrtc/videotrackrenderers.h index 3262e22dff..2a73ed4fef 100644 --- a/talk/app/webrtc/videotrackrenderers.h +++ b/talk/app/webrtc/videotrackrenderers.h @@ -47,7 +47,8 @@ class VideoTrackRenderers : public cricket::VideoRenderer { VideoTrackRenderers(); ~VideoTrackRenderers(); - // Implements cricket::VideoRenderer + // Implements cricket::VideoRenderer. If the track is disabled, + // incoming frames are replaced by black frames. virtual bool RenderFrame(const cricket::VideoFrame* frame); void AddRenderer(VideoRendererInterface* renderer); @@ -55,6 +56,10 @@ class VideoTrackRenderers : public cricket::VideoRenderer { 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); + bool enabled_; std::set renderers_;