Issue metadata
Sign in to add a comment
|
Synthetic pinch gesture creates out of bounds touch events in pinch zoom benchmark |
||||||||||||||||||||||||
Issue descriptionI ran the smoothness.gpu_rasterization.tough_pinch_zoom_cases benchmark with r445851 on nexus 5 like so: tools/perf/run_benchmark run --browser=android-chromium -d smoothness.gpu_rasterization.tough_pinch_zoom_cases The first page https://www.google.com/#hl=en&q=barack+obama makes telemetry throw a WebSocketConnectionClosedException which indicates that the browser crashed. Stack trace (attached) from tombstones.py has this: [FATAL:synthetic_gesture_target_base.cc(61)] Check failed: web_touch.touches[i].state != WebTouchPoint::StatePressed || PointIsWithinContents(web_touch.touches[i].position.x, web_touch.touches[i].position.y). Touch coordinates are not within content bounds on TouchStart. From a casual reading of SyntheticTouchscreenPinchGesture::SetupCoordinatesAndStopTime it looks like it's possible for final_distance_to_anchor to be greater than 0.5 * content width which could trigger the above check.
,
Feb 7 2017
I'm trying to implement a unit test for the telemetry pinch action but I'm running into an issue.
I removed the page scale factor scaling in GpuBenchmarking::PinchBy so that it expects anchor point, delta, and speed in DIPs. If I call chrome.gpuBenchmarking.pinchBy(2, 180, 256, function() {}, 800) from devtools console with telemetry's blank.html page, I don't always get 2x page scale, more like 1.8x. Visual viewport goes from 979 x 1393 (scale 0.367) to 547 x 778 (scale 0.657). Given this, how would you write a unit test for the pinch action?
I logged the touch events generated:
Press finger 0 at (180.000000, 221.500000)
Press finger 1 at (180.000000, 290.500000)
View size (360, 512)
Move finger 0 to (180.000000, 214.185196)
Move finger 1 to (180.000000, 297.814789)
View size (360, 512)
Move finger 0 to (180.000000, 207.385193)
Move finger 1 to (180.000000, 304.614807)
View size (360, 512)
Move finger 0 to (180.000000, 199.217194)
Move finger 1 to (180.000000, 312.782806)
View size (360, 512)
Move finger 0 to (180.000000, 192.683197)
Move finger 1 to (180.000000, 319.316803)
View size (360, 512)
Move finger 0 to (180.000000, 184.347198)
Move finger 1 to (180.000000, 327.652802)
View size (360, 512)
Move finger 0 to (180.000000, 180.031601)
Move finger 1 to (180.000000, 331.968384)
View size (360, 512)
Move finger 0 to (180.000000, 173.295197)
Move finger 1 to (180.000000, 338.704803)
View size (360, 512)
Move finger 0 to (180.000000, 171.000000)
Move finger 1 to (180.000000, 341.000000)
View size (360, 512)
Release finger 0
Release finger 1
,
Feb 7 2017
Ah, IIRC, I ran into this at some point. The gesture recognizer doesn't actually start zooming until the fingers have exceeded a "slop" threshold so the distance for the gesture needs to take that slop into account. +tdresser@ who knows this more intimately than I do and did something here recently. For telemetry though, I think it's probably fine if we're off by a little here since we're just measuring relative performance.
,
Feb 7 2017
Oh, and regarding using element bounding rects, yes, <html> and <body> will have their height calculated based on their children. So if there's no children or just a few, body.clientHeight will be smaller than the viewport. Similarly, if there's overflow, body.clientHeight will be larger than the viewport. IIRC, most telemetry tests were trying to just pinch in the middle of the viewport. In that case, they shouldn't be using body or html. You could use window.innerWidth/Height instead but make sure to multiply by the pageScaleFactor
,
Feb 7 2017
Yeah, this is due to the slop region. Do we need to be able to do this precisely? You could hard code the slop region size, but that doesn't sound great...
,
Feb 7 2017
I think we do take slop region into account:
if (params_.scale_factor > 1.0f) { // zooming in
initial_distance_to_anchor = target->GetMinScalingSpanInDips() / 2.0f;
final_distance_to_anchor =
(initial_distance_to_anchor + target->GetTouchSlopInDips()) *
params_.scale_factor;
} else { // zooming out
final_distance_to_anchor = target->GetMinScalingSpanInDips() / 2.0f;
initial_distance_to_anchor =
(final_distance_to_anchor / params_.scale_factor) +
target->GetTouchSlopInDips();
}
https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/synthetic_touchscreen_pinch_gesture.cc?rcl=9c327a61afee72b188f3e80f6bfb576836b8ce1c&l=116
That calculation seems to be wrong though, shouldn't that be:
final_distance_to_anchor =
initial_distance_to_anchor * params_.scale_factor +
target->GetTouchSlopInDips();
I tried this but I still get less than 2x zoom.
I should've mentioned that the final zoom is also different every time I run pinchBy. That's more worrying IMO.
In any case, we can deal with this later. Do you have any suggestions on what kind of test will be appropriate for the pinch gesture in telemetry?
,
Feb 7 2017
Yah, I expect you shouldn't be applying page scale to the slop. I seem to recall that there was a quirk in how the recognizer actually used the slop...I think it was something along the lines of using it diagonally even if there was no horizontal difference between the touch points, but it's been a long time since I looked at it and we didn't act on it since we just copy Android's recognizer and changing it is fraught with peril. The fact that the final zoom varies is a bit worrying - we should investigate that... For tests, smoothness.tough_pinch_zoom_cases is what I've used.
,
Feb 7 2017
In terms of testing, you're referring to functional testing of the pinch gesture, correct? Ah, yeah, looks like we are trying to account for the slop region. The variability is quite concerning. Does it still happen if you increase the duration of the pinch by a lot? I'm wondering if we're missing some of the events somehow.
,
Feb 7 2017
The tests for the other gestures, e.g. drag_unittest.py, scroll_unittest.py, etc., do functional testing for the gestures, and I was hoping we could do something like that for pinch.
,
Feb 8 2017
Yeah, I think we should do something similar to scroll_unittest.py. Bokan, with the viewport stuff up in the air, what's the best way in JS to determine how much we've zoomed by?
,
Feb 8 2017
We expose pageScaleFactor and the visual viewport dimensions (taking zoom into account so equivalent to innerWidth/Height today) through gpuBenchmarking.
,
Nov 23 2017
The pinch zoom tests were disabled for the "out of bounds" issues tracked in this bug. I'm going to dup this into the bug tracking reenabling of those tests. I've investigated the accuracy issues in more depth and found a few causes. There's a patch with fixes at: https://chromium-review.googlesource.com/c/chromium/src/+/784213 |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by sunn...@chromium.org
, Jan 30 2017