From 3aa9937e48fec17bfdcc3a65c141efb527a9d67c Mon Sep 17 00:00:00 2001 From: Stefan Mitic Date: Thu, 3 Feb 2022 04:26:22 -0800 Subject: [PATCH] Fix for payload type id collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This collision can occur when we have asymetrical send and receive codecs. This is the case in the current code base with the VP9 codec familly but is not visible untill more codecs are added. Added Nutanix Inc. to AUTHORS. Bug: chromium:1291956 Change-Id: I09d3f76161d984d2a3edf721639753bffd4947b9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250034 Reviewed-by: Henrik Boström Reviewed-by: Harald Alvestrand Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#35944} --- AUTHORS | 1 + media/engine/webrtc_video_engine.cc | 9 ++++- pc/media_session.cc | 51 +++++++++++++++++------------ pc/peer_connection_jsep_unittest.cc | 36 ++++++++++++++++++++ 4 files changed, 75 insertions(+), 22 deletions(-) diff --git a/AUTHORS b/AUTHORS index eb650c59d7..e4729a574b 100644 --- a/AUTHORS +++ b/AUTHORS @@ -138,6 +138,7 @@ Microsoft Corporation <*@microsoft.com> MIPS Technologies <*@mips.com> Mozilla Foundation <*@mozilla.com> Netgem S.A. <*@netgem.com> +Nutanix Inc. <*@nutanix.com> NVIDIA Corporation <*@nvidia.com> Opera Software ASA <*@opera.com> Optical Tone Ltd <*@opticaltone.com> diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 940105817d..58ae4995d7 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -135,8 +135,15 @@ bool IsCodecValidForLowerRange(const VideoCodec& codec) { return true; } else if (absl::EqualsIgnoreCase(codec.name, kH264CodecName)) { std::string profileLevelId; - // H264 with YUV444. + std::string packetizationMode; + if (codec.GetParam(kH264FmtpProfileLevelId, &profileLevelId)) { + if (absl::StartsWithIgnoreCase(profileLevelId, "4d00")) { + if (codec.GetParam(kH264FmtpPacketizationMode, &packetizationMode)) { + return packetizationMode == "0"; + } + } + // H264 with YUV444. return absl::StartsWithIgnoreCase(profileLevelId, "f400"); } } diff --git a/pc/media_session.cc b/pc/media_session.cc index ac57459078..96c285e67e 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1111,6 +1111,25 @@ static Codecs MatchCodecPreference( return filtered_codecs; } +// Compute the union of `codecs1` and `codecs2`. +template +std::vector ComputeCodecsUnion(const std::vector& codecs1, + const std::vector& codecs2) { + std::vector all_codecs; + UsedPayloadTypes used_payload_types; + for (const C& codec : codecs1) { + C codec_mutable = codec; + used_payload_types.FindAndSetIdUsed(&codec_mutable); + all_codecs.push_back(codec_mutable); + } + + // Use MergeCodecs to merge the second half of our list as it already checks + // and fixes problems with duplicate payload types. + MergeCodecs(codecs2, &all_codecs, &used_payload_types); + + return all_codecs; +} + // Adds all extensions from `reference_extensions` to `offered_extensions` that // don't already exist in `offered_extensions` and ensure the IDs don't // collide. If an extension is added, it's also added to `regular_extensions` or @@ -2696,7 +2715,9 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( } } } + // Add other supported video codecs. + VideoCodecs other_video_codecs; for (const VideoCodec& codec : supported_video_codecs) { if (FindMatchingCodec(supported_video_codecs, video_codecs, codec, nullptr) && @@ -2704,9 +2725,13 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( filtered_codecs, codec, nullptr)) { // We should use the local codec with local parameters and the codec id // would be correctly mapped in `NegotiateCodecs`. - filtered_codecs.push_back(codec); + other_video_codecs.push_back(codec); } } + + // Use ComputeCodecsUnion to avoid having duplicate payload IDs + filtered_codecs = + ComputeCodecsUnion(filtered_codecs, other_video_codecs); } if (session_options.raw_packetization_for_video) { @@ -2900,27 +2925,11 @@ void MediaSessionDescriptionFactory::ComputeAudioCodecsIntersectionAndUnion() { void MediaSessionDescriptionFactory::ComputeVideoCodecsIntersectionAndUnion() { video_sendrecv_codecs_.clear(); - all_video_codecs_.clear(); - // Compute the video codecs union. - for (const VideoCodec& send : video_send_codecs_) { - all_video_codecs_.push_back(send); - if (!FindMatchingCodec(video_send_codecs_, video_recv_codecs_, - send, nullptr)) { - // TODO(kron): This check is violated by the unit test: - // MediaSessionDescriptionFactoryTest.RtxWithoutApt - // Remove either the test or the check. - // It doesn't make sense to have an RTX codec we support sending but not - // receiving. - // RTC_DCHECK(!IsRtxCodec(send)); - } - } - for (const VideoCodec& recv : video_recv_codecs_) { - if (!FindMatchingCodec(video_recv_codecs_, video_send_codecs_, - recv, nullptr)) { - all_video_codecs_.push_back(recv); - } - } + // Use ComputeCodecsUnion to avoid having duplicate payload IDs + all_video_codecs_ = + ComputeCodecsUnion(video_recv_codecs_, video_send_codecs_); + // Use NegotiateCodecs to merge our codec lists, since the operation is // essentially the same. Put send_codecs as the offered_codecs, which is the // order we'd like to follow. The reasoning is that encoding is usually more diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index 4713068a15..66581ca852 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -505,6 +505,42 @@ TEST_F(PeerConnectionJsepTest, SetRemoteAnswerUpdatesCurrentDirection) { transceivers[0]->current_direction()); } +TEST_F(PeerConnectionJsepTest, + ChangeDirectionFromRecvOnlyToSendRecvDoesNotBreakVideoNegotiation) { + auto caller = CreatePeerConnection(); + auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + auto callee = CreatePeerConnection(); + caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); +} + +TEST_F(PeerConnectionJsepTest, + ChangeDirectionFromRecvOnlyToSendRecvDoesNotBreakAudioNegotiation) { + auto caller = CreatePeerConnection(); + auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + auto callee = CreatePeerConnection(); + caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); +} + // Tests for multiple round trips. // Test that setting a transceiver with the inactive direction does not stop it