diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index a0f6db4cad..586d356805 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2887,46 +2887,58 @@ WebRtcVideoChannel::MapCodecs(const std::vector& codecs) { RTC_DCHECK(!codecs.empty()); std::vector video_codecs; - std::map payload_used; std::map payload_codec_type; // |rtx_mapping| maps video payload type to rtx payload type. std::map rtx_mapping; webrtc::UlpfecConfig ulpfec_config; - int flexfec_payload_type = -1; + absl::optional flexfec_payload_type; - for (size_t i = 0; i < codecs.size(); ++i) { - const VideoCodec& in_codec = codecs[i]; - int payload_type = in_codec.id; + for (const VideoCodec& in_codec : codecs) { + const int payload_type = in_codec.id; - if (payload_used[payload_type]) { + if (payload_codec_type.find(payload_type) != payload_codec_type.end()) { RTC_LOG(LS_ERROR) << "Payload type already registered: " << in_codec.ToString(); - return std::vector(); + return {}; } - payload_used[payload_type] = true; payload_codec_type[payload_type] = in_codec.GetCodecType(); switch (in_codec.GetCodecType()) { case VideoCodec::CODEC_RED: { - // RED payload type, should not have duplicates. - RTC_DCHECK_EQ(-1, ulpfec_config.red_payload_type); - ulpfec_config.red_payload_type = in_codec.id; - continue; + if (ulpfec_config.red_payload_type != -1) { + RTC_LOG(LS_ERROR) + << "Duplicate RED codec: ignoring PT=" << payload_type + << " in favor of PT=" << ulpfec_config.red_payload_type + << " which was specified first."; + break; + } + ulpfec_config.red_payload_type = payload_type; + break; } case VideoCodec::CODEC_ULPFEC: { - // ULPFEC payload type, should not have duplicates. - RTC_DCHECK_EQ(-1, ulpfec_config.ulpfec_payload_type); - ulpfec_config.ulpfec_payload_type = in_codec.id; - continue; + if (ulpfec_config.ulpfec_payload_type != -1) { + RTC_LOG(LS_ERROR) + << "Duplicate ULPFEC codec: ignoring PT=" << payload_type + << " in favor of PT=" << ulpfec_config.ulpfec_payload_type + << " which was specified first."; + break; + } + ulpfec_config.ulpfec_payload_type = payload_type; + break; } case VideoCodec::CODEC_FLEXFEC: { - // FlexFEC payload type, should not have duplicates. - RTC_DCHECK_EQ(-1, flexfec_payload_type); - flexfec_payload_type = in_codec.id; - continue; + if (flexfec_payload_type) { + RTC_LOG(LS_ERROR) + << "Duplicate FLEXFEC codec: ignoring PT=" << payload_type + << " in favor of PT=" << *flexfec_payload_type + << " which was specified first."; + break; + } + flexfec_payload_type = payload_type; + break; } case VideoCodec::CODEC_RTX: { @@ -2937,49 +2949,57 @@ WebRtcVideoChannel::MapCodecs(const std::vector& codecs) { RTC_LOG(LS_ERROR) << "RTX codec with invalid or no associated payload type: " << in_codec.ToString(); - return std::vector(); + return {}; } - rtx_mapping[associated_payload_type] = in_codec.id; - continue; + rtx_mapping[associated_payload_type] = payload_type; + break; } - case VideoCodec::CODEC_VIDEO: + case VideoCodec::CODEC_VIDEO: { + video_codecs.emplace_back(); + video_codecs.back().codec = in_codec; break; + } } - - video_codecs.push_back(VideoCodecSettings()); - video_codecs.back().codec = in_codec; } // One of these codecs should have been a video codec. Only having FEC // parameters into this code is a logic error. RTC_DCHECK(!video_codecs.empty()); - for (std::map::const_iterator it = rtx_mapping.begin(); - it != rtx_mapping.end(); ++it) { - if (!payload_used[it->first]) { - RTC_LOG(LS_ERROR) << "RTX mapped to payload not in codec list."; - return std::vector(); + for (const auto& entry : rtx_mapping) { + const int associated_payload_type = entry.first; + const int rtx_payload_type = entry.second; + auto it = payload_codec_type.find(associated_payload_type); + if (it == payload_codec_type.end()) { + RTC_LOG(LS_ERROR) << "RTX codec (PT=" << rtx_payload_type + << ") mapped to PT=" << associated_payload_type + << " which is not in the codec list."; + return {}; } - if (payload_codec_type[it->first] != VideoCodec::CODEC_VIDEO && - payload_codec_type[it->first] != VideoCodec::CODEC_RED) { + const VideoCodec::CodecType associated_codec_type = it->second; + if (associated_codec_type != VideoCodec::CODEC_VIDEO && + associated_codec_type != VideoCodec::CODEC_RED) { RTC_LOG(LS_ERROR) - << "RTX not mapped to regular video codec or RED codec."; - return std::vector(); + << "RTX PT=" << rtx_payload_type + << " not mapped to regular video codec or RED codec (PT=" + << associated_payload_type << ")."; + return {}; } - if (it->first == ulpfec_config.red_payload_type) { - ulpfec_config.red_rtx_payload_type = it->second; + if (associated_payload_type == ulpfec_config.red_payload_type) { + ulpfec_config.red_rtx_payload_type = rtx_payload_type; } } - for (size_t i = 0; i < video_codecs.size(); ++i) { - video_codecs[i].ulpfec = ulpfec_config; - video_codecs[i].flexfec_payload_type = flexfec_payload_type; - if (rtx_mapping[video_codecs[i].codec.id] != 0 && - rtx_mapping[video_codecs[i].codec.id] != - ulpfec_config.red_payload_type) { - video_codecs[i].rtx_payload_type = rtx_mapping[video_codecs[i].codec.id]; + for (VideoCodecSettings& codec_settings : video_codecs) { + const int payload_type = codec_settings.codec.id; + codec_settings.ulpfec = ulpfec_config; + codec_settings.flexfec_payload_type = flexfec_payload_type.value_or(-1); + auto it = rtx_mapping.find(payload_type); + if (it != rtx_mapping.end()) { + const int rtx_payload_type = it->second; + codec_settings.rtx_payload_type = rtx_payload_type; } } diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index c2c137ccaa..8dd758f0eb 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -227,8 +227,8 @@ class WebRtcVideoChannel : public VideoMediaChannel, VideoCodec codec; webrtc::UlpfecConfig ulpfec; - int flexfec_payload_type; - int rtx_payload_type; + int flexfec_payload_type; // -1 if absent. + int rtx_payload_type; // -1 if absent. }; struct ChangedSendParameters { @@ -481,6 +481,10 @@ class WebRtcVideoChannel : public VideoMediaChannel, const webrtc::PacketOptions& options) override; bool SendRtcp(const uint8_t* data, size_t len) override; + // Generate the list of codec parameters to pass down based on the negotiated + // "codecs". Note that VideoCodecSettings correspond to concrete codecs like + // VP8, VP9, H264 while VideoCodecs correspond also to "virtual" codecs like + // RTX, ULPFEC, FLEXFEC. static std::vector MapCodecs( const std::vector& codecs); // Get all codecs that are compatible with the receiver. diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 44bb128b59..92ffc5c47f 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -3848,6 +3848,28 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, EXPECT_EQ(1, video_stream.GetNumRemovedSecondarySinks()); } +TEST_F(WebRtcVideoChannelFlexfecRecvTest, DuplicateFlexfecCodecIsDropped) { + constexpr int kUnusedPayloadType1 = 127; + + cricket::VideoRecvParameters recv_parameters; + recv_parameters.codecs.push_back(GetEngineCodec("VP8")); + recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); + cricket::VideoCodec duplicate = GetEngineCodec("flexfec-03"); + duplicate.id = kUnusedPayloadType1; + recv_parameters.codecs.push_back(duplicate); + ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); + + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + + const std::vector& streams = + fake_call_->GetFlexfecReceiveStreams(); + ASSERT_EQ(1U, streams.size()); + const FakeFlexfecReceiveStream* stream = streams.front(); + const webrtc::FlexfecReceiveStream::Config& config = stream->GetConfig(); + EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.payload_type); +} + // TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all // tests that use this test fixture into the corresponding "non-field trial" // tests. @@ -4555,6 +4577,40 @@ TEST_F(WebRtcVideoChannelTest, SetRecvCodecsWithPacketizationRecreatesStream) { EXPECT_EQ(fake_call_->GetNumCreatedReceiveStreams(), 2); } +TEST_F(WebRtcVideoChannelTest, DuplicateUlpfecCodecIsDropped) { + constexpr int kFirstUlpfecPayloadType = 126; + constexpr int kSecondUlpfecPayloadType = 127; + + cricket::VideoRecvParameters parameters; + parameters.codecs.push_back(GetEngineCodec("VP8")); + parameters.codecs.push_back( + cricket::VideoCodec(kFirstUlpfecPayloadType, cricket::kUlpfecCodecName)); + parameters.codecs.push_back( + cricket::VideoCodec(kSecondUlpfecPayloadType, cricket::kUlpfecCodecName)); + ASSERT_TRUE(channel_->SetRecvParameters(parameters)); + + FakeVideoReceiveStream* recv_stream = AddRecvStream(); + EXPECT_EQ(kFirstUlpfecPayloadType, + recv_stream->GetConfig().rtp.ulpfec_payload_type); +} + +TEST_F(WebRtcVideoChannelTest, DuplicateRedCodecIsDropped) { + constexpr int kFirstRedPayloadType = 126; + constexpr int kSecondRedPayloadType = 127; + + cricket::VideoRecvParameters parameters; + parameters.codecs.push_back(GetEngineCodec("VP8")); + parameters.codecs.push_back( + cricket::VideoCodec(kFirstRedPayloadType, cricket::kRedCodecName)); + parameters.codecs.push_back( + cricket::VideoCodec(kSecondRedPayloadType, cricket::kRedCodecName)); + ASSERT_TRUE(channel_->SetRecvParameters(parameters)); + + FakeVideoReceiveStream* recv_stream = AddRecvStream(); + EXPECT_EQ(kFirstRedPayloadType, + recv_stream->GetConfig().rtp.red_payload_type); +} + TEST_F(WebRtcVideoChannelTest, SetRecvCodecsWithChangedRtxPayloadType) { const int kUnusedPayloadType1 = 126; const int kUnusedPayloadType2 = 127;