Fix SPS/PPS insertion logic in MP4StreamParser.

media/formats/mp4/avc.cc

Index: media/formats/mp4/avc.cc
diff --git a/media/formats/mp4/avc.cc b/media/formats/mp4/avc.cc
index 6b670feba3c5dbcd03390b66baa076c81a26b4dd..c81bb0df29cc9a703a85a60d66e62f269fb8f62a 100644
--- a/media/formats/mp4/avc.cc
+++ b/media/formats/mp4/avc.cc
@@ -7,6 +7,8 @@
 #include <algorithm>
 #include <vector>
 
+#include "base/logging.h"
+#include "media/filters/h264_parser.h"
 #include "media/formats/mp4/box_definitions.h"
 #include "media/formats/mp4/box_reader.h"
 
@@ -20,10 +22,16 @@ static bool ConvertAVCToAnnexBInPlaceForLengthSize4(std::vector<uint8>* buf) {
   const int kLengthSize = 4;
   size_t pos = 0;
   while (pos + kLengthSize < buf->size()) {
-    int nal_size = (*buf)[pos];
+    uint32 nal_size = (*buf)[pos];
     nal_size = (nal_size << 8) + (*buf)[pos+1];
     nal_size = (nal_size << 8) + (*buf)[pos+2];
     nal_size = (nal_size << 8) + (*buf)[pos+3];
+
+    if (nal_size == 0) {
+      DVLOG(1) << " nal_size is 0";

[scherkus (not reviewing), 2014/04/24 17:16:35]

nit: remove leading space?

[acolwell GONE FROM CHROMIUM, 2014/04/24 23:52:15]

Done.

+      return false;
+    }
+
     std::copy(kAnnexBStartCode, kAnnexBStartCode + kAnnexBStartCodeSize,
               buf->begin() + pos);
     pos += kLengthSize + nal_size;
@@ -48,6 +56,11 @@ bool AVC::ConvertFrameToAnnexB(int length_size, std::vector<uint8>* buffer) {
     if (length_size == 2) nal_size = (nal_size << 8) + temp[pos+1];
     pos += length_size;
 
+    if (nal_size == 0) {
+      DVLOG(1) << " nal_size is 0";

[scherkus (not reviewing), 2014/04/24 17:16:35]

ditto

[acolwell GONE FROM CHROMIUM, 2014/04/24 23:52:15]

Done.

+      return false;
+    }
+
     RCHECK(pos + nal_size <= temp.size());
     buffer->insert(buffer->end(), kAnnexBStartCode,
                    kAnnexBStartCode + kAnnexBStartCodeSize);
@@ -59,9 +72,60 @@ bool AVC::ConvertFrameToAnnexB(int length_size, std::vector<uint8>* buffer) {
 }
 
 // static
+bool AVC::InsertParamSetsAnnexB(const AVCDecoderConfigurationRecord& avc_config,
+                                std::vector<uint8>* buffer,
+                                std::vector<SubsampleEntry>* subsamples) {
+  DCHECK(AVC::IsValidAnnexB(*buffer));
+
+  scoped_ptr<H264Parser> parser(new H264Parser());
+  const uint8* start = &(*buffer)[0];
+  parser->SetStream(start, buffer->size());
+
+  H264NALU nalu;
+  if (parser->AdvanceToNextNALU(&nalu) != H264Parser::kOk)
+    return false;
+
+  std::vector<uint8>::iterator config_insert_point = buffer->begin();
+  std::vector<SubsampleEntry>::iterator subsamples_insert_point =
+      subsamples->begin();
+
+  if (nalu.nal_unit_type == H264NALU::kAUD) {
+    // Move insert point to just after the AUD.
+    config_insert_point += (nalu.data + nalu.size) - start;
+
+    if (!subsamples->empty())
+      subsamples_insert_point++;

[damienv1, 2014/04/24 00:03:58]

I don't think the logic is correct here and will not be in the general case.
Let's say you have an AUD (from the CENC spec, it will not be encrypted), then
let's say you have a slice NALU, the NALU header is not ecnrypted but the
content could be encrypted.
In this case, you are adding one subsample entry at the wrong location.
I think the previous code was correct: only the first subsample should be
modified (no need for an extra entry) since an AUD should not be encrypted
according to the CENC spec (we could return false if the insert position is
strictly greater than subsamples[0].clear_bytes.

[acolwell GONE FROM CHROMIUM, 2014/04/24 23:52:15]

Left code as is based on our offline discussion. I did add the DCHECK you
suggested and test code to verify the subsamples code path. 

+  }
+
+  // Clear |parser| and |start| since they aren't needed anymore and
+  // will hold stale pointers once the insert happens.
+  parser.reset();
+  start = NULL;
+
+  std::vector<uint8> param_sets;
+  std::vector<SubsampleEntry> config_subsamples;
+  RCHECK(AVC::ConvertConfigToAnnexB(avc_config,
+                                    &param_sets,
+                                    &config_subsamples));
+
+  if (!subsamples->empty()) {
+    subsamples->insert(subsamples_insert_point,
+                       config_subsamples.begin(),
+                       config_subsamples.end());
+  }
+
+  buffer->insert(config_insert_point,
+                 param_sets.begin(), param_sets.end());
+
+  DCHECK(AVC::IsValidAnnexB(*buffer));
+  return true;
+}
+
+// static
 bool AVC::ConvertConfigToAnnexB(
     const AVCDecoderConfigurationRecord& avc_config,
-    std::vector<uint8>* buffer) {
+    std::vector<uint8>* buffer,
+    std::vector<SubsampleEntry>* subsamples) {
   DCHECK(buffer->empty());
   buffer->clear();
   int total_size = 0;
@@ -76,6 +140,11 @@ bool AVC::ConvertConfigToAnnexB(
                 kAnnexBStartCode + kAnnexBStartCodeSize);
     buffer->insert(buffer->end(), avc_config.sps_list[i].begin(),
                 avc_config.sps_list[i].end());
+
+    SubsampleEntry entry;
+    entry.clear_bytes = kAnnexBStartCodeSize + avc_config.sps_list[i].size();
+    entry.cypher_bytes = 0;
+    subsamples->push_back(entry);
   }
 
   for (size_t i = 0; i < avc_config.pps_list.size(); i++) {
@@ -83,9 +152,150 @@ bool AVC::ConvertConfigToAnnexB(
                    kAnnexBStartCode + kAnnexBStartCodeSize);
     buffer->insert(buffer->end(), avc_config.pps_list[i].begin(),
                    avc_config.pps_list[i].end());
+
+    SubsampleEntry entry;
+    entry.clear_bytes = kAnnexBStartCodeSize + avc_config.pps_list[i].size();
+    entry.cypher_bytes = 0;
+    subsamples->push_back(entry);
   }
   return true;
 }
 
+// Verifies AnnexB NALU order according to ISO/IEC 14496-10 Section 7.4.1.2.3
+bool AVC::IsValidAnnexB(const std::vector<uint8>& buffer) {
+  DVLOG(1) << __FUNCTION__;
+
+  if (buffer.empty())
+    return true;
+
+  H264Parser parser;
+  parser.SetStream(&buffer[0], buffer.size());
+
+  typedef enum {
+    kAUDAllowed,
+    kBeforeFirstVCL, // VCL == nal_unit_types 1-5

[scherkus (not reviewing), 2014/04/24 17:16:35]

nit: two spaces before // comments

[acolwell GONE FROM CHROMIUM, 2014/04/24 23:52:15]

Done.

+    kAfterFirstVCL,
+    kEOStreamAllowed,
+    kNoMoreDataAllowed,
+  } NALUOrderState;
+
+  H264NALU nalu;
+  NALUOrderState order_state = kAUDAllowed;
+  int last_nalu_type = H264NALU::kUnspecified;
+  for (bool done = false; !done;) {

[damienv1, 2014/04/24 00:03:58]

nit: would personally write a while loop here, but up to you.

[scherkus (not reviewing), 2014/04/24 17:16:35]

+1

[acolwell GONE FROM CHROMIUM, 2014/04/24 23:52:15]

Done.

+    switch (parser.AdvanceToNextNALU(&nalu)) {
+      case H264Parser::kOk:
+        DVLOG(1) << " nal_unit_type " << nalu.nal_unit_type;

[scherkus (not reviewing), 2014/04/24 17:16:35]

ditto w/ removing leading space?

[acolwell GONE FROM CHROMIUM, 2014/04/24 23:52:15]

Done.

+
+        switch (nalu.nal_unit_type) {
+          case H264NALU::kAUD:
+            if (order_state > kAUDAllowed) {
+              DVLOG(1) << "Unexpected AUD in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kBeforeFirstVCL;
+            break;
+
+          case H264NALU::kSEIMessage:
+          case H264NALU::kReserved14:
+          case H264NALU::kReserved15:
+          case H264NALU::kReserved16:
+          case H264NALU::kReserved17:
+          case H264NALU::kReserved18:
+          case H264NALU::kPPS:
+          case H264NALU::kSPS:
+            if (order_state > kBeforeFirstVCL) {
+              DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
+                       << " in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kBeforeFirstVCL;
+            break;
+
+          case H264NALU::kSPSExt:
+            if (last_nalu_type != H264NALU::kSPS) {
+              DVLOG(1) << "SPS extension does not follow an SPS.";
+              return false;
+            }
+            break;
+
+          case H264NALU::kNonIDRSlice:
+          case H264NALU::kSliceDataA:
+          case H264NALU::kSliceDataB:
+          case H264NALU::kSliceDataC:
+          case H264NALU::kIDRSlice:
+            if (order_state > kAfterFirstVCL) {
+              DVLOG(1) << "Unexpected VCL in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kAfterFirstVCL;
+            break;
+
+          case H264NALU::kCodedSliceAux:
+            if (order_state != kAfterFirstVCL) {
+              DVLOG(1) << "Unexpected extension in order_state " << order_state;
+              return false;
+            }
+            break;
+
+          case H264NALU::kEOSeq:
+            if (order_state != kAfterFirstVCL) {
+              DVLOG(1) << "Unexpected EOSeq in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kEOStreamAllowed;
+            break;
+
+          case H264NALU::kEOStream:
+            if (order_state < kAfterFirstVCL) {
+              DVLOG(1) << "Unexpected EOStream in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kNoMoreDataAllowed;
+            break;
+
+          case H264NALU::kFiller:
+          case H264NALU::kUnspecified:
+            if (!(order_state >= kAfterFirstVCL &&
+                  order_state < kEOStreamAllowed)) {
+              DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
+                       << " in order_state " << order_state;
+              return false;
+            }
+            break;
+
+          default:
+            DCHECK_GE(nalu.nal_unit_type, 20);
+
+            if (nalu.nal_unit_type >= 20 && nalu.nal_unit_type <= 31 &&
+                order_state != kAfterFirstVCL) {
+              DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
+                       << " in order_state " << order_state;
+              return false;
+            }
+        }
+        last_nalu_type = nalu.nal_unit_type;
+        break;
+
+      case H264Parser::kInvalidStream:
+        return false;
+
+      case H264Parser::kUnsupportedStream:
+        NOTREACHED() << "AdvanceToNextNALU() returned kUnsupportedStream!";
+        return false;
+
+      case H264Parser::kEOStream:
+        done = true;
+    }
+  }
+
+  return order_state >= kAfterFirstVCL;
+}
+
 }  // namespace mp4
 }  // namespace media