From 8a5f807313425e65ee3c53643cb88e8c5119a452 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 3 May 2024 09:09:52 +0000 Subject: [PATCH] Reland "h264: bail out early when failing to parse SPS/PPS ids" This reverts commit e1607ed3a619ae30cf8564ce401df5e03dd7bf4b. Reason for revert: downstream project adjusted Original change's description: > Revert "h264: bail out early when failing to parse SPS/PPS ids" > > This reverts commit 4344eb713bb9a6d04d922d00fb492dfb31c9111f. > > Reason for revert: Breaks downstream project. > > Original change's description: > > h264: bail out early when failing to parse SPS/PPS ids > > > > This currently gets caught later in the process by the H264 SPS/PPS > > tracker but can be rejected explicitly here. The network observable > > behavior should be similar and request a key frame after a 200ms delay, at least for entities that send such bad bitstreams > > > > BUG=webrtc:337076010 > > > > Change-Id: I239c64efa7db631460ef9e9986d283335303df5f > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349060 > > Reviewed-by: Danil Chapovalov > > Reviewed-by: Harald Alvestrand > > Commit-Queue: Philipp Hancke > > Cr-Commit-Position: refs/heads/main@{#42211} > > Bug: webrtc:337076010 > Change-Id: I15b815c69f1d25e41fb222d46359655242589fba > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349661 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Commit-Queue: Mirko Bonadei > Owners-Override: Mirko Bonadei > Cr-Commit-Position: refs/heads/main@{#42217} Bug: webrtc:337076010 Change-Id: Ibe5a960b9b5fdf9a35e5dfffb47b78ade36b0cec Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349700 Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Reviewed-by: Mirko Bonadei Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#42223} --- .../source/video_rtp_depacketizer_h264.cc | 74 ++++++++++--------- .../video_rtp_depacketizer_h264_unittest.cc | 38 ++++++++-- 2 files changed, 71 insertions(+), 41 deletions(-) diff --git a/modules/rtp_rtcp/source/video_rtp_depacketizer_h264.cc b/modules/rtp_rtcp/source/video_rtp_depacketizer_h264.cc index 9978e5f5fc..4198c80106 100644 --- a/modules/rtp_rtcp/source/video_rtp_depacketizer_h264.cc +++ b/modules/rtp_rtcp/source/video_rtp_depacketizer_h264.cc @@ -133,45 +133,47 @@ absl::optional ProcessStapAOrSingleNalu( SpsVuiRewriter::ParseResult result = SpsVuiRewriter::ParseAndRewriteSps( &payload_data[start_offset], end_offset - start_offset, &sps, nullptr, &output_buffer, SpsVuiRewriter::Direction::kIncoming); + switch (result) { + case SpsVuiRewriter::ParseResult::kFailure: + RTC_LOG(LS_WARNING) << "Failed to parse SPS NAL unit."; + return absl::nullopt; + case SpsVuiRewriter::ParseResult::kVuiRewritten: + if (modified_buffer) { + RTC_LOG(LS_WARNING) + << "More than one H264 SPS NAL units needing " + "rewriting found within a single STAP-A packet. " + "Keeping the first and rewriting the last."; + } - if (result == SpsVuiRewriter::ParseResult::kVuiRewritten) { - if (modified_buffer) { - RTC_LOG(LS_WARNING) - << "More than one H264 SPS NAL units needing " - "rewriting found within a single STAP-A packet. " - "Keeping the first and rewriting the last."; - } + // Rewrite length field to new SPS size. + if (h264_header.packetization_type == kH264StapA) { + size_t length_field_offset = + start_offset - (H264::kNaluTypeSize + kLengthFieldSize); + // Stap-A Length includes payload data and type header. + size_t rewritten_size = + output_buffer.size() - start_offset + H264::kNaluTypeSize; + ByteWriter::WriteBigEndian( + &output_buffer[length_field_offset], rewritten_size); + } - // Rewrite length field to new SPS size. - if (h264_header.packetization_type == kH264StapA) { - size_t length_field_offset = - start_offset - (H264::kNaluTypeSize + kLengthFieldSize); - // Stap-A Length includes payload data and type header. - size_t rewritten_size = - output_buffer.size() - start_offset + H264::kNaluTypeSize; - ByteWriter::WriteBigEndian( - &output_buffer[length_field_offset], rewritten_size); - } + parsed_payload->video_payload.SetData(output_buffer.data(), + output_buffer.size()); + // Append rest of packet. + parsed_payload->video_payload.AppendData( + &payload_data[end_offset], + nalu_length + kNalHeaderSize - end_offset); - parsed_payload->video_payload.SetData(output_buffer.data(), - output_buffer.size()); - // Append rest of packet. - parsed_payload->video_payload.AppendData( - &payload_data[end_offset], - nalu_length + kNalHeaderSize - end_offset); - - modified_buffer = true; + modified_buffer = true; + [[fallthrough]]; + case SpsVuiRewriter::ParseResult::kVuiOk: + RTC_DCHECK(sps); + nalu.sps_id = sps->id; + parsed_payload->video_header.width = sps->width; + parsed_payload->video_header.height = sps->height; + parsed_payload->video_header.frame_type = + VideoFrameType::kVideoFrameKey; + break; } - - if (sps) { - parsed_payload->video_header.width = sps->width; - parsed_payload->video_header.height = sps->height; - nalu.sps_id = sps->id; - } else { - RTC_LOG(LS_WARNING) << "Failed to parse SPS id from SPS slice."; - } - parsed_payload->video_header.frame_type = - VideoFrameType::kVideoFrameKey; break; } case H264::NaluType::kPps: { @@ -185,6 +187,7 @@ absl::optional ProcessStapAOrSingleNalu( } else { RTC_LOG(LS_WARNING) << "Failed to parse PPS id and SPS id from PPS slice."; + return absl::nullopt; } break; } @@ -200,6 +203,7 @@ absl::optional ProcessStapAOrSingleNalu( } else { RTC_LOG(LS_WARNING) << "Failed to parse PPS id from slice of type: " << static_cast(nalu.type); + return absl::nullopt; } break; } diff --git a/modules/rtp_rtcp/source/video_rtp_depacketizer_h264_unittest.cc b/modules/rtp_rtcp/source/video_rtp_depacketizer_h264_unittest.cc index f569c45fd3..7cc4108632 100644 --- a/modules/rtp_rtcp/source/video_rtp_depacketizer_h264_unittest.cc +++ b/modules/rtp_rtcp/source/video_rtp_depacketizer_h264_unittest.cc @@ -396,12 +396,6 @@ TEST(VideoRtpDepacketizerH264Test, TruncationJustAfterSingleStapANalu) { EXPECT_FALSE(depacketizer.Parse(rtc::CopyOnWriteBuffer(kPayload))); } -TEST(VideoRtpDepacketizerH264Test, ShortSpsPacket) { - const uint8_t kPayload[] = {0x27, 0x80, 0x00}; - VideoRtpDepacketizerH264 depacketizer; - EXPECT_TRUE(depacketizer.Parse(rtc::CopyOnWriteBuffer(kPayload))); -} - TEST(VideoRtpDepacketizerH264Test, SeiPacket) { const uint8_t kPayload[] = { kSei, // F=0, NRI=0, Type=6. @@ -421,5 +415,37 @@ TEST(VideoRtpDepacketizerH264Test, SeiPacket) { EXPECT_EQ(h264.nalus[0].pps_id, -1); } +TEST(VideoRtpDepacketizerH264Test, ShortSpsPacket) { + const uint8_t kPayload[] = {0x27, 0x80, 0x00}; + VideoRtpDepacketizerH264 depacketizer; + EXPECT_FALSE(depacketizer.Parse(rtc::CopyOnWriteBuffer(kPayload))); +} + +TEST(VideoRtpDepacketizerH264Test, BadSps) { + const uint8_t kPayload[] = { + kSps, 0x42, 0x41, 0x2a, 0xd3, 0x93, 0xd3, 0x3b // Payload. + }; + VideoRtpDepacketizerH264 depacketizer; + EXPECT_FALSE(depacketizer.Parse(rtc::CopyOnWriteBuffer(kPayload))); +} + +TEST(VideoRtpDepacketizerH264Test, BadPps) { + const uint8_t kPayload[] = { + kPps, + 0x00 // Payload. + }; + VideoRtpDepacketizerH264 depacketizer; + EXPECT_FALSE(depacketizer.Parse(rtc::CopyOnWriteBuffer(kPayload))); +} + +TEST(VideoRtpDepacketizerH264Test, BadSlice) { + const uint8_t kPayload[] = { + kIdr, + 0xc0 // Payload. + }; + VideoRtpDepacketizerH264 depacketizer; + EXPECT_FALSE(depacketizer.Parse(rtc::CopyOnWriteBuffer(kPayload))); +} + } // namespace } // namespace webrtc