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..fc88c7cfd4abc4034dc453cbde104194c7c3b45e 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) << __FUNCTION__ << " nal_size is 0";

[damienv1, 2014/04/23 19:07:28]

nit: Chome LOG already includes file and line number. Do we really need
__FUNCTION__ ?

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

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) << __FUNCTION__ << " nal_size is 0";

[damienv1, 2014/04/23 19:07:28]

Ditto.

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

Done.

+      return false;
+    }
+
     RCHECK(pos + nal_size <= temp.size());
     buffer->insert(buffer->end(), kAnnexBStartCode,
                    kAnnexBStartCode + kAnnexBStartCodeSize);
@@ -59,6 +72,41 @@ bool AVC::ConvertFrameToAnnexB(int length_size, std::vector<uint8>* buffer) {
 }
 
 // static
+bool AVC::InsertParamSetsAnnexB(const AVCDecoderConfigurationRecord& avc_config,
+                                std::vector<uint8>* buffer) {
+  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();
+
+  if (nalu.nal_unit_type == H264NALU::kAUD) {
+    // Move insert point to just after the AUD.
+    config_insert_point += (nalu.data + nalu.size) - start;
+  }
+
+  // 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;
+  RCHECK(AVC::ConvertConfigToAnnexB(avc_config, &param_sets));
+
+  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) {
@@ -87,5 +135,149 @@ bool AVC::ConvertConfigToAnnexB(
   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 {
+    kExpectAUD,

[damienv1, 2014/04/23 19:07:28]

Naming is not really inline with the state machine: the state is not really
"expect" but rather kAUDAllowed.

Same for the other states.

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

Done.

+    kExpectFirstVCL, // VCL == nal_unit_types 1-5
+    kExpectVCL,
+    kExpectEOStream,
+    kExpectNoMoreData,
+  } NALUOrderStates;
+
+  H264NALU nalu;
+  NALUOrderStates order_state = kExpectAUD;
+  int last_nalu_type = H264NALU::kUnspecified;
+  for (bool done = false; !done;) {
+    switch (parser.AdvanceToNextNALU(&nalu)) {
+      case H264Parser::kOk:
+        DVLOG(1) << " nal_unit_type " << nalu.nal_unit_type;
+
+        switch (nalu.nal_unit_type) {
+          case H264NALU::kAUD:
+            if (order_state > kExpectAUD) {
+              DVLOG(1) << "Unexpected AUD in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kExpectFirstVCL;
+            break;
+
+          case H264NALU::kSEIMessage:
+          case H264NALU::kReserved14:
+          case H264NALU::kReserved15:
+          case H264NALU::kReserved16:
+          case H264NALU::kReserved17:
+          case H264NALU::kReserved18:
+            if (order_state >  kExpectFirstVCL) {
+              DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
+                       << " in order_state " << order_state;
+              return false;
+            }
+
+            if (order_state < kExpectFirstVCL)

[damienv1, 2014/04/23 19:07:28]

The condition is not needed.
At this point, the next state is always kExpectFirstVCL.

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

Done.

+              order_state = kExpectFirstVCL;
+            break;
+
+          case H264NALU::kPPS:
+          case H264NALU::kSPS:
+            if (order_state > kExpectVCL) {

[damienv1, 2014/04/23 19:07:28]

I don't think this condition is correct.
It should be kExpectFirstVCL (and in this case, we could merge it with the
previous cases (which includes kSEIMessage).

Having an SPS/PPS after the first VCL NAL would mean the start of a new access
unit.

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

Done.
Yep. Forgot that. :)

+              DVLOG(1) << "Unexpected NALU type " << nalu.nal_unit_type
+                       << " in order_state " << order_state;
+              return false;
+            }
+
+            if (order_state < kExpectFirstVCL)
+              order_state = kExpectFirstVCL;
+            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 > kExpectVCL) {
+              DVLOG(1) << "Unexpected VCL in order_state " << order_state;
+              return false;
+            }
+
+            if (order_state < kExpectVCL)

[damienv1, 2014/04/23 19:07:28]

Condition could be removed.

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

Done.

+              order_state = kExpectVCL;
+            break;
+
+          case H264NALU::kCodedSliceAux:
+            if (order_state != kExpectVCL) {
+              DVLOG(1) << "Unexpected extension in order_state " << order_state;
+              return false;
+            }
+            break;
+
+          case H264NALU::kEOSeq:
+            if (order_state != kExpectVCL) {
+              DVLOG(1) << "Unexpected EOSeq in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kExpectEOStream;
+            break;
+
+          case H264NALU::kEOStream:
+            if (order_state < kExpectVCL) {
+              DVLOG(1) << "Unexpected EOStream in order_state " << order_state;
+              return false;
+            }
+
+            order_state = kExpectNoMoreData;
+            break;
+
+          case H264NALU::kFiller:
+          case H264NALU::kUnspecified:
+            if (order_state != kExpectVCL) {

[damienv1, 2014/04/23 19:07:28]

From the spec, seems like we could have a Filler NALU after an end of sequence.
The condition should be:
if (!(order_state >= kExpectVCL && order_state < kEOSStream))
  return false;

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

Done.

+              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 != kExpectVCL) {
+              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:
+      case H264Parser::kUnsupportedStream:

[damienv1, 2014/04/23 19:07:28]

Just for info:
1) Unsupported streams by the media::H264Parser does not mean it cannot be
supported by FFMpegVideoDecoder for example. So you might artificially add some
restrictions to the set of streams that can be supported by Chrome.
2) We have the same issue with the Mpeg2 TS parser with some of the streams: for
example, gaps_in_frame_num_value_allowed_flag returns kUnsupportedStream. But
this flag should not have any impact on the parsing. In this specific case, I
need to check whether it's returning kUnsupportedStream because the underlying
GPU video decoder does not support such stream (which is different from the
parser cannot parse such streams). Summary: the intent of kUnsupportedStream in
H264Parser is not really well defined. To me, it should not be related to the
capabilities of the GPU video decoder.

[acolwell GONE FROM CHROMIUM, 2014/04/23 23:06:53]

ok. Thanks for the info. It doesn't look like AdvanceToNextNALU() should ever
return kUnsupportedStream. I've separated these 2 cases and put a NOTREACHED()
for the kUnsupportedStream case so that we get notified if that method changes
its behavior.

+        return false;
+
+      case H264Parser::kEOStream:
+        done = true;
+    }
+  }
+
+  return order_state >= kExpectVCL;
+}
+
 }  // namespace mp4
 }  // namespace media