test and fix .mp4 generation code

* new, more thorough tests based on a "BoxCursor" which navigates the
  resulting .mp4. This tests everything the C++ code was testing on
  Mp4SamplePieces. And it goes beyond: it tests the actual resulting .mp4
  file, not some internal logic.

* fix recording::Segment::foreach to properly handle a truncated ending.
  Before this was causing a panic.

* get rid of the separate recording::Segment::init method. This was some of
  the first Rust I ever wrote, and I must have thought I couldn't loan it my
  locked database. I can, and that's more clean. Now Segments are never
  half-initialized. Less to test, less to go wrong.

* fix recording::Segment::new to treat a trailing zero duration on a segment
  with a non-zero start in the same way as it does with a zero start. I'm
  still not sure what I'm doing makes sense, but at least it's not
  surprisingly inconsistent.

* add separate, smaller tests of recording::Segment

* address a couple TODOs in the .mp4 code and add missing comments

* change a couple panics on database corruption into cleaner error returns

* increment the etag version given the .mp4 output has changed

src/pieces.rs

@@ -70,7 +70,8 @@ impl<W, C> fmt::Debug for Slices<W, C> where W: fmt::Debug {
         write!(f, "{} slices with overall length {}:", self.slices.len(), self.len)?;
         let mut start = 0;
         for (i, s) in self.slices.iter().enumerate() {
-            write!(f, "\n{:7}: [{:12}, {:12}): {:?}", i, start, s.end, s.writer)?;
+            write!(f, "\ni {:7}: range [{:12}, {:12}) len {:12}: {:?}",
+                   i, start, s.end, s.end - start, s.writer)?;
             start = s.end;
         }
         Ok(())

src/recording.rs

@@ -41,6 +41,7 @@ use std::fs;
 use std::io::Write;
 use std::ops::Range;
 use std::string::String;
+use std::sync::MutexGuard;
 use time;
 use uuid::Uuid;
 
@@ -435,18 +436,17 @@ pub struct Segment {
 }
 
 impl Segment {
-    /// Creates a segment in a semi-initialized state. This is very light initialization because
+    /// Creates a segment.
-    /// it is called with the database lock held. `init` must be called before usage, and the
-    /// Segment should not be used if `init` fails.
     ///
     /// `desired_range_90k` represents the desired range of the segment relative to the start of
     /// the recording. The actual range will start at the first key frame at or before the
     /// desired start time. (The caller is responsible for creating an edit list to skip the
     /// undesired portion.) It will end at the first frame after the desired range (unless the
     /// desired range extends beyond the recording).
-    pub fn new(recording: &db::ListCameraRecordingsRow,
+    pub fn new(db: &MutexGuard<db::LockedDatabase>,
-               desired_range_90k: Range<i32>) -> Segment {
+               recording: &db::ListCameraRecordingsRow,
-        Segment{
+               desired_range_90k: Range<i32>) -> Result<Segment, Error> {
+        let mut self_ = Segment{
             id: recording.id,
             start: recording.start,
             begin: SampleIndexIterator::new(),
@@ -456,27 +456,23 @@ impl Segment {
             frames: recording.video_samples,
             key_frames: recording.video_sync_samples,
             video_sample_entry_id: recording.video_sample_entry.id,
-        }
+        };
-    }
 
-    /// Completes initialization of the segment. Must be called without the database lock held;
+        if self_.desired_range_90k.start > self_.desired_range_90k.end ||
-    /// this will use the database to retrieve the video index for partial recordings.
+           self_.desired_range_90k.end > self_.actual_end_90k {
-    pub fn init(&mut self, db: &db::Database) -> Result<(), Error> {
-        if self.desired_range_90k.start > self.desired_range_90k.end ||
-           self.desired_range_90k.end > self.actual_end_90k {
             return Err(Error::new(format!(
                 "desired range [{}, {}) invalid for recording of length {}",
-                self.desired_range_90k.start, self.desired_range_90k.end, self.actual_end_90k)));
+                self_.desired_range_90k.start, self_.desired_range_90k.end, self_.actual_end_90k)));
         }
 
-        if self.desired_range_90k.start == 0 &&
+        if self_.desired_range_90k.start == 0 &&
-           self.desired_range_90k.end == self.actual_end_90k {
+           self_.desired_range_90k.end == self_.actual_end_90k {
             // Fast path. Existing entry is fine.
-            return Ok(())
+            return Ok(self_)
         }
 
         // Slow path. Need to iterate through the index.
-        let extra = db.lock().get_recording(self.id)?;
+        let extra = db.get_recording(self_.id)?;
         let data = &(&extra).video_index;
         let mut it = SampleIndexIterator::new();
         if !it.next(data)? {
@@ -487,36 +483,44 @@ impl Segment {
             return Err(Error{description: String::from("not key frame"),
                              cause: None});
         }
+
+        // Stop when hitting a frame with this start time.
+        // Going until the end of the recording is special-cased because there can be a trailing
+        // frame of zero duration. It's unclear exactly how this should be handled, but let's
+        // include it for consistency with the fast path. It'd be bizarre to have it included or
+        // not based on desired_range_90k.start.
+        let end_90k = if self_.desired_range_90k.end == self_.actual_end_90k {
+            i32::max_value()
+        } else {
+            self_.desired_range_90k.end
+        };
+
         loop {
-            if it.start_90k <= self.desired_range_90k.start && it.is_key {
+            if it.start_90k <= self_.desired_range_90k.start && it.is_key {
                 // new start candidate.
-                self.begin = it;
+                self_.begin = it;
-                self.frames = 0;
+                self_.frames = 0;
-                self.key_frames = 0;
+                self_.key_frames = 0;
             }
-            if it.start_90k >= self.desired_range_90k.end {
+            if it.start_90k >= end_90k {
                 break;
             }
-            self.frames += 1;
+            self_.frames += 1;
-            self.key_frames += it.is_key as i32;
+            self_.key_frames += it.is_key as i32;
             if !it.next(data)? {
                 break;
             }
         }
-        self.file_end = it.pos;
+        self_.file_end = it.pos;
-        self.actual_end_90k = it.start_90k;
+        self_.actual_end_90k = it.start_90k;
-        Ok(())
+        Ok(self_)
     }
 
     /// Returns the byte range within the sample file of data associated with this segment.
-    pub fn sample_file_range(&self) -> Range<u64> {
+    pub fn sample_file_range(&self) -> Range<u64> { self.begin.pos as u64 .. self.file_end as u64 }
-        Range{start: self.begin.pos as u64, end: self.file_end as u64}
-    }
 
     /// Returns the actual time range as described in `new`.
-    pub fn actual_time_90k(&self) -> Range<i32> {
+    pub fn actual_time_90k(&self) -> Range<i32> { self.begin.start_90k .. self.actual_end_90k }
-        Range{start: self.begin.start_90k, end: self.actual_end_90k}
-    }
 
     /// Iterates through each frame in the segment.
     /// Must be called without the database lock held; retrieves video index from the cache.
@@ -527,15 +531,36 @@ impl Segment {
         let data = &(&extra).video_index;
         let mut it = self.begin;
         if it.i == 0 {
-            assert!(it.next(data)?);
-            assert!(it.is_key);
-        }
-        loop {
-            f(&it)?;
             if !it.next(data)? {
-                return Ok(());
+                return Err(Error::new(format!("recording {}: no frames", self.id)));
+            }
+            if !it.is_key {
+                return Err(Error::new(format!("recording {}: doesn't start with key frame",
+                                              self.id)));
             }
         }
+        let mut have_frame = true;
+        let mut key_frame = 0;
+        for i in 0 .. self.frames {
+            if !have_frame {
+                return Err(Error::new(format!("recording {}: expected {} frames, found only {}",
+                                              self.id, self.frames, i+1)));
+            }
+            if it.is_key {
+                key_frame += 1;
+                if key_frame > self.key_frames {
+                    return Err(Error::new(format!("recording {}: more than expected {} key frames",
+                                                  self.id, self.key_frames)));
+                }
+            }
+            f(&it)?;
+            have_frame = it.next(data)?;
+        }
+        if key_frame < self.key_frames {
+            return Err(Error::new(format!("recording {}: expected {} key frames, found only {}",
+                                          self.id, self.key_frames, key_frame)));
+        }
+        Ok(())
     }
 }
 
@@ -546,6 +571,7 @@ mod tests {
     use super::{append_varint32, decode_varint32, unzigzag32, zigzag32};
     use super::*;
     use self::test::Bencher;
+    use testutil::TestDb;
 
     #[test]
     fn test_zigzag() {
@@ -646,7 +672,7 @@ mod tests {
         }
     }
 
-    /// Tests the example from design/schema.md.
+    /// Tests encoding the example from design/schema.md.
     #[test]
     fn test_encode_example() {
         let mut e = SampleIndexEncoder::new();
@@ -661,6 +687,7 @@ mod tests {
         assert_eq!(2, e.video_sync_samples);
     }
 
+    /// Tests a round trip from `SampleIndexEncoder` to `SampleIndexIterator`.
     #[test]
     fn test_round_trip() {
         #[derive(Debug, PartialEq, Eq)]
@@ -689,6 +716,8 @@ mod tests {
         assert!(!it.next(&e.video_index).unwrap());
     }
 
+    /// Tests that `SampleIndexIterator` spots several classes of errors.
+    /// TODO: test and fix overflow cases.
     #[test]
     fn test_iterator_errors() {
         struct Test {
@@ -711,6 +740,93 @@ mod tests {
         }
     }
 
+    /// Tests that a `Segment` correctly can clip at the beginning and end.
+    /// This is a simpler case; all sync samples means we can start on any frame.
+    #[test]
+    fn test_segment_clipping_with_all_sync() {
+        let mut encoder = SampleIndexEncoder::new();
+        for i in 1..6 {
+            let duration_90k = 2 * i;
+            let bytes = 3 * i;
+            encoder.add_sample(duration_90k, bytes, true);
+        }
+        let db = TestDb::new();
+        let row = db.create_recording_from_encoder(encoder);
+        // Time range [2, 2 + 4 + 6 + 8) means the 2nd, 3rd, 4th samples should be
+        // included.
+        let segment = Segment::new(&db.db.lock(), &row, 2 .. 2+4+6+8).unwrap();
+        let mut v = Vec::new();
+        segment.foreach(&db.db, |it| { v.push(it.duration_90k); Ok(()) }).unwrap();
+        assert_eq!(&v, &[4, 6, 8]);
+    }
+
+    /// Half sync frames means starting from the last sync frame <= desired point.
+    #[test]
+    fn test_segment_clipping_with_half_sync() {
+        let mut encoder = SampleIndexEncoder::new();
+        for i in 1..6 {
+            let duration_90k = 2 * i;
+            let bytes = 3 * i;
+            encoder.add_sample(duration_90k, bytes, (i % 2) == 1);
+        }
+        let db = TestDb::new();
+        let row = db.create_recording_from_encoder(encoder);
+        // Time range [2 + 4 + 6, 2 + 4 + 6 + 8) means the 4th sample should be included.
+        // The 3rd also gets pulled in because it is a sync frame and the 4th is not.
+        let segment = Segment::new(&db.db.lock(), &row, 2+4+6 .. 2+4+6+8).unwrap();
+        let mut v = Vec::new();
+        segment.foreach(&db.db, |it| { v.push(it.duration_90k); Ok(()) }).unwrap();
+        assert_eq!(&v, &[6, 8]);
+    }
+
+    #[test]
+    fn test_segment_clipping_with_trailing_zero() {
+        let mut encoder = SampleIndexEncoder::new();
+        encoder.add_sample(1, 1, true);
+        encoder.add_sample(1, 2, true);
+        encoder.add_sample(0, 3, true);
+        let db = TestDb::new();
+        let row = db.create_recording_from_encoder(encoder);
+        let segment = Segment::new(&db.db.lock(), &row, 1 .. 2).unwrap();
+        let mut v = Vec::new();
+        segment.foreach(&db.db, |it| { v.push(it.bytes); Ok(()) }).unwrap();
+        assert_eq!(&v, &[2, 3]);
+    }
+
+    /// Test a `Segment` which uses the whole recording.
+    /// This takes a fast path which skips scanning the index in `new()`.
+    #[test]
+    fn test_segment_fast_path() {
+        let mut encoder = SampleIndexEncoder::new();
+        for i in 1..6 {
+            let duration_90k = 2 * i;
+            let bytes = 3 * i;
+            encoder.add_sample(duration_90k, bytes, (i % 2) == 1);
+        }
+        let db = TestDb::new();
+        let row = db.create_recording_from_encoder(encoder);
+        let segment = Segment::new(&db.db.lock(), &row, 0 .. 2+4+6+8+10).unwrap();
+        let mut v = Vec::new();
+        segment.foreach(&db.db, |it| { v.push(it.duration_90k); Ok(()) }).unwrap();
+        assert_eq!(&v, &[2, 4, 6, 8, 10]);
+    }
+
+    #[test]
+    fn test_segment_fast_path_with_trailing_zero() {
+        let mut encoder = SampleIndexEncoder::new();
+        encoder.add_sample(1, 1, true);
+        encoder.add_sample(1, 2, true);
+        encoder.add_sample(0, 3, true);
+        let db = TestDb::new();
+        let row = db.create_recording_from_encoder(encoder);
+        let segment = Segment::new(&db.db.lock(), &row, 0 .. 2).unwrap();
+        let mut v = Vec::new();
+        segment.foreach(&db.db, |it| { v.push(it.bytes); Ok(()) }).unwrap();
+        assert_eq!(&v, &[1, 2, 3]);
+    }
+
+    // TODO: test segment error cases involving mismatch between row frames/key_frames and index.
+
     /// Benchmarks the decoder, which is performance-critical for .mp4 serving.
     #[bench]
     fn bench_decoder(b: &mut Bencher) {

src/testutil.rs

@@ -28,16 +28,32 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+extern crate tempdir;
+
+use db;
+use dir;
+use recording::{self, TIME_UNITS_PER_SEC};
+use rusqlite;
 use std::env;
 use std::sync;
+use std::thread;
 use slog::{self, DrainExt};
 use slog_envlogger;
 use slog_stdlog;
 use slog_term;
 use time;
+use uuid::Uuid;
 
 static INIT: sync::Once = sync::ONCE_INIT;
 
+lazy_static! {
+    static ref TEST_CAMERA_UUID: Uuid =
+        Uuid::parse_str("ce2d9bc2-0cd3-4204-9324-7b5ccb07183c").unwrap();
+}
+
+/// id of the camera created by `TestDb::new` below.
+pub const TEST_CAMERA_ID: i32 = 1;
+
 /// Performs global initialization for tests.
 ///    * set up logging. (Note the output can be confusing unless `RUST_TEST_THREADS=1` is set in
 ///      the program's environment prior to running.)
@@ -52,3 +68,81 @@ pub fn init() {
         time::tzset();
     });
 }
+
+pub struct TestDb {
+    pub db: sync::Arc<db::Database>,
+    pub dir: sync::Arc<dir::SampleFileDir>,
+    pub syncer_channel: dir::SyncerChannel,
+    pub syncer_join: thread::JoinHandle<()>,
+    pub tmpdir: tempdir::TempDir,
+}
+
+impl TestDb {
+    /// Creates a test database with one camera.
+    pub fn new() -> TestDb {
+        let tmpdir = tempdir::TempDir::new("moonfire-nvr-test").unwrap();
+
+        let conn = rusqlite::Connection::open_in_memory().unwrap();
+        let schema = include_str!("schema.sql");
+        conn.execute_batch(schema).unwrap();
+        let uuid_bytes = &TEST_CAMERA_UUID.as_bytes()[..];
+        conn.execute_named(r#"
+            insert into camera (uuid,  short_name,  description,  host,  username,  password,
+                                main_rtsp_path,  sub_rtsp_path,  retain_bytes)
+                        values (:uuid, :short_name, :description, :host, :username, :password,
+                                :main_rtsp_path, :sub_rtsp_path, :retain_bytes)
+        "#, &[
+            (":uuid", &uuid_bytes),
+            (":short_name", &"test camera"),
+            (":description", &""),
+            (":host", &"test-camera"),
+            (":username", &"foo"),
+            (":password", &"bar"),
+            (":main_rtsp_path", &"/main"),
+            (":sub_rtsp_path", &"/sub"),
+            (":retain_bytes", &1048576i64),
+        ]).unwrap();
+        assert_eq!(TEST_CAMERA_ID as i64, conn.last_insert_rowid());
+        let db = sync::Arc::new(db::Database::new(conn).unwrap());
+        let path = tmpdir.path().to_str().unwrap().to_owned();
+        let dir = dir::SampleFileDir::new(&path, db.clone()).unwrap();
+        let (syncer_channel, syncer_join) = dir::start_syncer(dir.clone()).unwrap();
+        TestDb{
+            db: db,
+            dir: dir,
+            syncer_channel: syncer_channel,
+            syncer_join: syncer_join,
+            tmpdir: tmpdir,
+        }
+    }
+
+    pub fn create_recording_from_encoder(&self, encoder: recording::SampleIndexEncoder)
+                                         -> db::ListCameraRecordingsRow {
+        let mut db = self.db.lock();
+        let video_sample_entry_id =
+            db.insert_video_sample_entry(1920, 1080, &[0u8; 100]).unwrap();
+        {
+            let mut tx = db.tx().unwrap();
+            tx.bypass_reservation_for_testing = true;
+            const START_TIME: recording::Time = recording::Time(1430006400i64 * TIME_UNITS_PER_SEC);
+            tx.insert_recording(&db::RecordingToInsert{
+                camera_id: TEST_CAMERA_ID,
+                sample_file_bytes: encoder.sample_file_bytes,
+                time: START_TIME ..
+                      START_TIME + recording::Duration(encoder.total_duration_90k as i64),
+                local_time: START_TIME,
+                video_samples: encoder.video_samples,
+                video_sync_samples: encoder.video_sync_samples,
+                video_sample_entry_id: video_sample_entry_id,
+                sample_file_uuid: Uuid::nil(),
+                video_index: encoder.video_index,
+                sample_file_sha1: [0u8; 20],
+            }).unwrap();
+            tx.commit().unwrap();
+        }
+        let mut row = None;
+        let all_time = recording::Time(i64::min_value()) .. recording::Time(i64::max_value());
+        db.list_recordings(TEST_CAMERA_ID, &all_time, |r| { row = Some(r); Ok(()) }).unwrap();
+        row.unwrap()
+    }
+}

src/web.rs

@@ -388,7 +388,7 @@ impl Handler {
                 } else {
                     r.duration_90k
                 };
-                builder.append(r, rel_start .. rel_end);
+                builder.append(&db, r, rel_start .. rel_end)?;
                 Ok(())
             }));
         }
@@ -405,7 +405,7 @@ impl Handler {
                    est_records, start, end, builder.len());
         }
         builder.include_timestamp_subtitle_track(include_ts);
-        let mp4 = try!(builder.build(self.db.clone(), self.dir.clone()));
+        let mp4 = builder.build(self.db.clone(), self.dir.clone())?;
         try!(resource::serve(&mp4, req, res));
         Ok(())
     }