Ship GenericDescriptor00 by default. The change ships GenericDescriptor00 and authentication by default, but doesn't expose it by default, and makes WebRTC respond to offers carrying it. The change adds a unit test for the new semantics. Tests well in munge-sdp. Frame marking replaced by http://www.webrtc.org/experiments/rtp-hdrext/generic-frame-descriptor-00 in the offer results in an answer containing the extension as first entry. Bug: webrtc:11367 Change-Id: I0ef91b7d4096d949c3d547ece7d6c4d39aa241da Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168661 Reviewed-by: Magnus Flodman <mflodman@webrtc.org> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Commit-Queue: Markus Handell <handellm@webrtc.org> Cr-Commit-Position: refs/heads/master@{#30542}
diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index f69a52b..31cb743 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc
@@ -140,7 +140,7 @@ generic_picture_id_experiment_( field_trial::IsEnabled("WebRTC-GenericPictureId")), generic_descriptor_experiment_( - field_trial::IsEnabled("WebRTC-GenericDescriptor")) { + !field_trial::IsDisabled("WebRTC-GenericDescriptor")) { for (auto& spatial_layer : last_shared_frame_id_) spatial_layer.fill(-1);
diff --git a/call/rtp_payload_params_unittest.cc b/call/rtp_payload_params_unittest.cc index b8fd4a5..54b4025 100644 --- a/call/rtp_payload_params_unittest.cc +++ b/call/rtp_payload_params_unittest.cc
@@ -160,7 +160,7 @@ h264info->temporal_idx = kNoTemporalIdx; RTPVideoHeader header = - params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); + params.GetRtpVideoHeader(encoded_image, &codec_info, 10); EXPECT_EQ(0, header.simulcastIdx); EXPECT_EQ(kVideoCodecH264, header.codec); @@ -172,7 +172,7 @@ h264info->base_layer_sync = true; h264info->idr_frame = false; - header = params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); + header = params.GetRtpVideoHeader(encoded_image, &codec_info, 20); EXPECT_EQ(kVideoCodecH264, header.codec); EXPECT_EQ(header.frame_marking.tl0_pic_idx, kInitialTl0PicIdx1); @@ -185,7 +185,7 @@ h264info->base_layer_sync = false; h264info->idr_frame = true; - header = params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); + header = params.GetRtpVideoHeader(encoded_image, &codec_info, 30); EXPECT_EQ(kVideoCodecH264, header.codec); EXPECT_EQ(header.frame_marking.tl0_pic_idx, kInitialTl0PicIdx1 + 1); @@ -327,10 +327,11 @@ EncodedImage encoded_image; CodecSpecificInfo codec_info; codec_info.codecType = kVideoCodecGeneric; + encoded_image._frameType = VideoFrameType::kVideoFrameKey; RtpPayloadParams params(kSsrc1, &state); RTPVideoHeader header = - params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); + params.GetRtpVideoHeader(encoded_image, &codec_info, 10); EXPECT_EQ(kVideoCodecGeneric, header.codec); const auto* generic = @@ -338,7 +339,8 @@ ASSERT_TRUE(generic); EXPECT_EQ(0, generic->picture_id); - header = params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); + encoded_image._frameType = VideoFrameType::kVideoFrameDelta; + header = params.GetRtpVideoHeader(encoded_image, &codec_info, 20); generic = absl::get_if<RTPVideoHeaderLegacyGeneric>(&header.video_type_header); ASSERT_TRUE(generic);
diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 2696514..78ece7f 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -269,7 +269,7 @@ require_frame_encryption_(config.require_frame_encryption), generic_descriptor_auth_experiment_( config.field_trials->Lookup("WebRTC-GenericDescriptorAuth") - .find("Enabled") == 0), + .find("Disabled") != 0), exclude_transport_sequence_number_from_fec_experiment_( config.field_trials ->Lookup(kExcludeTransportSequenceNumberFromFecFieldTrial) @@ -656,7 +656,7 @@ size_t bytes_written = 0; - // Only enable header authentication if the field trial is enabled. + // Enable header authentication if the field trial isn't disabled. rtc::ArrayView<const uint8_t> additional_data; if (generic_descriptor_auth_experiment_) { additional_data = generic_descriptor_raw;
diff --git a/pc/media_session.cc b/pc/media_session.cc index 35dd2e5..3df918a 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc
@@ -961,13 +961,13 @@ static bool FindByUriWithEncryptionPreference( const RtpHeaderExtensions& extensions, - const webrtc::RtpExtension& ext_to_match, + absl::string_view uri_to_match, bool encryption_preference, webrtc::RtpExtension* found_extension) { const webrtc::RtpExtension* unencrypted_extension = nullptr; for (const webrtc::RtpExtension& extension : extensions) { // We assume that all URIs are given in a canonical format. - if (extension.uri == ext_to_match.uri) { + if (extension.uri == uri_to_match) { if (!encryption_preference || extension.encrypt) { if (found_extension) { *found_extension = extension; @@ -1037,7 +1037,7 @@ // extensions. if (extension.encrypt || !webrtc::RtpExtension::IsEncryptionSupported(extension.uri) || - (FindByUriWithEncryptionPreference(*extensions, extension, true, + (FindByUriWithEncryptionPreference(*extensions, extension.uri, true, &existing) && existing.encrypt)) { continue; @@ -1073,11 +1073,14 @@ offered_extensions, webrtc::RtpExtension::kTransportSequenceNumberV2Uri); + bool frame_descriptor_in_local = false; for (const webrtc::RtpExtension& ours : local_extensions) { + if (ours.uri == webrtc::RtpExtension::kGenericFrameDescriptorUri00) + frame_descriptor_in_local = true; webrtc::RtpExtension theirs; if (FindByUriWithEncryptionPreference( - offered_extensions, ours, enable_encrypted_rtp_header_extensions, - &theirs)) { + offered_extensions, ours.uri, + enable_encrypted_rtp_header_extensions, &theirs)) { if (transport_sequence_number_v2_offer && ours.uri == webrtc::RtpExtension::kTransportSequenceNumberUri) { // Don't respond to @@ -1096,6 +1099,17 @@ // Respond that we support kTransportSequenceNumberV2Uri. negotiated_extensions->push_back(*transport_sequence_number_v2_offer); } + + // Frame descriptor support. If the extension is not present locally, but is + // in the offer, we add it to the list. + if (!frame_descriptor_in_local) { + webrtc::RtpExtension theirs; + if (FindByUriWithEncryptionPreference( + offered_extensions, + webrtc::RtpExtension::kGenericFrameDescriptorUri00, + enable_encrypted_rtp_header_extensions, &theirs)) + negotiated_extensions->push_back(theirs); + } } static void StripCNCodecs(AudioCodecs* audio_codecs) {
diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index a2416c4..389b6a0 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc
@@ -238,6 +238,12 @@ 2), }; +static const RtpExtension kRtpExtensionGenericFrameDescriptorUri00[] = { + RtpExtension("http://www.webrtc.org/experiments/rtp-hdrext/" + "generic-frame-descriptor-00", + 3), +}; + static const uint32_t kSimulcastParamsSsrc[] = {10, 11, 20, 21, 30, 31}; static const uint32_t kSimSsrc[] = {10, 20, 30}; static const uint32_t kFec1Ssrc[] = {10, 11}; @@ -1672,6 +1678,50 @@ } TEST_F(MediaSessionDescriptionFactoryTest, + TestNegotiateFrameDescriptorWhenUnexposedLocally) { + MediaSessionOptions opts; + AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts); + + const auto offered = MAKE_VECTOR(kRtpExtensionGenericFrameDescriptorUri00); + f1_.set_audio_rtp_header_extensions(offered); + f1_.set_video_rtp_header_extensions(offered); + const auto local = MAKE_VECTOR(kRtpExtensionTransportSequenceNumber01); + f2_.set_audio_rtp_header_extensions(local); + f2_.set_video_rtp_header_extensions(local); + std::unique_ptr<SessionDescription> offer = f1_.CreateOffer(opts, nullptr); + std::unique_ptr<SessionDescription> answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + EXPECT_THAT( + GetFirstAudioContentDescription(answer.get())->rtp_header_extensions(), + ElementsAreArray(offered)); + EXPECT_THAT( + GetFirstVideoContentDescription(answer.get())->rtp_header_extensions(), + ElementsAreArray(offered)); +} + +TEST_F(MediaSessionDescriptionFactoryTest, + TestNegotiateFrameDescriptorWhenExposedLocally) { + MediaSessionOptions opts; + AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts); + + const auto offered = MAKE_VECTOR(kRtpExtensionGenericFrameDescriptorUri00); + f1_.set_audio_rtp_header_extensions(offered); + f1_.set_video_rtp_header_extensions(offered); + const auto local = MAKE_VECTOR(kRtpExtensionGenericFrameDescriptorUri00); + f2_.set_audio_rtp_header_extensions(local); + f2_.set_video_rtp_header_extensions(local); + std::unique_ptr<SessionDescription> offer = f1_.CreateOffer(opts, nullptr); + std::unique_ptr<SessionDescription> answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + EXPECT_THAT( + GetFirstAudioContentDescription(answer.get())->rtp_header_extensions(), + ElementsAreArray(offered)); + EXPECT_THAT( + GetFirstVideoContentDescription(answer.get())->rtp_header_extensions(), + ElementsAreArray(offered)); +} + +TEST_F(MediaSessionDescriptionFactoryTest, TestOfferAnswerWithEncryptedRtpExtensionsBoth) { MediaSessionOptions opts; AddAudioVideoSections(RtpTransceiverDirection::kRecvOnly, &opts);
diff --git a/video/buffered_frame_decryptor.cc b/video/buffered_frame_decryptor.cc index ae83da9..fc9dff5 100644 --- a/video/buffered_frame_decryptor.cc +++ b/video/buffered_frame_decryptor.cc
@@ -24,7 +24,7 @@ OnDecryptedFrameCallback* decrypted_frame_callback, OnDecryptionStatusChangeCallback* decryption_status_change_callback) : generic_descriptor_auth_experiment_( - field_trial::IsEnabled("WebRTC-GenericDescriptorAuth")), + !field_trial::IsDisabled("WebRTC-GenericDescriptorAuth")), decrypted_frame_callback_(decrypted_frame_callback), decryption_status_change_callback_(decryption_status_change_callback) {} @@ -76,7 +76,7 @@ rtc::ArrayView<uint8_t> inline_decrypted_bitstream(frame->data(), max_plaintext_byte_size); - // Only enable authenticating the header if the field trial is enabled. + // Enable authenticating the header if the field trial isn't disabled. std::vector<uint8_t> additional_data; if (generic_descriptor_auth_experiment_) { additional_data = RtpDescriptorAuthentication(frame->GetRtpVideoHeader());