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 <danilchap@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30396}
diff --git a/call/rtp_payload_params.cc b/call/rtp_payload_params.cc
index c71af6b..70b156a 100644
--- a/call/rtp_payload_params.cc
+++ b/call/rtp_payload_params.cc
@@ -240,15 +240,10 @@
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<RTPVideoHeaderLegacyGeneric>()
+ .picture_id = state_.picture_id;
}
}
diff --git a/call/rtp_payload_params_unittest.cc b/call/rtp_payload_params_unittest.cc
index 90b08a2..ad5d8e1 100644
--- a/call/rtp_payload_params_unittest.cc
+++ b/call/rtp_payload_params_unittest.cc
@@ -333,12 +333,16 @@
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<RTPVideoHeaderLegacyGeneric>(&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<RTPVideoHeaderLegacyGeneric>(&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 cf2bf19..35d0f3d 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 @@
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<RTPVideoHeaderLegacyGeneric>(
+ &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 a13b531..35e7fe7 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 @@
const uint8_t kPayload[kPayloadSize] = {};
RTPVideoHeader rtp_video_header;
- rtp_video_header.generic.emplace().frame_id = 37;
+ rtp_video_header.video_type_header.emplace<RTPVideoHeaderLegacyGeneric>()
+ .picture_id = 37;
rtp_video_header.frame_type = VideoFrameType::kVideoFrameKey;
RtpPacketizerGeneric packetizer(kPayload, kNoSizeLimits, rtp_video_header);
@@ -97,7 +98,8 @@
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<RTPVideoHeaderLegacyGeneric>()
+ .picture_id = 37;
RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header);
std::vector<int> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
@@ -112,7 +114,8 @@
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<RTPVideoHeaderLegacyGeneric>()
+ .picture_id = 37;
RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header);
std::vector<int> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
@@ -126,7 +129,8 @@
const uint8_t kPayload[kPayloadSize] = {};
RTPVideoHeader rtp_video_header;
- rtp_video_header.generic.emplace().frame_id = 0x8137;
+ rtp_video_header.video_type_header.emplace<RTPVideoHeaderLegacyGeneric>()
+ .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 7b7e018..9779df1 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -806,6 +806,9 @@
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 9af2c09..b66cba8 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<absl::monostate,
RTPVideoHeaderVP8,
RTPVideoHeaderVP9,
- RTPVideoHeaderH264>;
+ 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 e601eae..6010771 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 @@
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<RTPVideoHeaderLegacyGeneric>()
+ .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 524fc3f..860ddab 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 @@
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<RTPVideoHeaderLegacyGeneric>(
+ &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 873e71a..5122821 100644
--- a/modules/video_coding/rtp_frame_reference_finder.cc
+++ b/modules/video_coding/rtp_frame_reference_finder.cc
@@ -111,15 +111,14 @@
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<RTPVideoHeaderLegacyGeneric>(
+ &frame->GetRtpVideoHeader().video_type_header)) {
+ return ManageFramePidOrSeqNum(frame, generic_header->picture_id);
+ }
+ ABSL_FALLTHROUGH_INTENDED;
+ default:
+ return ManageFramePidOrSeqNum(frame, kNoPictureId);
}
}