Refer to Android device, the screen rotation is according to the event from Acceleration sensor not a Orientation sensor (Virtual sensor made by Acceleration and magnetic sensors). So in the older Android devices they can do screen rotation event no magnetic or orientation sensor.

1. Modify orientation observer to monitor screen rotation by acceleration not orientation sensor. 2. Apply mechanisms for stabilizing screen rotation. Know issue on this WIP: 1. If next rotation is not the adjacent one then user need to tap screen for performing rotation. I checked that OrientationObserver indeed called SetRotation() to update rotation plan so it may be the issue on the other components. Ex: The rotation plan from 0 DEG to 180 DEG immediately.

Hi, Michael Wu (:mwu). If you have time, please help me to review the patch. Thanks very much for your effort.

For patches that are WIP and have known issues, asking for feedback may be more appropriate than review.

(In reply to Marco Chen [:mchen] from comment #0) > Refer to Android device, the screen rotation is according to the event from > Acceleration sensor not a Orientation sensor (Virtual sensor made by > Acceleration and magnetic sensors). > > So in the older Android devices they can do screen rotation event no > magnetic or orientation sensor. How old? We only care about Android devices that can run ICS, and we definitely should not prefer our own orientation calculation over the one made by the sensor HAL.

Comment on attachment 659663 [details] [diff] [review] WIPv1 dougt may have more thoughts on orientation vs. acceleration sensors.

We can't guarantee (could anyone?) the new shipping device (especially on low end product) will have Acceleration + Magnetic and which vendor will provide functions for calculating orientation value. If a new device just equipped with Acceleration sensor then our B2G device can't do screen rotation. It is not the expected result from user experience and brand name company who want to reduce the cost for low-end product. So I just made this modification. If someone can made this decision on B2G didn't support screen rotation if device equipped Acceleration sensor only. Then I will close this issue.

And I found the known issue I posted on comment 1 is also happened on current build. So it is not caused by my WIPv1. Sorry to make confuse.

(In reply to Marco Chen [:mchen] from comment #6) > We can't guarantee (could anyone?) the new shipping device (especially on > low end product) will have Acceleration + Magnetic and which vendor will > provide functions for calculating orientation value. We have confirmed that our v1 device will have an orientation sensor.

Hi Michael, 1. Please refer to description of bug 891481 for the reason on using acceleration instead of orientation sensor for screen rotation. 2. And in order to save the cost for ultra-low-end market, our partner also considered to remove orientation sensor and just keep acceleration sensor only. (ex: helix project didn't have orientation sensor now.) So may I know your opinion on the purpose of this bug? Is it worth to do? Thanks.

(In reply to Marco Chen [:mchen] from comment #10) > Hi Michael, > > 1. Please refer to description of bug 891481 for the reason on using > acceleration instead of orientation sensor for screen rotation. > > 2. And in order to save the cost for ultra-low-end market, our partner also > considered to remove orientation sensor and just keep acceleration sensor > only. (ex: helix project didn't have orientation sensor now.) > > So may I know your opinion on the purpose of this bug? Is it worth to do? > Thanks. I agree with Marco. Please also note that SENSOR_TYPE_ORIENTATION is known to use more power. So it is not good to use it for orientation. This sensor type is also depreciated in Android jelly bean .

(In reply to Marco Chen [:mchen] from comment #1) > Created attachment 659663 [details] [diff] [review] > WIPv1 > > 1. Modify orientation observer to monitor screen rotation by acceleration > not orientation sensor. > 2. Apply mechanisms for stabilizing screen rotation. > > Know issue on this WIP: > 1. If next rotation is not the adjacent one then user need to tap screen for > performing rotation. I checked that OrientationObserver indeed called > SetRotation() to update rotation plan so it may be the issue on the other > components. > Ex: The rotation plan from 0 DEG to 180 DEG immediately. I tried to apply your patch on gecko. But it fails . Can you please upload another patch to remove conflicts. I am trying to apply in master branch of gecko . I just saw your patch . I liked the way you are calculating orientation. But Android is using a LOW PASS FILTER to filter out accelerometer vector (which you get from ACCLEREMETER SENSOR output ). This helps you not to rotate screen while you are flapping device https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/WindowOrientationListener.java#564 . Moreover, android mechanism also doesn't have the problem of "0 DEG to 180 DEG immediately" . Can you please update a new patch considering these .

(In reply to Marco Chen [:mchen] from comment #10) > So may I know your opinion on the purpose of this bug? Is it worth to do? > Thanks. We'll need to do it for JB for support.

Tapas, you have a WIP patch for this as well right, and I assume it already addresses your points in comment 12? If so maybe we can get that patch cleaned up and posted here for feedback.

I wi(In reply to Michael Vines [:m1] [:evilmachines] from comment #14) > Tapas, you have a WIP patch for this as well right, and I assume it already > addresses your points in comment 12? If so maybe we can get that patch > cleaned up and posted here for feedback. I will post my patch here .

Hi Tapas, The patch from me was uploaded almost one year ago so there is no surprise on conflict. If you have patch on newest base then welcome for uploading yours. Thanks.

Hi Michael/Marco, I will upload a patch here soon. I have one doubt. Currently , we use 200ms as delay for all sensors in GonkSensor.cpp . Android is using SensorManager.SENSOR_DELAY_UI (66.667 ms)delay for accelerometer sensor. What is your opinion about setting accelerometer sensor delay to 66.667 ms ?

Hi Tapas I think the delay time of sensors can be set to other value if there is a supported reason. May I know the benefit of changing to 66.667ms? Thanks.

I don't have any strong reason for staying with 200ms. However, I would like to know why we would want to change, other than matching what Android does.

Hi Tapas, Since you are working on this, I will re-assign the owner to you. Or if you are not, please drop it. Thanks.

Tapas has written patch for this and it's working well, but the patch is stuck in internal process and it's looking like it'll be O(weeks) to get it posted here (we hope!). The reason is very sad. We can either see that through, or Tapas could provide rough details about what his patch does and somebody else could carry the bug forward. The patch is based on existing AOSP Apache2 Java stuff.

I will be very happy to see anyone implementing this faster as my patch is taking long time in internet review process. Basically, we need to convert https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/WindowOrientationListener.java#564 java class in c++ class and integrate whole logic in OrientationObserver.cpp. Please ping me here if anyone need help on this. :)

Currently, Orientation Observer is using Orientation sensor output which known to use more power. With this patch, Orientation Observer will use accelerometer sensor output. Currently, we have sensor delay fixed to 200ms (for all type of sensors) in gecko. Accelerometer sensor poll rate is changed to Android SENSOR_DELAY_UI (66ms) with this patch. However it does not change sensor poll rate for other sensor type. This new poll rate helps to improve UI response and detecting delays between rotation angles.

Sorry to attach same patch multiple times. Its my mistake. Please review it.

Comment on attachment 785180 [details] [diff] [review] Orientation Observer uses accelerometer sensor Changing the reviewer .

Comment on attachment 785180 [details] [diff] [review] Orientation Observer uses accelerometer sensor Review of attachment 785180 [details] [diff] [review]: ----------------------------------------------------------------- I have a few comments on style issues while I read the code and figure out what it's doing. ::: hal/gonk/GonkSensor.cpp @@ +38,5 @@ > namespace mozilla { > > // The value from SensorDevice.h (Android) > #define DEFAULT_DEVICE_POLL_RATE 200000000 /*200ms*/ > +// Value of SENSOR_DELAY_UI from WindowOrientationListener.java (Android) Please make a comment that refers to ProcessOrientation.cpp. ::: widget/gonk/ProcessOrientation.cpp @@ +1,3 @@ > +/* > + * Copyright (c) 2013, Linux Foundation. All rights reserved > + * Not a Contribution Wha? @@ +34,5 @@ > +// define ORIENTATION_SENSOR_LOG_ENABLED to enable all logs from orientation > +// calculation algorithm > +#undef ORIENTATION_SENSOR_LOG_ENABLED > + > +#define PI 3.141592653f Use M_PI @@ +154,5 @@ > +ProcessOrientation::tiltTolerance[][4] = { > + {-25, 70}, //ROTATION_0 > + {-25, 65}, //ROTATION_90 > + {-25, 60}, //ROTATION_180 > + {-25, 65} //ROTATION_270 Add a space after every // @@ +178,5 @@ > + > +#ifdef ORIENTATION_SENSOR_LOG_ENABLED > + printf_stderr > + ("ProcessOrientation: Raw acceleration vector: x = %f, y = %f, z = %f," > + "magnitude = %f\n", x, y, z, sqrt(x * x + y * y + z * z)); Please use a standard logging function such as ALOG that can be toggled on and off without surrounding with #ifdefs @@ +347,5 @@ > +#endif > + return mProposedRotation; > + } > +// Don't rotate screen > +return -1; Bad indentation. @@ +355,5 @@ > +ProcessOrientation::IsTiltAngleAcceptable(int rotation, int tiltAngle) > +{ > + return (tiltAngle >= tiltTolerance[rotation][0] > + && tiltAngle <= tiltTolerance[rotation][1]); > + Remove this extra empty line.

Comment on attachment 785180 [details] [diff] [review] Orientation Observer uses accelerometer sensor Review of attachment 785180 [details] [diff] [review]: ----------------------------------------------------------------- In case that you will update the next version for mwu's comment, so I leave what I found now. Thanks for your contribution. ::: widget/gonk/OrientationObserver.cpp @@ +170,2 @@ > , mAllowedOrientations(sDefaultOrientations) > + , mOrientation(new mozilla::ProcessOrientation()) I didn't see anywhere to release this memory or you can use nsAutoPtr to help release it automatically. @@ +203,2 @@ > > + int rotation = mOrientation->OnSensorChanged(aSensorData, mCurrentRotation); Since the rotation will be returned as -1 (not valid), here should check this condition and return earlier. @@ +227,1 @@ > if (NS_FAILED(screen->SetRotation(rotation))) { The data type of argument here is uint32_t but rotation is int32_T. ::: widget/gonk/ProcessOrientation.cpp @@ +18,5 @@ > + */ > + > +#include "base/basictypes.h" > +#include "mozilla/ClearOnShutdown.h" > +#include "mozilla/StaticPtr.h" Does this file use anything from ClearOnShutdown.h and StaticPtr.h? @@ +164,5 @@ > + return mProposedRotation; > +} > + > +int > +ProcessOrientation::OnSensorChanged(const SensorData & event, The style on OrientationObserver class is "datatype& data". so it would be "const SensorData& event" @@ +366,5 @@ > +{ > + // If there is no current rotation, then there is no gap. > + // The gap is used only to introduce hysteresis among advertised orientation > + // changes to avoid flapping. > + if (currentRotation >= 0) { if (currentRotation < 0) { return true; } If we do this first then we can save one layer of if-else in code section below. ::: widget/gonk/ProcessOrientation.h @@ +31,5 @@ > +public: > + ProcessOrientation() {}; > + ~ProcessOrientation() {}; > + > + int OnSensorChanged(const mozilla::hal::SensorData &event, int deviceCurrentRotation); mozilla::hal::SensorData& event

Hi, I changed code as per your suggestion. Please review it .

Comment on attachment 788534 [details] [diff] [review] Orientation Observer uses Accelerometer sensor Review of attachment 788534 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your great work here and please address the comments. ::: widget/gonk/OrientationObserver.cpp @@ +228,5 @@ > // The orientation from sensor is not allowed. > return; > } > > + if (NS_FAILED(screen->SetRotation((uint32_t)rotation))) { Please use C++ way to do this - static_cast. ::: widget/gonk/OrientationObserver.h @@ +62,2 @@ > > + int mCurrentRotation; May I know the purpose of this variable? It seems be no useful in OrientationObserver and we can move it into ProcessOrientation. ::: widget/gonk/ProcessOrientation.cpp @@ +203,5 @@ > + > + bool isAccelerating = false; > + bool isFlat = false; > + bool isSwinging = false; > + if (!skipSample) { Since this sample is skipped, I think we can return directly here. if (skipSample) { return -1; } @@ +210,5 @@ > + if (magnitude < NEAR_ZERO_MAGNITUDE) { > + ALOGD > + ("ProcessOrientation: Ignoring sensor data, magnitude too close to" > + " zero."); > + ClearPredictedRotation(); Since we hit the bad condition here and tried to clear predictedRotation, we can return here directly if the code after line 288 didn't need to process in this case. @@ +243,5 @@ > + if (abs(tiltAngle) > MAX_TILT) { > + ALOGD > + ("ProcessOrientation: Ignoring sensor data, tilt angle too high:" > + " tiltAngle=%d", tiltAngle); > + ClearPredictedRotation(); Same comment as line 214. @@ +278,5 @@ > + ("ProcessOrientation: Ignoring sensor data, no predicted rotation:" > + " tiltAngle=%d, orientationAngle=%d", > + tiltAngle, > + orientationAngle); > + ClearPredictedRotation(); Same comment as line 214. ::: widget/gonk/ProcessOrientation.h @@ +44,5 @@ > + // Returns true if the orientation angle is acceptable for a given predicted > + // rotation. This function takes into account the gap between adjacent > + // orientations for hysteresis. > + bool IsOrientationAngleAcceptable(int rotation, int orientationAngle, > + int currentRotation); Nits: Indent.

Did you mean to feedback+ this or r+?

(In reply to Andreas Gal :gal from comment #33) > Did you mean to feedback+ this or r+? Hi, I give the feedback+ only, because I have no permission to give review+. After Tapas update next patch and address my comments I will transfer it to Michael for review.

>> ::: widget/gonk/OrientationObserver.h >> @@ +62,2 @@ >> > >> > + int mCurrentRotation; >> May I know the purpose of this variable? It seems be no useful in OrientationObserver and we >> can move it into ProcessOrientation. I removed it. We don't need that at all in any files. Good Point. :). >>Since we hit the bad condition here and tried to clear predictedRotation, >>we can return here directly if the code after line 288 didn't need to process in this case. Do you mean that we shouldn't call UpdatePredictedRotation() based on some condition (which may or may not occur in future i.e in next polling sensor data) ? If this is the case then it is not possible. Please let me know if I need to more changes. Thanks a lot for your help.

(In reply to Tapas Kumar Kundu from comment #35) > Created attachment 790990 [details] [diff] [review] > Orientation Observer uses Accelerometer sensor > > >> ::: widget/gonk/OrientationObserver.h > >> @@ +62,2 @@ > >> > > >> > + int mCurrentRotation; > > >> May I know the purpose of this variable? It seems be no useful in OrientationObserver and we >> can move it into ProcessOrientation. > > I removed it. We don't need that at all in any files. Good Point. :). > > >>Since we hit the bad condition here and tried to clear predictedRotation, > >>we can return here directly if the code after line 288 didn't need to process in this case. > > Do you mean that we shouldn't call UpdatePredictedRotation() based on some > condition (which may or may not occur in future i.e in next polling sensor > data) ? If this is the case then it is not possible. > I think that I misunderstood line number. You are telling about line mProposedRotation = mPredictedRotation; Still, I think that I should not return just after calling ClearPredictedRotation() . This is needed because "mProposedRotation = mPredictedRotation;" ==> This line will reset mProposedRotation of low pass filter in current pass and this will also force to accept new proposed rotation value from low pass filter algorithm in next polling sensor data. Please let me know if I need to do more changes. > Please let me know if I need to more changes. Thanks a lot for your help.

Comment on attachment 790990 [details] [diff] [review] Orientation Observer uses Accelerometer sensor Review of attachment 790990 [details] [diff] [review]: ----------------------------------------------------------------- It is good for me then transfer to :mwu for final review. Thanks for addressing my comments.

Comment on attachment 790990 [details] [diff] [review] Orientation Observer uses Accelerometer sensor I met some build errors in ICS within this patch. /gecko/widget/gonk/ProcessOrientation.cpp:173: error: 'ALOGD' was not declared in this scope

(In reply to viral [:viralwang] from comment #38) > Comment on attachment 790990 [details] [diff] [review] > Orientation Observer uses Accelerometer sensor > > I met some build errors in ICS within this patch. > > /gecko/widget/gonk/ProcessOrientation.cpp:173: error: 'ALOGD' was not > declared in this scope ALOGD works fine in jb_mr2 port of FFOS. I can define a typical macro LOGD and upload a new patch here. It will work for ICS. @mwu/mchen : Please let me know your suggestions.

You can use the define - ANDROID_VERSION to check what log function you need to use for ICS or JB.

#ifndef ALOGD #define ALOGD LOGD #endif

Please make sure Gecko works against ICS and JB gonk as long we support ICS (as in the binary gecko build can be put onto either system).

I used following code for logging in new file ProcessOrientation.cpp: +#undef LOG + +#include <android/log.h> + +#define LOGI(args...) __android_log_print(ANDROID_LOG_INFO, "ProcessOrientation" , ## args) BTW, this is the same code which is in GonkSensor.cpp . It works for both jb_mr2 and ics. Please let me know if I need to make any changes. Thanks for your help

Comment on attachment 793747 [details] [diff] [review] orientation_fix_for_gecko_ICS_JB_MR2.patch Review of attachment 793747 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/OrientationObserver.h @@ +60,2 @@ > uint32_t mAllowedOrientations; > + mozilla::ProcessOrientation* mOrientation; Please use mozilla::ScopedDeletePtr - this will automatically delete ProcessOrientation if it's necessary. ::: widget/gonk/ProcessOrientation.cpp @@ +25,5 @@ > +#include "mozilla/HalSensor.h" > +#include "math.h" > +#include "limits.h" > + > +#undef LOG Hmm I would prefer not to undefine macros like this. I also only see the use of LOGI in this function - what are we undefining this for? @@ +29,5 @@ > +#undef LOG > + > +#include <android/log.h> > + > +#define LOGI(args...) __android_log_print(ANDROID_LOG_INFO, "ProcessOrientation" , ## args) This seems like it could be *extremely* noisy since there is logging every time we get new sensor events in ProcessOrientation::OnSensorChanged. Can we disable logging by default?

I changed it with your suggestions

I see that LOG is no longer undefined, and LOGV is used instead of LOGI now. However, the switch to ScopedDeletePtr was not done and there should be no logging at all rather than logging at the verbose log level. Android usually strips verbose logging rather than leaving it in.

>> I see that LOG is no longer undefined, and LOGV is used instead of LOGI now. However, the >> switch to ScopedDeletePtr was not done and there should be no logging at all rather than >> logging at the verbose log level. Android usually strips verbose logging rather than >> leaving it in. ScopedDeletePtr is fixed now. I should have done it earlier. I changed log level to debug as these logs will be helpful during orientation debugging.

Comment on attachment 794297 [details] [diff] [review] orientation_ICS_JB_MR2.patch Review of attachment 794297 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. Also a general comment on what we're doing here - I don't really think Android's orientation heuristics are that good, but they seem better tested and smarter than what we have now, so let's take it. ::: widget/gonk/ProcessOrientation.cpp @@ +26,5 @@ > +#include "math.h" > +#include "limits.h" > +#include "android/log.h" > + > +#define LOGD(args...) __android_log_print(ANDROID_LOG_DEBUG, "ProcessOrientation" , ## args) I meant that there should be no logging at all by default, but a compile time switch to turn it on is fine. So, something like #if 0 #define LOGD(args...) __android_log_print(ANDROID_LOG_DEBUG, "ProcessOrientation" , ## args) #else #define LOGD(args...) #endif

This has only following changes : #if 0 #define LOGD(args...) __android_log_print(ANDROID_LOG_DEBUG, "ProcessOrientation" , ## args) #else #define LOGD(args...) #endif

Comment on attachment 796425 [details] [diff] [review] orientation_ICS_JB_MR2__.patch Review of attachment 796425 [details] [diff] [review]: ----------------------------------------------------------------- Carrying r+ from mwu . #comment 49 (https://bugzilla.mozilla.org/show_bug.cgi?id=788975#c49) .

Please check-in #attachment 796425 [details] [diff] [review] (https://bugzilla.mozilla.org/attachment.cgi?id=796425)

(hint: mark the older patch obsolete to avoid confusion)