Last Comment Bug 709590 - Listen for orientation changes to implement screen rotation in gonk widget backend
: Listen for orientation changes to implement screen rotation in gonk widget ba...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla13
Assigned To: Cervantes Yu [:cyu] [:cervantes]
:
Mentors:
Depends on: 712973 714413
Blocks: 714414 b2g-dev-phone
  Show dependency treegraph
 
Reported: 2011-12-11 08:28 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-04-23 12:23 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
update screen orientation from sensor notification. (5.72 KB, patch)
2012-02-22 01:09 PST, Cervantes Yu [:cyu] [:cervantes]
no flags Details | Diff | Splinter Review
update screen orientation from sensor notification. (4.93 KB, patch)
2012-02-22 08:20 PST, Cervantes Yu [:cyu] [:cervantes]
mwu.code: review+
Details | Diff | Splinter Review
update screen orientation from sensor notification. v2 (5.06 KB, patch)
2012-02-23 03:25 PST, Cervantes Yu [:cyu] [:cervantes]
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-11 08:28:31 PST
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) 2011-12-11 16:46:18 PST
> 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) 2011-12-11 18:47:38 PST
I'm happy to mentor someone through this process.
Comment 3 Mounir Lamouri (:mounir) 2011-12-14 02:11:11 PST
(good first bug has been deprecated by [mentor=] AFAIK)
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-14 02:16:01 PST
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) 2011-12-14 04:55:18 PST
Anyone new on the B2G team want this?
Comment 6 StevenLee[:slee] 2011-12-14 18:24:45 PST
Hello~ I am interested in this topic. Where should I start ?
Comment 7 Kan-Ru Chen [:kanru] (UTC+8) 2011-12-14 20:58:42 PST
How does the underlying gfx handle the orientation change?
Comment 8 StevenLee[:slee] 2011-12-15 02:31:54 PST
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] 2011-12-15 02:51:41 PST
Should we create another bug for g sensor and block this one?
Comment 10 Justin Lebar (not reading bugmail) 2011-12-15 07:09:39 PST
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) 2011-12-15 07:22:05 PST
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] 2011-12-16 10:00:35 PST
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) 2011-12-16 10:04:56 PST
So you want to move all the existing orientation code over to the new sensor API?
Comment 14 Thinker Li [:sinker] 2011-12-16 11:25:55 PST
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) 2011-12-16 11:39:47 PST
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 2011-12-16 12:41:53 PST
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 2011-12-30 20:43:09 PST
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] 2012-02-13 18:25:07 PST
I'll take this bug.
Comment 19 Cervantes Yu [:cyu] [:cervantes] 2012-02-22 01:09:20 PST
Created attachment 599516 [details] [diff] [review]
update screen orientation from sensor notification.
Comment 20 Michael Wu [:mwu] 2012-02-22 02:18:59 PST
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) 2012-02-22 02:29:08 PST
> 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] 2012-02-22 08:20:00 PST
Created attachment 599620 [details] [diff] [review]
update screen orientation from sensor notification.
Comment 23 Justin Lebar (not reading bugmail) 2012-02-22 08:21:52 PST
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) 2012-02-23 02:36:32 PST
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] 2012-02-23 03:25:21 PST
Created attachment 599940 [details] [diff] [review]
update screen orientation from sensor notification. v2

update patch in hg format
Comment 27 Justin Lebar (not reading bugmail) 2012-02-23 06:28:05 PST
Heh, I don't think I should have gotten credit for reviewing that.  :)
Comment 28 Richard Newman [:rnewman] 2012-02-23 18:50:53 PST
https://hg.mozilla.org/mozilla-central/rev/e65188cac8b5
Comment 29 Andreas Gal :gal 2012-03-17 23:34:23 PDT
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] 2012-03-19 03:16:13 PDT
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] 2012-04-23 11:55:16 PDT
Removing the mentor tag so this doesn't show up on automated searches/tools for newcomers.
Comment 32 Justin Lebar (not reading bugmail) 2012-04-23 12:23:02 PDT
> 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.

Note You need to log in before you can comment on or make changes to this bug.