Closed Bug 904484 Opened 11 years ago Closed 11 years ago

[Camera] Camera app can not calculate the orientation if there's no orientation sensor

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Milestone:
1.3 Sprint 3 - 10/25
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: viralwang, Assigned: johnhu)

References

Details

Attachments

(1 file, 3 obsolete files)

use accelerometer to calculate orientation.
88 bytes, text/html
djf
: review+
Details 
In bug 788975, we plan to used gravity sensor to calculate orientation for power saving.
For some devices without orientaion sensor support, we can not have beta and gamma from orientatoin sensor.
The calculate in camera.js will get wrong orientation since no beta and gamma available.
I am not sure that screen orientation APIs can provide such capability
  a. App fixed it's orientation to a certain position. ex: primary-portrait.
  b. App still can listen event of orientationchange for knowing the device position now.
What we learn from bug 840147 is: (See 898385)
Use 'deviceorientation' event is consuming additional power, so we don't use it anymore in system to decide the default orientation for the app when it's doing transition. But orientation lock should be accessible anyway in any device. 

Avoid using deviceorientation event to calculate the orientation by app itself, but orientaitonchange event should also be ok if the orientation is not locked by app or someone else. (though there's a delay between device orients and the event really fired as we observed.)

What's the need to calculate orientation on your own? Let's find alternative way because this is the native restrict of the sensor.
Since we have no beta and gamma from orientation sensor in some devices, the alternative solution is |window.screen.mozOrientation|.

If we change to use this way, we need to do something as the following:

1. remove |"orientation": "portrait-primary"| in camera/manifest.webapp

2. use |window.screen.mozOrientation| to replace |e.beta and e.gamma| in |orientChange: function camera_orientChange(e) {}|


3. add media query rules for landscape mode in camera.css.

Because these changes look huge we would like to hear your suggestion.
Flags: needinfo?(dflanagan)
Flags: needinfo?(dale)
Yeh deviceorientation doesnt get fired when we lock the orientation. The main reason we use the screenorientation API and lock deviceorierntation is to remove the rather nasty transition that is needed if we rotate the screen. Android does have this nasty transition, I dont have an iOS device to test off hand but I believe it doesnt.

Doing the above wont be a huge change, it is quite big, but if we arent going to have an orientation then it seems like the only way, up to the product owners to decide though
Flags: needinfo?(dale)
There may be another thing need to be discussed. Does the preview from camera object can be rotated on the fly?? Its size and rotation is calculated while camera is got.
Since bug 788975 is koi+ now, shall we also mark this bug as koi+
blocking-b2g: --- → koi?
Please file a bug for doing this for video too if this doesn't fix both.
blocking-b2g: koi? → koi+
Option 1: Can we modify the camera to use the gravity sensor instead of the orientation sensor? That would give us the power savings, but allow us to keep the nice smooth rotation that Dale describes in comment 4.

Option 2: What if we don't do anything on devices without an orientation sensor? If I recall correctly, the orientation is only used to rotate a few icons so that they look right in landscape mode.  What if we just don't rotate them?  The user can still rotate the phone and take a picture. The icons will still be recognizable, and the buttons will still function even if they don't get rotated.

Option 3: We've got various on-screen toggles in the camera (flash on/off, photo/video, front/back). We could also add a landscape/portrait toggle for devices that don't know their orientation, if that would help users.  (Can Gaia determine whether the underlying hardware supports orientation, or would that have to be a build-time option?)

I'm a little more concerned about the camera firmware: does it know its orientation well enough to produce a rotated image or to set an EXIF rotation flag for the image?  Or will cameras without the orientation sensor require the user to manually rotate photos in the Gallery?

(In reply to John Hu [:johnhu] from comment #5)
> There may be another thing need to be discussed. Does the preview from
> camera object can be rotated on the fly?? Its size and rotation is
> calculated while camera is got.

I'd guess that the preview stream would not change, but we'd have to use a CSS rotation to display it correctly if we switched to using mozOrientation

(In reply to Dietrich Ayala (:dietrich) from comment #7)
> Please file a bug for doing this for video too if this doesn't fix both.

I'm almost certain that this bug will cover both still photos and video recording.
Flags: needinfo?(dflanagan)
> I'm a little more concerned about the camera firmware: does it know its
> orientation well enough to produce a rotated image or to set an EXIF
> rotation flag for the image?  Or will cameras without the orientation sensor
> require the user to manually rotate photos in the Gallery?

I don't know if camera firmware knows it internal, or not. But the current implementation is to use the calculated orientation at gaia side to tell camera(picture: [1], video: [2]). So the Option 2 is not a possible solution that it won't have the orientation for camera to write the correct EXIF.

[1] https://github.com/mozilla-b2g/gaia/blob/726777cbe7dc4f3a3731fcf4629bea56c352a603/apps/camera/js/camera.js#L1319
[2] https://github.com/mozilla-b2g/gaia/blob/726777cbe7dc4f3a3731fcf4629bea56c352a603/apps/camera/js/camera.js#L511
(In reply to John Hu [:johnhu] from comment #9)
> > I'm a little more concerned about the camera firmware: does it know its
> > orientation well enough to produce a rotated image or to set an EXIF
> > rotation flag for the image?  Or will cameras without the orientation sensor
> > require the user to manually rotate photos in the Gallery?
> 
> I don't know if camera firmware knows it internal, or not. But the current
> implementation is to use the calculated orientation at gaia side to tell
> camera(picture: [1], video: [2]). So the Option 2 is not a possible solution
> that it won't have the orientation for camera to write the correct EXIF.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> 726777cbe7dc4f3a3731fcf4629bea56c352a603/apps/camera/js/camera.js#L1319
> [2]
> https://github.com/mozilla-b2g/gaia/blob/
> 726777cbe7dc4f3a3731fcf4629bea56c352a603/apps/camera/js/camera.js#L511

I wonder why not use screen.orientation directly?
Yes, I think Gary is proposing to do so. That's also the way suggested by Dale.
Hi all,
   Thanks for everyone's feedbacks. It seems that using |screen.orientation| is a good way and will cover much more things.
Since Gary is still busy on keyboard bugs, I can take it and start to implement it.
Assignee: nobody → johu
I realize the nasty transition mentioned by Dale.

If we remove the |"orientation"| settings from manifest.webapp to use native rotation support. When we rotate the phone, the screen is frozen about 1 to 2 seconds and rendered. I will continue to make a patch and find if there is any way to prevent it.
After some implementation tests, to use |screen.orientation| seems not possible. If we need to use |screen.orientation|, we have to remove |"orientation"| settings from manifest.webapp. But once doing that, the rotation is not smooth, like comment 4 mentioned.
I am not entirely certain about the webcontent implications, but changing the screenOrientation api to fire even if orientation is locked may be possible?
from an app perspective that would be perfect, our code isnt quite as smart as the platforms (and duplicates it + has had a bunch of bugs)
Yes, Dale. It is an important to have screenOrientation api to fire event if it is locked. I will try to discuss with viral and related guys about this question.
Attached video wip based on screen.orientation (obsolete) — 
The attached video is the WIP with screen.orientation. The drawback of this version is the froze symptom. We may need an API to lock the screen orientation and still have the orientationchange event fired.
I talked with Marco about this today.

I think that we should switch the camera app to use the devicemotion events here. I understand that it'd be easier to somehow hook up to the existing gecko logic for screen orientation, but we should generally stick to existing web standards where possible.

If someone wants to propose an addition to an existing spec, or propose a new spec, to add support for reusing the platform logic for screen rotation even when the screen orientation is locked, then I'm happy to help with proposing that to W3C.

But we shouldn't add a new non-standard platform API here given that there are existing ones that we can use.
Jonas, 

Thank you. You give us another light, using device motion. I am not familiar with it. But after some tests, we may use the similar way to estimate the screen orientation. We may change the calculation from using angle of alpha, beta, and gamma to use gravity value of x, y, and z.

I am not sure if there is any best equation to calculate it. But I found a simple rule may have correct behavior, like the one using deviceorientation event:
abs(x) < 3 and gravity(y) > 5 => 0
abs(y) < 3 and gravity(x) < -5 => 90
abs(x) < 3 and gravity(y) < -5 => 180
abs(y) < 3 and gravity(x) > 5 => 270

If I am wrong, please tell me.
Attached patch WIP based on device motion (obsolete) — — Splinter Review 
Hi Dale and Gary,

I am not sure if this patch is the correct way to fix this bug. But the device motion looks similar to device orientation. So, I use similar algorithm to calculate the orientation with device motion information.

Please give me some feedback about this implementation.

Gary, I heard that you have lots special cases for testing the orientation bug. Please help me to test it.
Attachment #804288 - Flags: feedback?(gchen)
Attachment #804288 - Flags: feedback?(dale)
Attachment #796427 - Attachment is obsolete: true
Comment on attachment 804288 [details] [diff] [review]
WIP based on device motion

Can you please explain why we need to use devicemotion instead of device orientation ? Sorry for jumping in. I did some work in https://bugzilla.mozilla.org/show_bug.cgi?id=788975 for orientation sensor.
Hi Marco/John,

Is camera app orientation not same as browser orientation? If browser app orientation is working properly (using accelerometer sensor) then camera app orientation should also work in same way.
Flags: needinfo?(mchen)
Comment on attachment 804288 [details] [diff] [review]
WIP based on device motion

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

looks good, but I'm not sure |devicemotion| value get from which hardware sensor?
maybe we can ask viral next Monday?
Attachment #804288 - Flags: feedback?(gchen) → feedback+
can you help to check out which sensor support |devicemotion|?
Flags: needinfo?(vwang)
(In reply to Tapas Kumar Kundu from comment #24)
> Hi Marco/John,
> 
> Is camera app orientation not same as browser orientation? If browser app
> orientation is working properly (using accelerometer sensor) then camera app
> orientation should also work in same way.

Hi Tapas Kumar Kundu
    John Hu had WIP patch before with same orientation as browser, but the performance looks not good.
    You can see the video https://bug904484.bugzilla.mozilla.org/attachment.cgi?id=796427
(In reply to GaryChen [:GaryChen] from comment #27)
> (In reply to Tapas Kumar Kundu from comment #24)
> > Hi Marco/John,
> > 
> > Is camera app orientation not same as browser orientation? If browser app
> > orientation is working properly (using accelerometer sensor) then camera app
> > orientation should also work in same way.
> 
> Hi Tapas Kumar Kundu
>     John Hu had WIP patch before with same orientation as browser, but the
> performance looks not good.
>     You can see the video
> https://bug904484.bugzilla.mozilla.org/attachment.cgi?id=796427

Hi,

I found that devicemotion is activating 3 sensors Linear accelerometer, accelerometer and gyroscope
from below reference :

[1] http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp?from=nsEventListenerManager.cpp#l313 
[2] http://dxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp?from=nsEventListenerManager.cpp#l407

I want to know why we want to use device motion for orientation. Activating 3 sensors for orientation is very bad idea. It will effect power usage of device.

Why don't we use existing orientation sensor observer like Browser app ? It will help to reduce power usage and also you don't need to recalculate angle of rotation again :).
(In reply to Tapas Kumar Kundu from comment #28)
> 
> Hi,
> 
> I found that devicemotion is activating 3 sensors Linear accelerometer,
> accelerometer and gyroscope

Please and welcome to join Bug 910150 for discussing this issue.

> Why don't we use existing orientation sensor observer like Browser app ? It
> will help to reduce power usage and also you don't need to recalculate angle
> of rotation again :).

Because Camera app want to customize it's own transition or animation of screen rotation, you can see the attached video for how bad of the camera using auto rotation.

So Camera app tried to detect the angle by itself then use CSS transform to do some transition of icons. That is beautiful indeed.

According to acceleration sensor is more popular then orientation sensor especially on low end device so it is better that Camera app uses acceleration sensor to calculate the angle not orientation sensor.
Flags: needinfo?(mchen)
> So Camera app tried to detect the angle by itself then use CSS transform to
> do some transition of icons. That is beautiful indeed.

Oh ok.. I got it. I think that we can still use existing ProcessOrientation Class to acheive this. ProcessOrientation Class suggests new orientation angle and camera app may ignore it or rotate screen as per its needs.

That way, we can reuse same orientation calculation algorithm. We need to do smaller change in sensor listener class for this.

> According to acceleration sensor is more popular then orientation sensor
> especially on low end device so it is better that Camera app uses
> acceleration sensor to calculate the angle not orientation sensor.

ProcessOrientation class depends on Accelerometer sensor output which known to use less power on all devices.
Hi Gary,

I found the patch will need to use DeviceMotionEvent.accelerationIncludingGravity for calculate.
It comes from gravity (accelerometer) sensor.
Flags: needinfo?(vwang)
(In reply to Tapas Kumar Kundu from comment #30)
> ProcessOrientation class depends on Accelerometer sensor output which known
> to use less power on all devices.

Tapas,

Thanks for your suggestion. But what's the ProcessOrientation?? Does it have a web api for us to use it??
(In reply to viral [:viralwang] from comment #31)
> Hi Gary,
> 
> I found the patch will need to use
> DeviceMotionEvent.accelerationIncludingGravity for calculate.
> It comes from gravity (accelerometer) sensor.

thanks
Macro, 

What do you suggest about the power consumption issue?? Should I make this bug depend on bug 910150?? Or should I just use it and wait for power consumption issue??
Flags: needinfo?(mchen)
Issues been clarified since needinfo, but yeh if the patch doesnt have any power consumption improvements I would suggest blocking it on the less power wasteful api being exposed
Flags: needinfo?(mchen)
Sorry meant to clear my f?, not the needinfo from https://bugzilla.mozilla.org/show_bug.cgi?id=904484#c34
Flags: needinfo?(mchen)
Comment on attachment 804288 [details] [diff] [review]
WIP based on device motion

Issues been clarified since needinfo, but yeh if the patch doesnt have any power consumption improvements I would suggest blocking it on the less power wasteful api being exposed
Attachment #804288 - Flags: feedback?(dale) → feedback+
(In reply to John Hu [:johnhu] from comment #32)
> (In reply to Tapas Kumar Kundu from comment #30)
> > ProcessOrientation class depends on Accelerometer sensor output which known
> > to use less power on all devices.
> 
> Tapas,
> 
> Thanks for your suggestion. But what's the ProcessOrientation?? Does it have
> a web api for us to use it??
No.

I think that marco is doing discussion on https://bugzilla.mozilla.org/show_bug.cgi?id=910150 . I think that he can clear your doubt :).
Hi all,

After discussing with Jonas during Oslo workweek, the conclusion are

  1. The new sensor WEB API [1] didn't be accepted because sensor Web API almost is a W3C standard already. We still can propose this idea into W3C for considering as a standard.

  2. For power issue on comment 18 and bug 910150, it is under the same situation. And it is under discussion on mozilla web api mail loop too. And this one is more general and should be fixed surely.

So for V1.2, I would say that we still use devicemotion sensor to detect the angle by camera app and accept the drawback of power consumption. 
For long term, it is not the issue from Camera but a general issue from sensor API of view. So we need to propose it into W3C.

[1] To propose a new sensor Web API for reporting the screen rotation value (portrait or landscape) but didn't do auto screen rotation.
Flags: needinfo?(mchen)
This patch uses device motion to calculate the device orientation. The main algorithm is to use acceleration of X and Y to estimate the device orientation.
Attachment #808999 - Flags: review?(dflanagan)
Target Milestone: --- → 1.3 Sprint 1 - 9/27
Comment on attachment 808999 [details]
use device motion to calculate the orientation

John,

The patch seems to work, but I'm giving r- because I think we can do better.

1) The spec says that deviceorientation events are only sent when the orientation changes significantly, but that devicemotion events are sent frequently. I'd like to know if there is a practical difference in the number of events generated and whether that has an impact on battery use and performance.  If we're always handling device motion events is that going to affect performance elsewhere in the app?

2) With this patch, if I move my phone rapidly to the left or right, I can get it to change orientation by simulating gravity.  I think we should either "debounce" the sensor by doing a time weighted average of the readings.  Or you should subtract acceleration from accelerationIncludingGravity to get gravity only

3) Your heuristic algorithm seems to work well enough, but I'd like more justification for it. Why 3 and 5?  I think that you should at least choose a value of z acceleration above which you won't change the orientation.   I guess I'd just like to be a little more mathematical here so we know what we're doing and how to tweak the algorithm if we want to change it later.  I'm sort of fascinated by this, so I'm happy to discuss this with you if you want.
Attachment #808999 - Flags: review?(dflanagan) → review-
Also, I think we should add code to stop listening to the sensors when visiblity changes (we already do this for geolocation, but should also do it for devicemotion, I think).  Maybe even turn off the sensors when in the filmstrip. So it that case, we'd register and unresister the device motion sensor when we lock and unlock the screen.

We never currently turn off the deviceorientation event handler.  I wonder if those events are not delivered when the camera app is not visible.  If they are delivered, then that is presumably a background drain on the battery any time the camera app is running.
Here's more thoughts about how we can make the algorithm more mathematical:

If the camera is pointing straight up or straight down, then the acceleration will all be in the z component and will be 0 in x and y axes.  We can't determine the orientation in that case and just leave it at whatever orientation is currently is in.  If the camera is mostly straight up or straight down, then we don't want to change the orientation either.

Let's pick an angle and then say that we only switch the orientation if the phone is more than that angle from horizontal. I think (if I've done my trig correctly) that at an angle a from horizontal, the z component of acceleration will be 9.8 * cos(a).  So if we want to keep orientation constant at angles less than 20 degrees, we should check z, and if it is > than 9.2, we know that the phone is close to horizontal and we can just return without changing the orientation.

If z is < 9.2, then we know that it is more than 20 degrees from horizontal. In that case, we can just look at the values of x and y and base the orientation on whichever one has the largest (farthest from 0) value.

As the code stands now, with the 5 and 3, I think the angle at which it stops changing orientation varies between 30 and 36 degrees, depending on the x and the y values. 

As noted above, you might want to subtract acceleration from accelerationIncludingGravity to get only the forces due to gravity and ignore any user-caused acceleration before making these calculations.  Or, you could just do a simple test: if x*x + y*y + z*z > 100 then assume the phone is being accelerated by motion and don't change the orientation until the external acceleration stops.
Thanks David, 

I need the knowledge about orientation staff. 

Replying the questions in comment 41:
1. with the test sensors app, I found the device orientation also changes frequently. Because it is calculated from the magnetometer which changes frequently. As to device motion, it changes on the predefined time interval which is 100ms in the test sensors app. To have better performance, I can change the code to by pass some events.

2. Yes, I also found it. Thanks for this finding. I will subtract the acceleration part.

3. It's a little shame to say that I am not familiar with orientation things. So, I just make a porting from the device orientation version. I will read comment 43 to have another version of algorithm.

As to comment 42, gecko stops all events when the app is removed from iframe. So, we may not need to remove it when the visibility changed. As to the part of filmstrip, I don't think we should stop the event. The rotation of filmstrip depends on this. If we turn it off, the filmstrip won't rotate anymore.
I found the subtraction of acceleration is not possible to fix the bouncing issue. I should introduce a code to debounce it.
According to the spec, the acceleration may be null when the hardware doesn't know how to remove gravity from the acceleration data.
Wow, the comment 43 is really useful. I found the x*x + y*y + z*z > 100 is the magic equation for debouncing. We may catch almost all fast movement when it is true. Although it somethings catches the normal rotation,  we can still correct it with the upcoming events.
BTW, the <3 is for less than 30 degree and >5 is for larger than 45 degree.
Comment on attachment 808999 [details]
use device motion to calculate the orientation

I had added the following to this patch:
1. the explicit add/remove event listener about devicemotion. This may be redundant. But I feel it is more robust when we handles it in camera app.
2. add the code to bypass some events. The code processes one event within a 500ms timeframe. I found Inari, ZTE One, and Unagi have 100 as its e.interval. So, We can bypass 4 events after processing one event.
3. add the code to debounce. I use the equation in comment 43: x*x+y*y+z*z > 100. But I changed the value to 110. That's because we may get value over than 100 but less than 110 when we rotate the phone gently. With the bypassing code, the value 100 makes phone bypass 9 events after processing one.
4. add the code to handle the case of z. I use the same algorithm from comment 43 to do it.
Attachment #808999 - Flags: review- → review?(dflanagan)
Comment on attachment 808999 [details]
use device motion to calculate the orientation

I still think that this algorithm can be better (at least theoretically).  See my comments on github.
Attachment #808999 - Flags: review?(dflanagan) → review-
Comment on attachment 808999 [details]
use device motion to calculate the orientation

I had changed the patch to use the algorithm to calculate the pitch and roll and use them as beta and gamma.

Then, we can keep the original 45 and 135 values of beta and gamma to determine the orientation. The result looks correct.

I found the equations from some chipset venders' website. But some of them may put accelerometer chip in different orientation. So, the pitch and roll may be reversed.
Attachment #808999 - Flags: review- → review?(dflanagan)
Comment on attachment 808999 [details]
use device motion to calculate the orientation

See my comments on Github. The final step that does the beta and gamma angle comparisons is opaque to me. Explain in a comment, or link to code in gecko that uses the same comparisons.  (I'd also recommend changing the code to use if/else instead of the deeply nested conditional operators.)

I think if you're going to use beta and gamma, then you don't need the z>9.2 comparison.  And note my comment about the range of the gamma angle in the deviceorientation spec.

Or, if you don't understand the existing code either, change it to something that you can explain. For me, computing the atan2(y,x) to get the angle of rotation around the z axis is how I can comprehend this code, and that is how I would tackle it.

I haven't tried that, though, and maybe I'm just doing the math wrong and that doesn't work. If the beta and gamma solution is what works for you, then I'm okay landing this code with just an added comment.
Attachment #808999 - Flags: review?(dflanagan) → review-
Sorry about that.

I do not understand the existing code either. I will rethink the angle comparisons part. I just feel that the existing code is the correct version and try to make a equivalent algorithm to convert device motion to device orientation.

I had thought the way using z axis. But I can't understand about using the angle of z to determine the orientation. I think the x and y is more important than z when determining orientation. That's why I change back to beta and gamma version. May you tell me more about using z to determine the orientation??
Flags: needinfo?(dflanagan)
David,

I found a similar patch with your thought in gecko side, bug 788975. The patch faces similar problem. But they use tilt, atan(y/x), to calculate the orientation[1]. I will check the code and understand how they do it.

I had also discussed the orientation calculation with Viral Wang. He told me that the pitch and roll is easy to calculate and the angle 45 is the original version in gecko with orientation sensor. But after the removal of orientation sensor, the patch of bug 788975 changed the way to use tilt. The computation of tilt is relatively more complex than pitch and roll. I will also try to realize why they use tilt to calculate it instead of pitch and roll.

[1]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/ProcessOrientation.cpp
Sorry, please ignore my comment 54.

I am wrong about atan2(y, x), again. After discussing with Marco and Viral, I realize that atan2(y,x) is not tilt and more useful than pitch and roll. But gecko use tilt to surprise the unwanted rotation when we put the phone on a flat plane. In my implementation, we use abs(rawZ) > 9.2 to do that. Beside that, we also need another stabilization works to eliminate the fast orientation. If our devices have orientation sensor, to use pitch and roll is simpler than using atan(y, x) because the flat plane, debouncing and stabilization are handled by chipset. If we don't have the orientation sensor, pitch and roll seems not much useful.

I will create another version to use angle of z axis to deal with it and add other codes to handle stabilization.
Flags: needinfo?(dflanagan)
Sorry for my previous brainless patch. I change the implementation in this path and use the rotation angle of z axis to calculate the orientation in this patch.

This patch contains the following changes:
1. rename orientChange to handleMotionEvent
2. use low-pass filter to remove the noise of accelerometer data which is copied from Gecko's PhoneOrientation.cpp
3. use x*x + y*y + z*z to filter fast movement
4. use 9.2 (20 degrees) to filter low tilt degrees
5. use atan2(-x, y) to calculate the rotation angle of z axis
6. use timer to filter the fast rotation
Attachment #804288 - Attachment is obsolete: true
Attachment #808999 - Attachment is obsolete: true
Attachment #817711 - Flags: review?(dflanagan)
Target Milestone: 1.3 Sprint 1 - 9/27 → 1.3 Sprint 3 - 10/25
Comment on attachment 817711 [details]
use accelerometer to calculate orientation.

John,

As usual, I'm sorry that it has taken me so long to review.

I have some suggestions on Github. Mostly about commenting things. And one more serious suggestion about avoiding unnecessary object creation.  But otherwise, this looks ready to land.

Unless you are willing to consider factoring it all out into a separate file. I imagine a file that contains a self-invoking function and defines no global symbols.  It would listen to device motion events and emit a custom event "orientationchange" or something when the device is rotated.  In a separate module the code becomes simpler because you can just use variables inside the function rather than defining more properties of the Camera object.

It is not necessary to do that, but eventually when we refactor Camera, we'll want to break stuff like this out, so feel free to do it now, if you want.

I've really enjoyed getting to review this patch. I've learned from it and hope you have as well.  Thanks for your patience!
Attachment #817711 - Flags: review?(dflanagan) → review+
Thanks.

I had also learned lots from this app. I will move the code out as a separate file.
The testing errors in travis is not related to this patch. They are:

test_number_keyboard.py test_number_keyboard.TestNumberKeyboard.test_number_keyboard
test_persona_app.py test_persona_app.TestPersonaStandard.test_persona_standard_sign_in

The patch is merged to master:
https://github.com/mozilla-b2g/gaia/commit/d7bc63777b7baba1dbc96e2f2ffaeb244cb3c86c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Uplifted d7bc63777b7baba1dbc96e2f2ffaeb244cb3c86c to:
v1.2: 9396d219c7974f2f36548d6206f949027d1f2959
status-b2g-v1.2: --- → fixed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: