From e322a175f6f38c4ed39296d9724edf005e536a63 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Fri, 13 Jun 2014 11:47:28 +0000 Subject: [PATCH] Implement RTX tests+fixes in WebRtcVideoEngine2. BUG=1788 R=pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/15759004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6430 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/webrtc/webrtcvideoengine2.cc | 85 ++++++---- .../webrtc/webrtcvideoengine2_unittest.cc | 157 +++++++++++------- 2 files changed, 153 insertions(+), 89 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 6ee07a32f..bf731c4ca 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -329,6 +329,7 @@ WebRtcVideoChannel2* WebRtcVideoEngine2::CreateChannel( delete channel; return NULL; } + channel->SetRecvCodecs(video_codecs_); return channel; } @@ -725,15 +726,6 @@ bool WebRtcVideoChannel2::Init() { return true; } namespace { -static bool ValidateCodecFormats(const std::vector& codecs) { - for (size_t i = 0; i < codecs.size(); ++i) { - if (!codecs[i].ValidateCodecFormat()) { - return false; - } - } - return true; -} - static std::string CodecVectorToString(const std::vector& codecs) { std::stringstream out; out << '{'; @@ -747,6 +739,24 @@ static std::string CodecVectorToString(const std::vector& codecs) { return out.str(); } +static bool ValidateCodecFormats(const std::vector& codecs) { + bool has_video = false; + for (size_t i = 0; i < codecs.size(); ++i) { + if (!codecs[i].ValidateCodecFormat()) { + return false; + } + if (codecs[i].GetCodecType() == VideoCodec::CODEC_VIDEO) { + has_video = true; + } + } + if (!has_video) { + LOG(LS_ERROR) << "Setting codecs without a video codec is invalid: " + << CodecVectorToString(codecs); + return false; + } + return true; +} + } // namespace bool WebRtcVideoChannel2::SetRecvCodecs(const std::vector& codecs) { @@ -852,30 +862,37 @@ static bool ConfigureSendSsrcs(webrtc::VideoSendStream::Config* config, 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) { - LOG(LS_ERROR) << "Grouped StreamParams without regular SSRC group: " - << sp.ToString(); - return false; - } - - // Map RTX SSRCs. - std::vector rtx_ssrcs; - for (size_t i = 0; i < sim_group->ssrcs.size(); ++i) { + ssrcs.push_back(sp.first_ssrc()); uint32_t rtx_ssrc; - if (!sp.GetFidSsrc(sim_group->ssrcs[i], &rtx_ssrc)) { - continue; + 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() && sim_group->ssrcs.size() != rtx_ssrcs.size()) { + 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 = sim_group->ssrcs; + config->rtp.ssrcs = ssrcs; return true; } @@ -1006,13 +1023,6 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) { webrtc::VideoReceiveStream::Config config = call_->GetDefaultReceiveConfig(); config.rtp.remote_ssrc = ssrc; config.rtp.local_ssrc = rtcp_receiver_report_ssrc_; - uint32 rtx_ssrc = 0; - if (sp.GetFidSsrc(ssrc, &rtx_ssrc)) { - // TODO(pbos): Right now, VideoReceiveStream accepts any rtx payload, this - // should use the actual codec payloads that may be received. - // (for each receive payload, set rtx[payload].ssrc = rtx_ssrc. - config.rtp.rtx[0].ssrc = rtx_ssrc; - } config.rtp.nack.rtp_history_ms = kNackHistoryMs; config.rtp.remb = true; @@ -1068,7 +1078,9 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) { for (size_t i = 0; i < recv_codecs_.size(); ++i) { if (recv_codecs_[i].codec.id == codec.plType) { config.rtp.fec = recv_codecs_[i].fec; - if (recv_codecs_[i].rtx_payload_type != -1 && rtx_ssrc != 0) { + uint32 rtx_ssrc; + if (recv_codecs_[i].rtx_payload_type != -1 && + sp.GetFidSsrc(ssrc, &rtx_ssrc)) { config.rtp.rtx[codec.plType].ssrc = rtx_ssrc; config.rtp.rtx[codec.plType].payload_type = recv_codecs_[i].rtx_payload_type; @@ -1607,6 +1619,7 @@ WebRtcVideoChannel2::MapCodecs(const std::vector& codecs) { std::vector video_codecs; std::map payload_used; + std::map payload_codec_type; std::map rtx_mapping; // video payload type -> rtx payload type. webrtc::FecConfig fec_settings; @@ -1621,6 +1634,7 @@ WebRtcVideoChannel2::MapCodecs(const std::vector& codecs) { return std::vector(); } payload_used[payload_type] = true; + payload_codec_type[payload_type] = in_codec.GetCodecType(); switch (in_codec.GetCodecType()) { case VideoCodec::CODEC_RED: { @@ -1661,6 +1675,19 @@ WebRtcVideoChannel2::MapCodecs(const std::vector& codecs) { // parameters into this code is a logic error. assert(!video_codecs.empty()); + for (std::map::const_iterator it = rtx_mapping.begin(); + it != rtx_mapping.end(); + ++it) { + if (!payload_used[it->first]) { + LOG(LS_ERROR) << "RTX mapped to payload not in codec list."; + return std::vector(); + } + if (payload_codec_type[it->first] != VideoCodec::CODEC_VIDEO) { + LOG(LS_ERROR) << "RTX not mapped to regular video codec."; + return std::vector(); + } + } + // TODO(pbos): Write tests that figure out that I have not verified that RTX // codecs aren't mapped to bogus payloads. for (size_t i = 0; i < video_codecs.size(); ++i) { diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 4d40ac4dc..4dd27bc79 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -328,6 +328,62 @@ TEST_F(WebRtcVideoEngine2Test, CreateChannelWithVoiceEngine) { << "Different VoiceChannel set than the provided one."; } +TEST_F(WebRtcVideoEngine2Test, FindCodec) { + const std::vector& c = engine_.codecs(); + EXPECT_EQ(4U, c.size()); + + cricket::VideoCodec vp8(104, "VP8", 320, 200, 30, 0); + EXPECT_TRUE(engine_.FindCodec(vp8)); + + cricket::VideoCodec vp8_ci(104, "vp8", 320, 200, 30, 0); + EXPECT_TRUE(engine_.FindCodec(vp8)); + + cricket::VideoCodec vp8_diff_fr_diff_pref(104, "VP8", 320, 200, 50, 50); + EXPECT_TRUE(engine_.FindCodec(vp8_diff_fr_diff_pref)); + + cricket::VideoCodec vp8_diff_id(95, "VP8", 320, 200, 30, 0); + EXPECT_FALSE(engine_.FindCodec(vp8_diff_id)); + vp8_diff_id.id = 97; + EXPECT_TRUE(engine_.FindCodec(vp8_diff_id)); + + cricket::VideoCodec vp8_diff_res(104, "VP8", 320, 111, 30, 0); + EXPECT_FALSE(engine_.FindCodec(vp8_diff_res)); + + // PeerConnection doesn't negotiate the resolution at this point. + // Test that FindCodec can handle the case when width/height is 0. + cricket::VideoCodec vp8_zero_res(104, "VP8", 0, 0, 30, 0); + EXPECT_TRUE(engine_.FindCodec(vp8_zero_res)); + + cricket::VideoCodec red(101, "RED", 0, 0, 30, 0); + EXPECT_TRUE(engine_.FindCodec(red)); + + cricket::VideoCodec red_ci(101, "red", 0, 0, 30, 0); + EXPECT_TRUE(engine_.FindCodec(red)); + + cricket::VideoCodec fec(102, "ULPFEC", 0, 0, 30, 0); + EXPECT_TRUE(engine_.FindCodec(fec)); + + cricket::VideoCodec fec_ci(102, "ulpfec", 0, 0, 30, 0); + EXPECT_TRUE(engine_.FindCodec(fec)); + + cricket::VideoCodec rtx(96, "rtx", 0, 0, 30, 0); + EXPECT_TRUE(engine_.FindCodec(rtx)); +} + +TEST_F(WebRtcVideoEngine2Test, DefaultRtxCodecHasAssociatedPayloadTypeSet) { + std::vector engine_codecs = engine_.codecs(); + for (size_t i = 0; i < engine_codecs.size(); ++i) { + if (engine_codecs[i].name != kRtxCodecName) + continue; + int associated_payload_type; + EXPECT_TRUE(engine_codecs[i].GetParam(kCodecParamAssociatedPayloadType, + &associated_payload_type)); + EXPECT_EQ(default_codec_.id, associated_payload_type); + return; + } + FAIL() << "No RTX codec found among default codecs."; +} + class WebRtcVideoChannel2BaseTest : public VideoMediaChannelTest { protected: @@ -607,7 +663,7 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_RembEnabledOnReceiveChannels) { FAIL() << "Not implemented."; // TODO(pbos): Implement. } -TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) { +TEST_F(WebRtcVideoChannel2Test, RecvStreamWithSimAndRtx) { EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs())); EXPECT_TRUE(channel_->SetSend(true)); cricket::VideoOptions options; @@ -636,12 +692,23 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) { // TODO(pbos): Make sure we set the RTX for correct payloads etc. } -TEST_F(WebRtcVideoChannel2Test, DISABLED_RecvStreamWithRtxOnMultiplePayloads) { - FAIL() << "Not implemented."; +TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) { + // Setup one channel with an associated RTX stream. + cricket::StreamParams params = + cricket::StreamParams::CreateLegacy(kSsrcs1[0]); + params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]); + FakeVideoReceiveStream* recv_stream = AddRecvStream(params); + ASSERT_EQ(1u, recv_stream->GetConfig().rtp.rtx.size()); + EXPECT_EQ(kRtxSsrcs1[0], + recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc); } -TEST_F(WebRtcVideoChannel2Test, DISABLED_RecvStreamNoRtx) { - FAIL() << "Not implemented."; // TODO(pbos): Implement. +TEST_F(WebRtcVideoChannel2Test, RecvStreamNoRtx) { + // Setup one channel without an associated RTX stream. + cricket::StreamParams params = + cricket::StreamParams::CreateLegacy(kSsrcs1[0]); + FakeVideoReceiveStream* recv_stream = AddRecvStream(params); + ASSERT_TRUE(recv_stream->GetConfig().rtp.rtx.empty()); } TEST_F(WebRtcVideoChannel2Test, DISABLED_RtpTimestampOffsetHeaderExtensions) { @@ -777,61 +844,6 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_WebRtcShouldNotLog) { FAIL() << "Not implemented."; // TODO(pbos): Implement. } -TEST_F(WebRtcVideoEngine2Test, FindCodec) { - const std::vector& c = engine_.codecs(); - EXPECT_EQ(4U, c.size()); - - cricket::VideoCodec vp8(104, "VP8", 320, 200, 30, 0); - EXPECT_TRUE(engine_.FindCodec(vp8)); - - cricket::VideoCodec vp8_ci(104, "vp8", 320, 200, 30, 0); - EXPECT_TRUE(engine_.FindCodec(vp8)); - - cricket::VideoCodec vp8_diff_fr_diff_pref(104, "VP8", 320, 200, 50, 50); - EXPECT_TRUE(engine_.FindCodec(vp8_diff_fr_diff_pref)); - - cricket::VideoCodec vp8_diff_id(95, "VP8", 320, 200, 30, 0); - EXPECT_FALSE(engine_.FindCodec(vp8_diff_id)); - vp8_diff_id.id = 97; - EXPECT_TRUE(engine_.FindCodec(vp8_diff_id)); - - cricket::VideoCodec vp8_diff_res(104, "VP8", 320, 111, 30, 0); - EXPECT_FALSE(engine_.FindCodec(vp8_diff_res)); - - // PeerConnection doesn't negotiate the resolution at this point. - // Test that FindCodec can handle the case when width/height is 0. - cricket::VideoCodec vp8_zero_res(104, "VP8", 0, 0, 30, 0); - EXPECT_TRUE(engine_.FindCodec(vp8_zero_res)); - - cricket::VideoCodec red(101, "RED", 0, 0, 30, 0); - EXPECT_TRUE(engine_.FindCodec(red)); - - cricket::VideoCodec red_ci(101, "red", 0, 0, 30, 0); - EXPECT_TRUE(engine_.FindCodec(red)); - - cricket::VideoCodec fec(102, "ULPFEC", 0, 0, 30, 0); - EXPECT_TRUE(engine_.FindCodec(fec)); - - cricket::VideoCodec fec_ci(102, "ulpfec", 0, 0, 30, 0); - EXPECT_TRUE(engine_.FindCodec(fec)); - - cricket::VideoCodec rtx(96, "rtx", 0, 0, 30, 0); - EXPECT_TRUE(engine_.FindCodec(rtx)); -} - -TEST_F(WebRtcVideoEngine2Test, DefaultRtxCodecHasAssociatedPayloadTypeSet) { - for (size_t i = 0; i < engine_.codecs().size(); ++i) { - if (engine_.codecs()[i].name != kRtxCodecName) - continue; - int associated_payload_type; - EXPECT_TRUE(engine_.codecs()[i].GetParam(kCodecParamAssociatedPayloadType, - &associated_payload_type)); - EXPECT_EQ(default_codec_.id, associated_payload_type); - return; - } - FAIL() << "No RTX codec found among default codecs."; -} - TEST_F(WebRtcVideoChannel2Test, SetDefaultSendCodecs) { ASSERT_TRUE(channel_->SetSendCodecs(engine_.codecs())); @@ -975,6 +987,31 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithOnlyVp8) { EXPECT_TRUE(channel_->SetRecvCodecs(codecs)); } +// Test that we set our inbound RTX codecs properly. +TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithRtx) { + std::vector codecs; + codecs.push_back(kVp8Codec); + cricket::VideoCodec rtx_codec(96, "rtx", 0, 0, 0, 0); + codecs.push_back(rtx_codec); + EXPECT_FALSE(channel_->SetRecvCodecs(codecs)) + << "RTX codec without associated payload should be rejected."; + + codecs[1].SetParam("apt", kVp8Codec.id + 1); + EXPECT_FALSE(channel_->SetRecvCodecs(codecs)) + << "RTX codec with invalid associated payload type should be rejected."; + + codecs[1].SetParam("apt", kVp8Codec.id); + EXPECT_TRUE(channel_->SetRecvCodecs(codecs)); + + cricket::VideoCodec rtx_codec2(97, "rtx", 0, 0, 0, 0); + rtx_codec2.SetParam("apt", rtx_codec.id); + codecs.push_back(rtx_codec2); + + EXPECT_FALSE(channel_->SetRecvCodecs(codecs)) << "RTX codec with another RTX " + "as associated payload type " + "should be rejected."; +} + TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsDifferentPayloadType) { std::vector codecs; codecs.push_back(kVp8Codec);