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

4 files changed, 31 insertions, 18 deletions

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