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