Tab Capture: AnimatedContentSampler subsampling and phase fixes in tests

Tab Capture: AnimatedContentSampler subsampling and phase fixes in tests

This change allows the client to specify a target sampling rate, aside
from the maximum frame rate.  When animated content is detected, an
approprate subsampling of the relevant frames will be proposed.  This is
meant to be used for clients wishing to detect high-frame-rate content
(e.g., 48 or 60 FPS), but only sample at traditional rates (e.g., 24 or
30 FPS).

In testing this change, it became apparent that the current tests were
highly sensitive to phase issues, and were erroneously generating
discontinuous sequences of events in some scenarios.  The tests have
been fixed and additional testing for the new functionality added.

BUG=156767
Committed: https://crrev.com/b6b84f4691f44c62ec0ee5f82355ad3d283d4012
Cr-Commit-Position: refs/heads/master@{#329550}

[miu, 2015-05-05 01:04:15]

miu@chromium.org changed reviewers:
+ hubbe@chromium.org

[miu, 2015-05-05 01:04:15]

hubbe: PTAL.

[hubbe, 2015-05-06 19:53:18]

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
File content/browser/media/capture/animated_content_sampler.cc (right):

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:71: sampling_period_ =
ComputeSamplingPeriod(detected_period_,
Do we need to do this here?
Can we do it after the two ifs below?
If not, perhaps add a comment as to why.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:109: if (token_bucket_
>= sampling_period_) {
Correct me if I'm wrong, but it seems like this token bucket is normally going
to be right on the edge of being full when we get a frame that we want to
sample. That would could lead to instability in the sampling.
It seems like we want to try to make sure that in normal operation, the
token_bucket is more than full when we should sample and less than full when we
shouldn't.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:245: return
ideal_timestamp - drift / correct_over_num_frames;
Wonder if it would make sense to break out ClockSmoother from
base/media/audio_shifter.cc and use here?

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:266: if
(std::abs(animation_fps / ratio - target_fps) <
I don't think you need this if statement.
I think ratio is already correct. (It kind of looks like you meant to do a
floor() call when you calculated ratio.)

Without the floor call, ratio should already be the correct value and ratio+1 is
not.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
File content/browser/media/capture/animated_content_sampler.h (right):

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.h:103: static
base::TimeDelta ComputeSamplingPeriod(
Comment?

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.h:133: enum {
NOT_SAMPLING, START_SAMPLING,
one per line?

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.h:136: // A token bucket
that is used to decide which subset of the frames containing
Wait, what?
How exactly is TimeDelta == token bucket?

[miu, 2015-05-09 20:57:39]

Addressed your comments.  Granted, there's a lot going on in the math/heuristics
in this class, so I chose to expand/rewrite some code comments to clarify the
issues you brought up.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
File content/browser/media/capture/animated_content_sampler.cc (right):

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:71: sampling_period_ =
ComputeSamplingPeriod(detected_period_,
On 2015/05/06 19:53:18, hubbe wrote:
> Do we need to do this here?
> Can we do it after the two ifs below?
> If not, perhaps add a comment as to why.

Added a comment explaining this.  It could be moved one, but not two "ifs" down,
but no need to do that.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:109: if (token_bucket_
>= sampling_period_) {
On 2015/05/06 19:53:18, hubbe wrote:
> Correct me if I'm wrong, but it seems like this token bucket is normally going
> to be right on the edge of being full when we get a frame that we want to
> sample. That would could lead to instability in the sampling.

It doesn't.  Both real-world testing and the unit tests show this isn't a
problem.  The specific cases:

1. Perfect, unchanging |detected_period_|: For all but the first frame, the
token bucket is incremented by one |detected_period_| for each animation event. 
|sampling_period_| has to be an integer multiple of |detection_period_| so the
instability is never realized.  (In other words, instead of a token bucket, we
could have kept a "num_animation_events" counter and just sampled whenever
num_animation_events % N == 0.)

2. Typical case, where |detected_period_| is fuzzy: |detected_period_| will
fluctuate up or down randomly, by up to a few dozen microseconds.  If this
causes us to be short of a tiny amount of tokens for an event we really do want
to sample, then that's just tough: We *will* drop that frame.  However, the
token bucket will now have almost a full sampling period's worth of "buffer
tokens" that hang around, preventing the noise from causing any future frame
drops.

If you're still not convinced, consider:

1. This is exactly the heuristic used by the SmoothEventSampler, and we have
years of evidence that this works well.  ;-)

2. This is a dual of the "borrowed time" scheme the AnimatedContentSampler was
using prior to this change.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:245: return
ideal_timestamp - drift / correct_over_num_frames;
On 2015/05/06 19:53:18, hubbe wrote:
> Wonder if it would make sense to break out ClockSmoother from
> base/media/audio_shifter.cc and use here?

As discussed face-to-face, I'll consider this for future clean-up work.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:266: if
(std::abs(animation_fps / ratio - target_fps) <
On 2015/05/06 19:53:18, hubbe wrote:
> I don't think you need this if statement.
> I think ratio is already correct. (It kind of looks like you meant to do a
> floor() call when you calculated ratio.)

|ratio| is an int64, so there's an implicit floor'ing happening there.  I moved
the statement to the top of this code block to help avoid confusion.

> Without the floor call, ratio should already be the correct value and ratio+1
is
> not.

Consider target_fps=30, animation_fps=42.  Then:

  ratio = floor(42/30) = floor(1.4) = 1

and then we find out we want to use ratio+1 because this is false:

  std::abs(42 / 1 - 30) < std::abs(42 / 2 - 30)

In other words, we want to use ratio+1 because a sampling rate of 21 FPS is
closer to the target of 30 than 42.  This is the desired result.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
File content/browser/media/capture/animated_content_sampler.h (right):

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.h:103: static
base::TimeDelta ComputeSamplingPeriod(
On 2015/05/06 19:53:18, hubbe wrote:
> Comment?

Done.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.h:133: enum {
NOT_SAMPLING, START_SAMPLING,
On 2015/05/06 19:53:18, hubbe wrote:
> one per line?

Done.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.h:136: // A token bucket
that is used to decide which subset of the frames containing
On 2015/05/06 19:53:18, hubbe wrote:
> Wait, what?
> How exactly is TimeDelta == token bucket?

One microsecond == one token.  When you can take, say, 33333 tokens, you get to
sample an event.  I updated the comment to clarify.

[hubbe, 2015-05-12 19:29:03]

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
File content/browser/media/capture/animated_content_sampler.cc (right):

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:109: if (token_bucket_
>= sampling_period_) {
I think you're right. I was thinking that the problem would persist after a
frame drop in (2).  I think we can still improve a little bit by initializing
the token bucket to sampling_period_ + animation_period_/2 (or should it be
minus instead of plus?), thus hopefully avoiding having to ever drop any frames
in the (2) case.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:245: return
ideal_timestamp - drift / correct_over_num_frames;
On 2015/05/09 20:57:38, miu wrote:
> On 2015/05/06 19:53:18, hubbe wrote:
> > Wonder if it would make sense to break out ClockSmoother from
> > base/media/audio_shifter.cc and use here?
> 
> As discussed face-to-face, I'll consider this for future clean-up work.

Optional: Add a TODO?

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:266: if
(std::abs(animation_fps / ratio - target_fps) <
On 2015/05/09 20:57:38, miu wrote:
> On 2015/05/06 19:53:18, hubbe wrote:
> > I don't think you need this if statement.
> > I think ratio is already correct. (It kind of looks like you meant to do a
> > floor() call when you calculated ratio.)
> 
> |ratio| is an int64, so there's an implicit floor'ing happening there.  I
moved
> the statement to the top of this code block to help avoid confusion.
> 
> > Without the floor call, ratio should already be the correct value and
ratio+1
> is
> > not.
> 
> Consider target_fps=30, animation_fps=42.  Then:
> 
>   ratio = floor(42/30) = floor(1.4) = 1
> 
> and then we find out we want to use ratio+1 because this is false:
> 
>   std::abs(42 / 1 - 30) < std::abs(42 / 2 - 30)
> 
> In other words, we want to use ratio+1 because a sampling rate of 21 FPS is
> closer to the target of 30 than 42.  This is the desired result.

Ok, I'm officially an idiot, now I've read up on float rounding again....

Anywho, you should still be able to remove the if statement by doing:

  ratio = target_sampling_period / animation_period + 0.5;

(or by using round())

[miu, 2015-05-13 00:01:01]

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
File content/browser/media/capture/animated_content_sampler.cc (right):

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:109: if (token_bucket_
>= sampling_period_) {
On 2015/05/12 19:29:03, hubbe wrote:
> I think you're right. I was thinking that the problem would persist after a
> frame drop in (2).  I think we can still improve a little bit by initializing
> the token bucket to sampling_period_ + animation_period_/2 (or should it be
> minus instead of plus?), thus hopefully avoiding having to ever drop any
frames
> in the (2) case.

Ah, yes.  I little extra shouldn't hurt and should help prevent any initial
dropping of one frame.  Done.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:245: return
ideal_timestamp - drift / correct_over_num_frames;
On 2015/05/12 19:29:03, hubbe wrote:
> On 2015/05/09 20:57:38, miu wrote:
> > On 2015/05/06 19:53:18, hubbe wrote:
> > > Wonder if it would make sense to break out ClockSmoother from
> > > base/media/audio_shifter.cc and use here?
> > 
> > As discussed face-to-face, I'll consider this for future clean-up work.
> 
> Optional: Add a TODO?

Done.

https://codereview.chromium.org/1123623005/diff/1/content/browser/media/captu...
content/browser/media/capture/animated_content_sampler.cc:266: if
(std::abs(animation_fps / ratio - target_fps) <
On 2015/05/12 19:29:03, hubbe wrote:
> On 2015/05/09 20:57:38, miu wrote:
> > On 2015/05/06 19:53:18, hubbe wrote:
> > > I don't think you need this if statement.
> > > I think ratio is already correct. (It kind of looks like you meant to do a
> > > floor() call when you calculated ratio.)
> > 
> > |ratio| is an int64, so there's an implicit floor'ing happening there.  I
> moved
> > the statement to the top of this code block to help avoid confusion.
> > 
> > > Without the floor call, ratio should already be the correct value and
> ratio+1
> > is
> > > not.
> > 
> > Consider target_fps=30, animation_fps=42.  Then:
> > 
> >   ratio = floor(42/30) = floor(1.4) = 1
> > 
> > and then we find out we want to use ratio+1 because this is false:
> > 
> >   std::abs(42 / 1 - 30) < std::abs(42 / 2 - 30)
> > 
> > In other words, we want to use ratio+1 because a sampling rate of 21 FPS is
> > closer to the target of 30 than 42.  This is the desired result.
> 
> Ok, I'm officially an idiot, now I've read up on float rounding again....
> 
> Anywho, you should still be able to remove the if statement by doing:
> 
>   ratio = target_sampling_period / animation_period + 0.5;
> 
> (or by using round())
> 

As discussed, our mad algebra and spreadsheet skills determined the existing
solution is the better one.

[hubbe, 2015-05-13 00:03:04]

lgtm

[miu, 2015-05-13 00:05:11]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-05-13 00:05:52]

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

[commit-bot: I haz the power, 2015-05-13 01:06:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-13 01:07:26]

Patchset 4 (id:??) landed as
https://crrev.com/b6b84f4691f44c62ec0ee5f82355ad3d283d4012
Cr-Commit-Position: refs/heads/master@{#329550}