FakeVideoCaptureDevice: Regulate framerate without drifting

media/video/capture/fake_video_capture_device.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/video/capture/fake_video_capture_device.h"
 
 
 #include "base/bind.h"
 #include "base/strings/stringprintf.h"
 #include "media/audio/fake_audio_input_stream.h"
@@ skipping 105 matching lines @@
         weak_factory_.GetWeakPtr(), (device_type_ == USING_CLIENT_BUFFERS_I420
                                          ? PIXEL_FORMAT_I420
                                          : PIXEL_FORMAT_GPUMEMORYBUFFER)));
   } else {
     client_->OnError("Unknown Fake Video Capture Device type.");
   }
 }
 
 void FakeVideoCaptureDevice::StopAndDeAllocate() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  next_frame_time_ = base::TimeTicks();
   client_.reset();
 }
 
 void FakeVideoCaptureDevice::CaptureUsingOwnBuffers() {
   DCHECK(thread_checker_.CalledOnValidThread());
   const size_t frame_size = capture_format_.ImageAllocationSize();
   memset(fake_frame_.get(), 0, frame_size);
 
   DrawPacman(false  /* use_argb */,
              fake_frame_.get(),
@@ skipping 53 matching lines @@
     client_->OnIncomingCapturedBuffer(capture_buffer.Pass(), format,
                                       base::TimeTicks::Now());
   }
 
   BeepAndScheduleNextCapture(
       base::Bind(&FakeVideoCaptureDevice::CaptureUsingClientBuffers,
                  weak_factory_.GetWeakPtr(), pixel_format));
 }
 
 void FakeVideoCaptureDevice::BeepAndScheduleNextCapture(
     const base::Closure& next_capture) {

[mcasas, 2015/06/08 17:30:35]

What about adding a parameter to BeepAndScheduleNextCapture(),
such as |expected_execution_time| instead of adding a member
variable? I don't like adding a new internal state to the class.

This also applies to FileVideoCaptureDevice modifications.

   // Generate a synchronized beep sound every so many frames.
   if (frame_count_++ % kFakeCaptureBeepCycle == 0)
     FakeAudioInputStream::BeepOnce();
 
   // Reschedule next CaptureTask.
-  base::MessageLoop::current()->PostDelayedTask(FROM_HERE, next_capture,
-      base::TimeDelta::FromMilliseconds(kFakeCapturePeriodMs));
+  const base::TimeTicks current_time = base::TimeTicks::Now();
+  const base::TimeDelta frame_interval =
+      base::TimeDelta::FromMilliseconds(kFakeCapturePeriodMs);
+  if (next_frame_time_.is_null()) {

[mcasas, 2015/06/08 17:30:35]

I'm not super happy with l.209, it feels like we'll be testing
next_frame_time_.is_null() every frame, when it'll only
verify on first startup (and after StopAndDeallocate()).
Why don't you initialise it (with perhaps appropriate 
expected delay) in AllocateAndStart() ?

+    next_frame_time_ = current_time + frame_interval;
+  } else {
+    next_frame_time_ += frame_interval;
+    // Don't accumulate any debt if we are lagging behind - just post the next
+    // frame immediately and continue as normal.
+    if (next_frame_time_ < current_time)
+      next_frame_time_ = current_time;
+  }
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE, next_capture, next_frame_time_ - current_time);
 }
 
 }  // namespace media