From 9ae09dc11a858068eeab52069c902ae8c31d09de Mon Sep 17 00:00:00 2001
From: "strobe@google.com"
 <strobe@google.com@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri, 21 Sep 2012 19:34:27 +0000
Subject: Eliminate box reordering in media::mp4::BoxReader.

ScanChildren() will reorder boxes based on their box type. The
SampleDescription box is the only supported box which is agnostic about
the box type of its children, and it is sensitive to the child order.
This patch modifies ReadAllChildren() to read children in order without
first invoking ScanChildren().

R=acolwell
BUG=
TEST=BoxReaderTest.ReadAllChildrenTest


Review URL: https://chromiumcodereview.appspot.com/10938034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@158046 0039d316-1c4b-4281-b951-d872f2087c98
---
 media/mp4/box_definitions.cc     |  3 +--
 media/mp4/box_reader.cc          |  2 --
 media/mp4/box_reader.h           | 26 ++++++++++++++------------
 media/mp4/box_reader_unittest.cc | 18 ++++++++++++++++--
 4 files changed, 31 insertions(+), 18 deletions(-)

(limited to 'media/mp4')

diff --git a/media/mp4/box_definitions.cc b/media/mp4/box_definitions.cc
index 9b64f48..aa179e1 100644
--- a/media/mp4/box_definitions.cc
+++ b/media/mp4/box_definitions.cc
@@ -225,8 +225,7 @@ FourCC SampleDescription::BoxType() const { return FOURCC_STSD; }
 bool SampleDescription::Parse(BoxReader* reader) {
   uint32 count;
   RCHECK(reader->SkipBytes(4) &&
-         reader->Read4(&count) &&
-         reader->ScanChildren());
+         reader->Read4(&count));
   video_entries.clear();
   audio_entries.clear();
 
diff --git a/media/mp4/box_reader.cc b/media/mp4/box_reader.cc
index b352664..1285ebf 100644
--- a/media/mp4/box_reader.cc
+++ b/media/mp4/box_reader.cc
@@ -163,8 +163,6 @@ bool BoxReader::ScanChildren() {
   scanned_ = true;
 
   bool err = false;
-  // TODO(strobe): Check or correct for multimap not inserting elements in
-  // consistent order.
   while (pos() < size()) {
     BoxReader child(&buf_[pos_], size_ - pos_);
     if (!child.ReadHeader(&err)) break;
diff --git a/media/mp4/box_reader.h b/media/mp4/box_reader.h
index 211d99a..967bf93 100644
--- a/media/mp4/box_reader.h
+++ b/media/mp4/box_reader.h
@@ -118,6 +118,7 @@ class MEDIA_EXPORT BoxReader : public BufferReader {
 
   // Read all children, regardless of FourCC. This is used from exactly one box,
   // corresponding to a rather significant inconsistency in the BMFF spec.
+  // Note that this method is mutually exclusive with ScanChildren().
   template<typename T> bool ReadAllChildren(
       std::vector<T>* children) WARN_UNUSED_RESULT;
 
@@ -185,19 +186,20 @@ bool BoxReader::MaybeReadChildren(std::vector<T>* children) {
 
 template<typename T>
 bool BoxReader::ReadAllChildren(std::vector<T>* children) {
-  DCHECK(scanned_);
-  DCHECK(children->empty());
-  RCHECK(!children_.empty());
-
-  children->resize(children_.size());
-  typename std::vector<T>::iterator child_itr = children->begin();
-  for (ChildMap::iterator itr = children_.begin();
-       itr != children_.end(); ++itr) {
-    RCHECK(child_itr->Parse(&itr->second));
-    ++child_itr;
+  DCHECK(!scanned_);
+  scanned_ = true;
+
+  bool err = false;
+  while (pos() < size()) {
+    BoxReader child_reader(&buf_[pos_], size_ - pos_);
+    if (!child_reader.ReadHeader(&err)) break;
+    T child;
+    RCHECK(child.Parse(&child_reader));
+    children->push_back(child);
+    pos_ += child_reader.size();
   }
-  children_.clear();
-  return true;
+
+  return !err;
 }
 
 }  // namespace mp4
diff --git a/media/mp4/box_reader_unittest.cc b/media/mp4/box_reader_unittest.cc
index da0a041..38fd896 100644
--- a/media/mp4/box_reader_unittest.cc
+++ b/media/mp4/box_reader_unittest.cc
@@ -145,7 +145,7 @@ TEST_F(BoxReaderTest, WrongFourCCTest) {
   EXPECT_TRUE(err);
 }
 
-TEST_F(BoxReaderTest, ChildrenTest) {
+TEST_F(BoxReaderTest, ScanChildrenTest) {
   std::vector<uint8> buf = GetBuf();
   bool err;
   scoped_ptr<BoxReader> reader(
@@ -160,12 +160,26 @@ TEST_F(BoxReaderTest, ChildrenTest) {
 
   std::vector<PsshBox> kids;
 
-  EXPECT_TRUE(reader->ReadAllChildren(&kids));
+  EXPECT_TRUE(reader->ReadChildren(&kids));
   EXPECT_EQ(2u, kids.size());
   kids.clear();
   EXPECT_FALSE(reader->ReadChildren(&kids));
   EXPECT_TRUE(reader->MaybeReadChildren(&kids));
 }
 
+TEST_F(BoxReaderTest, ReadAllChildrenTest) {
+  std::vector<uint8> buf = GetBuf();
+  // Modify buffer to exclude its last 'free' box
+  buf[3] = 0x38;
+  bool err;
+  scoped_ptr<BoxReader> reader(
+      BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err));
+
+  std::vector<PsshBox> kids;
+  EXPECT_TRUE(reader->SkipBytes(16) && reader->ReadAllChildren(&kids));
+  EXPECT_EQ(2u, kids.size());
+  EXPECT_EQ(kids[0].val, 0xdeadbeef);   // Ensure order is preserved
+}
+
 }  // namespace mp4
 }  // namespace media
-- 
cgit v1.1