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

RESOLVED FIXED in mozilla13

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: cyu)

Tracking

unspecified
mozilla13
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
> 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.
I'm happy to mentor someone through this process.
Whiteboard: [good first bug][lang=c++][mentor=jlebar]
(good first bug has been deprecated by [mentor=] AFAIK)
Whiteboard: [good first bug][lang=c++][mentor=jlebar] → [lang=c++][mentor=jlebar]
Let's not let this sit waiting for a new contributor.  This is critically important for games, e.g.
Anyone new on the B2G team want this?

Comment 6

5 years ago
Hello~ I am interested in this topic. Where should I start ?
How does the underlying gfx handle the orientation change?

Comment 8

5 years ago
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

5 years ago
Should we create another bug for g sensor and block this one?
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
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.
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.
So you want to move all the existing orientation code over to the new sensor API?
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.
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.
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.
Depends on: 712973
Depends on: 714413
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/.
Summary: Listen for orientation changes in gonk → Listen for orientation changes to implement screen rotation in gonk widget backend
Blocks: 714414
Blocks: 715781
(Assignee)

Comment 18

5 years ago
I'll take this bug.
Assignee: nobody → cyu
(Assignee)

Comment 19

5 years ago
Created attachment 599516 [details] [diff] [review]
update screen orientation from sensor notification.
Attachment #599516 - Flags: review?(justin.lebar+bug)

Comment 20

5 years ago
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.
Attachment #599516 - Flags: review?(justin.lebar+bug)
> 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.
(Assignee)

Comment 22

5 years ago
Created attachment 599620 [details] [diff] [review]
update screen orientation from sensor notification.
Attachment #599516 - Attachment is obsolete: true
Attachment #599620 - Flags: review?(justin.lebar+bug)
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.
Attachment #599620 - Flags: review?(justin.lebar+bug) → review?(mwu)

Updated

5 years ago
Attachment #599620 - Flags: review?(mwu) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 25

5 years ago
Created attachment 599940 [details] [diff] [review]
update screen orientation from sensor notification. v2

update patch in hg format
Attachment #599620 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/e65188cac8b5
Keywords: checkin-needed
Target Milestone: --- → mozilla13
Heh, I don't think I should have gotten credit for reviewing that.  :)
https://hg.mozilla.org/mozilla-central/rev/e65188cac8b5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 29

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 30

5 years ago
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.
Removing the mentor tag so this doesn't show up on automated searches/tools for newcomers.
Whiteboard: [lang=c++][mentor=jlebar]
> 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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.