From 84f39119854f37aa6b3bb1138c1fce8e7d74cc91 Mon Sep 17 00:00:00 2001 From: Kongqun Yang Date: Fri, 27 May 2016 14:38:14 -0700 Subject: [PATCH] Remove top level box check in BoxReader Change-Id: I3d0e5865851030264d7174031a98031b413e90ce --- .../formats/mp4/box_definitions_unittest.cc | 2 +- packager/media/formats/mp4/box_reader.cc | 52 +++---------------- packager/media/formats/mp4/box_reader.h | 22 +++----- .../media/formats/mp4/box_reader_unittest.cc | 44 ++++------------ .../media/formats/mp4/mp4_media_parser.cc | 11 ++-- 5 files changed, 33 insertions(+), 98 deletions(-) diff --git a/packager/media/formats/mp4/box_definitions_unittest.cc b/packager/media/formats/mp4/box_definitions_unittest.cc index cdb23859ee..eb8328f956 100644 --- a/packager/media/formats/mp4/box_definitions_unittest.cc +++ b/packager/media/formats/mp4/box_definitions_unittest.cc @@ -49,7 +49,7 @@ class BoxDefinitionsTestGeneral : public testing::Test { buffer_->AppendInt(static_cast(FOURCC_skip)); buffer_->AppendBuffer(buffer); bool err = false; - return BoxReader::ReadTopLevelBox(buffer_->Buffer(), buffer_->Size(), &err); + return BoxReader::ReadBox(buffer_->Buffer(), buffer_->Size(), &err); } bool ReadBack(Box* box) { diff --git a/packager/media/formats/mp4/box_reader.cc b/packager/media/formats/mp4/box_reader.cc index 3ea717ce24..1f86c8db52 100644 --- a/packager/media/formats/mp4/box_reader.cc +++ b/packager/media/formats/mp4/box_reader.cc @@ -34,9 +34,9 @@ BoxReader::~BoxReader() { } // static -BoxReader* BoxReader::ReadTopLevelBox(const uint8_t* buf, - const size_t buf_size, - bool* err) { +BoxReader* BoxReader::ReadBox(const uint8_t* buf, + const size_t buf_size, + bool* err) { scoped_ptr reader(new BoxReader(buf, buf_size)); if (!reader->ReadHeader(err)) return NULL; @@ -45,11 +45,6 @@ BoxReader* BoxReader::ReadTopLevelBox(const uint8_t* buf, if (reader->type() == FOURCC_mdat) return reader.release(); - if (!IsValidTopLevelBox(reader->type())) { - *err = true; - return NULL; - } - if (reader->size() <= buf_size) return reader.release(); @@ -57,50 +52,19 @@ BoxReader* BoxReader::ReadTopLevelBox(const uint8_t* buf, } // static -bool BoxReader::StartTopLevelBox(const uint8_t* buf, - const size_t buf_size, - FourCC* type, - uint64_t* box_size, - bool* err) { +bool BoxReader::StartBox(const uint8_t* buf, + const size_t buf_size, + FourCC* type, + uint64_t* box_size, + bool* err) { BoxReader reader(buf, buf_size); if (!reader.ReadHeader(err)) return false; - if (!IsValidTopLevelBox(reader.type())) { - *err = true; - return false; - } *type = reader.type(); *box_size = reader.size(); return true; } -// static -bool BoxReader::IsValidTopLevelBox(const FourCC& type) { - switch (type) { - case FOURCC_ftyp: - case FOURCC_pdin: - case FOURCC_bloc: - case FOURCC_moov: - case FOURCC_moof: - case FOURCC_mfra: - case FOURCC_mdat: - case FOURCC_free: - case FOURCC_skip: - case FOURCC_meta: - case FOURCC_meco: - case FOURCC_styp: - case FOURCC_sidx: - case FOURCC_ssix: - case FOURCC_prft: - case FOURCC_uuid: - return true; - default: - // Hex is used to show nonprintable characters and aid in debugging - LOG(ERROR) << "Unrecognized top-level box type 0x" << std::hex << type; - return false; - } -} - bool BoxReader::ScanChildren() { DCHECK(!scanned_); scanned_ = true; diff --git a/packager/media/formats/mp4/box_reader.h b/packager/media/formats/mp4/box_reader.h index 7d0d112f83..18fda7008c 100644 --- a/packager/media/formats/mp4/box_reader.h +++ b/packager/media/formats/mp4/box_reader.h @@ -35,9 +35,9 @@ class BoxReader : public BufferReader { /// function may return NULL if an intact, complete box is not /// available in the buffer. For MDAT box only, a BoxReader object is /// returned as long as the box header is available. - static BoxReader* ReadTopLevelBox(const uint8_t* buf, - const size_t buf_size, - bool* err); + static BoxReader* ReadBox(const uint8_t* buf, + const size_t buf_size, + bool* err); /// Read the box header from the current buffer. /// @param buf is not retained. @@ -48,17 +48,11 @@ class BoxReader : public BufferReader { /// reading the box. /// @return true if there is enough data to read the header and the header is /// sane, which does not imply that the entire box is in the buffer. - static bool StartTopLevelBox(const uint8_t* buf, - const size_t buf_size, - FourCC* type, - uint64_t* box_size, - bool* err) WARN_UNUSED_RESULT; - - /// @return true if @a type is recognized to be the fourcc of a top-level box, - /// false otherwise. This returns true for some boxes which we do not - /// parse. - /// This method is helpful for debugging misaligned appends. - static bool IsValidTopLevelBox(const FourCC& type); + static bool StartBox(const uint8_t* buf, + const size_t buf_size, + FourCC* type, + uint64_t* box_size, + bool* err) WARN_UNUSED_RESULT; /// Scan through all boxes within the current box, starting at the current /// buffer position. Must be called before any of the @b *Child functions diff --git a/packager/media/formats/mp4/box_reader_unittest.cc b/packager/media/formats/mp4/box_reader_unittest.cc index 10371a8528..d94a5a976b 100644 --- a/packager/media/formats/mp4/box_reader_unittest.cc +++ b/packager/media/formats/mp4/box_reader_unittest.cc @@ -93,8 +93,7 @@ class BoxReaderTest : public testing::Test { TEST_F(BoxReaderTest, ExpectedOperationTest) { std::vector buf = GetBuf(); bool err; - scoped_ptr reader( - BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err)); + scoped_ptr reader(BoxReader::ReadBox(&buf[0], buf.size(), &err)); EXPECT_FALSE(err); EXPECT_TRUE(reader.get()); @@ -121,8 +120,7 @@ TEST_F(BoxReaderTest, OuterTooShortTest) { bool err; // Create a soft failure by truncating the outer box. - scoped_ptr r( - BoxReader::ReadTopLevelBox(&buf[0], buf.size() - 2, &err)); + scoped_ptr r(BoxReader::ReadBox(&buf[0], buf.size() - 2, &err)); EXPECT_FALSE(err); EXPECT_FALSE(r.get()); @@ -134,30 +132,16 @@ TEST_F(BoxReaderTest, InnerTooLongTest) { // Make an inner box too big for its outer box. buf[25] = 1; - scoped_ptr reader( - BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err)); + scoped_ptr reader(BoxReader::ReadBox(&buf[0], buf.size(), &err)); SkipBox box; EXPECT_FALSE(box.Parse(reader.get())); } -TEST_F(BoxReaderTest, WrongFourCCTest) { - std::vector buf = GetBuf(); - bool err; - - // Set an unrecognized top-level FourCC. - buf[5] = 1; - scoped_ptr reader( - BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err)); - EXPECT_FALSE(reader.get()); - EXPECT_TRUE(err); -} - TEST_F(BoxReaderTest, ScanChildrenTest) { std::vector buf = GetBuf(); bool err; - scoped_ptr reader( - BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err)); + scoped_ptr reader(BoxReader::ReadBox(&buf[0], buf.size(), &err)); EXPECT_TRUE(reader->SkipBytes(16) && reader->ScanChildren()); @@ -180,8 +164,7 @@ TEST_F(BoxReaderTest, ReadAllChildrenTest) { // Modify buffer to exclude its last 'free' box. buf[3] = 0x38; bool err; - scoped_ptr reader( - BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err)); + scoped_ptr reader(BoxReader::ReadBox(&buf[0], buf.size(), &err)); std::vector kids; EXPECT_TRUE(reader->SkipBytes(16) && reader->ReadAllChildren(&kids)); @@ -190,21 +173,16 @@ TEST_F(BoxReaderTest, ReadAllChildrenTest) { } TEST_F(BoxReaderTest, SkippingBloc) { - static const uint8_t kData[] = {0x00, - 0x00, - 0x00, - 0x09, // Box size. - 'b', - 'l', - 'o', - 'c', // FourCC. - 0x00}; // Reserved byte. + static const uint8_t kData[] = { + 0x00, 0x00, 0x00, 0x09, // Box size. + 'b', 'l', 'o', 'c', // FourCC. + 0x00, // Reserved byte. + }; std::vector buf(kData, kData + sizeof(kData)); bool err; - scoped_ptr reader( - BoxReader::ReadTopLevelBox(&buf[0], buf.size(), &err)); + scoped_ptr reader(BoxReader::ReadBox(&buf[0], buf.size(), &err)); EXPECT_FALSE(err); EXPECT_TRUE(reader); diff --git a/packager/media/formats/mp4/mp4_media_parser.cc b/packager/media/formats/mp4/mp4_media_parser.cc index 158b383380..75b73bf824 100644 --- a/packager/media/formats/mp4/mp4_media_parser.cc +++ b/packager/media/formats/mp4/mp4_media_parser.cc @@ -192,10 +192,9 @@ bool MP4MediaParser::LoadMoov(const std::string& file_path) { uint64_t box_size; FourCC box_type; bool err; - if (!BoxReader::StartTopLevelBox(&buffer[0], kBoxHeaderReadSize, &box_type, - &box_size, &err)) { - LOG(ERROR) << "Could not start top level box from file '" << file_path - << "'"; + if (!BoxReader::StartBox(&buffer[0], kBoxHeaderReadSize, &box_type, + &box_size, &err)) { + LOG(ERROR) << "Could not start box from file '" << file_path << "'"; return false; } if (box_type == FOURCC_mdat) { @@ -245,7 +244,7 @@ bool MP4MediaParser::ParseBox(bool* err) { if (!size) return false; - scoped_ptr reader(BoxReader::ReadTopLevelBox(buf, size, err)); + scoped_ptr reader(BoxReader::ReadBox(buf, size, err)); if (reader.get() == NULL) return false; @@ -749,7 +748,7 @@ bool MP4MediaParser::ReadAndDiscardMDATsUntil(const int64_t offset) { FourCC type; uint64_t box_sz; - if (!BoxReader::StartTopLevelBox(buf, size, &type, &box_sz, &err)) + if (!BoxReader::StartBox(buf, size, &type, &box_sz, &err)) break; mdat_tail_ += box_sz;