Some code clean ups in box definitions

- Add Box::WriteHeader and Box::HeaderSize methods. The concrete
  box implementations should not need care about whether it is a
  Box or FullBox normally.
- Rename Box::ReadWrite to Box::ReadWriteInternal to make it clear
  that it is an internal methods. Make it private so it cannot be
  incorrectly called in box implementions.
- Make an internal implementation of ComputeSizeInternal to compute
  box size. |atom_size| is updated in ComputeSize, which wraps
  ComputeSizeInternal.

Change-Id: I3fbcf8c527581b676d9d13f1ac1dd798da7c4d5f

packager/media/formats/mp4/box.cc

@@ -17,23 +17,47 @@ Box::Box() : atom_size(0) {}
 Box::~Box() {}
 
 bool Box::Parse(BoxReader* reader) {
-  DCHECK(reader != NULL);
+  DCHECK(reader);
   BoxBuffer buffer(reader);
-  return ReadWrite(&buffer);
+  return ReadWriteInternal(&buffer);
 }
 
 void Box::Write(BufferWriter* writer) {
-  DCHECK(writer != NULL);
+  DCHECK(writer);
+  // Compute and update atom_size.
   uint32_t size = ComputeSize();
   DCHECK_EQ(size, this->atom_size);
 
   size_t buffer_size_before_write = writer->Size();
   BoxBuffer buffer(writer);
-  CHECK(ReadWrite(&buffer));
+  CHECK(ReadWriteInternal(&buffer));
   DCHECK_EQ(this->atom_size, writer->Size() - buffer_size_before_write);
 }
 
-bool Box::ReadWrite(BoxBuffer* buffer) {
+void Box::WriteHeader(BufferWriter* writer) {
+  DCHECK(writer);
+  // Compute and update atom_size.
+  uint32_t size = ComputeSize();
+  DCHECK_EQ(size, this->atom_size);
+
+  size_t buffer_size_before_write = writer->Size();
+  BoxBuffer buffer(writer);
+  CHECK(ReadWriteHeaderInternal(&buffer));
+  DCHECK_EQ(HeaderSize(), writer->Size() - buffer_size_before_write);
+}
+
+uint32_t Box::ComputeSize() {
+  this->atom_size = ComputeSizeInternal();
+  return this->atom_size;
+}
+
+uint32_t Box::HeaderSize() const {
+  const uint32_t kFourCCSize = 4;
+  // We don't support 64-bit size.
+  return kFourCCSize + sizeof(uint32_t);
+}
+
+bool Box::ReadWriteHeaderInternal(BoxBuffer* buffer) {
   if (buffer->Reading()) {
     // Skip for read mode, which is handled already in BoxReader.
   } else {
@@ -47,8 +71,13 @@ bool Box::ReadWrite(BoxBuffer* buffer) {
 FullBox::FullBox() : version(0), flags(0) {}
 FullBox::~FullBox() {}
 
-bool FullBox::ReadWrite(BoxBuffer* buffer) {
+uint32_t FullBox::HeaderSize() const {
-  RCHECK(Box::ReadWrite(buffer));
+  // Additional 1-byte version and 3-byte flags.
+  return Box::HeaderSize() + 1 + 3;
+}
+
+bool FullBox::ReadWriteHeaderInternal(BoxBuffer* buffer) {
+  RCHECK(Box::ReadWriteHeaderInternal(buffer));
 
   uint32_t vflags;
   if (buffer->Reading()) {

packager/media/formats/mp4/box.h

@@ -9,7 +9,6 @@
 
 #include <stdint.h>
 
-#include "packager/base/compiler_specific.h"
 #include "packager/media/formats/mp4/fourccs.h"
 
 namespace edash_packager {
@@ -32,24 +31,46 @@ struct Box {
   /// Parse the mp4 box.
   /// @param reader points to a BoxReader object which parses the box.
   bool Parse(BoxReader* reader);
-  /// Write the box to buffer.
+  /// Write the box to buffer. This function calls ComputeSize internally to
-  /// This function calls ComputeSize internally to compute box size.
+  /// compute and update box size.
   /// @param writer points to a BufferWriter object which wraps the buffer for
   ///        writing.
   void Write(BufferWriter* writer);
-  /// Compute the size of this box.
+  /// Write the box header to buffer. This function calls ComputeSize internally
-  /// The calculated size will be saved in |atom_size| for later consumption.
+  /// to compute and update box size.
-  virtual uint32_t ComputeSize() = 0;
+  /// @param writer points to a BufferWriter object which wraps the buffer for
+  ///        writing.
+  void WriteHeader(BufferWriter* writer);
+  /// Compute the size of this box. It will also update box size.
+  /// @return The size of result box including child boxes. A value of 0 should
+  ///         be returned if the box should not be written.
+  uint32_t ComputeSize();
+  /// @return box header size in bytes.
+  virtual uint32_t HeaderSize() const;
+  /// @return box type.
   virtual FourCC BoxType() const = 0;
 
  protected:
-  friend class BoxBuffer;
+  /// Read/write mp4 box header. Note that this function expects box size
-  /// Read/write the mp4 box from/to BoxBuffer.
+  /// updated already.
-  virtual bool ReadWrite(BoxBuffer* buffer);
+  /// @return true on success, false otherwise.
+  virtual bool ReadWriteHeaderInternal(BoxBuffer* buffer);
 
-  /// We don't support 64-bit atom sizes. 32-bit should be large enough for our
+ private:
-  /// current needs.
+  friend class BoxBuffer;
+  // Read/write the mp4 box from/to BoxBuffer. Note that this function expects
+  // box size updated already.
+  virtual bool ReadWriteInternal(BoxBuffer* buffer) = 0;
+  // Compute the size of this box. A value of 0 should be returned if the box
+  // should not be written. Note that this function won't update box size.
+  virtual uint32_t ComputeSizeInternal() = 0;
+
+  // We don't support 64-bit atom sizes. 32-bit should be large enough for our
+  // current needs.
   uint32_t atom_size;
+
+  // Not using DISALLOW_COPY_AND_ASSIGN here intentionally to allow the compiler
+  // generated copy constructor and assignment operator.
 };
 
 /// Defines FullBox, the other base ISO BMFF box objects as defined in
@@ -60,11 +81,16 @@ struct FullBox : Box {
   FullBox();
   ~FullBox() override;
 
+  uint32_t HeaderSize() const final;
+
   uint8_t version;
   uint32_t flags;
 
  protected:
-  bool ReadWrite(BoxBuffer* buffer) override;
+  bool ReadWriteHeaderInternal(BoxBuffer* buffer) final;
+
+  // Not using DISALLOW_COPY_AND_ASSIGN here intentionally to allow the compiler
+  // generated copy constructor and assignment operator.
 };
 
 }  // namespace mp4

packager/media/formats/mp4/box_buffer.h

@@ -146,7 +146,7 @@ class BoxBuffer {
       return reader_->ReadChild(box);
     // The box is mandatory, i.e. the box size should not be 0.
     DCHECK_NE(0u, box->atom_size);
-    CHECK(box->ReadWrite(this));
+    CHECK(box->ReadWriteInternal(this));
     return true;
   }
 
@@ -157,7 +157,7 @@ class BoxBuffer {
       return reader_->TryReadChild(box);
     // The box is optional, i.e. it can be skipped if the box size is 0.
     if (box->atom_size != 0)
-      CHECK(box->ReadWrite(this));
+      CHECK(box->ReadWriteInternal(this));
     return true;
   }
 

packager/media/formats/mp4/box_definitions.h

@@ -29,11 +29,16 @@ enum TrackType {
 class BoxBuffer;
 
 #define DECLARE_BOX_METHODS(T)                        \
+ public:                                              \
   T();                                                \
-  ~T() override;                                       \
+  ~T() override;                                      \
-  bool ReadWrite(BoxBuffer* buffer) override; \
+  FourCC BoxType() const override;                    \
-  FourCC BoxType() const override;            \
+                                                      \
-  uint32_t ComputeSize() override;
+ private:                                             \
+  bool ReadWriteInternal(BoxBuffer* buffer) override; \
+  uint32_t ComputeSizeInternal() override;            \
+                                                      \
+ public:
 
 struct FileType : Box {
   DECLARE_BOX_METHODS(FileType);
@@ -44,7 +49,7 @@ struct FileType : Box {
 };
 
 struct SegmentType : FileType {
-  DECLARE_BOX_METHODS(SegmentType);
+  FourCC BoxType() const override;
 };
 
 struct ProtectionSystemSpecificHeader : FullBox {
@@ -168,12 +173,13 @@ struct CodecConfigurationRecord : Box {
   DECLARE_BOX_METHODS(CodecConfigurationRecord);
 
   FourCC box_type;
-  // Contains full codec configuration record, including possible extension boxes.
+  // Contains full codec configuration record, including possible extension
+  // boxes.
   std::vector<uint8_t> data;
 };
 
-struct PixelAspectRatioBox : Box {
+struct PixelAspectRatio : Box {
-  DECLARE_BOX_METHODS(PixelAspectRatioBox);
+  DECLARE_BOX_METHODS(PixelAspectRatio);
 
   uint32_t h_spacing;
   uint32_t v_spacing;
@@ -191,7 +197,7 @@ struct VideoSampleEntry : Box {
   uint16_t width;
   uint16_t height;
 
-  PixelAspectRatioBox pixel_aspect;
+  PixelAspectRatio pixel_aspect;
   ProtectionSchemeInfo sinf;
   CodecConfigurationRecord codec_config_record;
 };
@@ -203,8 +209,8 @@ struct ElementaryStreamDescriptor : FullBox {
   ESDescriptor es_descriptor;
 };
 
-struct DTSSpecificBox : Box {
+struct DTSSpecific : Box {
-  DECLARE_BOX_METHODS(DTSSpecificBox);
+  DECLARE_BOX_METHODS(DTSSpecific);
 
   std::vector<uint8_t> data;
 };
@@ -224,7 +230,7 @@ struct AudioSampleEntry : Box {
 
   ProtectionSchemeInfo sinf;
   ElementaryStreamDescriptor esds;
-  DTSSpecificBox ddts;
+  DTSSpecific ddts;
 };
 
 struct SampleDescription : FullBox {
@@ -578,14 +584,9 @@ struct SegmentIndex : FullBox {
   std::vector<SegmentReference> references;
 };
 
-// The actual data is parsed and written separately, so we do not inherit it
+// The actual data is parsed and written separately.
-// from Box.
+struct MediaData : Box {
-struct MediaData {
+  DECLARE_BOX_METHODS(MediaData);
-  MediaData();
-  ~MediaData();
-  void Write(BufferWriter* buffer_writer);
-  uint32_t ComputeSize();
-  FourCC BoxType() const;
 
   uint32_t data_size;
 };

packager/media/formats/mp4/box_definitions_comparison.h

@@ -170,8 +170,8 @@ inline bool operator==(const CodecConfigurationRecord& lhs,
   return lhs.data == rhs.data;
 }
 
-inline bool operator==(const PixelAspectRatioBox& lhs,
+inline bool operator==(const PixelAspectRatio& lhs,
-                       const PixelAspectRatioBox& rhs) {
+                       const PixelAspectRatio& rhs) {
   return lhs.h_spacing == rhs.h_spacing && lhs.v_spacing == rhs.v_spacing;
 }
 
@@ -194,8 +194,8 @@ inline bool operator==(const ElementaryStreamDescriptor& lhs,
   return lhs.es_descriptor == rhs.es_descriptor;
 }
 
-inline bool operator==(const DTSSpecificBox& lhs,
+inline bool operator==(const DTSSpecific& lhs,
-                       const DTSSpecificBox& rhs) {
+                       const DTSSpecific& rhs) {
   return lhs.data == rhs.data;
 }
 

packager/media/formats/mp4/box_definitions_unittest.cc

@@ -267,12 +267,12 @@ class BoxDefinitionsTestGeneral : public testing::Test {
 
   void Modify(HandlerReference* hdlr) { hdlr->type = kAudio; }
 
-  void Fill(PixelAspectRatioBox* pasp) {
+  void Fill(PixelAspectRatio* pasp) {
     pasp->h_spacing = 5;
     pasp->v_spacing = 8;
   }
 
-  void Modify(PixelAspectRatioBox* pasp) { pasp->v_spacing *= 8; }
+  void Modify(PixelAspectRatio* pasp) { pasp->v_spacing *= 8; }
 
   void Fill(CodecConfigurationRecord* codec_config_record) {
     const uint8_t kAvccData[] = {
@@ -724,7 +724,7 @@ class BoxDefinitionsTestGeneral : public testing::Test {
   bool IsOptional(const EditList* box) { return true; }
   bool IsOptional(const Edit* box) { return true; }
   bool IsOptional(const CodecConfigurationRecord* box) { return true; }
-  bool IsOptional(const PixelAspectRatioBox* box) { return true; }
+  bool IsOptional(const PixelAspectRatio* box) { return true; }
   bool IsOptional(const ElementaryStreamDescriptor* box) { return true; }
   bool IsOptional(const CompositionTimeToSample* box) { return true; }
   bool IsOptional(const SyncSample* box) { return true; }
@@ -754,7 +754,7 @@ typedef testing::Types<
     Edit,
     HandlerReference,
     CodecConfigurationRecord,
-    PixelAspectRatioBox,
+    PixelAspectRatio,
     VideoSampleEntry,
     ElementaryStreamDescriptor,
     AudioSampleEntry,
@@ -797,6 +797,15 @@ typedef testing::Types<
 
 TYPED_TEST_CASE_P(BoxDefinitionsTestGeneral);
 
+TYPED_TEST_P(BoxDefinitionsTestGeneral, WriteHeader) {
+  TypeParam box;
+  LOG(INFO) << "Processing " << FourCCToString(box.BoxType());
+  this->Fill(&box);
+  box.WriteHeader(this->buffer_.get());
+  // Box header size should be either 8 bytes or 12 bytes.
+  EXPECT_TRUE(this->buffer_->Size() == 8 || this->buffer_->Size() == 12);
+}
+
 TYPED_TEST_P(BoxDefinitionsTestGeneral, WriteReadbackCompare) {
   TypeParam box;
   LOG(INFO) << "Processing " << FourCCToString(box.BoxType());
@@ -840,6 +849,7 @@ TYPED_TEST_P(BoxDefinitionsTestGeneral, Empty) {
 }
 
 REGISTER_TYPED_TEST_CASE_P(BoxDefinitionsTestGeneral,
+                           WriteHeader,
                            WriteReadbackCompare,
                            WriteModifyWrite,
                            Empty);

packager/media/formats/mp4/box_reader_unittest.cc

@@ -30,22 +30,22 @@ static const uint8_t kSkipBox[] = {
     0x00};
 
 struct FreeBox : Box {
-  bool ReadWrite(BoxBuffer* buffer) override {
+  FourCC BoxType() const override { return FOURCC_FREE; }
+  bool ReadWriteInternal(BoxBuffer* buffer) override {
     return true;
   }
-  FourCC BoxType() const override { return FOURCC_FREE; }
+  uint32_t ComputeSizeInternal() override {
-  uint32_t ComputeSize() override {
     NOTIMPLEMENTED();
     return 0;
   }
 };
 
 struct PsshBox : Box {
-  bool ReadWrite(BoxBuffer* buffer) override {
+  FourCC BoxType() const override { return FOURCC_PSSH; }
+  bool ReadWriteInternal(BoxBuffer* buffer) override {
     return buffer->ReadWriteUInt32(&val);
   }
-  FourCC BoxType() const override { return FOURCC_PSSH; }
+  uint32_t ComputeSizeInternal() override {
-  uint32_t ComputeSize() override {
     NOTIMPLEMENTED();
     return 0;
   }
@@ -54,8 +54,9 @@ struct PsshBox : Box {
 };
 
 struct SkipBox : FullBox {
-  bool ReadWrite(BoxBuffer* buffer) override {
+  FourCC BoxType() const override { return FOURCC_SKIP; }
-    RCHECK(FullBox::ReadWrite(buffer) && buffer->ReadWriteUInt8(&a) &&
+  bool ReadWriteInternal(BoxBuffer* buffer) override {
+    RCHECK(ReadWriteHeaderInternal(buffer) && buffer->ReadWriteUInt8(&a) &&
            buffer->ReadWriteUInt8(&b) && buffer->ReadWriteUInt16(&c) &&
            buffer->ReadWriteInt32(&d) &&
            buffer->ReadWriteInt64NBytes(&e, sizeof(uint32_t)));
@@ -68,8 +69,7 @@ struct SkipBox : FullBox {
     }
     return buffer->TryReadWriteChild(&empty);
   }
-  FourCC BoxType() const override { return FOURCC_SKIP; }
+  uint32_t ComputeSizeInternal() override {
-  uint32_t ComputeSize() override {
     NOTIMPLEMENTED();
     return 0;
   }

packager/media/formats/mp4/segmenter.cc

@@ -408,7 +408,7 @@ Status Segmenter::FinalizeFragment(bool finalize_segment,
     Fragmenter* fragmenter = fragmenters_[i];
     mdat.data_size =
         fragmenter->aux_data()->Size() + fragmenter->data()->Size();
-    mdat.Write(fragment_buffer_.get());
+    mdat.WriteHeader(fragment_buffer_.get());
     if (fragmenter->aux_data()->Size()) {
       fragment_buffer_->AppendBuffer(*fragmenter->aux_data());
     }