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}

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<RTPVideoHeaderLegacyGeneric>()
+        .picture_id = state_.picture_id;
   }
 }
 

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);
+  const auto* generic =
-  EXPECT_EQ(0, header.generic->frame_id);
+      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);
+  generic =
-  EXPECT_EQ(1, header.generic->frame_id);
+      absl::get_if<RTPVideoHeaderLegacyGeneric>(&header.video_type_header);
+  ASSERT_TRUE(generic);
+  EXPECT_EQ(1, generic->picture_id);
 }
 
 TEST(RtpPayloadParamsTest, GenericDescriptorForGenericCodec) {

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<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;

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<RTPVideoHeaderLegacyGeneric>()
+      .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<RTPVideoHeaderLegacyGeneric>()
+      .picture_id = 37;
   RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header);
 
   std::vector<int> 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<RTPVideoHeaderLegacyGeneric>()
+      .picture_id = 37;
   RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header);
   std::vector<int> 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<RTPVideoHeaderLegacyGeneric>()
+      .picture_id = 0x8137;
   rtp_video_header.frame_type = VideoFrameType::kVideoFrameKey;
   RtpPacketizerGeneric packetizer(kPayload, kNoSizeLimits, rtp_video_header);
 

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) {

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 {

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.video_type_header
-    parsed->video_header.generic->frame_id =
+        .emplace<RTPVideoHeaderLegacyGeneric>()
-        ((payload_data[1] & 0x7F) << 8) | payload_data[2];
+        .picture_id = ((payload_data[1] & 0x7F) << 8) | payload_data[2];
     offset += kExtendedHeaderLength;
   }
 

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);
+  const auto* generic_header = absl::get_if<RTPVideoHeaderLegacyGeneric>(
-  EXPECT_EQ(parsed->video_header.generic->frame_id, 0x1337);
+      &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));
 }
 

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: {
+    case kVideoCodecGeneric:
-      // Use 15 first bits of frame ID as picture ID if available.
+      if (auto* generic_header = absl::get_if<RTPVideoHeaderLegacyGeneric>(
-      const RTPVideoHeader& video_header = frame->GetRtpVideoHeader();
+              &frame->GetRtpVideoHeader().video_type_header)) {
-      int picture_id = kNoPictureId;
+        return ManageFramePidOrSeqNum(frame, generic_header->picture_id);
-      if (video_header.generic)
-        picture_id = video_header.generic->frame_id & 0x7fff;
-
-      return ManageFramePidOrSeqNum(frame, picture_id);
       }
+      ABSL_FALLTHROUGH_INTENDED;
+    default:
+      return ManageFramePidOrSeqNum(frame, kNoPictureId);
   }
 }