"Buttery Smooth" Tab Capture.

"Buttery Smooth" Tab Capture.

There are three main changes being made with the overall goal of
improving the smoothness of the video coming out of the tab capture
pipeline:

1. A new AnimatedContentSampler has been added that identifies which
frames were rendered due to animated content (e.g., a video or Flash
widget).  VideoCaptureOracle is then informed of exactly which subset of
frames should be captured from the compositor and provided near-perfect
frame timestamps for downstream consumers.

2. Additional plumbing in the FrameSubscriber interface to provide the
damage region of each frame.  This is used by AnimatedContentSampler.
Also, impls now provide true vsync-based presentation timestamps.

3. Raised the VideoCaptureController buffer pool count from 3 to 5.
This value is based on the logical capacity of the capture pipeline, and
not on hardware performance.  Testing revealed that more buffers are
needed to account for both multiple pipelined GPU readbacks as well as
those frames undergoing video encoding dowstream.

Testing: Tested mirroring using the usual self_mirroring.zip test
extension as well as the Google Cast extension to a Chromecast device.
Tested lock-in/out scenarios with a number of well- and ill- behaving
video playbacks.  Observed dramatic improvement in end-to-end smoothness
scores in the lab.  New unit tests to cover new code, and made sure all
relevant existing unit tests still pass.

BUG=258630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287447

[miu, 2014-07-25 07:44:35]

nick: PTAL at content/browser/media/capture/* and also
content/browser/renderer_host/media/video_capture_controller*

ccameron: PTAL at everything else (changes to DelegatedFrameHost and RWHV).

Thanks in advance!

[ccameron, 2014-07-25 17:21:53]

lgtm (with one issue, which maybe just gets a comment, cause it's on an obsolete
path).

https://codereview.chromium.org/418283003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:1743: gfx::Rect(),
I think that this should be from params.size ... but this is dead code (only
works with --disable-delegated-rendering, which will probably be deleted soon).

[miu, 2014-07-25 18:00:14]

Thanks!

https://codereview.chromium.org/418283003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:1743: gfx::Rect(),
On 2014/07/25 17:21:52, ccameron1 wrote:
> I think that this should be from params.size ... but this is dead code (only
> works with --disable-delegated-rendering, which will probably be deleted
soon).

tl;dr: I fell back on passing an empty Rect since that's the indicator for
"damage rect unknown."

Looks like params.size is the "backbuffer size" (i.e., the surface size?)
instead of the damage size.  Do correct me here if I've got that wrong. 
Unfortunately, the params struct here doesn't seem to have the needed
information (it's different than the one used on line 1790).

[ncarter (slow), 2014-07-26 00:57:58]

Overall I'm pretty excited that you've found a useful signal in the dirty-rect
stream.

Looking back on our shared experience of previous iterations of the sampling
heuristic, I do wonder if there are ways to make it (conceptually) simpler.

In particular the following parts, when taken in combination with the other
complexities in this implementation, lead to behavior I'm having trouble fully
reasoning about: 
 - the appearance of a "stopped animating for a period" bubble causes the
sampler to seize up fully. Seems more natural (to me) to ignore such periods.
 - the way that the new sampler falls back to the old sampler, with the capture
decisions of one sampler feeding into the other, is pretty hard to analyze.
 - the use of majority as a threshold, a quantity that may oscillate between
being defined and not defined under small perturbations.

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:61: // rate.
This is really clever.

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:93: typedef
std::pair<gfx::Rect, base::TimeTicks> Observation;
I'd consider making this a struct so you can use .damage_rect and .timestamp
instead of .first and .second.

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:485: }
As I look through these I think we're missing a test for when the clock moves
back in time. I think SmoothEventSampler used to do handle that gracefully (by
dumping its state when monotonicity is violated).  As far as I can tell, the new
sampler will shut down in that case.

Is the idea that the use of TimeTicks() ought to guarantee monotonicity?

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:861: 
Given how the oracle in practice drives both samplers, and the samplers could
possibly interact with each other, should there also be tests that exercise the
fused-together samplers?

For that, you might consider capturing traces in the manner of the data_points[]
type test.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:48: const int
kMaxLockInPeriodMillis = 83333;  // 12 FPS
83333 milliseconds is 83 seconds? What does 12FPS have to do with that?

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:50: // The amount of time
over which to fully correct clock drift, when computing
Do you mean http://en.wikipedia.org/wiki/Clock_drift , or are you describing a
different phenomenon? What two time counters are drifting relative to each
other?

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:99: if
(content_sampler_.ConsiderPresentationEvent(damage_rect, event_time)) {
I got confused trying to understand how the two samplers fit together, because
the two Consider functions have pretty different semantics on the return value.

I wonder if the code would be more readable if the content sampler decision
function evaluated to a tri-state enum: DONT_SAMPLE, DO_SAMPLE,
IM_CONFUSED_ASK_SOMEBODY_ELSE?

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:326: // Majority Vote
Algorithm.
Why is majority appropriate, rather than plurality value?

When you have 24fps video but 60fps animations (IIRC you'd see this on youtube
content), will we find the video?

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:339: }
This is subtle enough that I would consider:
 - Breaking it into a private static FindMajority function
 - adding a FRIEND_TEST declaration
 - Having a unit test for just the majority detection.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:361:
base::TimeDelta::FromMilliseconds(kNonAnimatingThresholdMillis)) {
Seems like anytime a bubble appears, the AnimatedContentSampler is going to
faint for a second or two (and fall back to interval based capture). Is that a
little weird? Did you consider, instead of "return false", resetting count to 1
or just excluding the current interval from the tally?

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:369: if (count <=
observations_.size() / 2)
If you're using a 50% threshold here, what happens on an input of exactly two
rect values in strict alternation? Wouldn't the majority alternate hysterically
as the window slides along?

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:379: 
Random thought: once a majority is established, you could avoid recomputing the
majority each time by tracking its frequency as items are added to and removed
from the container. Only when the frequency falls below 50% do you need to
retally. Though with list sizes of ~125, I'm pretty sure this isn't necessary.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:59: // subsets by their
reported damage Rect.  Then, the largest of the subsets, if
Could you say "most frequently occurring rect" instead of "largest of the
subsets"?

The first time I read this, I thought you were looking for whether the
largest-by-area rectangle was the most frequently painted. In fact you're not
really using the rects as rects, but as a pseudoidentifier for an animation
cause.

https://codereview.chromium.org/418283003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:45: const int
kNoOfBuffers = 5;
This carries some risk, as it seems like there's some danger of this increasing
the latency for non-tabcapture uses of the pipeline. You should probably give a
heads up to the potentially affected parties. If the encoders are set up to drop
(and not queue up) frames, then it might not be as big a deal.

If the other teams do have worries about this, it would not be hard to vary the
kNoOfBuffers depending on the MediaStreamType.

[miu, 2014-07-28 21:54:13]

No code changes yet.  Replying to some of your comments for general discussion:

> Looking back on our shared experience of previous iterations of the sampling
> heuristic, I do wonder if there are ways to make it (conceptually) simpler.

As discussed over IM, I think the class/method interfaces could be reworked a
bit to make it clearer what's going on.

I'm not sure how make the general approach simpler.  The issue is that the
general approach works well, but reveals a number of little edge issues that
have to be resolved for longer-term smooth playback (e.g. clock drift issues).

>  - the way that the new sampler falls back to the old sampler, with the
capture
> decisions of one sampler feeding into the other, is pretty hard to analyze.

This was intentional, so that transitionary periods (between being
locked-in/out) were stable and with minimal jank.  I'll see if I can make this
clearer in the code.

> ...

I replied to your other points where specific comments were being made about the
code:

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:485: }
On 2014/07/26 00:57:57, ncarter wrote:
> As I look through these I think we're missing a test for when the clock moves
> back in time. I think SmoothEventSampler used to do handle that gracefully (by
> dumping its state when monotonicity is violated).  As far as I can tell, the
new
> sampler will shut down in that case.
> 
> Is the idea that the use of TimeTicks() ought to guarantee monotonicity?

Yes.  This code was all originally written back when base::Time was being used
instead in base::TimeTicks.  (mcasas@, IIRC, replaced the data type in response
to my migration of all of src/media to base::TimeTicks.)  So, there are still
vestiges of code that deals with clock skew which shouldn't be a problem
anymore.

I think I got all the code in video_capture_oracle.cc cleaned up, w.r.t. this
issue, but I'll look over it again, and also take a closer look at the tests.

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:861: 
On 2014/07/26 00:57:58, ncarter wrote:
> Given how the oracle in practice drives both samplers, and the samplers could
> possibly interact with each other, should there also be tests that exercise
the
> fused-together samplers?

Good point.  I'll work on adding this.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:48: const int
kMaxLockInPeriodMillis = 83333;  // 12 FPS
On 2014/07/26 00:57:58, ncarter wrote:
> 83333 milliseconds is 83 seconds? What does 12FPS have to do with that?

Doh!  That should be microseconds.  ;-)

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:50: // The amount of time
over which to fully correct clock drift, when computing
On 2014/07/26 00:57:58, ncarter wrote:
> Do you mean http://en.wikipedia.org/wiki/Clock_drift , or are you describing a
> different phenomenon? What two time counters are drifting relative to each
> other?

I should update this comment/naming since there are really two things going on:

1. Clock drift: The local video hardware can/will occasionally drift w.r.t. the
system clock (TimeTicks).  My understanding is the vsync timebase/interval will
be updated in RWHV/DelegatedFrameHost to account for this.

2. "Drift" due to frame timestamp rewriting: The frame timestamps are rewritten
based on the detected animation rate.  That's based on an average over the past
1000-2000 ms which, being an estimation, can introduce significant error over
long periods of time.  "error" here refers to the frame timestamps drifting too
far off from the raw event_times from the compositor.  In other words, the logic
is designed to gently push the rewritten frame timestamps towards the
event_times.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:326: // Majority Vote
Algorithm.
On 2014/07/26 00:57:58, ncarter wrote:
> Why is majority appropriate, rather than plurality value?

I thought about this for a while, and I'm not sure it makes a huge difference
either way.  In typical cases, the plurality value is very likely to be the same
as the majority.  I'm inclined to use the majority because:

1. Using the majority forces the code to estimate the animation rate with more
data points, improving accuracy.  Likewise, lock-in only happens when there is
more certainty about what we really should be locking in to.

2. It's a lot simpler (both code and run-time complexity) to find a majority
than a plurality.

> When you have 24fps video but 60fps animations (IIRC you'd see this on youtube
> content), will we find the video?

This is an excellent point.  I thought about this for a while as well, and I
think I should modify the heuristics to be weighted on "number of pixels
changed."  Meaning, for example:

Video: 24 FPS * 720 * 480 = 8.3 million pixels/sec
Spinner: 60 FPS * 96 * 96 = 0.55 million pixels/sec

This also jives with my intuition as well, since our human perception of
smoothness is very much related to how much of the video frames are in motion. 
Think of a spinner running at 24 FPS, which we would perceive as smooth, but a
large 24 FPS video panning over a nature scene like a canyon landscape will look
very flickery.

I'll try out this idea and get back to you...

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:361:
base::TimeDelta::FromMilliseconds(kNonAnimatingThresholdMillis)) {
On 2014/07/26 00:57:58, ncarter wrote:
> Seems like anytime a bubble appears, the AnimatedContentSampler is going to
> faint for a second or two (and fall back to interval based capture). Is that a
> little weird? Did you consider, instead of "return false", resetting count to
1
> or just excluding the current interval from the tally?

Good catch.  Yes, we should be locking into the most recent continuous sequence,
rather than requiring all event history to match.  That was my intention here. 
I'll fix this.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:369: if (count <=
observations_.size() / 2)
On 2014/07/26 00:57:58, ncarter wrote:
> If you're using a 50% threshold here, what happens on an input of exactly two
> rect values in strict alternation? Wouldn't the majority alternate
hysterically
> as the window slides along?

Yes.  The harm would be in the flip-flopping between which subset of the frames
we lock in to (the estimated animation period would be the same).

The idea I have to make all this pixel-weighted should make such a situation
less likely to occur, but two identically-sized videos playing on the same
screen would still cause the problem.

The simplest solution may be to require, say, a >75% majority.  I'll have to run
some experiments...

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:379: 
On 2014/07/26 00:57:58, ncarter wrote:
> Random thought: once a majority is established, you could avoid recomputing
the
> majority each time by tracking its frequency as items are added to and removed
> from the container. Only when the frequency falls below 50% do you need to
> retally. Though with list sizes of ~125, I'm pretty sure this isn't necessary.

At one point, I tried a more "online" algorithm and, as you noted, it was a lot
more code complexity with very little run-time benefit.

[miu, 2014-07-31 01:25:33]

nick: Iterated.  ;-)  PTAL.  If it looks like I'm not addressing any one comment
completely here, it might be because I discussed it in the last e-mail:

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:93: typedef
std::pair<gfx::Rect, base::TimeTicks> Observation;
On 2014/07/26 00:57:57, ncarter wrote:
> I'd consider making this a struct so you can use .damage_rect and .timestamp
> instead of .first and .second.

Done.

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/418283003/diff/20001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:861: 
On 2014/07/26 00:57:58, ncarter wrote:
> Given how the oracle in practice drives both samplers, and the samplers could
> possibly interact with each other, should there also be tests that exercise
the
> fused-together samplers?

Done.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:48: const int
kMaxLockInPeriodMillis = 83333;  // 12 FPS
On 2014/07/26 00:57:58, ncarter wrote:
> 83333 milliseconds is 83 seconds? What does 12FPS have to do with that?

Done.  I meant microseconds.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:50: // The amount of time
over which to fully correct clock drift, when computing
On 2014/07/26 00:57:58, ncarter wrote:
> Do you mean http://en.wikipedia.org/wiki/Clock_drift , or are you describing a
> different phenomenon? What two time counters are drifting relative to each
> other?

Done.  Documented this with code comments where this constant is used.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:99: if
(content_sampler_.ConsiderPresentationEvent(damage_rect, event_time)) {
On 2014/07/26 00:57:58, ncarter wrote:
> I got confused trying to understand how the two samplers fit together, because
> the two Consider functions have pretty different semantics on the return
value.

I made this clearer by using the same API for both, and I've separated out the
"consider event" from the "should sample" method calls.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:339: }
On 2014/07/26 00:57:58, ncarter wrote:
> This is subtle enough that I would consider:
>  - Breaking it into a private static FindMajority function
>  - adding a FRIEND_TEST declaration
>  - Having a unit test for just the majority detection.

Done.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:361:
base::TimeDelta::FromMilliseconds(kNonAnimatingThresholdMillis)) {
On 2014/07/26 00:57:58, ncarter wrote:
> Seems like anytime a bubble appears, the AnimatedContentSampler is going to
> faint for a second or two (and fall back to interval based capture). Is that a
> little weird? Did you consider, instead of "return false", resetting count to
1
> or just excluding the current interval from the tally?

Done.  Fixed this by iterating |observations_| backwards, and breaking from the
loop at the first non-animating interval.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:369: if (count <=
observations_.size() / 2)
> On 2014/07/26 00:57:58, ncarter wrote:
> > If you're using a 50% threshold here, what happens on an input of exactly
two
> > rect values in strict alternation? Wouldn't the majority alternate
> hysterically
> > as the window slides along?

I looked at this all again.  While the resulting |detected_period_| won't change
during the flip-flop, the frame selection will.  Thus, I've imposed a 2/3
majority requirement, which seems to work well.  Also, I added some unit testing
to confirm the desired behavior.

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:59: // subsets by their
reported damage Rect.  Then, the largest of the subsets, if
On 2014/07/26 00:57:58, ncarter wrote:
> Could you say "most frequently occurring rect" instead of "largest of the
> subsets"?
> 
> The first time I read this, I thought you were looking for whether the
> largest-by-area rectangle was the most frequently painted. In fact you're not
> really using the rects as rects, but as a pseudoidentifier for an animation
> cause.

Done.  I rewrote much of the comment to make the whole purpose and overall
behavior of AnimatedContentSampler clearer.

https://codereview.chromium.org/418283003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/418283003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:45: const int
kNoOfBuffers = 5;
On 2014/07/26 00:57:58, ncarter wrote:
> This carries some risk, as it seems like there's some danger of this
increasing
> the latency for non-tabcapture uses of the pipeline.

Done.  I've eliminated the risk by implementing the
buffer-pool-size-per-MediaStreamType solution you mentioned.

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:49: return
base::TimeTicks() + base::TimeDelta::FromSeconds(1);
I changed this because, while debugging, I noticed the TimeTicks internal values
were negative which could cause TimeTicks to "cross null" (i.e., cross zero).  I
think this is a good solution to ensure a deterministic starting point.

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:82: }  //
namespace
Note: I moved the TEST() code out of the anonymous namespace throughout this
module because I needed to make AnimatedContentSamplerTest a private friend of
AnimatedContentSampler.

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:795:
EXPECT_NEAR(expected_drop_rate, actual_drop_rate, 0.015);
I raised the error threshold because I lowered the total number of events the
tests were chugging through.  This was all in response to noticing how slow all
these tests were running in a debug build.

[ncarter (slow), 2014-08-01 23:36:39]

lgtm, only nits

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:103: should_sample =
smoothing_sampler_.should_sample();
Much better, thanks!

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:231: << "Content changed;
capture will resume.";
I think these VLOGS should probably either be updated or simply go away, since
the decision of the smoothsampler is no longer final.

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:85: bool has_proposal()
const { return detected_period_ > base::TimeDelta(); }
The Chrome style guide says that inline unix_hacker() methods should only be for
simple accessors. Although a lot of the methods here have simple bodies, I'm not
sure I'd consider all of them accessors?  Ultimately your call.

http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:49: return
base::TimeTicks() + base::TimeDelta::FromSeconds(1);
On 2014/07/31 01:25:33, miu wrote:
> I changed this because, while debugging, I noticed the TimeTicks internal
values
> were negative which could cause TimeTicks to "cross null" (i.e., cross zero). 
I
> think this is a good solution to ensure a deterministic starting point.

I don't follow the problem you fixed -- how would timeticks wind up with a value
greater than 2^63 microseconds? And if the values went negative in the test,
what's to prevent them from going negative in real life?

Just be sure you're not papering over a real bug.

[miu, 2014-08-04 18:46:05]

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.cc:231: << "Content changed;
capture will resume.";
On 2014/08/01 23:36:38, ncarter wrote:
> I think these VLOGS should probably either be updated or simply go away, since
> the decision of the smoothsampler is no longer final.

Done.  Yeah, even as debugging aids, they're no longer useful.

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle.h:85: bool has_proposal()
const { return detected_period_ > base::TimeDelta(); }
On 2014/08/01 23:36:39, ncarter wrote:
> The Chrome style guide says that inline unix_hacker() methods should only be
for
> simple accessors. Although a lot of the methods here have simple bodies, I'm
not
> sure I'd consider all of them accessors?  Ultimately your call.
> 
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

Done.  tl;dr: Chromium style is right, but for the wrong reasons.

In the past, I've been of the opinion that inlining is a good thing whenever it
results in smaller code generated on the CALLING side.  Meaning, some code
calling a non-inlined has_proposal() would involve saving registers to memory,
invoking an expensive "jump" instruction, stack pointer adjustments, ..., and
then unwinding all of that (including a "jump-return" instruction).  This is all
much more expensive than the few instructions to load and compare an integer to
zero.

However, optimizing compilers are much much better these days.  They (or the
linker) can decide when to inline functions automatically, even those functions
not explicitly inlined.  Also, the C++ standard says the use of inlining in a
header file is only a signal to the linker to expect multiple definitions of the
same function (which would normally trigger an error), and nothing more.

For more, I found this discussion enlightening:
http://stackoverflow.com/questions/1443982/when-do-compilers-inline-c-code

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
File content/browser/media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/418283003/diff/80001/content/browser/media/ca...
content/browser/media/capture/video_capture_oracle_unittest.cc:49: return
base::TimeTicks() + base::TimeDelta::FromSeconds(1);
On 2014/08/01 23:36:39, ncarter wrote:
> On 2014/07/31 01:25:33, miu wrote:
> > I changed this because, while debugging, I noticed the TimeTicks internal
> values
> > were negative which could cause TimeTicks to "cross null" (i.e., cross
zero). 
> 
> Just be sure you're not papering over a real bug.

Sorry, I explained this the wrong way.

What I meant was, the impl often uses TimeTicks::is_null() to determine whether
the value has not yet been set.  Internally, this is implemented as "is the
internal int64 equal to zero."

The problem I'm solving here is that TimeTicks, in a normal operating
environment, should never be negative.  So, our tests starting with a calendar
day in the past is problematic because negative TimeTicks values would be
generated from that, and that means incrementing forward could result in landing
on zero, the "null" value.

So, no, not papering over a real bug.  Just preventing false negatives from the
tests.

[miu, 2014-08-04 18:46:49]

avi: Need OWNERS stamp for changes to
content/public/browser/render_widget_host_view_frame_subscriber.h

[Avi (use Gerrit), 2014-08-04 18:48:03]

content/public/browser/render_widget_host_view_frame_subscriber.h lgtm

[miu, 2014-08-04 18:49:17]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-08-04 18:50:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/418283003/100001

[commit-bot: I haz the power, 2014-08-04 21:53:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-05 04:15:13]

Change committed as 287447