Fix AnnexB validation logic to work with encrypted content.

media/filters/h264_parser.cc

Index: media/filters/h264_parser.cc
diff --git a/media/filters/h264_parser.cc b/media/filters/h264_parser.cc
index ee21ab82a1fca5381d91e9abe5908a23d831ca76..7d6334b71a58a040fef802d7b759e29a899204ab 100644
--- a/media/filters/h264_parser.cc
+++ b/media/filters/h264_parser.cc
@@ -6,6 +6,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 
+#include "media/base/decrypt_config.h"
 #include "media/filters/h264_parser.h"
 
 namespace media {
@@ -129,14 +130,33 @@ H264Parser::~H264Parser() {
 void H264Parser::Reset() {
   stream_ = NULL;
   bytes_left_ = 0;
+  clear_ranges_.clear();
+  clear_range_index_ = 0;
 }
 
 void H264Parser::SetStream(const uint8* stream, off_t stream_size) {
+  std::vector<SubsampleEntry> subsamples;
+  SetEncryptedStream(stream, stream_size, subsamples);
+}
+
+void H264Parser::SetEncryptedStream(
+    const uint8* stream, off_t stream_size,
+    const std::vector<SubsampleEntry>& subsamples) {
   DCHECK(stream);
   DCHECK_GT(stream_size, 0);
 
   stream_ = stream;
   bytes_left_ = stream_size;
+
+  clear_ranges_.clear();
+  clear_range_index_ = 0;
+  const uint8* start = stream;
+  const uint8* stream_end = stream_ + bytes_left_;
+  for (size_t i = 0; i < subsamples.size() && start < stream_end; ++i) {
+    const uint8* end = std::min(start + subsamples[i].clear_bytes, stream_end);

[xhwang, 2014/07/16 05:06:13]

It seems possible that
start + subsamples[i].clear_bytes > stream_end
?
Is that caused by appending partial buffer? In that case, how will the remaining
encrypted bytes be excluded? Is this what the clear_range_index_ is used for?

[acolwell GONE FROM CHROMIUM, 2014/07/28 19:43:12]

This is to protect this code from invalid subsample data in the file.
Part of this comment no longer applies. Basically we only keep information for
regions that actually exist inside the buffer. Anything outside of that doesn't
really matter since it would result in an invalid memory access if it was
allowed to occur.

+    clear_ranges_.Add(start, end);
+    start += subsamples[i].clear_bytes + subsamples[i].cypher_bytes;
+  }
 }
 
 const H264PPS* H264Parser::GetPPS(int pps_id) {
@@ -191,8 +211,9 @@ bool H264Parser::LocateNALU(off_t* nalu_size, off_t* start_code_size) {
   // Find the start code of next NALU.
   off_t nalu_start_off = 0;
   off_t annexb_start_code_size = 0;
-  if (!FindStartCode(stream_, bytes_left_,
-                     &nalu_start_off, &annexb_start_code_size)) {
+
+  if (!FindStartCodeInClearRanges(stream_, bytes_left_,
+                                  &nalu_start_off, &annexb_start_code_size)) {
     DVLOG(4) << "Could not find start code, end of stream?";
     return false;
   }
@@ -216,8 +237,9 @@ bool H264Parser::LocateNALU(off_t* nalu_size, off_t* start_code_size) {
   // belong to the current NALU.
   off_t next_start_code_size = 0;
   off_t nalu_size_without_start_code = 0;
-  if (!FindStartCode(nalu_data, max_nalu_data_size,
-                     &nalu_size_without_start_code, &next_start_code_size)) {
+  if (!FindStartCodeInClearRanges(nalu_data, max_nalu_data_size,
+                                  &nalu_size_without_start_code,
+                                  &next_start_code_size)) {
     nalu_size_without_start_code = max_nalu_data_size;
   }
   *nalu_size = nalu_size_without_start_code + annexb_start_code_size;
@@ -225,6 +247,50 @@ bool H264Parser::LocateNALU(off_t* nalu_size, off_t* start_code_size) {
   return true;
 }
 
+bool H264Parser::FindStartCodeInClearRanges(

[damienv1, 2014/07/16 14:33:17]

The likelihood of a start code emulation (although it can exist) is pretty low.
So why not having a linear search (just like FindStartCode) and when we get a
start code, we could have a small utility function which checks given the offset
of the start code whether the start code overlaps with some encrypted content
(using the subsamples vector, no need to have additional clear ranges).

I agree that if there was a fake start codes, this would increase the complexity
but in this case, it seems that code simplicity takes precedence.

[acolwell GONE FROM CHROMIUM, 2014/07/28 19:43:12]

I've restored the code to what I had in Patch Set 1. This is a linear scan like
you suggested and I think is easier to follow.

+    const uint8* data,
+    off_t data_size,
+    off_t* offset,
+    off_t* start_code_size) {
+  if (clear_ranges_.size() == 0)
+    return FindStartCode(data, data_size, offset, start_code_size);

[xhwang, 2014/07/16 05:06:13]

If there's no clear ranges, we don't need to search?

[acolwell GONE FROM CHROMIUM, 2014/07/28 19:43:12]

Yes. A lack of clear ranges, or now encrypted ranges means that none of the data
is encrypted. Now that this is encrypted_ranges_ again, I think that is a little
more intuitive.

+
+  if (clear_range_index_ >= clear_ranges_.size())
+    return false;
+
+  DCHECK_GE(data_size, 0);
+  const uint8* start = data;
+  const uint8* end = data + data_size;
+  while (start < end) {
+    while (start >= clear_ranges_.end(clear_range_index_)) {
+      clear_range_index_++;
+      if (clear_range_index_ >= clear_ranges_.size())
+        return false;
+      if (start < clear_ranges_.start(clear_range_index_))
+        start = clear_ranges_.start(clear_range_index_);
+    }

[xhwang, 2014/07/16 05:06:13]

This seems complicated. Can you add some comments explaining the basics how it
works?

[acolwell GONE FROM CHROMIUM, 2014/07/28 19:43:12]

Code is no longer here anymore.

+
+    DCHECK_LT(start, end);
+    DCHECK_GE(start, clear_ranges_.start(clear_range_index_));
+    DCHECK_LT(start, clear_ranges_.end(clear_range_index_));
+    off_t clear_bytes_left =
+        clear_ranges_.end(clear_range_index_) - start;
+    off_t bytes_left = std::min(end - start, clear_bytes_left);
+
+    if (FindStartCode(start, bytes_left, offset, start_code_size))
+      break;
+
+    start += bytes_left;
+  }
+
+  if (start >= end)
+    return false;
+
+  // Update |*offset| to include the data we skipped over.
+  *offset += start - data;
+  return true;
+}
+
 H264Parser::Result H264Parser::ReadUE(int* val) {
   int num_bits = -1;
   int bit;