From b6bf0b2546f3f5eeec88112431c8a58e86a2e19a Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 28 Jan 2020 18:36:57 +0100 Subject: [PATCH] Pass picture_id from generic packetizer through codec-specific field To free up RtpVideoHeader::generic field for codec agnostic details from an rtp header extension. Bug: webrtc:10342 Change-Id: I7b9d869b2ecfedb96dfd860be47ed8dffa058749 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166175 Commit-Queue: Danil Chapovalov Reviewed-by: Niels Moller Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#30396} --- call/rtp_payload_params.cc | 9 ++------- call/rtp_payload_params_unittest.cc | 12 ++++++++---- .../rtp_rtcp/source/rtp_format_video_generic.cc | 5 +++-- .../source/rtp_format_video_generic_unittest.cc | 12 ++++++++---- modules/rtp_rtcp/source/rtp_sender_video.cc | 3 +++ modules/rtp_rtcp/source/rtp_video_header.h | 10 +++++++++- .../source/video_rtp_depacketizer_generic.cc | 6 +++--- .../video_rtp_depacketizer_generic_unittest.cc | 6 ++++-- .../video_coding/rtp_frame_reference_finder.cc | 17 ++++++++--------- 9 files changed, 48 insertions(+), 32 deletions(-) diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc index c71af6b097..70b156a1ea 100644 --- a/call/rtp_payload_params.cc +++ b/call/rtp_payload_params.cc @@ -240,15 +240,10 @@ void RtpPayloadParams::SetCodecSpecific(RTPVideoHeader* rtp_video_header, rtp_video_header->frame_marking.tl0_pic_idx = state_.tl0_pic_idx; } } - // There are currently two generic descriptors in WebRTC. The old descriptor - // can not share a picture id space between simulcast streams, so we use the - // |picture_id| in this case. We let the |picture_id| tag along in |frame_id| - // until the old generic format can be removed. - // TODO(philipel): Remove this when the new generic format has been fully - // implemented. if (generic_picture_id_experiment_ && rtp_video_header->codec == kVideoCodecGeneric) { - rtp_video_header->generic.emplace().frame_id = state_.picture_id; + rtp_video_header->video_type_header.emplace() + .picture_id = state_.picture_id; } } diff --git a/call/rtp_payload_params_unittest.cc b/call/rtp_payload_params_unittest.cc index 90b08a27e7..ad5d8e1303 100644 --- a/call/rtp_payload_params_unittest.cc +++ b/call/rtp_payload_params_unittest.cc @@ -333,12 +333,16 @@ TEST(RtpPayloadParamsTest, PictureIdForOldGenericFormat) { params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); EXPECT_EQ(kVideoCodecGeneric, header.codec); - ASSERT_TRUE(header.generic); - EXPECT_EQ(0, header.generic->frame_id); + const auto* generic = + absl::get_if(&header.video_type_header); + ASSERT_TRUE(generic); + EXPECT_EQ(0, generic->picture_id); header = params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare); - ASSERT_TRUE(header.generic); - EXPECT_EQ(1, header.generic->frame_id); + generic = + absl::get_if(&header.video_type_header); + ASSERT_TRUE(generic); + EXPECT_EQ(1, generic->picture_id); } TEST(RtpPayloadParamsTest, GenericDescriptorForGenericCodec) { diff --git a/modules/rtp_rtcp/source/rtp_format_video_generic.cc b/modules/rtp_rtcp/source/rtp_format_video_generic.cc index cf2bf19820..35d0f3dcc5 100644 --- a/modules/rtp_rtcp/source/rtp_format_video_generic.cc +++ b/modules/rtp_rtcp/source/rtp_format_video_generic.cc @@ -87,10 +87,11 @@ void RtpPacketizerGeneric::BuildHeader(const RTPVideoHeader& rtp_video_header) { if (rtp_video_header.frame_type == VideoFrameType::kVideoFrameKey) { header_[0] |= RtpFormatVideoGeneric::kKeyFrameBit; } - if (rtp_video_header.generic.has_value()) { + if (const auto* generic_header = absl::get_if( + &rtp_video_header.video_type_header)) { // Store bottom 15 bits of the picture id. Only 15 bits are used for // compatibility with other packetizer implemenetations. - uint16_t picture_id = rtp_video_header.generic->frame_id & 0x7FFF; + uint16_t picture_id = generic_header->picture_id; header_[0] |= RtpFormatVideoGeneric::kExtendedHeaderBit; header_[1] = (picture_id >> 8) & 0x7F; header_[2] = picture_id & 0xFF; diff --git a/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc b/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc index a13b53154d..35e7fe7ead 100644 --- a/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc @@ -75,7 +75,8 @@ TEST(RtpPacketizerVideoGeneric, WritesExtendedHeaderWhenPictureIdIsSet) { const uint8_t kPayload[kPayloadSize] = {}; RTPVideoHeader rtp_video_header; - rtp_video_header.generic.emplace().frame_id = 37; + rtp_video_header.video_type_header.emplace() + .picture_id = 37; rtp_video_header.frame_type = VideoFrameType::kVideoFrameKey; RtpPacketizerGeneric packetizer(kPayload, kNoSizeLimits, rtp_video_header); @@ -97,7 +98,8 @@ TEST(RtpPacketizerVideoGeneric, RespectsMaxPayloadSizeWithExtendedHeader) { RtpPacketizer::PayloadSizeLimits limits; limits.max_payload_len = 6; RTPVideoHeader rtp_video_header; - rtp_video_header.generic.emplace().frame_id = 37; + rtp_video_header.video_type_header.emplace() + .picture_id = 37; RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header); std::vector payload_sizes = NextPacketFillPayloadSizes(&packetizer); @@ -112,7 +114,8 @@ TEST(RtpPacketizerVideoGeneric, UsesMaxPayloadSizeWithExtendedHeader) { RtpPacketizer::PayloadSizeLimits limits; limits.max_payload_len = 6; RTPVideoHeader rtp_video_header; - rtp_video_header.generic.emplace().frame_id = 37; + rtp_video_header.video_type_header.emplace() + .picture_id = 37; RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header); std::vector payload_sizes = NextPacketFillPayloadSizes(&packetizer); @@ -126,7 +129,8 @@ TEST(RtpPacketizerVideoGeneric, FrameIdOver15bitsWrapsAround) { const uint8_t kPayload[kPayloadSize] = {}; RTPVideoHeader rtp_video_header; - rtp_video_header.generic.emplace().frame_id = 0x8137; + rtp_video_header.video_type_header.emplace() + .picture_id = 0x8137; rtp_video_header.frame_type = VideoFrameType::kVideoFrameKey; RtpPacketizerGeneric packetizer(kPayload, kNoSizeLimits, rtp_video_header); diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 7b7e018464..9779df1361 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -806,6 +806,9 @@ uint8_t RTPSenderVideo::GetTemporalId(const RTPVideoHeader& header) { return vp9.temporal_idx; } uint8_t operator()(const RTPVideoHeaderH264&) { return kNoTemporalIdx; } + uint8_t operator()(const RTPVideoHeaderLegacyGeneric&) { + return kNoTemporalIdx; + } uint8_t operator()(const absl::monostate&) { return kNoTemporalIdx; } }; switch (header.codec) { diff --git a/modules/rtp_rtcp/source/rtp_video_header.h b/modules/rtp_rtcp/source/rtp_video_header.h index 9af2c094b6..b66cba8404 100644 --- a/modules/rtp_rtcp/source/rtp_video_header.h +++ b/modules/rtp_rtcp/source/rtp_video_header.h @@ -28,10 +28,18 @@ #include "modules/video_coding/codecs/vp9/include/vp9_globals.h" namespace webrtc { +// Details passed in the rtp payload for legacy generic rtp packetizer. +// TODO(bugs.webrtc.org/9772): Deprecate in favor of passing generic video +// details in an rtp header extension. +struct RTPVideoHeaderLegacyGeneric { + uint16_t picture_id; +}; + using RTPVideoTypeHeader = absl::variant; + RTPVideoHeaderH264, + RTPVideoHeaderLegacyGeneric>; struct RTPVideoHeader { struct GenericDescriptorInfo { diff --git a/modules/rtp_rtcp/source/video_rtp_depacketizer_generic.cc b/modules/rtp_rtcp/source/video_rtp_depacketizer_generic.cc index e601eae614..6010771318 100644 --- a/modules/rtp_rtcp/source/video_rtp_depacketizer_generic.cc +++ b/modules/rtp_rtcp/source/video_rtp_depacketizer_generic.cc @@ -59,9 +59,9 @@ VideoRtpDepacketizerGeneric::Parse(rtc::CopyOnWriteBuffer rtp_payload) { RTC_LOG(LS_WARNING) << "Too short payload for generic header."; return absl::nullopt; } - parsed->video_header.generic.emplace(); - parsed->video_header.generic->frame_id = - ((payload_data[1] & 0x7F) << 8) | payload_data[2]; + parsed->video_header.video_type_header + .emplace() + .picture_id = ((payload_data[1] & 0x7F) << 8) | payload_data[2]; offset += kExtendedHeaderLength; } diff --git a/modules/rtp_rtcp/source/video_rtp_depacketizer_generic_unittest.cc b/modules/rtp_rtcp/source/video_rtp_depacketizer_generic_unittest.cc index 524fc3f775..860ddab4fd 100644 --- a/modules/rtp_rtcp/source/video_rtp_depacketizer_generic_unittest.cc +++ b/modules/rtp_rtcp/source/video_rtp_depacketizer_generic_unittest.cc @@ -46,8 +46,10 @@ TEST(VideoRtpDepacketizerGeneric, ExtendedHeaderParsesFrameId) { depacketizer.Parse(rtp_payload); ASSERT_TRUE(parsed); - ASSERT_TRUE(parsed->video_header.generic); - EXPECT_EQ(parsed->video_header.generic->frame_id, 0x1337); + const auto* generic_header = absl::get_if( + &parsed->video_header.video_type_header); + ASSERT_TRUE(generic_header); + EXPECT_EQ(generic_header->picture_id, 0x1337); EXPECT_THAT(parsed->video_payload, SizeIs(kRtpPayloadSize - 3)); } diff --git a/modules/video_coding/rtp_frame_reference_finder.cc b/modules/video_coding/rtp_frame_reference_finder.cc index 873e71a1b0..51228218d0 100644 --- a/modules/video_coding/rtp_frame_reference_finder.cc +++ b/modules/video_coding/rtp_frame_reference_finder.cc @@ -111,15 +111,14 @@ RtpFrameReferenceFinder::ManageFrameInternal(RtpFrameObject* frame) { return ManageFrameVp9(frame); case kVideoCodecH264: return ManageFrameH264(frame); - default: { - // Use 15 first bits of frame ID as picture ID if available. - const RTPVideoHeader& video_header = frame->GetRtpVideoHeader(); - int picture_id = kNoPictureId; - if (video_header.generic) - picture_id = video_header.generic->frame_id & 0x7fff; - - return ManageFramePidOrSeqNum(frame, picture_id); - } + case kVideoCodecGeneric: + if (auto* generic_header = absl::get_if( + &frame->GetRtpVideoHeader().video_type_header)) { + return ManageFramePidOrSeqNum(frame, generic_header->picture_id); + } + ABSL_FALLTHROUGH_INTENDED; + default: + return ManageFramePidOrSeqNum(frame, kNoPictureId); } }