Telemtry: hooks "chrome.gpuBenchmarking.smoothScrollBy" with java on android.

Telemtry: hooks "chrome.gpuBenchmarking.smoothScrollBy" with java on android.

In order to provide more realistic benchmarks, instead of shortcutting in native via mousewheels,
let Telemetry cross all the boundaries and synthesize MotionEvents all the way up in java.

BUG=
TEST=./tools/perf/run_multipage_benchmarks --browser=system smoothness_benchmark tools/perf/page_sets/key_mobile_sites.json


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

[bulach, 2012-11-30 14:21:02]

ptal and let me know if this is the right way :)

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:58:
// TODO(nduca) from render_widget_host_view_base.cc.
I think we can use  "ValueAnimator" here (which I think uses vsync) and then
this "forwardInputEvents" would just return true whilst the "animation" is
running... wdyt?


http://developer.android.com/reference/android/animation/ValueAnimator.html

[Sami, 2012-11-30 15:45:52]

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:50:
long eventTime = SystemClock.uptimeMillis();
This value should also come from the animation framework.

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:54:
// We can only "fling" until the "target" hits the edge of the screen.
Nit: this isn't flinging, just scrolling :)

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:58:
// TODO(nduca) from render_widget_host_view_base.cc.
On 2012/11/30 14:21:02, bulach wrote:
> I think we can use  "ValueAnimator" here (which I think uses vsync) and then
> this "forwardInputEvents" would just return true whilst the "animation" is
> running... wdyt?
> 
> 
> http://developer.android.com/reference/android/animation/ValueAnimator.html

I think ValueAnimator is a bit overkill here because it wants to calculate the
interpolated animation value for every frame and works in terms of milliseconds
instead of individual ticks. Let's go with View.postOnAnimation() like we talked
offline -- and perhaps View.postDelayed() or just by running this existing code
on ICS. Doing that should have the input events delivered right before the vsync
callback fires.

[bulach, 2012-11-30 16:30:24]

thanks sami! another patchset up, ptal!

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:50:
long eventTime = SystemClock.uptimeMillis();
On 2012/11/30 15:45:53, Sami wrote:
> This value should also come from the animation framework.

I'm using postOnAnimation as suggested, but that only takes a Runnable, not sure
how to get this?

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:54:
// We can only "fling" until the "target" hits the edge of the screen.
On 2012/11/30 15:45:53, Sami wrote:
> Nit: this isn't flinging, just scrolling :)

:) reworded with the latest patch..

https://codereview.chromium.org/11415227/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:58:
// TODO(nduca) from render_widget_host_view_base.cc.
On 2012/11/30 15:45:53, Sami wrote:
> On 2012/11/30 14:21:02, bulach wrote:
> > I think we can use  "ValueAnimator" here (which I think uses vsync) and then
> > this "forwardInputEvents" would just return true whilst the "animation" is
> > running... wdyt?
> > 
> > 
> > http://developer.android.com/reference/android/animation/ValueAnimator.html
> 
> I think ValueAnimator is a bit overkill here because it wants to calculate the
> interpolated animation value for every frame and works in terms of
milliseconds
> instead of individual ticks. Let's go with View.postOnAnimation() like we
talked
> offline -- and perhaps View.postDelayed() or just by running this existing
code
> on ICS. Doing that should have the input events delivered right before the
vsync
> callback fires.

done! thanks for the pointers!

[Sami, 2012-11-30 17:00:09]

On 2012/11/30 16:30:24, bulach wrote:
> I'm using postOnAnimation as suggested, but that only takes a Runnable, not
sure
> how to get this?

I'm sorry, you're right. While Choreographer does provide the animation time, it
isn't propagated to that callback. TimeAnimator and its TimeListener should both
call us at the right time and give us the timestamp.

[nduca, 2012-11-30 18:45:50]

Get hartmanng  and vollick to review, they're in the midst of tryign to make
this clean  on desktop  (https://codereview.chromium.org/11416283/ and
https://codereview.chromium.org/11413199/)

[bulach, 2012-11-30 19:05:04]

now commenting in the correct patch :)

thanks for the pointers nat!
vollick / hartmanng: 
in this patch, we're hooking up with java's TimeAnimator, which is supposed to
be called just prior to vsync.. I can try to use the linearization mechanism
from https://codereview.chromium.org/11416283/, but perhaps it may be nicer to
land all this plumbing first... please let me know your thoughts!

[Sami, 2012-12-03 12:34:12]

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:67:
mCurrentY += mScrollDown ? -10 : 10;
This can take us beyond the edge of the screen if the distance between the
starting point and the edge wasn't divisible by 10. Maybe we could assume that
the starting point is always at least 10 pixels from the edge (and assert that),
and check the condition below to:

  if (mCurrentY <= 10 || mCurrentY >= mContentViewCore.getHeight() - 10)

That's probably a few 10s too much so you might want to make that a named
constant.

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:103:
mPixelsToScroll = pixelsToScroll;
As far as I can see, pixelsToScroll, mouseEventX and mouseEventY are all in CSS
pixels as calculated by scroll.js. Touch gestures should be in logical pixels,
so I think we should multiply these values by mContentViewCore.getScale().

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:108:
@Override
Maybe it's the review tool but it looks like there's a double indent here.

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:112:
}
Should we call mTimeAnimator.end() here? Right now looks like the same
SmoothScroller can't be started again, but that's by design, right?

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:125:
@Override
Double indent here too.

[bulach, 2012-12-03 15:47:02]

thanks sami! comments addressed, another look please?

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:67:
mCurrentY += mScrollDown ? -10 : 10;
On 2012/12/03 12:34:12, Sami wrote:
> This can take us beyond the edge of the screen if the distance between the
> starting point and the edge wasn't divisible by 10. Maybe we could assume that
> the starting point is always at least 10 pixels from the edge (and assert
that),
> and check the condition below to:
> 
>   if (mCurrentY <= 10 || mCurrentY >= mContentViewCore.getHeight() - 10)
> 
> That's probably a few 10s too much so you might want to make that a named
> constant.

several good points here, thanks! all done.

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:103:
mPixelsToScroll = pixelsToScroll;
On 2012/12/03 12:34:12, Sami wrote:
> As far as I can see, pixelsToScroll, mouseEventX and mouseEventY are all in
CSS
> pixels as calculated by scroll.js. Touch gestures should be in logical pixels,
> so I think we should multiply these values by mContentViewCore.getScale().

Done.

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:108:
@Override
On 2012/12/03 12:34:12, Sami wrote:
> Maybe it's the review tool but it looks like there's a double indent here.

Done.

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:112:
}
On 2012/12/03 12:34:12, Sami wrote:
> Should we call mTimeAnimator.end() here? Right now looks like the same
> SmoothScroller can't be started again, but that's by design, right?

yeah, I split this into "createJBRunnable" and "createPreJBRunnable", I hope
it's clearer...

neither of them need to be a member, and they're only created once and can't be
re-started.. let me know if it's any clearer..

https://codereview.chromium.org/11415227/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:125:
@Override
On 2012/12/03 12:34:12, Sami wrote:
> Double indent here too.

Done.

[Sami, 2012-12-03 16:02:54]

Thanks Marcus, I think this is looking very good.

https://codereview.chromium.org/11415227/diff/10001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:68:
mAccumulated += SCROLL_DELTA;
mAccumulated should only be updated if canMove == true, right? Otherwise we'll
undershoot.

[bulach, 2012-12-03 17:40:42]

thanks sami! comment addressed, another look please?

https://codereview.chromium.org/11415227/diff/10001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/10001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:68:
mAccumulated += SCROLL_DELTA;
On 2012/12/03 16:02:55, Sami wrote:
> mAccumulated should only be updated if canMove == true, right? Otherwise we'll
> undershoot.

good point! done..

[Sami, 2012-12-03 17:45:11]

lgtm, thanks Marcus!

[nduca, 2012-12-04 03:40:39]

Sorry, i think more work needed.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:16:
import org.chromium.base.CalledByNative;
Is there a way to write this so that the actual curve code lives next to the
render_widgeT_host_view_base code in c++ and then delegates to the view for even
injection and timing? I think that we should have one place where we do curve
interpolation --- and if you look at it, the curve interpolation here is the
same as the regular one does.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:69:

i'm worried that you're not correcting by time here. what happens if the main
thread hitches for a bit? you'll scroll down by a constant value rather than
giving the appearance of continuous velocity.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:74:
} else if (mCurrentY >= mContentViewCore.getHeight()) {
why are you doing this? if i tell it to scroll 1000 pixels, then send 1000
pixels of scrolls to the renderer. The renderer wont explode in this case...

If you have some custom handling to do if it stops scrolling early, then we can
modify the scroll ack to say the scroll didn't happen. But I'm skeptical that
that needs to be done.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:130:
mTimeAnimator.setTimeListener(new TimeListener() {
is this a realistic time to inject input? Does vsync come *before* input or
*after* input on JB, i forget? make sure to match it.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:149:
if (mEventStreamSender.sendEvents(SystemClock.uptimeMillis())) {
how often is this? When we dont have rAF, the other system ticks at 2*vsync. Not
sure thats better but we should be disciplined in what we do here.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:160:
mStartTime = SystemClock.uptimeMillis();
is this the right value to use? not knowing js i just dont know. We typically
animate usnig base::TimeTicks::HighResNow. Whatever that plumbs to probably
should be used here.

[bulach, 2012-12-04 10:25:41]

thanks nat! inline.

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
(right):

https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:16:
import org.chromium.base.CalledByNative;
On 2012/12/04 03:40:40, nduca wrote:
> Is there a way to write this so that the actual curve code lives next to the
> render_widgeT_host_view_base code in c++ and then delegates to the view for
even
> injection and timing? I think that we should have one place where we do curve
> interpolation --- and if you look at it, the curve interpolation here is the
> same as the regular one does.

hmm... that's a good question.. afaict, we want the "Tick" callback to be
generated via this TimeAnimator up here, my understanding (sami, please correct
me!) is that it's called   right before the vsync at a similar point as the
input events would be sent...

I can propagate this callback down, then let the C++ interpolate the curve, then
pass up to java again, which will then go back to the c++ side... :) let me give
it a shot.. the other comments may disappear with this new design, so I'll reply
to them later.

[nduca, 2012-12-04 10:38:01]

Totally cool if the bouncing-back-and-forth is in your estimation a nightmare.
The biggest comment I've got is that we've actually had a number of
interpolation/etc bugs in this code, and rewritten it twice for better accuracy,
so it might be worth sharing so we dont get android-specific issues.

[Sami, 2012-12-04 10:41:25]

On 2012/12/04 10:25:41, bulach wrote:
https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
> File
>
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java
> (right):
> 
>
https://codereview.chromium.org/11415227/diff/6002/content/public/android/jav...
>
content/public/android/java/src/org/chromium/content/browser/SmoothScroller.java:16:
> import org.chromium.base.CalledByNative;
> On 2012/12/04 03:40:40, nduca wrote:
> > Is there a way to write this so that the actual curve code lives next to the
> > render_widgeT_host_view_base code in c++ and then delegates to the view for
> even
> > injection and timing? I think that we should have one place where we do
curve
> > interpolation --- and if you look at it, the curve interpolation here is the
> > same as the regular one does.
> 
> hmm... that's a good question.. afaict, we want the "Tick" callback to be
> generated via this TimeAnimator up here, my understanding (sami, please
correct
> me!) is that it's called   right before the vsync at a similar point as the
> input events would be sent...

Right, Android provides 3 callbacks related to vsync: input processing,
animation and view traversal/rendering. AFAIK there's no way to inject events to
the first stage (without root), so the next best thing is to use TimeAnimator to
get called at animation time. Since that also happens before the actual vsync
event, the renderer gets the events at the correct time and the rest of
rendering proceeds normally.

[bulach, 2012-12-04 12:52:31]

ok, here's my overall findings so far, let me know if they make sense :)

1. finger scrolling is a bit different from mouse wheel, in the sense that it
can't go "offscreen"... as in, when a "real finger" :) does this stream (down,
move, reach edge, up, repeat), the subsequent "down" may trigger all sorts of
hit detection / touch prevention... so I think we need to keep this logic if we
want to synthesize a realistic event stream?

2. the curve interpolation you mention is the one in
BasicMouseWheelSmoothScrollGesture::ForwardInputEvents?
if so, I think it'd be an overkill to try to go up and down java just to reuse
those few lines...

and now for the questions :)
1. the API is "BeginSmoothScrool", and given how mouse wheel and finger drag are
treated differently, I think it's better to keep to keep their implementations
separately...

2. there's no much commonality except that both expose a "Tick"; but even that
it's triggered by different paths (mouse wheel can handle with c++ messages,
android post JB via TimeAnimator)..

all in all, I can't think of a design that would be simpler / provide better
cases than the current one, but would be happy to hear your thoughts!

[bulach, 2012-12-04 16:37:06]

forget all the above... :)
new update!

1. there was a bug (many thanks sami!!) in cc/layer_tree_host_impl.cc that was
interfering with this events... that's what led me to believe it had to have a
UP / DOWN before goung offscreen... it doesn't seem to be the case once this bug
is fixed!

2. so I can simplify all of this in order to just use the timing from java, and
send the events up there again.... 
(the benchmark itself would then be responsible to not "overscroll" and restart
the "down" / "move" / "up" in a more realistic manner...)

how does it sound?

[nduca, 2012-12-04 17:07:37]

hehe having fun yet? :)

This sounds good. The key part is sharing code around the "how much should the
scroll_update have this tick" with the "how much should the mousewheel have this
tick."

[bulach, 2012-12-04 19:19:04]

aw, snap... :{ I have to pack and will be OOO flying tomorrow (but in will try
to catch up with nat on thu / fri)..

the latest patch is NOT ready yet, and there's no rush to review, but please
take a moment to check the overall interaction with the java side, and the split
with TimeTracker...

it's not working :( but I'll get back to it tomorrow afternoon in some crazy
timezone... :)

[bulach, 2012-12-10 13:00:00]

apologies for the delay, the trip last week was a bit unexpected, but it was
great to meet you all! :)
vollick, thanks for the basic_mouse_wheel_smooth_scroll_gesture.h, it helped to
simplify this patch.

sami's fix has landed too, and yay, this pipeline is now working fine!

would you all please take a look?

[nduca, 2012-12-10 17:27:27]

lgtm. @vollick for final review.

[Sami, 2012-12-10 18:43:52]

Thanks Marcus, lgtm!

[Ian Vollick, 2012-12-11 00:30:48]

On 2012/12/10 18:43:52, Sami wrote:
> Thanks Marcus, lgtm!

Great change! lgtm2

[commit-bot: I haz the power, 2012-12-11 10:41:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11415227/14001

[commit-bot: I haz the power, 2012-12-11 10:41:54]

Presubmit check for 11415227-14001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/browser/renderer_host
    content/port

Presubmit checks took 2.3s to calculate.

[bulach, 2012-12-11 11:32:19]

+avi for OWNERS on content/

[bulach, 2012-12-14 12:23:00]

avi: ping? :)

[Avi (use Gerrit), 2012-12-14 16:12:05]

rubber stamp lgtm

[commit-bot: I haz the power, 2012-12-14 16:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11415227/14001

[commit-bot: I haz the power, 2012-12-14 16:31:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-14 16:38:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11415227/30002

[commit-bot: I haz the power, 2012-12-14 16:54:06]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-14 17:28:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11415227/26004

[commit-bot: I haz the power, 2012-12-14 22:41:18]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-17 11:40:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11415227/26004

[commit-bot: I haz the power, 2012-12-17 13:33:32]

Change committed as 173457