806582 - [camera] Still images captured by camera are saved to disk with a 90 degree rotation (portrait or landscape)

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of issue

Unagi device using:

Gaia: ddafa86fc92914306be99c8458362cfc96d44e25
Gecko: f950df2007d07a8a6f5c5d5d29383c832249a22e

STR:
1. Take a picture with the camera in portrait mode
2. Open the gallery to see it

Expected: I would see the picture oriented in portrait mode
Actual: The picture is show rotated 90 degrees.

See attached screenshot.

Comment 1

[Tony Chung [:tchung]]

smoketest blocker on 10/29 unagi daily build.   This was working last week.

regression from bug 803733?

Comment 2

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

This was an unexpected regression from bug 803733.

What's responsible for setting the orientation of saved photos?

Comment 4

[Dale Harvey (:daleharvey)]

:cjones

The camera app can provide an orentation to the CameraControl API at which angle to save photos, however as mentioned in the other bug it seems different hardware has different opinions on what the base orientation is. 

If I download the latest nightly and do a full flash, will I get a normalised base orientation which works across at least otoro + unagi?

Comment 5

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

We're supposed to be normalizing away the vendor quirks in gecko, but that's apparently not happening for saved photos.

I can confirm that the *preview* is rendered at the correct orientation, so this bug seems to just affect the saved images.

tchung tells me that a clean flash with the latest nightly reproduces this bug.

Comment 6

[John Shih]

I'm sure it's the dupe of bug 805737

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Fix orientation of still photos

https://github.com/mozilla-b2g/gaia/pull/5909 removed this code which corrected the orientation of still images.

The 270 isnt a hardcoded reference to the camera angle, its because the camera is mounted in landscape and the application is fixed to portrait (so a 90 degree difference), also the angle passed in is the angle the icons rotate which is the opposite direction to the way the device is rotating (so the icons stay at the same angle)

Comment 8

[Dale Harvey (:daleharvey)]

Tested with a fresh flash of: https://releases.mozilla.com/b2g/latest/otoro_2012-10-29_ics_us.zip

Comment 9

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

Comment on attachment 676437 [details]
Fix orientation of still photos

This is going to break the "real" device.

We need to fix this in gecko.

Comment 10

[Dale Harvey (:daleharvey)]

What orientation is the camera expected to be mounted at?

This code expects the camera orientation to be 270deg when in portrait and 0deg when the device is in landscape with the 'top' facing right

Comment 11

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

Bug 803733 added a device property ro.moz.cam.%d.sensor_offset that's the "natural" offset of the mounted camera.  We use this in gecko but have apparently "missed a spot".  That's what we need to fix here.

Gaia should not even be attempting to work around hw quirks.  That way lies madness! ;)

Comment 13

[Dale Harvey (:daleharvey)]

:cjones

By 'fix in gecko' do you mean take the ability to specify the orientation out of API that cameracontrol gives to gaia / web content and have it figure that out from the sensors itself?

If so going to reassign to :mikeh as that sounds like a large change that he will need to know about

Comment 14

[Dale Harvey (:daleharvey)]

And just to follow up, this wont break on other devices as long as the natural offset is used to normalise the orientation passed in by gaia, the patch isnt (cant) work around hardware quirks, just quirks with the orientation value I was already storing. whether its up to gaia or gecko to figure out what angle to store the photo at is a different question (that can be quite problematic, say the phone is parallel to the floor should it be landscape or portrait?)

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

Note: 90-degrees CW.

Comment 16

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Mike Habicher [:mikeh] from comment #15)
> Note: 90-degrees CW.

Actually, I just took a series of test images:
- phone in portrait, image is rotated 90o CW
- phone rotated 90o CW from portrait, image is rotated 90o CCW
- phone upside down (180o CW from portrait), image is rotated 90o CW
- phone rotated 270o CW (or 90o CCW from portrait), image is rotated 90o CCW

So (1) there's definitely an offset in the camera, but (2) it looks like there's a disagreement between the camera and the orientation API on which way is +degrees.

Comment 17

[Dale Harvey (:daleharvey)]

The orientation that is passed in is backwards since it is reusing the rotation value used for the icons (which rotate in the opposite direction of the device). Hence the patch ^

Comment 18

[Mike Habicher [:mikeh] (high bugzilla latency)]

In Gecko:
I/PRLog   ( 9911): 23607880[44712b00]: base sensor orientation = 90
I/PRLog   ( 9911): 23607880[44712b00]: 'ro.moz.cam.0.sensor_offset' = 270 (270) --> modified sensor orientation = 0

So net orientation at the hardware level is 0 degrees.

With layoutToPhoneOrientation returning |this._phoneOrientation|, JS passes to takePicture():
I/PRLog   ( 9911): 23607880[44712b00]: setting picture rotation to 0 degrees (mapped from 0)

With layoutToPhoneOrientation returning |270 - this._phoneOrientation|, JS passes to takePicture():
I/PRLog   (10036): 18807368[44812d30]: setting picture rotation to 270 degrees (mapped from 270)

So the solution is a two-parter:
1. 'ro.moz.cam.0.sensor_offset' needs to be dropped to 180 degrees
2. camera.js::layoutToPhoneOrientation needs to return |-this._phoneOrientation|.

Yeah?

Comment 19

[Dale Harvey (:daleharvey)]

I have updated the PR for the new reality :)

Also updating from a comment on the PR, either we have to take orientation out of the API exposed to content and have the camera figure it all out manually.

Or we keep orientation in the API, and gecko deals with normalising any orientation parameters to handle any differences on the hardware side.

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Mike Habicher [:mikeh] from comment #18)
> 
> So the solution is a two-parter:
> 1. 'ro.moz.cam.0.sensor_offset' needs to be dropped to 180 degrees
> 2. camera.js::layoutToPhoneOrientation needs to return |-this._phoneOrientation|.

With these changes:
I/PRLog   (10400): 25963080[44912b70]: base sensor orientation = 90
I/PRLog   (10400): 25963080[44912b70]: 'ro.moz.cam.0.sensor_offset' = 180 (180) --> modified sensor orientation = 270

...phone upright (earpiece at the top, mic at the bottom):
I/PRLog   (10400): 25963080[44912b70]: setting picture rotation to 270 degrees (mapped from 0)
...phone rotated 90o CW from upright:
I/PRLog   (10400): 25963080[44912b70]: setting picture rotation to 0 degrees (mapped from -270)
...phone upside down:
I/PRLog   (10400): 25963080[44912b70]: setting picture rotation to 90 degrees (mapped from -180)
...phone rotated 270o CW (or 90o CCW) from upright:
I/PRLog   (10400): 25963080[44912b70]: setting picture rotation to 180 degrees (mapped from -90)

...and saved images appear properly oriented in the gallery and in Ubuntu's Image Viewer.

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Dale Harvey (:dale) from comment #19)
> 
> Or we keep orientation in the API, and gecko deals with normalising any
> orientation parameters to handle any differences on the hardware side.

I'd rather not have Gecko trying to sort out the orientation sensor data in the camera driver.

I think the solution is to have JS feed us the orientation it believes applies to the device[1], and the platform layer will sort out any weirdness.

1. Orientation in degrees from the natural[2] orientation of the device, with positive values indicating clockwise rotation;
2. What we think of as 'natural' may vary from device to device, but will affect both the value returned by the orientation API *and* the .sensor_offset property, so that the net effect is the same: pictures appear upright[3] regardless of the rotation of the phone;
3. (For a person who is standing upright, not standing on her/his head. :)

Comment 22

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Adjust ro.moz.cam.0.sensor_offset (unagi)

This will require the following change in camera.js:

   layoutToPhoneOrientation: function camera_layoutToPhoneOrientation() {
-    return 270 - this._phoneOrientation;
+    return -this._phoneOrientation;
   },

Comment 23

[Dale Harvey (:daleharvey)]

Sorry yes when I said 'differences on the hardware side' I meant in relation to the angle at which the camera is mounted, not orientation sensor weirdness.

If the cameraControl API expects an orientation relative to the 'natural orientation' (and fixes for mounting angle issues etc), then the current PR should be good alongside its platform fix. 

Thanks

Comment 24

[Dale Harvey (:daleharvey)]

Comment on attachment 676437 [details]
Fix orientation of still photos

:mikeh confirmed patch worked alongside platform fix (for both otoro + unagi)

Comment 25

[Mike Habicher [:mikeh] (high bugzilla latency)]

This affects otoro as well.

Comment 26

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Adjust ro.moz.cam.0.sensor_offset (otoro)

Comment 27

[Dale Harvey (:daleharvey)]

gaia side pushed in: https://github.com/mozilla-b2g/gaia/commit/99a61770f49bb99e907ab499dccb26bf1abe03ad

(dont need to worry about breakage since its already broken)

Comment 28

[John Shih]

It turns into 180 degree now, please check bug 808918

Comment 29

[Andreas Gal :gal]

Dale, how are we doing here?

Comment 30

[Dale Harvey (:daleharvey)]

We had 3 changes land (gonk / gecko / gaia), that should have resulted in correct rotations on both devices once they all landed, I expect that people that still have incorrectly rotated photos arent getting a gonk update, but I will test that now

Comment 31

[Dale Harvey (:daleharvey)]

Confirmed with the latest nightly on unagi

Phones with an outdated gonk will need to flash nightly until fota updates are available

Comment 32

[John Shih]

Hi, I still meet this issue on otoro
is the fix only for unagi?

build info
2012-11-08
gaia master : 96594a9abb3bd9ebf3ccfd763e28caa64ce2776c
mozilla-aurora(hg) : 8ae22fe748fc

Comment 33

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to John Shih from comment #32)
> Hi, I still meet this issue on otoro
> is the fix only for unagi?

There's a fix for both, but it has only been merged on unagi.  Otoro is pending.

Comment 34

[Hubert Figuiere [:hub]]

I did ./repo sync && ./config.sh unagi && ./flash.sh

I am still getting the wrong orientation.

(on Unagi)

Comment 35

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Hub Figuiere [:hub] from comment #34)
> I did ./repo sync && ./config.sh unagi && ./flash.sh
> 
> I am still getting the wrong orientation.

Just to be absolutely clear, you did ./build.sh as well?

Try |rm -rf out| in your B2G folder to make sure there are no stale artifacts.

Comment 36

[Hubert Figuiere [:hub]]

Yes I did build.sh. My bad I didn't write it in the comment :-/ But I did.

I can try a super clean build. Will do.

Comment 37

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Hub Figuiere [:hub] from comment #36)
> Yes I did build.sh. My bad I didn't write it in the comment :-/ But I did.

Okay, just making sure.

> I can try a super clean build. Will do.

Merci beaucoup.  Can you also run this command and post the output?  (It should be 180.)
|adb shell getprop | grep sensor_offset|

Comment 38

[Hubert Figuiere [:hub]]

Sounds like our build system is imperfect. I indeed clobbered as advised in comment 37 and it now produce the correct result.

Thanks.

(yes I get 180 for the ro.moz.cam.0.sensor_offset property)

Comment 39

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Hub Figuiere [:hub] from comment #38)
> Sounds like our build system is imperfect. I indeed clobbered as advised in
> comment 37 and it now produce the correct result.
> 
> Thanks.
> 
> (yes I get 180 for the ro.moz.cam.0.sensor_offset property)

The fix involves components in gonk, gecko, and gaia.  If any of them are missing, the image will show up with an incorrect orientation.  Thanks for making sure!

Comment 40

[Dale Harvey (:daleharvey)]

The otoro patch has also landed so subsequent nightly / clean builds should be fixed on that device as well

Comment 41

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Dale Harvey (:daleharvey) from comment #40)
> The otoro patch has also landed so subsequent nightly / clean builds should
> be fixed on that device as well

\o/

(Thanks, cjones!)

Comment 42

[mlevin]

Verified fixed in Unagi build 20130102070202. Still needs to be verified for Otoro though.

Comment 43

[Stoyan Dimitrov [:stoyan]]

PR fixing the issue for inari
https://github.com/mozilla-b2g/device-inari/pull/18