H264SpsPpsTracker.InsertSpsPpsNalus() should accept Nalus with header.

webrtc/modules/video_coding/h264_sps_pps_tracker.cc

Index: webrtc/modules/video_coding/h264_sps_pps_tracker.cc
diff --git a/webrtc/modules/video_coding/h264_sps_pps_tracker.cc b/webrtc/modules/video_coding/h264_sps_pps_tracker.cc
index e13c9f4364b4ca0b4fb6ff6ebbefbae38fca7d57..bc814bde32e34eb601271afce04b8dfde6023cf5 100644
--- a/webrtc/modules/video_coding/h264_sps_pps_tracker.cc
+++ b/webrtc/modules/video_coding/h264_sps_pps_tracker.cc
@@ -182,12 +182,39 @@ H264SpsPpsTracker::PacketAction H264SpsPpsTracker::CopyAndFixBitstream(
   return kInsert;
 }
 
-void H264SpsPpsTracker::InsertSpsPps(const std::vector<uint8_t>& sps,
-                                     const std::vector<uint8_t>& pps) {
-  rtc::Optional<SpsParser::SpsState> parsed_sps =
-      SpsParser::ParseSps(sps.data(), sps.size());
-  rtc::Optional<PpsParser::PpsState> parsed_pps =
-      PpsParser::ParsePps(pps.data(), pps.size());
+void H264SpsPpsTracker::InsertSpsPpsNalus(const std::vector<uint8_t>& sps,
+                                          const std::vector<uint8_t>& pps) {
+  constexpr size_t kNaluHeaderOffset = 1;
+  if (sps.size() < kNaluHeaderOffset) {

[philipel, 2017/01/18 12:51:48]

Why do we perform these checks before parsing? For better logging? In that case
don't you think it's better to add more logging into the Sps/PpsParser?

[johan, 2017/01/18 14:26:04]

The SpsPraser and PpsParser implementations handle only Nal-unit "payload" not
the Nal-unit "headers". At least that's my conclusion from debugging unit tests
and from comparing parser implementation to H264 spec, section 7.3.1.ff.

So this function should do a plausibility check on the nalu headers before
handing the nalu payloads to the actual SpsParser/PpsParser.

[philipel, 2017/01/18 14:28:43]

Acknowledged.

+    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<SpsParser::SpsState> parsed_sps = SpsParser::ParseSps(
+      sps.data() + kNaluHeaderOffset, sps.size() - kNaluHeaderOffset);
+  rtc::Optional<PpsParser::PpsState> 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) {

[philipel, 2017/01/18 12:51:48]

Remove this if block.

[johan, 2017/01/18 14:26:04]

Removing the log, sticking with the return.

[philipel, 2017/01/18 14:28:43]

Ah... ofc

     LOG(LS_WARNING) << "Failed to parse SPS or PPS parameters.";
@@ -208,6 +235,10 @@ void H264SpsPpsTracker::InsertSpsPps(const std::vector<uint8_t>& 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