Onchange IPC from browser does not seem to fit well for motion sensors with continuous reporting mode.

At the moment sensor polling frequency on platform side is limited with 60 Hz but the limit can be increased in future in order to decrease latency.
The sensor actually gets updated values at every polling. So broadcasting such a frequent data to all clients over MOJO will bloat IPC channels and bring additional latency (up to 2 ms on Nexus).

Our intention is to provide the freshest possible sensor data for every new frame, so the platform sensor just writes the data to shared buffer and clients on renderer side read the data in sync with rAF so that their Critical Render path is not interrupted. 

Reading of a few double values from shared memory is not expensive.

Could you please give more details about the issue?

Is the problem that using Document::EnqueueAnimationFrameTask is affecting the actual frame rate exhausting cpu/battery? If so, IMO we should just find some more appropriate mechanism for scheduling readings.

Queuing an animation task every frame is making a large amount of the system churn even if nothing in the UI is changing in response to the sensor data. We generally consider an infinite raf loop where nothing is changing in the UI a bug in apps because it uses a lot of power. Creating any continuously polling sensor is pretty much the same in the current design.

Note that the raf system is sending plenty of IPCs around the system every 60fps due to the BeginFrameSource and the way the browser/gpu process and renderer talk to each other. I'd really like to see data on the 2ms you're quoting, that's 2ms for what in which situations?

Also the system you're coupled to currently (raf) is throttled for lots of reasons, for example third party iframes, off screen iframes, it doesn't run at all inside frames that are still loading, it doesn't run in background tabs, it'll block on the GPU being busy, ... So this means that if you insert a bunch of DOM into the page that takes 100ms to raster you're losing 100ms of sensor samples even if the main thread was idle.

I think you want a separate system that schedules the tasks unless you want all those side effects for sensor events.

I'm also curious to see an example sensor data stream and how important it is to get the changes in real time at 60fps. For lots of sensors (ex. ALS) getting it periodically and in batches is often enough. Never letting the app sleep because it creates an instance of the AmbientLightSensor or AccelerometerSensor might not be the best design. It's so easy to end up in a situation where your page is using infinite power.

esprehn@ thanks for your explanation, I did not realize that Document::EnqueueAnimationFrameTask was so expensive.

Initially we used Timers to poll shared buffer but were advised to sync 'Sensor.onchange' notification with rAF so that Critical Render path is
not interrupted (so all timers were substituted with 'EnqueueAnimationFrameTask').

I'm working now to improve the current implementation, the current plan is to use task runner (TaskRunnerHelper::Get(TaskType::kSensor, .. ) ) for scheduling the polling of the shared buffer; further if (1) the sensor data has been modified and (2) there are listeners for 'onchange' event schedule animation task for 'onchange' event propagation. WDYT?

(Please note that above I described plan for motion sensors with continuous reporting mode, sensor like ALS are different, they have 'onchange' reporting mode and for them implementation is relying on OnChange IPCs from the browser)

I was wrong earlier to suggest such an explicit method of synchronizing these events to the rendering pipeline because I didn't understand what input was actually doing. If the sensor data is modified we shouldn't queue a render frame task. Instead we should either,

 (1) do what input events do and set the appropriate dirty bit so that the rendering pipeline knows that there is some work to be done and deliver all sensor events at once as part of the next frame (and add a getCoalescedEvents() method like https://w3c.github.io/pointerevents/extension.html#dom-pointerevent-getcoalescedevents)

 (2) dispatch the event immediately and if script wants to run the rendering pipeline it will call requestAnimationFrame() itself. If not it can simply record the event as perhaps part of some input smoothing and use it at a later time.

In general it sounds like maybe we should deliver/synchronize sensor data like we do with input events, i.e., aligned with BeginFrames. If there isn't active rendering going on maybe it then makes sense to fall back to polling. I guess it depends a bit on what folks generally use this data for.

So far we're interested in the most recent sensor reading for the next rendered frame, so that the UI can be updated based on the freshest sensor input. 

So if 'EnqueueAnimationFrameTask' did not schedule a new frame rendering but simply attach a task for the already scheduled frame it would fit quite well.

You're saying you want to change the UI in response to the sensor data, but if EnqueueAnimationFrameTask doesn't schedule a frame then no frame will ever happen since nothing is dirty. The system is a pipeline with inputs, if no inputs are dirty then no frame will happen.

In general I caution against assuming sensor data is always UI input data. The same could be said for any number of things, like JS Streams getting data off the network, but in practice apps do lots of things in the background that are not about updating the DOM.

ex. If you have a temperature sensor sending temp data, that might be changing frequently, but the app UI may only show changes for increments of 10+ degrees. Scheduling frames for every sensor data change is using a lot of power there.

I don't think you should use EnqueueAnimationFrameTask at all. There's lots of use cases for sensor data that are not about updating the DOM. If the author wants to be aligned they can use requestAnimationFrame inside the onchange event. Doing that implicitly for the author for all sensor types is magical I think has more downsides than upsides.

esprehn@, then I think we should just follow option (2) from https://bugs.chromium.org/p/chromium/issues/detail?id=715872#c7 and probably add a note to the spec with a caution that UI updates from 'onchange' handler are not advisable. WDYT?
Thanks again for commenting on this!

That option seems fine to me. If possible we should also leave the door open for getCoalescedEvents() if we ever want to limit event delivery to once per frame (regardless or not whether the script is doing UI updates based on the input).

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

commit d0458b1c5e117db5cb8b63688ef39af43fd4fc97
Author: mikhail.pozdnyakov <mikhail.pozdnyakov@intel.com>
Date: Mon May 15 11:46:51 2017

[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG= 715872 

Review-Url: https://codereview.chromium.org/2870073002
Cr-Commit-Position: refs/heads/master@{#471726}

[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/BUILD.gn
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/OrientationSensor.h
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/Sensor.cpp
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/Sensor.h
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/SensorProxy.cpp
[modify] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/third_party/WebKit/Source/modules/sensor/SensorProxy.h
[delete] https://crrev.com/9f5f6b9548bc4cad0c64d3aa2b5b58ae8dad595b/third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp
[delete] https://crrev.com/9f5f6b9548bc4cad0c64d3aa2b5b58ae8dad595b/third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h

Before we close this issue I want to check with rockot@ that the analysis of sending 60Hz of IPCs is correct.

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

commit f206f95fe2658bac111dee99ece3fce0e4ec90c6
Author: Mikhail Pozdnyakov <mikhail.pozdnyakov@intel.com>
Date: Wed Aug 23 08:11:48 2017

Test that access to Generic Sensor APIs is autogranted

Bug:  715872 
Change-Id: Iaf2a6cd00933cdcc4187252eee5f8e0d6ff5f790
Reviewed-on: https://chromium-review.googlesource.com/623789
Commit-Queue: Mikhail Pozdnyakov <mikhail.pozdnyakov@intel.com>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496621}
[modify] https://crrev.com/f206f95fe2658bac111dee99ece3fce0e4ec90c6/third_party/WebKit/LayoutTests/http/tests/permissions/resources/test-query.js

The assigned owner "mikhail.pozdnyakov@intel.com" is not able to receive e-mails, please re-triage.

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

ping, reillyg@, is there anything left to be done for this bug?

Thanks for checking. This work is done.