From d2b092f38abad228b4c38697e0c56a189044d9c5 Mon Sep 17 00:00:00 2001 From: johan Date: Tue, 24 Jan 2017 02:38:17 -0800 Subject: [PATCH] Reland of H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. Fixed memory leak in test case. Original issue's description > Revert of H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. (patchset #3 id:40001 of https://codereview.webrtc.org/2638933002/ ) > > Reason for revert: > Triggers leak on Linux memcheck (non-default trybot): > > Original issue's description: > > H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header. > > > > - Changed method name to clarify that entire Nalus are expected. > > - Added unit test code. > > - Adjusted InsetSpsPpsNalus() implementation to above requirement. > > > > BUG=webrtc:5948 > > > > Review-Url: https://codereview.webrtc.org/2638933002 > > Cr-Commit-Position: refs/heads/master@{#16221} > > Committed: https://chromium.googlesource.com/external/webrtc/+/f53d7374cfa59440f777729d3a0b7dd39830d6ec > > TBR=philipel@webrtc.org,sprang@webrtc.org,johan@webrtc.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=webrtc:5948 > Review-Url: https://codereview.webrtc.org/2649113003 > Cr-Commit-Position: refs/heads/master@{#16225} > Committed: https://chromium.googlesource.com/external/webrtc/+/914d49d0fd43e5c650edd0d1e44667da924202c3 > BUG=webrtc:5948 Review-Url: https://codereview.webrtc.org/2651593004 Cr-Commit-Position: refs/heads/master@{#16235} --- .../video_coding/h264_sps_pps_tracker.cc | 44 +++++++++++-- .../video_coding/h264_sps_pps_tracker.h | 5 +- .../h264_sps_pps_tracker_unittest.cc | 66 +++++++++++++++++++ webrtc/video/rtp_stream_receiver.cc | 3 +- 4 files changed, 108 insertions(+), 10 deletions(-) diff --git a/webrtc/modules/video_coding/h264_sps_pps_tracker.cc b/webrtc/modules/video_coding/h264_sps_pps_tracker.cc index e13c9f4364..5dfdb49146 100644 --- a/webrtc/modules/video_coding/h264_sps_pps_tracker.cc +++ b/webrtc/modules/video_coding/h264_sps_pps_tracker.cc @@ -182,15 +182,41 @@ H264SpsPpsTracker::PacketAction H264SpsPpsTracker::CopyAndFixBitstream( return kInsert; } -void H264SpsPpsTracker::InsertSpsPps(const std::vector& sps, - const std::vector& pps) { - rtc::Optional parsed_sps = - SpsParser::ParseSps(sps.data(), sps.size()); - rtc::Optional parsed_pps = - PpsParser::ParsePps(pps.data(), pps.size()); +void H264SpsPpsTracker::InsertSpsPpsNalus(const std::vector& sps, + const std::vector& pps) { + constexpr size_t kNaluHeaderOffset = 1; + if (sps.size() < kNaluHeaderOffset) { + LOG(LS_WARNING) << "SPS size " << sps.size() << " is smaller than " + << kNaluHeaderOffset; + return; + } + if ((sps[0] & 0x1f) != H264::NaluType::kSps) { + LOG(LS_WARNING) << "SPS Nalu header missing"; + return; + } + if (pps.size() < kNaluHeaderOffset) { + LOG(LS_WARNING) << "PPS size " << pps.size() << " is smaller than " + << kNaluHeaderOffset; + return; + } + if ((pps[0] & 0x1f) != H264::NaluType::kPps) { + LOG(LS_WARNING) << "SPS Nalu header missing"; + return; + } + rtc::Optional parsed_sps = SpsParser::ParseSps( + sps.data() + kNaluHeaderOffset, sps.size() - kNaluHeaderOffset); + rtc::Optional parsed_pps = PpsParser::ParsePps( + pps.data() + kNaluHeaderOffset, pps.size() - kNaluHeaderOffset); + + if (!parsed_sps) { + LOG(LS_WARNING) << "Failed to parse SPS."; + } + + if (!parsed_pps) { + LOG(LS_WARNING) << "Failed to parse PPS."; + } if (!parsed_pps || !parsed_sps) { - LOG(LS_WARNING) << "Failed to parse SPS or PPS parameters."; return; } @@ -208,6 +234,10 @@ void H264SpsPpsTracker::InsertSpsPps(const std::vector& sps, memcpy(pps_data, pps.data(), pps_info.size); pps_info.data.reset(pps_data); pps_data_[parsed_pps->id] = std::move(pps_info); + + LOG(LS_INFO) << "Inserted SPS id " << parsed_sps->id << " and PPS id " + << parsed_pps->id << " (referencing SPS " << parsed_pps->sps_id + << ")"; } } // namespace video_coding diff --git a/webrtc/modules/video_coding/h264_sps_pps_tracker.h b/webrtc/modules/video_coding/h264_sps_pps_tracker.h index db44ea5f41..e769268779 100644 --- a/webrtc/modules/video_coding/h264_sps_pps_tracker.h +++ b/webrtc/modules/video_coding/h264_sps_pps_tracker.h @@ -29,8 +29,9 @@ class H264SpsPpsTracker { enum PacketAction { kInsert, kDrop, kRequestKeyframe }; PacketAction CopyAndFixBitstream(VCMPacket* packet); - void InsertSpsPps(const std::vector& sps, - const std::vector& pps); + + void InsertSpsPpsNalus(const std::vector& sps, + const std::vector& pps); private: struct PpsInfo { diff --git a/webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc b/webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc index 83330b934e..9d4d64f97e 100644 --- a/webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc +++ b/webrtc/modules/video_coding/h264_sps_pps_tracker_unittest.cc @@ -261,5 +261,71 @@ TEST_F(TestH264SpsPpsTracker, SpsPpsIdrInStapA) { delete[] packet.dataPtr; } +TEST_F(TestH264SpsPpsTracker, SpsPpsOutOfBand) { + constexpr uint8_t kData[] = {1, 2, 3}; + + // Generated by "ffmpeg -r 30 -f avfoundation -i "default" out.h264" on macos. + const std::vector sps( + {0x67, 0x7a, 0x00, 0x0d, 0xbc, 0xd9, 0x41, 0x41, 0xfa, 0x10, 0x00, 0x00, + 0x03, 0x00, 0x10, 0x00, 0x00, 0x03, 0x03, 0xc0, 0xf1, 0x42, 0x99, 0x60}); + const std::vector pps({0x68, 0xeb, 0xe3, 0xcb, 0x22, 0xc0}); + tracker_.InsertSpsPpsNalus(sps, pps); + + // Insert first packet of the IDR. + VCMPacket idr_packet = GetDefaultPacket(); + idr_packet.video_header.is_first_packet_in_frame = true; + AddIdr(&idr_packet, 0); + idr_packet.dataPtr = kData; + idr_packet.sizeBytes = sizeof(kData); + EXPECT_EQ(H264SpsPpsTracker::kInsert, + tracker_.CopyAndFixBitstream(&idr_packet)); + if (idr_packet.dataPtr != kData) { + // In case CopyAndFixBitStream() prepends SPS/PPS nalus to the packet, it + // uses new uint8_t[] to allocate memory. Caller of CopyAndFixBitStream() + // needs to take care of freeing the memory. + delete[] idr_packet.dataPtr; + } +} + +TEST_F(TestH264SpsPpsTracker, SpsPpsOutOfBandWrongNaluHeader) { + constexpr uint8_t kData[] = {1, 2, 3}; + + // Generated by "ffmpeg -r 30 -f avfoundation -i "default" out.h264" on macos. + // Nalu headers manupilated afterwards. + const std::vector sps( + {0xff, 0x7a, 0x00, 0x0d, 0xbc, 0xd9, 0x41, 0x41, 0xfa, 0x10, 0x00, 0x00, + 0x03, 0x00, 0x10, 0x00, 0x00, 0x03, 0x03, 0xc0, 0xf1, 0x42, 0x99, 0x60}); + const std::vector pps({0xff, 0xeb, 0xe3, 0xcb, 0x22, 0xc0}); + tracker_.InsertSpsPpsNalus(sps, pps); + + // Insert first packet of the IDR. + VCMPacket idr_packet = GetDefaultPacket(); + idr_packet.video_header.is_first_packet_in_frame = true; + AddIdr(&idr_packet, 0); + idr_packet.dataPtr = kData; + idr_packet.sizeBytes = sizeof(kData); + EXPECT_EQ(H264SpsPpsTracker::kRequestKeyframe, + tracker_.CopyAndFixBitstream(&idr_packet)); +} + +TEST_F(TestH264SpsPpsTracker, SpsPpsOutOfBandIncompleteNalu) { + constexpr uint8_t kData[] = {1, 2, 3}; + + // Generated by "ffmpeg -r 30 -f avfoundation -i "default" out.h264" on macos. + // Nalus damaged afterwards. + const std::vector sps({0x67, 0x7a, 0x00, 0x0d, 0xbc, 0xd9}); + const std::vector pps({0x68, 0xeb, 0xe3, 0xcb, 0x22, 0xc0}); + tracker_.InsertSpsPpsNalus(sps, pps); + + // Insert first packet of the IDR. + VCMPacket idr_packet = GetDefaultPacket(); + idr_packet.video_header.is_first_packet_in_frame = true; + AddIdr(&idr_packet, 0); + idr_packet.dataPtr = kData; + idr_packet.sizeBytes = sizeof(kData); + EXPECT_EQ(H264SpsPpsTracker::kRequestKeyframe, + tracker_.CopyAndFixBitstream(&idr_packet)); +} + } // namespace video_coding } // namespace webrtc diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index 33fb50cc3a..2cc8ddbef3 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -676,7 +676,8 @@ void RtpStreamReceiver::InsertSpsPpsIntoTracker(uint8_t payload_type) { if (!sprop_decoder.DecodeSprop(sprop_base64_it->second)) return; - tracker_.InsertSpsPps(sprop_decoder.sps_nalu(), sprop_decoder.pps_nalu()); + tracker_.InsertSpsPpsNalus(sprop_decoder.sps_nalu(), + sprop_decoder.pps_nalu()); } } // namespace webrtc