709590 - Listen for orientation changes to implement screen rotation in gonk widget backend

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Currently we're not, so the screen and apps stay in portrait forever.

This feels like something for hal/, but I'm not sure what it would take to hook that up to our current orientation-tracking code.

Comment 1

[Justin Lebar (not reading bugmail)]

> This feels like something for hal/

IIRC there's a component which tells us when the orientation changes, just like there's a component which tells us when a key is pressed.  If so, this should go in the appshell.

Comment 2

[Justin Lebar (not reading bugmail)]

I'm happy to mentor someone through this process.

Comment 3

[Mounir Lamouri (:mounir)]

(good first bug has been deprecated by [mentor=] AFAIK)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's not let this sit waiting for a new contributor.  This is critically important for games, e.g.

Comment 5

[Justin Lebar (not reading bugmail)]

Anyone new on the B2G team want this?

Comment 6

[StevenLee[:slee]]

Hello~ I am interested in this topic. Where should I start ?

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

How does the underlying gfx handle the orientation change?

Comment 8

[StevenLee[:slee]]

I will try to connect the g-sensor events to nsISensorManager interface first. If g-sensor works, I will port the other sensors.

Comment 9

[Thinker Li [:sinker]]

Should we create another bug for g sensor and block this one?

Comment 10

[Justin Lebar (not reading bugmail)]

I don't see an interface nsISensorManager.

So I thought this would be simple, because I thought there was a /dev/input device corresponding to the orientation sensor.  But apparently not [1]!

Steven, do you know what Android code handles orientation changes?  What I've done for similar things in the past is dug through the Java code (it's all in the B2G tree, for now anyway) until I found the relevant JNI call.

Here are some docs which likely have keywords you can grep the Java code for:

http://developer.android.com/reference/android/hardware/SensorEvent.html
http://developer.android.com/reference/android/hardware/Sensor.html

[1] Look for "Found device" in http://pastebin.com/UALP5j7U

Comment 11

[Justin Lebar (not reading bugmail)]

So it looks like there may be code for this already.  It may just be a matter of finding the right file to read.

See dom/system/unix/nsDeviceMotion.cpp.

Comment 12

[Thinker Li [:sinker]]

Bug 697361 is working on nsISensorManager.  It is for proximity sensor for Android platform although we like to kill Android supporting.  But, we still can reuse this interface to manage all sensor events.

Comment 13

[Justin Lebar (not reading bugmail)]

So you want to move all the existing orientation code over to the new sensor API?

Comment 14

[Thinker Li [:sinker]]

I don't think we need do such a big task.  nsISensorManger is only a place to collect all sensor events from backends.  All events are dispatched from sensor manger to rest of the Gecko.  For example, fire orientation events to nsDeviceMotion.  It provides a generic interface that can be used for sensors having no dom api.  nsISensorManger is also a boundary between sensor backends and rest of gecko, and provide some kind of portability.

Comment 15

[Justin Lebar (not reading bugmail)]

Okay.

If you're not moving all the nsDeviceMotion code over to the new API, it may be easier to add gonk support to the existing nsDeviceMotion code.  It already has branches for a variety of different machines.  But maybe using the sensor manager would be easier.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm away from a real machine so haven't followed 100%, but my plan is to move the low-level sensor code into hal, but not right now.  A good path forward might be adding a "hal" backend fir the existing dom impls.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There are three orientation-related "clients" we need to support
 1. gonk widget layer, to switch the screen rotation
 2. deviceorientation/devicemotion DOM events
 3. raw sensor API

All three are technically orthogonal.  Let's re-focus this bug on (1).  They'll share common code through hal/.

Comment 18

[Cervantes Yu [:cyu] [:cervantes]]

I'll take this bug.

Comment 19

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: update screen orientation from sensor notification.

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 599516 [details] [diff] [review]
update screen orientation from sensor notification.

Review of attachment 599516 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like this here, but we should be able to do this properly once we have the screen lock API.

::: widget/gonk/nsAppShell.cpp
@@ +48,5 @@
>  #include <sys/param.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include <unistd.h>
> +#include <math.h>

Please place this alphabetically  in the set of <> includes above.

@@ +68,5 @@
>  #include "android/log.h"
>  #include "ui/EventHub.h"
>  #include "ui/InputReader.h"
>  #include "ui/InputDispatcher.h"
> +#include "mozilla/HalSensor.h"

Please place this near the other mozilla/*.h includes, alphabetically if possible.

@@ +664,5 @@
>      return OK;
>  }
>  
> +class OrientationSensorObserver : public ISensorObserver
> +{

{ on the same line as class in this file.

@@ +666,5 @@
>  
> +class OrientationSensorObserver : public ISensorObserver
> +{
> +private:
> +  class ScreenRotateEvent : public nsRunnable {

Pull this outside of this class. There's better ways of scheduling something onto the main thread but I don't expect this to live here too long..

@@ +679,5 @@
> +
> +  private:
> +    nsCOMPtr<nsIScreen> mScreen;
> +    PRUint32 mRotation;
> +  }; // class OrientationNotifier

Inaccurate comment

@@ +688,5 @@
> +  }
> +  void Notify(const SensorData& aSensorData) {
> +    nsresult result;
> +    nsCOMPtr<nsIScreenManager> screenMgr =
> +        do_GetService("@mozilla.org/gfx/screenmanager;1", &result);

I recommend using the single argument do_GetService instead.

@@ +712,5 @@
> +
> +    PRUint32 currRotation;
> +    nsresult res;
> +    res = screen->GetRotation(&currRotation);
> +    if (res == NS_OK && rotation != currRotation) {

NS_SUCCEEDED(res) should be used to check nsresult since there can be more than one success return code.

The logic here should be inverted to minimize indenting -

if (NS_FAILED(res) || rotation == currRotation)
  return;

@@ +714,5 @@
> +    nsresult res;
> +    res = screen->GetRotation(&currRotation);
> +    if (res == NS_OK && rotation != currRotation) {
> +      PRTime now = PR_Now();
> +      MOZ_ASSERT (now > mLastUpdate);

Remove the space between MOZ_ASSERT and (

@@ +715,5 @@
> +    res = screen->GetRotation(&currRotation);
> +    if (res == NS_OK && rotation != currRotation) {
> +      PRTime now = PR_Now();
> +      MOZ_ASSERT (now > mLastUpdate);
> +      if (now - mLastUpdate > sMinUpdateInterval) {

Invert the logic here too.

@@ +717,5 @@
> +      PRTime now = PR_Now();
> +      MOZ_ASSERT (now > mLastUpdate);
> +      if (now - mLastUpdate > sMinUpdateInterval) {
> +        LOG("Rotate screen: azimuth: %f, pitch: %f, roll: %f, rotation: %d",
> +            azimuth, pitch, roll, rotation);

Remove/comment out this logging

@@ +719,5 @@
> +      if (now - mLastUpdate > sMinUpdateInterval) {
> +        LOG("Rotate screen: azimuth: %f, pitch: %f, roll: %f, rotation: %d",
> +            azimuth, pitch, roll, rotation);
> +        mLastUpdate = now;
> +        nsCOMPtr<nsIRunnable> event = new ScreenRotateEvent(screen, rotation);

We should be able to use nsRefPtr<ScreenRotateEvent>, or just put new ScreenRotateEvent directly in the NS_DispatchToMainThread call.

::: widget/gonk/nsAppShell.h
@@ +45,5 @@
>  #include "nsRect.h"
>  #include "nsTArray.h"
>  
>  #include "utils/RefBase.h"
> +#include "mozilla/Observer.h"

Please place after mozilla/Mutex.h if possible.

@@ +84,5 @@
> +namespace hal {
> +class SensorData;
> +typedef Observer<SensorData> ISensorObserver;
> +}
> +}

There is a namespace mozilla in this file you can put these hal declarations into.

@@ +116,5 @@
>      android::sp<GeckoInputReaderPolicy> mReaderPolicy;
>      android::sp<GeckoInputDispatcher>   mDispatcher;
>      android::sp<android::InputReader>            mReader;
>      android::sp<android::InputReaderThread>      mReaderThread;
> +    mozilla::hal::ISensorObserver *mObserver;

Use nsAutoPtr. This will save you from having to delete mObserver in the destructor.

Comment 21

[Justin Lebar (not reading bugmail)]

> Use nsAutoPtr. This will save you from having to delete mObserver in the destructor.

To clarify, nsAutoPtr is an owning pointer.  When the value of nsAutoPtr is changed, or the nsAutoPtr is destructed, the object pointed to is deleted.

Comment 22

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: update screen orientation from sensor notification.

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 599620 [details] [diff] [review]
update screen orientation from sensor notification.

mwu, do you want to do this, since you looked at the earlier patch?  I'm happy to do the review myself if you'd prefer.

Comment 24

[Justin Lebar (not reading bugmail)]

This is a git patch, which is unfortunately slightly different from an hg export'ed patch.

Please use git-patch-to-hg patch from [1], or export your patch straight from git to bugzilla using git-bz [1].

[1] https://github.com/jlebar/moz-git-tools

Comment 25

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: update screen orientation from sensor notification. v2

update patch in hg format

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e65188cac8b5

Comment 27

[Justin Lebar (not reading bugmail)]

Heh, I don't think I should have gotten credit for reviewing that.  :)

Comment 28

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/e65188cac8b5

Comment 29

[Andreas Gal :gal]

This patch drains the battery like nuts. Listen to orientation changes causes polling. This patch doesn't even set the minimum interval. We should not listen while the device sleeps, and there should be a coarse minimum time if no content listeners are attached (for device rotation, interval == 1s or so) and higher frequency when content is listening. For the future please check for this kind of stuff. This is right now the majority of the wakeup and battery use of the device. I suggest filing a follow-up bug and closing this one.

Comment 30

[Cervantes Yu [:cyu] [:cervantes]]

We made some tests with 2 devices (SGS2 and another with lower CPU powerhorse) and the results are:

- Whether we set the interval or not, the orientation sensor reports every 200 milliseconds. Actually, android sensor service code provides SensorDevice::setDelay() for this purpose, but it has no effect on either device. Looks like this setting is not respected on the lower layer of implementation.
- The sensor callback is not executed when the device is put to sleep. I added a logcat message to show the timestamps when sensor reports, and no message is shown after I pushed the power button to put the device to sleep when the device is not usb-connected to the host computer (one of the device will not sleep if usb-connected).

I admit the patch is not considerate enough on the above aspects, but we don't see the the batteries drained for this patch on the devices we have. Andreas, may we have more information on how you reproduced the problem for further investigation? Thanks.

Comment 31

[Reuben Morais [:reuben]]

Removing the mentor tag so this doesn't show up on automated searches/tools for newcomers.

Comment 32

[Justin Lebar (not reading bugmail)]

> This patch drains the battery like nuts.  [...]  I suggest filing a follow-up bug and closing this 
> one.

Indeed, this should be a separate bug.  Seems like we're waiting on at least a rough STR from gal.