New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment
link

Issue 796518: DeviceMotionEvent.acceleration returns only NULL values on mobile Chrome

Reported by kenneth....@gmail.com, Dec 20 2017

Issue description

Steps to reproduce the problem:
Run https://kenchris.github.io/sensor-polyfills/run-tests.html

DeviceMotionEvent.acceleration.{x, y, z} are equal to null, even thought Android APIs Sensor.TYPE_LINEAR_ACCELERATION and Sensor.TYPE_GRAVITY work fine. (try one of the sensor testing apps).

This is on the Google Pixel 2.

I see other people complaining https://productforums.google.com/forum/#!msg/chrome/ezXIXO7nLa4/m2BGgbvHAAAJ

What is the expected behavior?
.acceleration.{x, y, z} should expose acceleration values with gravity using Sensor.TYPE_LINEAR_ACCELERATION below.

What went wrong?
Got null

Did this work before? N/A 

Chrome version: 64  Channel: n/a
OS Version: 8.1
Flash Version:
 

Comment 1 by kenneth....@gmail.com, Dec 20 2017

Chrome version: 63

Comment 2 by alexande...@intel.com, Dec 20 2017

Cc: reillyg@chromium.org mikhail....@intel.com
Components: Blink>Sensor
Owner: juncai@chromium.org
juncai@ Were there any changes recently in DevMotion code?

kenneth.christiansen@ what about LinearAccelerationSensor, does it work if you enable (chrome://flags/#enable-generic-sensor)

Comment 3 by kenneth....@gmail.com, Dec 20 2017

Tested in Chrome Canary (65) and it is also broken there.

Comment 4 by kenneth....@gmail.com, Dec 20 2017

Yes, I tested in 65 with LinearAccelerationSensor and it works fine (with flags enabled)

Comment 5 by alexande...@intel.com, Dec 20 2017

kenneth.christiansen@ I quickly tested (snippet below) with 63.0.3239.111 and 65.0.3288.0. Works as expected with Pixel XL and doesn't work with Pixel 2 XL.

window.addEventListener('devicemotion', function(event) {
  console.log(`Acceleration is ${event.accelerationIncludingGravity.z} m/s2, LinearAcceleration ${event.acceleration.z}`);
});

Comment 6 by alexande...@intel.com, Dec 20 2017

Status: Untriaged (was: Unconfirmed)

Comment 7 by alexande...@intel.com, Dec 20 2017

Cc: juncai@chromium.org
Owner: ----
Status: Available (was: Untriaged)
juncai@ Could you take a look, unfortunately I don't have Pixel 2 (XL) device (borrowed one to quickly reproduce the bug).

Comment 8 by reillyg@chromium.org, Dec 21 2017

Owner: juncai@chromium.org
Status: Assigned (was: Available)
Confirmed on my Pixel 2 that the Linear Accelerometer works both through an Android test app and the Generic Sensors API in Chrome but the DeviceMotionEvent.acceleration fields are not being populated.

Comment 9 by juncai@chromium.org, Jan 5 2018

Status: Started (was: Assigned)

Comment 10 by juncai@chromium.org, Jan 23 2018

After some debugging, I found the device motion event linear acceleration sensor is not initialized because the device sensor event pump requests 60Hz frequency:
https://cs.chromium.org/chromium/src/content/renderer/device_sensors/device_sensor_event_pump.h?l=39
But on Pixel 2, the following function returns false:
https://cs.chromium.org/chromium/src/services/device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java?l=189
because the |mMinDelayUsec| is 20000, which converts to frequency is 50Hz, so the linear acceleration sensor is not initialized.

I prepared a CL:
https://chromium-review.googlesource.com/c/chromium/src/+/879586
to lower the requested frequency by half each time sensor->AddConfiguration() fails until it tries the minimum frequency 1Hz, and if that fails, the sensor will be marked as not initialized.

Comment 11 by reillyg@chromium.org, Jan 25 2018

Labels: -Type-Bug M-65 Type-Bug-Regression
Tagging appropriately to make sure that this can be merged to M-65 which just branched.

Comment 12 by bugdroid1@chromium.org, Jan 26 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f77dd4080f61d859e741f16af53f6a95a84a2489

commit f77dd4080f61d859e741f16af53f6a95a84a2489
Author: Jun Cai <juncai@chromium.org>
Date: Fri Jan 26 18:24:32 2018

[DeviceOrientation] Clamp to maximum supported frequency

Currently, the device motion/orientation event pump requests the sensor
data reporting frequency to be 60Hz. However on some devices, this
frequency is larger than the maximum frequency the device can provide.
This CL modifies device event pump code to set the sensor frequency
not exceed the maximum frequency the platform supports.

Bug:  796518 
Change-Id: I1cd6215f3b35498482ed3ca26cd9b44d4eb7eb1e
Reviewed-on: https://chromium-review.googlesource.com/879586
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532022}
[modify] https://crrev.com/f77dd4080f61d859e741f16af53f6a95a84a2489/content/renderer/device_sensors/device_sensor_event_pump.h

Comment 13 by juncai@chromium.org, Jan 26 2018

Status: Fixed (was: Started)

Comment 14 by juncai@chromium.org, Jan 26 2018

Status: Started (was: Fixed)
Reopen this issue to prepare merging it to M-65.

Comment 15 by juncai@chromium.org, Jan 26 2018

I tested the above fix on the M-65 branch in Pixel 2, and DeviceMotionEvent.acceleration works.

Comment 16 by juncai@chromium.org, Jan 26 2018

Labels: Merge-Request-65

Comment 17 by sheriffbot@chromium.org, Jan 27 2018

Project Member
Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 18 by bugdroid1@chromium.org, Jan 29 2018

Project Member
Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b29ba0d062280291845dd8a5bd9dc052e439bbb5

commit b29ba0d062280291845dd8a5bd9dc052e439bbb5
Author: Jun Cai <juncai@chromium.org>
Date: Mon Jan 29 18:33:22 2018

[DeviceOrientation] Clamp to maximum supported frequency

Currently, the device motion/orientation event pump requests the sensor
data reporting frequency to be 60Hz. However on some devices, this
frequency is larger than the maximum frequency the device can provide.
This CL modifies device event pump code to set the sensor frequency
not exceed the maximum frequency the platform supports.

TBR=juncai@chromium.org

(cherry picked from commit f77dd4080f61d859e741f16af53f6a95a84a2489)

Bug:  796518 
Change-Id: I1cd6215f3b35498482ed3ca26cd9b44d4eb7eb1e
Reviewed-on: https://chromium-review.googlesource.com/879586
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#532022}
Reviewed-on: https://chromium-review.googlesource.com/891678
Reviewed-by: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#143}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/b29ba0d062280291845dd8a5bd9dc052e439bbb5/content/renderer/device_sensors/device_sensor_event_pump.h

Comment 19 by juncai@chromium.org, Jan 29 2018

On Gogle Pixel 2 I tested the branch 3325 after the above merge, the DeviceMotionEvent.acceleration works as expected.

Comment 20 by juncai@chromium.org, Jan 29 2018

Status: Fixed (was: Started)

Comment 21 by reillyg@chromium.org, Mar 2 2018

Status: Assigned (was: Fixed)
While investigating  issue 818290  I noticed that this is still reproducible on my Pixel 2 with https://code.viget.com/device-motion-demo/. I only get data for accelerationIncludingGravity.

Comment 22 by juncai@chromium.org, Mar 3 2018

It is related to the  issue 805146  which needs to be merged to M-65 branch. I reopened that issue and added merge request.

Comment 23 by dustin.k...@gmail.com, Mar 6 2018

As has been discussed on  issue 805146 , the fix for 796518 causes a regression in the sensor readout for an unknown but likely large number of devices for the deviceOrientation API (in use by many more websites than compared to deviceMotion).

Try this link - https://curly-age.glitch.me and keep your phone moving (or else the values will drift to 0) and note the 'events per second'. Here are the results for the following devices:

Pixel XL    12hz
Moto Z      60hz
Galaxy S6   5hz

On Chrome m64 all of these devices report 60hz.

As M65 is going stable today, it seems like we may be a little too late to change/revert this fix. As such, should I create a new separate Chromium issue to track this?

Comment 24 by reillyg@chromium.org, Mar 6 2018

According to the logic in DeviceSensorEventPump::DidStartIfPossible the timer that fires 'deviceorientation' and 'devicemotion' events is always set to fire at 60 Hz regardless of the actual rate at which the browser process will be updating the sensor reading buffer. However, for 'deviceorientation' events we first check that IsSignificantlyDifferent returns true for the new sensor reading which means an event won't be fired if the data in the buffer is the same.

If Sensor.getMinDelay() is returning a bogus value then that could explain the difference. The old sensor code didn't check what the hardware supported and simply requested 60 Hz regardless.

Comment 25 by dustin.k...@gmail.com, Mar 6 2018

Yeah, I believe it's returning a bogus value. It's easy to see from deviceOrientation demos (https://threejs.org/examples/?q=dev#misc_controls_deviceorientation) that the M64 code not only fires at 60hz, but indeed has the sensor granularity to support the 60hz output, ie. the demos are smooth in M64 but very choppy in M65 (for the devices that report bogus values Sensor.getMinDelay() - which in my limited testing is 2 out of the 3 devices I have).

Comment 26 by reillyg@chromium.org, Mar 6 2018

juncai@, it would be worth experimenting with ignoring Sensor.getMinDelay() and hard-coding the maximum supported frequency to 60 Hz on the browser side.

Comment 27 by juncai@chromium.org, Mar 6 2018

I'll try that. Thanks!

Comment 28 by juncai@chromium.org, Mar 6 2018

Status: Started (was: Assigned)

Comment 29 by reillyg@chromium.org, Mar 6 2018

When you're ready to land a patch for this please create a new issue so we can track the merge request more easily. The issue in the description has been resolved, we just introduced a new issue in the process. Feel free to mark this bug as Fixed again when you do that.

Comment 30 by juncai@chromium.org, Mar 6 2018

Status: Fixed (was: Started)
Created a new issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=819413

Sign in to add a comment