Implement DeviceMotionEvent API

content/browser/device_sensors/sensor_manager_chromeos.cc

Index: content/browser/device_sensors/sensor_manager_chromeos.cc
diff --git a/content/browser/device_sensors/sensor_manager_chromeos.cc b/content/browser/device_sensors/sensor_manager_chromeos.cc
index 899002ee9e38bafed49f97cb4743596b3f363d4f..eb64d2a60caa7e1e08ea631bc23a729a4c3afc25 100644
--- a/content/browser/device_sensors/sensor_manager_chromeos.cc
+++ b/content/browser/device_sensors/sensor_manager_chromeos.cc
@@ -6,24 +6,63 @@
 
 #include <math.h>
 
-#include "base/logging.h"
 #include "chromeos/accelerometer/accelerometer_reader.h"
 #include "chromeos/accelerometer/accelerometer_types.h"
+#include "content/browser/device_sensors/inertial_sensor_consts.h"
 #include "ui/gfx/geometry/vector3d_f.h"
 
 namespace {
 // Conversion ratio from radians to degrees.
 const double kRad2deg = 180.0 / M_PI;
+
+// The portion of previous gravity readings to perserve when updating with the
+// latest data.
+const double kGravityFilterRatio = 0.8;
 }
 
 namespace content {
 
-SensorManagerChromeOS::SensorManagerChromeOS() : orientation_buffer_(nullptr) {
+SensorManagerChromeOS::SensorManagerChromeOS()
+    : motion_buffer_(nullptr), orientation_buffer_(nullptr) {
 }
 
 SensorManagerChromeOS::~SensorManagerChromeOS() {
 }
 
+bool SensorManagerChromeOS::StartFetchingDeviceMotionData(
+    DeviceMotionHardwareBuffer* buffer) {
+  DCHECK(buffer);
+  {
+    base::AutoLock autolock(motion_buffer_lock_);
+    if (motion_buffer_)
+      return false;
+    motion_buffer_ = buffer;
+
+    motion_buffer_->seqlock.WriteBegin();
+    motion_buffer_->data.interval = kInertialSensorIntervalMicroseconds / 1000;

[flackr, 2015/02/18 22:34:09]

But this is not actually the update interval. Eventually it should be though:
http://crbug.com/459218

[jonross, 2015/02/24 00:02:47]

This is the interval with which events are sent to Javascript.

The buffer is read at a continuous interval, each time sending the current
values. So this is the interval that websites will receive.

It is true that this is not the rate which updates are received here. I'll add a
todo to track this

[flackr, 2015/02/25 23:15:16]

But when I tested it, it seems that JS is only notified if the value actually
changed (See deviceorientation rate test page I wrote here:
http://www.dynprojects.com/dp/tests/deviceorientationrate.html), so from the
consumer's point of view the rate is actually
max(kInertialSensorIntervalMicroseconds / 1000, kDelayBetweenReadsMs)

[jonross, 2015/03/05 14:28:02]

Done.

+    motion_buffer_->seqlock.WriteEnd();
+  }
+
+  StartObservingAccelerometer();
+  return true;
+}
+
+bool SensorManagerChromeOS::StopFetchingDeviceMotionData() {
+  {
+    base::AutoLock autolock(motion_buffer_lock_);
+    if (!motion_buffer_)
+      return false;
+    // Make sure to indicate that the sensor data is no longer available.
+    motion_buffer_->seqlock.WriteBegin();
+    motion_buffer_->data.allAvailableSensorsAreActive = false;
+    motion_buffer_->seqlock.WriteEnd();
+
+    motion_buffer_ = nullptr;
+  }
+  StopObservingAccelerometer();
+  return true;
+}
+
 bool SensorManagerChromeOS::StartFetchingDeviceOrientationData(
     DeviceOrientationHardwareBuffer* buffer) {
   DCHECK(buffer);
@@ -39,7 +78,6 @@ bool SensorManagerChromeOS::StartFetchingDeviceOrientationData(
     orientation_buffer_->data.hasAbsolute = true;
     orientation_buffer_->seqlock.WriteEnd();
   }
-
   StartObservingAccelerometer();
   return true;
 }
@@ -62,23 +100,77 @@ bool SensorManagerChromeOS::StopFetchingDeviceOrientationData() {
 
 void SensorManagerChromeOS::OnAccelerometerUpdated(

[flackr, 2015/02/18 22:34:09]

Given the accelerometer source reader runs on a blocking thread, and the target
receiving the events is on another thread, we should eventually use a mechanism
which doesn't stepping over to the UI thread to deliver these updates.

[jonross, 2015/02/25 00:09:56]

For the DeviceMotion API this will be partially addressed by the changes to the
observer list.

For the remainder you filed crbug.com/461433 which will be addressed separately.

     const chromeos::AccelerometerUpdate& update) {
-  base::AutoLock autolock(orientation_buffer_lock_);
-  if (!orientation_buffer_)
-    return;
-
   chromeos::AccelerometerSource source;
-  if (update.has(chromeos::ACCELEROMETER_SOURCE_SCREEN)) {
+  if (update.has(chromeos::ACCELEROMETER_SOURCE_SCREEN))
     source = chromeos::ACCELEROMETER_SOURCE_SCREEN;
-  } else if (update.has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD)) {
+  else if (update.has(chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD))
     source = chromeos::ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD;
-  } else {
+  else
     return;
-  }
 
   double x = update.get(source).x;
   double y = update.get(source).y;
   double z = update.get(source).z;
 
+  GenerateMotionEvent(x, y, z);
+  GenerateOrientationEvent(x, y, z);
+}
+
+void SensorManagerChromeOS::StartObservingAccelerometer() {
+  if (chromeos::AccelerometerReader::GetInstance()->HasObserver(this))
+    return;
+  chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
+}
+
+void SensorManagerChromeOS::StopObservingAccelerometer() {
+  {
+    base::AutoLock auto_motion_lock(motion_buffer_lock_);
+    base::AutoLock auto_orientation_lock(orientation_buffer_lock_);
+    if (orientation_buffer_ || motion_buffer_)
+      return;
+  }
+  if (!chromeos::AccelerometerReader::GetInstance()->HasObserver(this))

[flackr, 2015/02/18 22:34:09]

It should always have an observer if StopObserving is called shouldn't it?
DCHECK that this is the case.

[jonross, 2015/02/25 00:09:56]

Done.

+    return;
+  chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
+}
+
+void SensorManagerChromeOS::GenerateMotionEvent(double x, double y, double z) {
+  base::AutoLock autlock(motion_buffer_lock_);
+  if (!motion_buffer_)
+    return;
+
+  motion_buffer_->seqlock.WriteBegin();
+  motion_buffer_->data.accelerationIncludingGravityX = x;
+  motion_buffer_->data.hasAccelerationIncludingGravityX = true;
+  motion_buffer_->data.accelerationIncludingGravityY = y;
+  motion_buffer_->data.hasAccelerationIncludingGravityY = true;
+  motion_buffer_->data.accelerationIncludingGravityZ = z;
+  motion_buffer_->data.hasAccelerationIncludingGravityZ = true;
+  motion_buffer_->data.allAvailableSensorsAreActive = true;
+
+  gravity_.set_x(kGravityFilterRatio * gravity_.x() +

[flackr, 2015/02/18 22:34:09]

I'm not sure we should be using a low pass filter to compute gravity, as having
only an accelerometer for measurement makes this extremely likely to have
errors. Unless I'm misreading this or missing something, it looks like android
uses at least two (accel + magnetometer or gyro) if not all three sensors to
determine the gravity component in order to provide a more accurate reading 
(http://developer.android.com/guide/topics/sensors/sensors_motion.html).
Instead, we could tell developers that they can approximate gravity if there is
not a sensor for it using a low pass filter and hopefully that would allow each
app to make the right decision whether to use a low pass filter or not.

Take for example turning the device. I have no accelerated it in any significant
way along any of the axes, but it will temporarily read as an acceleration.
Similarly if the device is in freefall we would determine the acceleration due
to gravity to be approaching (0, 0, 0), or if the device is in a vehicle turning
at constant velocity it will eventually see all of that force as acceleration
due to gravity whereas the spec suggests it should not:
http://www.w3.org/TR/orientation-event/.

[jonross, 2015/02/24 00:02:47]

The idea for the filter actually came from Android documentation:

http://developer.android.com/reference/android/hardware/SensorEvent.html#values

This is likely for hardware devices in which only the accelerometer exists.

If we remove this to be consistent with the w3c spec, then I would recommend
that we document the filtering for any developer that wants to implement it
themselves.

[flackr, 2015/02/25 23:15:16]

My understanding from reading this was that it's a suggestion for developers if
they want to infer gravity from the accelerometer updates, given that it's
written in onSensorChanged as if it were a client app.
I agree, it would be nice to document (or link to) the filter from
http://www.html5rocks.com/en/tutorials/device/orientation/ where it mentions
that "some devices might not have the hardware to exclude the effect of
gravity".

[jonross, 2015/03/05 14:28:02]

Acknowledged.

+                 (1.0f - kGravityFilterRatio) * x);
+  gravity_.set_y(kGravityFilterRatio * gravity_.y() +
+                 (1.0f - kGravityFilterRatio) * y);
+  gravity_.set_z(kGravityFilterRatio * gravity_.z() +
+                 (1.0f - kGravityFilterRatio) * z);
+
+  motion_buffer_->data.accelerationX = x - gravity_.x();
+  motion_buffer_->data.hasAccelerationX = true;
+  motion_buffer_->data.accelerationY = y - gravity_.y();
+  motion_buffer_->data.hasAccelerationY = true;
+  motion_buffer_->data.accelerationZ = z - gravity_.z();
+  motion_buffer_->data.hasAccelerationZ = true;
+  motion_buffer_->seqlock.WriteEnd();
+}
+
+void SensorManagerChromeOS::GenerateOrientationEvent(double x,
+                                                     double y,
+                                                     double z) {
+  base::AutoLock autolock(orientation_buffer_lock_);
+  if (!orientation_buffer_)
+    return;
+
   // Create a unit vector for trigonometry
   // TODO(jonross): Stop reversing signs for vector components once
   // accelerometer values have been fixed. crbug.com/431391
@@ -110,12 +202,4 @@ void SensorManagerChromeOS::OnAccelerometerUpdated(
   orientation_buffer_->seqlock.WriteEnd();
 }
 
-void SensorManagerChromeOS::StartObservingAccelerometer() {
-  chromeos::AccelerometerReader::GetInstance()->AddObserver(this);
-}
-
-void SensorManagerChromeOS::StopObservingAccelerometer() {
-  chromeos::AccelerometerReader::GetInstance()->RemoveObserver(this);
-}
-
 }  // namespace content