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

Issue 819413 link

Starred by 5 users

Issue metadata

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



Sign in to add a comment

Override PlatformSensor::GetMaximumSupportedFrequency() in PlatformSensorFusion

Project Member Reported by juncai@chromium.org, Mar 6 2018

Issue description

As discussed in:
https://bugs.chromium.org/p/chromium/issues/detail?id=805146
https://bugs.chromium.org/p/chromium/issues/detail?id=796518

DeviceOrientation API users experienced lower than expected sensor data readout frequency on some devices. This is a regression and it is caused by PlatformSensorFusion not overriding PlatformSensor::GetMaximumSupportedFrequency() which returns 5Hz.
 
Status: Started (was: Assigned)
Summary: Override PlatformSensor::GetMaximumSupportedFrequency() in PlatformSensorFusion (was: Hardcode the maximum supported frequency to 60 Hz on the browser side on Android)
After some further debug, I found that the problem is not related to the Sensor.getMinDelay(). The return value from Sensor.getMinDelay() is actually good. The reason that the DeviceOrientation event frequency is low is that on Android it is using RELATIVE_ORIENTATION_EULER_ANGLES which is a fusion sensor (the source sensor is RELATIVE_ORIENTATION_QUATERNION), and the PlatformSensorFusion class doesn't override PlatformSensor::GetMaximumSupportedFrequency(), so it returns 5Hz as default. So when creating the orientation sensor at DeviceSensorEventPump, the |params->maximum_frequency| is 5Hz.

Description: Show this description
Description: Show this description
Interesting. Does that explain why on some devices I see 12hz, 5hz, and 60hz? It sounds like it should always be reporting 5hz.
I think 12hz, 5hz, and 60hz are the frequency of DeviceSensorEventPump::FireEvent():
https://cs.chromium.org/chromium/src/content/renderer/device_sensors/device_sensor_event_pump.h?l=303-306
The timer is set at frequency of 60Hz. And even though the sensor is reporting 5Hz, sensor reading at higher frequency 60Hz may read slightly different data sometimes, so DeviceOrientationEventPump::ShouldFireEvent() may return true:
https://cs.chromium.org/chromium/src/content/renderer/device_sensors/device_orientation_event_pump.cc?l=220-231
And it will trigger a new DeviceOrientation Event.

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89fd25bc1ec30c17939a50684df8fbc84746d961

commit 89fd25bc1ec30c17939a50684df8fbc84746d961
Author: Jun Cai <juncai@chromium.org>
Date: Thu Mar 08 18:18:08 2018

Override PlatformSensor::GetMaximumSupportedFrequency() in PlatformSensorFusion

This is a follow-up CL for:
https://chromium-review.googlesource.com/c/chromium/src/+/879586
which clamps sensor frequency to maximum supported frequency.

However, for fusion sensor, it needs to override
PlatformSensor::GetMaximumSupportedFrequency(), otherwise it will get 5Hz as default.
This has caused DeviceOrientation API to report sensor data in a low frequency. This
CL fixes this issue.

Bug:  819413 
Change-Id: Ib7c888227d3a09c7df281c06a7dbb3290de34d4f
Reviewed-on: https://chromium-review.googlesource.com/953127
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541840}
[modify] https://crrev.com/89fd25bc1ec30c17939a50684df8fbc84746d961/services/device/generic_sensor/fake_platform_sensor_and_provider.cc
[modify] https://crrev.com/89fd25bc1ec30c17939a50684df8fbc84746d961/services/device/generic_sensor/fake_platform_sensor_and_provider.h
[modify] https://crrev.com/89fd25bc1ec30c17939a50684df8fbc84746d961/services/device/generic_sensor/platform_sensor_fusion.cc
[modify] https://crrev.com/89fd25bc1ec30c17939a50684df8fbc84746d961/services/device/generic_sensor/platform_sensor_fusion.h
[modify] https://crrev.com/89fd25bc1ec30c17939a50684df8fbc84746d961/services/device/generic_sensor/platform_sensor_fusion_unittest.cc

Labels: Merge-Request-66

Comment 9 by cmasso@google.com, Mar 8 2018

Please verify in Canary
On my Pixel 2 phone, I can verify the device orientation event frequency is working as expected by using:
https://curly-age.glitch.me
https://threejs.org/examples/?q=dev#misc_controls_deviceorientation
https://code.viget.com/device-motion-demo/
which were mentioned in  issue 796518 .
If this fix looks good in Canary, do you think we could get it hotfixed in M65 (and also merged into M66)?
Project Member

Comment 12 by sheriffbot@chromium.org, Mar 9 2018

Labels: -Merge-Request-66 Merge-Approved-66 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

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

Comment 13 by bugdroid1@chromium.org, Mar 9 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5

commit 5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5
Author: Jun Cai <juncai@chromium.org>
Date: Fri Mar 09 18:29:24 2018

Override PlatformSensor::GetMaximumSupportedFrequency() in PlatformSensorFusion

This is a follow-up CL for:
https://chromium-review.googlesource.com/c/chromium/src/+/879586
which clamps sensor frequency to maximum supported frequency.

However, for fusion sensor, it needs to override
PlatformSensor::GetMaximumSupportedFrequency(), otherwise it will get 5Hz as default.
This has caused DeviceOrientation API to report sensor data in a low frequency. This
CL fixes this issue.

Bug:  819413 
Change-Id: Ib7c888227d3a09c7df281c06a7dbb3290de34d4f
Reviewed-on: https://chromium-review.googlesource.com/953127
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#541840}(cherry picked from commit 89fd25bc1ec30c17939a50684df8fbc84746d961)
Reviewed-on: https://chromium-review.googlesource.com/957104
Reviewed-by: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#133}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5/services/device/generic_sensor/fake_platform_sensor_and_provider.cc
[modify] https://crrev.com/5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5/services/device/generic_sensor/fake_platform_sensor_and_provider.h
[modify] https://crrev.com/5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5/services/device/generic_sensor/platform_sensor_fusion.cc
[modify] https://crrev.com/5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5/services/device/generic_sensor/platform_sensor_fusion.h
[modify] https://crrev.com/5f53f6fb575d2ed86aaa65fbbf19fdcd9e438fb5/services/device/generic_sensor/platform_sensor_fusion_unittest.cc

Status: Fixed (was: Started)
I'm testing this link: https://kuula.co/post/7lVD1 on Samsung S6 and a Motorola Moto X. 

The Moto X has a steady 60 fps reading, while on Samsung it seems to be closer to 5hz, and this happens on every version of the browser - Chrome (65) Chrome Dev (66) and Canary (67). So I'm a but confused when juncai@chromium.org means saying that it works on Canary now.

We are running a large 360 photo sharing site and now gyroscope controls (and, by consequence, the VR mode of viewing) is completely useless. This is a really mission critical issue for us and we are already getting complaints from our customers.


What is the Canary's version that you used? I think the version 67.0.3365.0 doesn't have the fix yet. I tested it on the master branch. Sorry about the confusion. The fix should be in the next Canary version.
Precisely, I have 67.0.3365.0 now
I will check again when the next release is up.
On a quick glance this looks good in Canary. Do you think it's possible to get this fix into M65 (along with 805146)?

I can also confirm that on Samsung S6, where it was 5hz the other day, now on Canary 67.0.3368.0 it works smoothly.
Hi dustin.kerstein@gmail.com, thanks for the testing. Since M65 is in stable, and we just merged the fix to M66, we can wait a bit to see how it goes in M66.
Any info about when the M66 will be released in stable? Our users are complaining because Web VR is a big part of our apps functionality and we're not sure what to tell them.
Chrome's public release calendar (always subject to change) is here:

https://www.chromium.org/developers/calendar

The estimated stable date for M-66 is April 17th, 2018. Be aware that new releases are rolled out over a period of days so devices may not be updated immediately.

Comment 23 Deleted

There is no need to allow this conversation to devolve into personal attacks. We are working with release management to establish the severity of this issue and whether it justifies the resources necessary to ship a new version of M-65 to all users.

The best way to help us at this time is to verify that the issue has been completely resolved on M-66 so that we do not have a repeat of these issue when that is released to stable-channel users. Chrome has an large test suite that is run for every version however it would be hubris to call it exhaustive. We rely on developers reporting issues discovered on the beta- and dev-channels in order to find places where our tests are incomplete. The changes above include new tests that exercise how we handle differences in hardware capabilities in ways we had not foreseen.

Sign in to add a comment