FakeVideoCaptureDevice: Regulate framerate without drifting

FakeVideoCaptureDevice: Regulate framerate without drifting

We currently rely on PostDelayedTask with a constant frame interval to get a specified frame rate.
This CL dynamically adapts the delay to PostDelayedTask to keep the frame rate in sync.

This CL is similar to https://codereview.chromium.org/1162903004/.

BUG=492189
Committed: https://crrev.com/592dfd1e550e16fbfcd9840e5dd6a244fbb022cc
Cr-Commit-Position: refs/heads/master@{#333723}

[magjed_chromium, 2015-06-05 13:04:50]

The CQ bit was checked by magjed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 13:04:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153713006/1

[commit-bot: I haz the power, 2015-06-05 15:32:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 15:32:47]

Dry run: This issue passed the CQ dry run.

[magjed_chromium, 2015-06-08 09:53:22]

magjed@chromium.org changed reviewers:
+ mcasas@chromium.org

[magjed_chromium, 2015-06-08 09:55:42]

Please take a look.

[mcasas, 2015-06-08 17:30:36]

https://codereview.chromium.org/1153713006/diff/1/media/video/capture/fake_vi...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/1153713006/diff/1/media/video/capture/fake_vi...
media/video/capture/fake_video_capture_device.cc:200: const base::Closure&
next_capture) {
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.

https://codereview.chromium.org/1153713006/diff/1/media/video/capture/fake_vi...
media/video/capture/fake_video_capture_device.cc:209: if
(next_frame_time_.is_null()) {
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() ?

[magjed_chromium, 2015-06-09 17:09:34]

PTAL

[mcasas, 2015-06-09 19:41:16]

LGTM % my comments.

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
media/video/capture/fake_video_capture_device.cc:221:
base::MessageLoop::current()->PostDelayedTask(
Nit: To make the dynamic adjustment of the delay more evident
I propose calling a  name to |next_execution_time - current_time|,
e.g. |time_to_next_capture| or so. WDYT?

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
media/video/capture/fake_video_capture_device.h:44: void
CaptureUsingOwnBuffers(const base::TimeTicks& expected_execution_time);
Please read [1] l.24-26, IOW in this case don't pass
by reference. I find it counterintuitive, but well...

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s...

[magjed_chromium, 2015-06-10 09:53:47]

The CQ bit was checked by magjed@chromium.org to run a CQ dry run

[magjed_chromium, 2015-06-10 09:53:47]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1153713006/#ps40001
(title: "mcasas@ comments #2")

[commit-bot: I haz the power, 2015-06-10 09:54:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153713006/40001

[magjed_chromium, 2015-06-10 11:19:55]

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device.cc (right):

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
media/video/capture/fake_video_capture_device.cc:221:
base::MessageLoop::current()->PostDelayedTask(
On 2015/06/09 19:41:16, mcasas wrote:
> Nit: To make the dynamic adjustment of the delay more evident
> I propose calling a  name to |next_execution_time - current_time|,
> e.g. |time_to_next_capture| or so. WDYT?

Done.

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
File media/video/capture/fake_video_capture_device.h (right):

https://codereview.chromium.org/1153713006/diff/20001/media/video/capture/fak...
media/video/capture/fake_video_capture_device.h:44: void
CaptureUsingOwnBuffers(const base::TimeTicks& expected_execution_time);
On 2015/06/09 19:41:16, mcasas wrote:
> Please read [1] l.24-26, IOW in this case don't pass
> by reference. I find it counterintuitive, but well...
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s...

Done.

[magjed_chromium, 2015-06-10 11:20:04]

The CQ bit was unchecked by magjed@chromium.org

[magjed_chromium, 2015-06-10 11:20:08]

The CQ bit was checked by magjed@chromium.org

[commit-bot: I haz the power, 2015-06-10 11:20:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153713006/40001

[commit-bot: I haz the power, 2015-06-10 12:12:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-10 12:13:32]

Patchset 3 (id:??) landed as
https://crrev.com/592dfd1e550e16fbfcd9840e5dd6a244fbb022cc
Cr-Commit-Position: refs/heads/master@{#333723}

[brucedawson, 2015-06-10 17:00:09]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-06-10 17:00:10]

Thanks for the fix. lgtm.