Chrome version: 63

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)

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

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

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}`);
});

juncai@ Could you take a look, unfortunately I don't have Pixel 2 (XL) device (borrowed one to quickly reproduce the bug).

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.

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.

Tagging appropriately to make sure that this can be merged to M-65 which just branched.

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

Reopen this issue to prepare merging it to M-65.

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

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

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

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

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.

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

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?

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.

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).

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

I'll try that. Thanks!

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.

Created a new issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=819413