Fixes/improvements to mp4 VirtualFile generation.

* Fix the mdat box size, which was not properly including the length of the
  header itself. (The "mp4file" tool nicely diagnosed this corruption.)

* Fix the stsc box. The first number of each entry is meant to be a chunk
  index, not a sample index. This was causing strange behavior in basically
  any video player for multi-recording videos.

* Populate etag and last-modified so that Range: requests can work properly.
  The etag must be changed every time the generated file format changes.
  There's a serial number constant for this purpose and a test meant to help
  catch such problems.

src/mp4-test.cc

@@ -34,6 +34,7 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 
+#include <event2/buffer.h>
 #include <gflags/gflags.h>
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -66,6 +67,22 @@ std::string ToHex(const FileSlice *slice, bool pad) {
       pad);
 }
 
+std::string Digest(const FileSlice *slice) {
+  EvBuffer buf;
+  std::string error_message;
+  size_t size = slice->size();
+  CHECK(slice->AddRange(ByteRange(0, size), &buf, &error_message))
+      << error_message;
+  evbuffer_iovec vec;
+  auto digest = Digest::SHA1();
+  while (evbuffer_peek(buf.get(), -1, nullptr, &vec, 1) > 0) {
+    digest->Update(re2::StringPiece(
+        reinterpret_cast<const char *>(vec.iov_base), vec.iov_len));
+    evbuffer_drain(buf.get(), vec.iov_len);
+  }
+  return ::moonfire_nvr::ToHex(digest->Finalize());
+}
+
 TEST(Mp4SampleTablePiecesTest, Stts) {
   SampleIndexEncoder encoder;
   for (int i = 1; i <= 5; ++i) {
@@ -123,21 +140,6 @@ TEST(Mp4SampleTablePiecesTest, Stss) {
   EXPECT_EQ(kExpectedSampleNumbers, ToHex(pieces.stss_entries(), true));
 }
 
-TEST(Mp4SampleTablePiecesTest, Stsc) {
-  SampleIndexEncoder encoder;
-  encoder.AddSample(1, 1, true);
-  encoder.AddSample(1, 1, false);
-  encoder.AddSample(1, 1, true);
-  encoder.AddSample(1, 1, false);
-  Mp4SampleTablePieces pieces;
-  std::string error_message;
-  ASSERT_TRUE(pieces.Init(encoder.data(), 2, 10, 0, 4, &error_message))
-      << error_message;
-  EXPECT_EQ(1, pieces.stsc_entry_count());
-  const char kExpectedEntries[] = "00 00 00 0a 00 00 00 04 00 00 00 02";
-  EXPECT_EQ(kExpectedEntries, ToHex(pieces.stsc_entries(), true));
-}
-
 TEST(Mp4SampleTablePiecesTest, Stsz) {
   SampleIndexEncoder encoder;
   for (int i = 1; i <= 5; ++i) {
@@ -210,18 +212,26 @@ class IntegrationTest : public testing::Test {
       ADD_FAILURE() << "Close: " << error_message;
     }
     recording_.video_index = index.data().as_string();
+
+    // Set start time to 2015-04-26 00:00:00 UTC.
+    recording_.start_time_90k = UINT64_C(1430006400) * kTimeUnitsPerSecond;
   }
 
-  void CopySingleRecordingToNewMp4() {
+  std::unique_ptr<VirtualFile> CreateMp4FromSingleRecording() {
     Mp4FileBuilder builder;
     builder.SetSampleEntry(video_sample_entry_);
     builder.Append(Recording(recording_), 0,
                    std::numeric_limits<int32_t>::max());
     std::string error_message;
     auto mp4 = builder.Build(&error_message);
-    ASSERT_TRUE(mp4 != nullptr) << error_message;
+    EXPECT_TRUE(mp4 != nullptr) << error_message;
+    return mp4;
+  }
+
+  void WriteMp4(VirtualFile *f) {
     EvBuffer buf;
-    ASSERT_TRUE(mp4->AddRange(ByteRange(0, mp4->size()), &buf, &error_message))
+    std::string error_message;
+    EXPECT_TRUE(f->AddRange(ByteRange(0, f->size()), &buf, &error_message))
         << error_message;
     WriteFileOrDie(StrCat(tmpdir_path_, "/clip.new.mp4"), &buf);
   }
@@ -272,16 +282,33 @@ class IntegrationTest : public testing::Test {
 
   std::string tmpdir_path_;
   std::unique_ptr<File> tmpdir_;
+  std::string etag_;
   Recording recording_;
   VideoSampleEntry video_sample_entry_;
 };
 
 TEST_F(IntegrationTest, RoundTrip) {
   CopyMp4ToSingleRecording();
-  CopySingleRecordingToNewMp4();
+  auto f = CreateMp4FromSingleRecording();
+  WriteMp4(f.get());
   CompareMp4s();
 }
 
+TEST_F(IntegrationTest, Metadata) {
+  CopyMp4ToSingleRecording();
+  auto f = CreateMp4FromSingleRecording();
+
+  // This test is brittle, which is the point. Any time the digest comparison
+  // here fails, it can be updated, but the etag must change as well!
+  // Otherwise clients may combine ranges from the new format with ranges
+  // from the old format!
+  EXPECT_EQ("1e5331e8371bd97ac3158b3a86494abc87cdc70e", Digest(f.get()));
+  EXPECT_EQ("\"62f5e00a6e1e6dd893add217b1bf7ed7446b8b9d\"", f->etag());
+
+  // 10 seconds later than the segment's start time.
+  EXPECT_EQ(1430006410, f->last_modified());
+}
+
 }  // namespace
 }  // namespace moonfire_nvr
 

src/mp4.cc

@@ -98,6 +98,12 @@ namespace moonfire_nvr {
 
 namespace {
 
+// This value should be incremented any time a change is made to this file
+// that causes the different bytes to be output for a particular set of
+// Mp4Builder options. Incrementing this value will cause the etag to change
+// as well.
+const char kFormatVersion[] = {0x00};
+
 // ISO/IEC 14496-12 section 4.3, ftyp.
 const char kFtypBox[] = {
     0x00, 0x00, 0x00, 0x20,  // length = 32, sizeof(kFtypBox)
@@ -367,9 +373,11 @@ class Mp4File : public VirtualFile {
     int64_t max_time_90k = 0;
     for (const auto &segment : segments_) {
       duration += segment->pieces.duration_90k();
-      max_time_90k = std::max(max_time_90k, segment->recording.start_time_90k +
+      int64_t end_90k =
-                                                segment->rel_end_90k);
+          segment->recording.start_time_90k + segment->pieces.end_90k();
+      max_time_90k = std::max(max_time_90k, end_90k);
     }
+    last_modified_ = max_time_90k / kTimeUnitsPerSecond;
     auto net_duration = ToNetworkU32(duration);
     auto net_creation_ts = ToNetworkU32(ToIso14496Timestamp(max_time_90k));
 
@@ -378,6 +386,7 @@ class Mp4File : public VirtualFile {
 
     // Add the mdat_ without using CONSTRUCT_BOX.
     // mdat_ is special because it uses largesize rather than size.
+    int64_t size_before_mdat = slices_.size();
     slices_.Append(mdat_.header_slice());
     initial_sample_byte_pos_ = slices_.size();
     for (const auto &segment : segments_) {
@@ -385,12 +394,24 @@ class Mp4File : public VirtualFile {
                                       segment->pieces.sample_pos());
       slices_.Append(&segment->sample_file_slice);
     }
-    mdat_.header().largesize =
+    mdat_.header().largesize = ToNetworkU64(slices_.size() - size_before_mdat);
-        ToNetworkU64(slices_.size() - initial_sample_byte_pos_);
+
+    auto etag_digest = Digest::SHA1();
+    etag_digest->Update(
+        re2::StringPiece(kFormatVersion, sizeof(kFormatVersion)));
+    std::string segment_times;
+    for (const auto &segment : segments_) {
+      segment_times.clear();
+      Append64(segment->pieces.sample_pos().begin, &segment_times);
+      Append64(segment->pieces.sample_pos().end, &segment_times);
+      etag_digest->Update(segment_times);
+      etag_digest->Update(segment->recording.sample_file_sha1);
+    }
+    etag_ = StrCat("\"", ToHex(etag_digest->Finalize()), "\"");
   }
 
-  time_t last_modified() const final { return 0; }  // TODO
+  time_t last_modified() const final { return last_modified_; }
-  std::string etag() const final { return ""; }     // TODO
+  std::string etag() const final { return etag_; }
   std::string mime_type() const final { return "video/mp4"; }
   int64_t size() const final { return slices_.size(); }
   bool AddRange(ByteRange range, EvBuffer *buf,
@@ -458,13 +479,14 @@ class Mp4File : public VirtualFile {
     }
     {
       CONSTRUCT_BOX(moov_trak_mdia_minf_stbl_stsc_);
-      uint32_t stsc_entry_count = 0;
+      moov_trak_mdia_minf_stbl_stsc_entries_.Init(
-      for (const auto &segment : segments_) {
+          3 * sizeof(uint32_t) * segments_.size(),
-        stsc_entry_count += segment->pieces.stsc_entry_count();
+          [this](std::string *s, std::string *error_message) {
-        slices_.Append(segment->pieces.stsc_entries());
+            return FillStscEntries(s, error_message);
-      }
+          });
       moov_trak_mdia_minf_stbl_stsc_.header().entry_count =
-          ToNetwork32(stsc_entry_count);
+          ToNetwork32(segments_.size());
+      slices_.Append(&moov_trak_mdia_minf_stbl_stsc_entries_);
     }
     {
       CONSTRUCT_BOX(moov_trak_mdia_minf_stbl_stsz_);
@@ -499,6 +521,16 @@ class Mp4File : public VirtualFile {
     }
   }
 
+  bool FillStscEntries(std::string *s, std::string *error_message) {
+    uint32_t chunk = 0;
+    for (const auto &segment : segments_) {
+      AppendU32(++chunk, s);
+      AppendU32(segment->pieces.samples(), s);
+      AppendU32(1, s);  // TODO: sample_description_index.
+    }
+    return true;
+  }
+
   bool FillCo64Entries(std::string *s, std::string *error_message) {
     int64_t pos = initial_sample_byte_pos_;
     for (const auto &segment : segments_) {
@@ -512,6 +544,8 @@ class Mp4File : public VirtualFile {
   std::vector<std::unique_ptr<Mp4FileSegment>> segments_;
   VideoSampleEntry video_sample_entry_;
   FileSlices slices_;
+  std::string etag_;
+  time_t last_modified_ = -1;
 
   StaticStringPieceSlice ftyp_;
   Mp4Box<MovieBox> moov_;
@@ -528,6 +562,7 @@ class Mp4File : public VirtualFile {
   CopyingStringPieceSlice moov_trak_mdia_minf_stbl_stsd_entry_;
   Mp4Box<TimeToSampleBoxVersion0> moov_trak_mdia_minf_stbl_stts_;
   Mp4Box<SampleToChunkBoxVersion0> moov_trak_mdia_minf_stbl_stsc_;
+  FillerFileSlice moov_trak_mdia_minf_stbl_stsc_entries_;
   Mp4Box<SampleSizeBoxVersion0> moov_trak_mdia_minf_stbl_stsz_;
   Mp4Box<ChunkLargeOffsetBoxVersion0> moov_trak_mdia_minf_stbl_co64_;
   FillerFileSlice moov_trak_mdia_minf_stbl_co64_entries_;
@@ -597,10 +632,6 @@ bool Mp4SampleTablePieces::Init(re2::StringPiece video_index_blob,
                      [this](std::string *s, std::string *error_message) {
                        return FillStssEntries(s, error_message);
                      });
-  stsc_entries_.Init(3 * sizeof(int32_t) * stsc_entry_count(),
-                     [this](std::string *s, std::string *error_message) {
-                       return FillStscEntries(s, error_message);
-                     });
   stsz_entries_.Init(sizeof(int32_t) * stsz_entry_count(),
                      [this](std::string *s, std::string *error_message) {
                        return FillStszEntries(s, error_message);

src/mp4.h

@@ -81,9 +81,6 @@ class Mp4SampleTablePieces {
   int32_t stss_entry_count() const { return key_frames_; }
   const FileSlice *stss_entries() const { return &stss_entries_; }
 
-  int32_t stsc_entry_count() const { return 1; }
-  const FileSlice *stsc_entries() const { return &stsc_entries_; }
-
   int32_t stsz_entry_count() const { return frames_; }
   const FileSlice *stsz_entries() const { return &stsz_entries_; }
 
@@ -94,6 +91,9 @@ class Mp4SampleTablePieces {
 
   uint64_t duration_90k() const { return actual_end_90k_ - begin_.start_90k(); }
 
+  int32_t start_90k() const { return begin_.start_90k(); }
+  int32_t end_90k() const { return actual_end_90k_; }
+
  private:
   bool FillSttsEntries(std::string *s, std::string *error_message) const;
   bool FillStssEntries(std::string *s, std::string *error_message) const;
@@ -110,7 +110,6 @@ class Mp4SampleTablePieces {
 
   FillerFileSlice stts_entries_;
   FillerFileSlice stss_entries_;
-  FillerFileSlice stsc_entries_;
   FillerFileSlice stsz_entries_;
 
   int sample_entry_index_ = -1;