From 5301b0f1fce9478dfa56476e174332a1d67b053a Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Thu, 17 Jul 2014 08:51:46 +0000 Subject: [PATCH] Move additional state into WebRtcVideoSendStream. Prevents having two places where codecs etc. are set up and allows us to avoid creating the underlying VideoSendStream before send codecs are set up. BUG=1788 R=pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/20939004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6716 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/libjingle_tests.gyp | 1 + talk/media/base/streamparams.cc | 20 ++ talk/media/base/streamparams.h | 10 + talk/media/base/streamparams_unittest.cc | 32 +++ talk/media/webrtc/webrtcvideoengine2.cc | 212 ++++++++---------- talk/media/webrtc/webrtcvideoengine2.h | 24 +- .../webrtc/webrtcvideoengine2_unittest.cc | 26 +-- 7 files changed, 178 insertions(+), 147 deletions(-) diff --git a/talk/libjingle_tests.gyp b/talk/libjingle_tests.gyp index 3c8df7e734..4157ad2c55 100755 --- a/talk/libjingle_tests.gyp +++ b/talk/libjingle_tests.gyp @@ -281,6 +281,7 @@ 'media/base/rtpdataengine_unittest.cc', 'media/base/rtpdump_unittest.cc', 'media/base/rtputils_unittest.cc', + 'media/base/streamparams_unittest.cc', 'media/base/testutils.cc', 'media/base/testutils.h', 'media/base/videocapturer_unittest.cc', diff --git a/talk/media/base/streamparams.cc b/talk/media/base/streamparams.cc index 19d8269044..09bfefb5d4 100644 --- a/talk/media/base/streamparams.cc +++ b/talk/media/base/streamparams.cc @@ -97,6 +97,26 @@ std::string StreamParams::ToString() const { ost << "}"; return ost.str(); } +void StreamParams::GetPrimarySsrcs(std::vector* ssrcs) const { + const SsrcGroup* sim_group = get_ssrc_group(kSimSsrcGroupSemantics); + if (sim_group == NULL) { + ssrcs->push_back(first_ssrc()); + } else { + for (size_t i = 0; i < sim_group->ssrcs.size(); ++i) { + ssrcs->push_back(sim_group->ssrcs[i]); + } + } +} + +void StreamParams::GetFidSsrcs(const std::vector& primary_ssrcs, + std::vector* fid_ssrcs) const { + for (size_t i = 0; i < primary_ssrcs.size(); ++i) { + uint32 fid_ssrc; + if (GetFidSsrc(primary_ssrcs[i], &fid_ssrc)) { + fid_ssrcs->push_back(fid_ssrc); + } + } +} bool StreamParams::AddSecondarySsrc(const std::string& semantics, uint32 primary_ssrc, diff --git a/talk/media/base/streamparams.h b/talk/media/base/streamparams.h index b57f1f7810..8be61b5b37 100644 --- a/talk/media/base/streamparams.h +++ b/talk/media/base/streamparams.h @@ -141,6 +141,16 @@ struct StreamParams { return GetSecondarySsrc(kFidSsrcGroupSemantics, primary_ssrc, fid_ssrc); } + // Convenience to get all the SIM SSRCs if there are SIM ssrcs, or + // the first SSRC otherwise. + void GetPrimarySsrcs(std::vector* ssrcs) const; + + // Convenience to get all the FID SSRCs for the given primary ssrcs. + // If a given primary SSRC does not have a FID SSRC, the list of FID + // SSRCS will be smaller than the list of primary SSRCs. + void GetFidSsrcs(const std::vector& primary_ssrcs, + std::vector* fid_ssrcs) const; + std::string ToString() const; // Resource of the MUC jid of the participant of with this stream. diff --git a/talk/media/base/streamparams_unittest.cc b/talk/media/base/streamparams_unittest.cc index f3a03d6363..0fd6771901 100644 --- a/talk/media/base/streamparams_unittest.cc +++ b/talk/media/base/streamparams_unittest.cc @@ -159,6 +159,38 @@ TEST(StreamParams, FidFunctions) { EXPECT_FALSE(sp_invalid.GetFidSsrc(13, &fid_ssrc)); } +TEST(StreamParams, GetPrimaryAndFidSsrcs) { + cricket::StreamParams sp; + sp.ssrcs.push_back(1); + sp.ssrcs.push_back(2); + sp.ssrcs.push_back(3); + + std::vector primary_ssrcs; + sp.GetPrimarySsrcs(&primary_ssrcs); + std::vector fid_ssrcs; + sp.GetFidSsrcs(primary_ssrcs, &fid_ssrcs); + ASSERT_EQ(1u, primary_ssrcs.size()); + EXPECT_EQ(1u, primary_ssrcs[0]); + ASSERT_EQ(0u, fid_ssrcs.size()); + + sp.ssrc_groups.push_back( + cricket::SsrcGroup(cricket::kSimSsrcGroupSemantics, sp.ssrcs)); + sp.AddFidSsrc(1, 10); + sp.AddFidSsrc(2, 20); + + primary_ssrcs.clear(); + sp.GetPrimarySsrcs(&primary_ssrcs); + fid_ssrcs.clear(); + sp.GetFidSsrcs(primary_ssrcs, &fid_ssrcs); + ASSERT_EQ(3u, primary_ssrcs.size()); + EXPECT_EQ(1u, primary_ssrcs[0]); + EXPECT_EQ(2u, primary_ssrcs[1]); + EXPECT_EQ(3u, primary_ssrcs[2]); + ASSERT_EQ(2u, fid_ssrcs.size()); + EXPECT_EQ(10u, fid_ssrcs[0]); + EXPECT_EQ(20u, fid_ssrcs[1]); +} + TEST(StreamParams, ToString) { cricket::StreamParams sp = CreateStreamParamsWithSsrcGroup("XYZ", kSsrcs2, ARRAY_SIZE(kSsrcs2)); diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index bdc6baa902..32c93cf693 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -878,52 +878,6 @@ bool WebRtcVideoChannel2::SetSend(bool send) { return true; } -static bool ConfigureSendSsrcs(webrtc::VideoSendStream::Config* config, - const StreamParams& sp) { - if (!sp.has_ssrc_groups()) { - config->rtp.ssrcs = sp.ssrcs; - return true; - } - - if (sp.get_ssrc_group(kFecSsrcGroupSemantics) != NULL) { - LOG(LS_ERROR) << "Standalone FEC SSRCs not supported."; - return false; - } - - // Map RTX SSRCs. - std::vector ssrcs; - std::vector rtx_ssrcs; - const SsrcGroup* sim_group = sp.get_ssrc_group(kSimSsrcGroupSemantics); - if (sim_group == NULL) { - ssrcs.push_back(sp.first_ssrc()); - uint32_t rtx_ssrc; - if (!sp.GetFidSsrc(sp.first_ssrc(), &rtx_ssrc)) { - LOG(LS_ERROR) << "Could not find FID ssrc for primary SSRC '" - << sp.first_ssrc() << "':" << sp.ToString(); - return false; - } - rtx_ssrcs.push_back(rtx_ssrc); - } else { - ssrcs = sim_group->ssrcs; - for (size_t i = 0; i < sim_group->ssrcs.size(); ++i) { - uint32_t rtx_ssrc; - if (!sp.GetFidSsrc(sim_group->ssrcs[i], &rtx_ssrc)) { - continue; - } - rtx_ssrcs.push_back(rtx_ssrc); - } - } - if (!rtx_ssrcs.empty() && ssrcs.size() != rtx_ssrcs.size()) { - LOG(LS_ERROR) - << "RTX SSRCs exist, but don't cover all SSRCs (unsupported): " - << sp.ToString(); - return false; - } - config->rtp.rtx.ssrcs = rtx_ssrcs; - config->rtp.ssrcs = ssrcs; - return true; -} - bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) { LOG(LS_INFO) << "AddSendStream: " << sp.ToString(); if (sp.ssrcs.empty()) { @@ -940,58 +894,25 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) { return false; } - webrtc::VideoSendStream::Config config; - - if (!ConfigureSendSsrcs(&config, sp)) { + std::vector primary_ssrcs; + sp.GetPrimarySsrcs(&primary_ssrcs); + std::vector rtx_ssrcs; + sp.GetFidSsrcs(primary_ssrcs, &rtx_ssrcs); + if (!rtx_ssrcs.empty() && primary_ssrcs.size() != rtx_ssrcs.size()) { + LOG(LS_ERROR) + << "RTX SSRCs exist, but don't cover all SSRCs (unsupported): " + << sp.ToString(); return false; } - VideoCodecSettings codec_settings; - if (!send_codec_.Get(&codec_settings)) { - // TODO(pbos): Set up a temporary fake encoder for VideoSendStream instead - // of setting default codecs not to break CreateEncoderSettings. - SetSendCodecs(DefaultVideoCodecs()); - assert(send_codec_.IsSet()); - send_codec_.Get(&codec_settings); - // This is only to bring up defaults to make VideoSendStream setup easier - // and avoid complexity. We still don't want to allow sending with the - // default codec. - send_codec_.Clear(); - } - - // CreateEncoderSettings will allocate a suitable VideoEncoder instance - // matching current settings. - std::vector video_streams = - encoder_factory_->CreateVideoStreams( - codec_settings.codec, options_, config.rtp.ssrcs.size()); - if (video_streams.empty()) { - return false; - } - - config.encoder_settings.encoder = - encoder_factory_->CreateVideoEncoder(codec_settings.codec, options_); - config.encoder_settings.payload_name = codec_settings.codec.name; - config.encoder_settings.payload_type = codec_settings.codec.id; - config.rtp.c_name = sp.cname; - config.rtp.fec = codec_settings.fec; - if (!config.rtp.rtx.ssrcs.empty()) { - config.rtp.rtx.payload_type = codec_settings.rtx_payload_type; - } - - config.rtp.extensions = send_rtp_extensions_; - - if (IsNackEnabled(codec_settings.codec)) { - config.rtp.nack.rtp_history_ms = kNackHistoryMs; - } - config.rtp.max_packet_size = kVideoMtu; - WebRtcVideoSendStream* stream = new WebRtcVideoSendStream(call_.get(), - config, + encoder_factory_, options_, - codec_settings.codec, - video_streams, - encoder_factory_); + send_codec_, + sp, + send_rtp_extensions_); + send_streams_[ssrc] = stream; if (rtcp_receiver_report_ssrc_ == kDefaultRtcpReceiverReportSsrc) { @@ -1338,6 +1259,12 @@ bool WebRtcVideoChannel2::SetMaxSendBandwidth(int bps) { bool WebRtcVideoChannel2::SetOptions(const VideoOptions& options) { LOG(LS_VERBOSE) << "SetOptions: " << options.ToString(); options_.SetAll(options); + for (std::map::iterator it = + send_streams_.begin(); + it != send_streams_.end(); + ++it) { + it->second->SetOptions(options_); + } return true; } @@ -1399,7 +1326,7 @@ void WebRtcVideoChannel2::SetCodecForAllSendStreams( it != send_streams_.end(); ++it) { assert(it->second != NULL); - it->second->SetCodec(options_, codec); + it->second->SetCodec(codec); } } @@ -1407,38 +1334,43 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::VideoSendStreamParameters:: VideoSendStreamParameters( const webrtc::VideoSendStream::Config& config, const VideoOptions& options, - const VideoCodec& codec, - const std::vector& video_streams) - : config(config), - options(options), - codec(codec), - video_streams(video_streams) { + const Settable& codec_settings) + : config(config), options(options), codec_settings(codec_settings) { } WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( webrtc::Call* call, - const webrtc::VideoSendStream::Config& config, + WebRtcVideoEncoderFactory2* encoder_factory, const VideoOptions& options, - const VideoCodec& codec, - const std::vector& video_streams, - WebRtcVideoEncoderFactory2* encoder_factory) + const Settable& codec_settings, + const StreamParams& sp, + const std::vector& rtp_extensions) : call_(call), - parameters_(config, options, codec, video_streams), + parameters_(webrtc::VideoSendStream::Config(), options, codec_settings), encoder_factory_(encoder_factory), capturer_(NULL), stream_(NULL), sending_(false), - muted_(false), - format_(static_cast(video_streams.back().height), - static_cast(video_streams.back().width), - VideoFormat::FpsToInterval(video_streams.back().max_framerate), - FOURCC_I420) { - RecreateWebRtcStream(); + muted_(false) { + parameters_.config.rtp.max_packet_size = kVideoMtu; + + sp.GetPrimarySsrcs(¶meters_.config.rtp.ssrcs); + sp.GetFidSsrcs(parameters_.config.rtp.ssrcs, + ¶meters_.config.rtp.rtx.ssrcs); + parameters_.config.rtp.c_name = sp.cname; + parameters_.config.rtp.extensions = rtp_extensions; + + VideoCodecSettings params; + if (codec_settings.Get(¶ms)) { + SetCodec(params); + } } WebRtcVideoChannel2::WebRtcVideoSendStream::~WebRtcVideoSendStream() { DisconnectCapturer(); - call_->DestroyVideoSendStream(stream_); + if (stream_ != NULL) { + call_->DestroyVideoSendStream(stream_); + } delete parameters_.config.encoder_settings.encoder; } @@ -1495,6 +1427,11 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( is_screencast = false; } talk_base::CritScope cs(&lock_); + if (stream_ == NULL) { + LOG(LS_WARNING) << "Capturer inputting frames before send codecs are " + "configured, dropping."; + return; + } if (format_.width == 0) { // Dropping frames. assert(format_.height == 0); LOG(LS_VERBOSE) << "VideoFormat 0x0 set, Dropping frame."; @@ -1520,7 +1457,7 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer( { talk_base::CritScope cs(&lock_); - if (capturer == NULL) { + if (capturer == NULL && stream_ != NULL) { LOG(LS_VERBOSE) << "Disabling capturer, sending black frame."; webrtc::I420VideoFrame black_frame; @@ -1587,30 +1524,54 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::DisconnectCapturer() { return true; } -void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodec( - const VideoOptions& options, - const VideoCodecSettings& codec) { +void WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions( + const VideoOptions& options) { talk_base::CritScope cs(&lock_); - + VideoCodecSettings codec_settings; + if (parameters_.codec_settings.Get(&codec_settings)) { + SetCodecAndOptions(codec_settings, options); + } else { + parameters_.options = options; + } +} +void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodec( + const VideoCodecSettings& codec_settings) { + talk_base::CritScope cs(&lock_); + SetCodecAndOptions(codec_settings, parameters_.options); +} +void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions( + const VideoCodecSettings& codec_settings, + const VideoOptions& options) { std::vector video_streams = encoder_factory_->CreateVideoStreams( - codec.codec, options, parameters_.video_streams.size()); + codec_settings.codec, options, parameters_.config.rtp.ssrcs.size()); if (video_streams.empty()) { return; } parameters_.video_streams = video_streams; - format_ = VideoFormat(codec.codec.width, - codec.codec.height, + format_ = VideoFormat(codec_settings.codec.width, + codec_settings.codec.height, VideoFormat::FpsToInterval(30), FOURCC_I420); webrtc::VideoEncoder* old_encoder = parameters_.config.encoder_settings.encoder; parameters_.config.encoder_settings.encoder = - encoder_factory_->CreateVideoEncoder(codec.codec, options); - parameters_.config.rtp.fec = codec.fec; - // TODO(pbos): Should changing RTX payload type be allowed? - parameters_.codec = codec.codec; + encoder_factory_->CreateVideoEncoder(codec_settings.codec, options); + parameters_.config.encoder_settings.payload_name = codec_settings.codec.name; + parameters_.config.encoder_settings.payload_type = codec_settings.codec.id; + parameters_.config.rtp.fec = codec_settings.fec; + + // Set RTX payload type if RTX is enabled. + if (!parameters_.config.rtp.rtx.ssrcs.empty()) { + parameters_.config.rtp.rtx.payload_type = codec_settings.rtx_payload_type; + } + + if (IsNackEnabled(codec_settings.codec)) { + parameters_.config.rtp.nack.rtp_history_ms = kNackHistoryMs; + } + + parameters_.codec_settings.Set(codec_settings); parameters_.options = options; RecreateWebRtcStream(); delete old_encoder; @@ -1639,13 +1600,16 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetDimensions(int width, void WebRtcVideoChannel2::WebRtcVideoSendStream::Start() { talk_base::CritScope cs(&lock_); + assert(stream_ != NULL); stream_->Start(); sending_ = true; } void WebRtcVideoChannel2::WebRtcVideoSendStream::Stop() { talk_base::CritScope cs(&lock_); - stream_->Stop(); + if (stream_ != NULL) { + stream_->Stop(); + } sending_ = false; } diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index 81466eb657..51223907c8 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -271,14 +271,17 @@ class WebRtcVideoChannel2 : public talk_base::MessageHandler, class WebRtcVideoSendStream : public sigslot::has_slots<> { public: - WebRtcVideoSendStream(webrtc::Call* call, - const webrtc::VideoSendStream::Config& config, - const VideoOptions& options, - const VideoCodec& codec, - const std::vector& video_streams, - WebRtcVideoEncoderFactory2* encoder_factory); + WebRtcVideoSendStream( + webrtc::Call* call, + WebRtcVideoEncoderFactory2* encoder_factory, + const VideoOptions& options, + const Settable& codec_settings, + const StreamParams& sp, + const std::vector& rtp_extensions); + ~WebRtcVideoSendStream(); - void SetCodec(const VideoOptions& options, const VideoCodecSettings& codec); + void SetOptions(const VideoOptions& options); + void SetCodec(const VideoCodecSettings& codec); void InputFrame(VideoCapturer* capturer, const VideoFrame* frame); bool SetCapturer(VideoCapturer* capturer); @@ -298,17 +301,18 @@ class WebRtcVideoChannel2 : public talk_base::MessageHandler, VideoSendStreamParameters( const webrtc::VideoSendStream::Config& config, const VideoOptions& options, - const VideoCodec& codec, - const std::vector& video_streams); + const Settable& codec_settings); webrtc::VideoSendStream::Config config; VideoOptions options; - VideoCodec codec; + Settable codec_settings; // Sent resolutions + bitrates etc. by the underlying VideoSendStream, // typically changes when setting a new resolution or reconfiguring // bitrates. std::vector video_streams; }; + void SetCodecAndOptions(const VideoCodecSettings& codec, + const VideoOptions& options); void RecreateWebRtcStream(); void SetDimensions(int width, int height); diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index a58ed93c6b..28abbd1dee 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -413,6 +413,17 @@ TEST_F(WebRtcVideoEngine2Test, SupportsAbsoluteSenderTimeHeaderExtension) { FAIL() << "Absolute Sender Time extension not in header-extension list."; } +TEST_F(WebRtcVideoEngine2Test, SetSendFailsBeforeSettingCodecs) { + talk_base::scoped_ptr channel(engine_.CreateChannel(NULL)); + + EXPECT_TRUE(channel->AddSendStream(StreamParams::CreateLegacy(123))); + + EXPECT_FALSE(channel->SetSend(true)) + << "Channel should not start without codecs."; + EXPECT_TRUE(channel->SetSend(false)) + << "Channel should be stoppable even without set codecs."; +} + class WebRtcVideoChannel2BaseTest : public VideoMediaChannelTest { protected: @@ -551,6 +562,7 @@ class WebRtcVideoChannel2Test : public WebRtcVideoEngine2Test { last_ssrc_ = 123; ASSERT_TRUE(fake_channel_ != NULL) << "Channel not created through factory."; + EXPECT_TRUE(fake_channel_->SetSendCodecs(engine_.codecs())); } protected: @@ -1257,19 +1269,7 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_ReceiveStreamReceivingByDefault) { } TEST_F(WebRtcVideoChannel2Test, SetSend) { - AddSendStream(); - EXPECT_FALSE(channel_->SetSend(true)) - << "Channel should not start without codecs."; - EXPECT_TRUE(channel_->SetSend(false)) - << "Channel should be stoppable even without set codecs."; - - std::vector codecs; - codecs.push_back(kVp8Codec); - channel_->SetSendCodecs(codecs); - std::vector streams = GetFakeSendStreams(); - ASSERT_EQ(1u, streams.size()); - FakeVideoSendStream* stream = streams.back(); - + FakeVideoSendStream* stream = AddSendStream(); EXPECT_FALSE(stream->IsSending()); // false->true