The Camera app needs to be updated to make use of the new .sensorAngle attribute; see bug 932669 comment 35.

rexboy, will you be working on this?

ok, I can take it.

http://jsfiddle.net/6dVk6/ Just made a test to make use of trigonometric functions to calculate the displacement. This way we can use one formula for every angle of previews. I'm trying to apply this logic to viewfinder of camera app.

Sorry, the above one contains unnecessary alert pollution. Please don't open it. See this one: http://jsfiddle.net/6dVk6/1/

WIP https://github.com/rexboy7/gaia/commit/68c8ab41949f0d9aff3de3a862ef96f628d8ec83 I tried to make the code clear but not really easy :-/ I will put it into review after testing with phone with front camera.

I just tested it on flatfish, and seems it works good on both front camera and back camera. So let's put it into review. Hello Dale, I think the reviewer here is still you... may you help on it?

(In reply to KM Lee [:rexboy] from comment #6) > > I just tested it on flatfish, and seems it works good on both front camera > and back camera. So let's put it into review. Please also make sure that this patch works on a non-Flatfish device such as Unagi.

Comment on attachment 8345777 [details] Patch This is causing the preview to be upside down on my Peak Also I no longer work on the camera, so follow up review should go to wilsonpage or djf Cheers

As we just talked, Dale would you confirm if the Gecko version is m-c after 12/7? (Sorry I forgot to note it explicitly.) Thanks for the help!

And I can transfer the reviewer after confirming it.

I ran this again with a gecko built from m-c right now, then after with the latest nightly from http://downloads.geeksphone.com/ Still upside down on the peak, the front camera is correct

Thanks for your help Dale. I haven't borrow Peak yet, but after some testing I found this is reproducible on Buri. And, the cause here is that the angle reported to Gaia is not consistent -- Unagi reports 270 degree, whereas Buri reports 90 degree with exactly the same direction of preview. As a result, the patch can't do it right. This should be a platform issue, so I'll report this on bug 932669 and discuss how to fix it.

Rex, Please provide ETA for patch to land.

The patch itself has finished. So we just need to go through the review process and see what need to be changed. Roughly one week? But we shouldn't land it before bug 949930 has fixed (which looks like a Gonk issue), or it will break feature on Buri and Keon. It shouldn't be a complex issue, but I can't tell certainly how long it needs.

KM Lee, Could you provide the latest update on this bug. Has Dale's comments been addressed? Also as suggested please switch review to djf. Thanks Hema

Dale's comment is addressed on bug 949930, which should be a Platform/Gecko issue. I didn't send the review since I'm not sure if the result of bug 949930 would cause some changes on this patch. Previous solution Mike proposed in bug 949930 doesn't seem to work well. I would suggest solving bug 949930 first based on current situation.

Rex, Does fixing bug 949930 also fix this bug? Please note that we need a fix for the same by 1/15/14.

It doesn't. In short, this bug tries to reflect .sensorAngle in Gaia; But .sensorAngle itself gives wrong value on specific devices. That's why we filed bug 949930. We're still investigating the cause of 949930 (with Vliu and Ben). I can prioritize them but we may need a Gecko change. Not very sure if it can be completed by 1/15. If only this bug is in urgent, I can try sending review first but on some devices the preview would become upside-down until bug 949930 is fixed.

(hrmph...seems the inverted camera preview no longer reproduces over here, even without this bug fixed. Moving the bug back to v1.3? 'cause it's not blocking here anymore and I +'ed it originally. Sry)

M1, the inverted camera preview no longer reproduces in our builds because I have temporarily landed the attachment 8345777 [details] patch from this bug in our builds till it lands in gaia. So, we definitely need this bug fixed to fix the inverted preview issue on our device. I think this bug should still be blocking our metabug.

(clean up on aisle 5)

Inder: may I know that what device are you using / or what version of patch did you landed in your build? (Since I made a minor change yesterday for testing.)

Comment on here and bug 949930 for reference. Tested on several devices. Seems that devices without ro.moz.cam.0.sensor_offset works well. Otherwise it fails. Devices works: Helix_back :sensor_offset=0 .sensorAngle returns= 90. Expected sensorAngle= 90. Helix_front :sensor_offset=0 .sensorAngle returns=270. Expected sensorAngle=270. Fugu_back :sensor_offset=0 .sensorAngle returns= 90. Expected sensorAngle= 90. Fugu_front :sensor_offset=0 .sensorAngle returns=270. Expected sensorAngle=270. Flatfish_back :sensor_offset=0 .sensorAngle returns= 0. Expected sensorAngle= 0. Flatfish_front :sensor_offset=0 .sensorAngle returns= 0. Expected sensorAngle= 0. Hamachi :sensor_offset=0 .sensorAngle returns= 90. Expected sensorAngle= 90. Devices doesn't works: Keon :sensor_offset=270.sensorAngle returns= 0. Expected sensorAngle= 90. Inari :sensor_offset=270.sensorAngle returns= 0. Expected sensorAngle= 90. Unagi :sensor_offset=180.sensorAngle returns=270. Expected sensorAngle= 90. Not very sure what's going wrong, but I guess we should do something on sensorAngle against sensor_offset. If we want to land this patch first, we should review and test with devices that works.

(In reply to KM Lee [:rexboy] (Away: 12/25-1/5) from comment #22) > Inder: may I know that what device are you using / or what version of patch > did you landed in your build? > (Since I made a minor change yesterday for testing.) KM Lee, I landed this patch in our build: https://github.com/rexboy7/gaia/commit/84cfe351229664cdfc474ec7f03f0ca62b5fee80.patch along with this change in our product property file: ro.moz.cam.0.sensor_offset=180 And, this seems to be working for us. Ours is a reference device.

(In reply to Inder from comment #24) > I landed this patch in our build: > https://github.com/rexboy7/gaia/commit/ > 84cfe351229664cdfc474ec7f03f0ca62b5fee80.patch > along with this change in our product property file: > ro.moz.cam.0.sensor_offset=180 Ok, this makes sense.. the original patch *happened to* work on devices with sensor_offset=180, but broken with devices of other sensor_offset because we're not aware of bug 949930 when I wrote it. If we want things going together, I would suggest the following: * I can put on a patch (that should work after bug 949930 fixed) and put it to review first. * Verify the patch using one of the currently working devices (e.g. Hamachi). * After bug 949930 is fixed, verify it again with not working devices(e.g. Unagi), then check-in.

Just found that we have a big refactoring on Camera app in master branch. So this patch is for v1.3 - The one for master should be very similar (but based on new view/model pattern) with unit test. I will work for master version quickly. :djf would you help review this patch? I addressed some major changes inside pull request that may need some suggestions.

Comment on attachment 8358269 [details] v1.3 patch r- because I think this patch breaks the preview selection code (the function inside the call to previewSizes.filter()). Also, the fact that you have to call your rotation function twice makes me suspicious. That doesn't seem right. See the comments on github. This was a hard patch to review because: 1) I don't know what sensorAngle is. Are there docs anywhere? 2) I don't know why this matters. I see that this blocks the 1.3 CS release, but I don't know what that is or what devices are involved. Is this a tablet-related bug? Which devices have a sensorAngle that is not 90? If I don't know that I won't know what devices to test this on. 3) The patch is not a minimal set of changes to setPreviewSize(), but also does things like inverting the aspect ratios and changing the transform origin. So it is harder to tell which changes are for sensorAngle and which changes are just a different way of doing the math. 4) CSS transforms are just hard. In order to be comfortable with this patch, I'd like it to include a comment explaining what the sensorAngle is and also explaining what other parts of the camera API depend on sensorAngle. Are the picture sizes and preview sizes reported by the camera hardware rotated by sensor angle? Or do they always just report the larger dimension as the width and the smaller dimension as the height? Also, please change the variable names you use, as requested on Github. And, assuming that I don't have a device with a non-90 sensorAngle to test this on, would you work through a real world example of how this algorithm works. That is, start with the screen size and picture size and available preview sizes and tell me what happens at each step. Or, modify the patch so that it makes fewer changes to the existing code and is therefore easier to review. Or, find a reviewer who can test it on a device where this matters and get them to review.

Thanks for the detailed review! And sorry that seems I didn't make the description inside the code clear enough.. I'll try my best to improve them. (In reply to David Flanagan [:djf] from comment #27) > Comment on attachment 8358269 [details] > v1.3 patch > > r- because I think this patch breaks the preview selection code (the > function inside the call to previewSizes.filter()). Also, the fact that you > have to call your rotation function twice makes me suspicious. That doesn't > seem right. > See the comments on github. > > This was a hard patch to review because: > > 1) I don't know what sensorAngle is. Are there docs anywhere? > As far as I know this is discussed in bug 932669. It just exposes the angle between camera mount angle and the device, exposed from underlying Android: http://developer.android.com/reference/android/hardware/Camera.CameraInfo.html#orientation But yes we don't have documentations inside Gecko/Gaia. > 2) I don't know why this matters. I see that this blocks the 1.3 CS release, > but I don't know what that is or what devices are involved. Is this a > tablet-related bug? Which devices have a sensorAngle that is not 90? If I > don't know that I won't know what devices to test this on. > Yes, originally it's for tablet.. But I'm not sure why this blocks 1.3 CS. From the discussion I guess there are reference devices for CS release that mounted in 270 degree? Then the original code won't apply. (the original code works only for 90 degree devices). Let's ask Inder if it is the case to make sure we really need to fix it within CS deadline. > In order to be comfortable with this patch, I'd like it to include a comment > explaining what the sensorAngle is and also explaining what other parts of > the camera API depend on sensorAngle. Are the picture sizes and preview > sizes reported by the camera hardware rotated by sensor angle? Or do they > always just report the larger dimension as the width and the smaller > dimension as the height? I'll try my best to add some explanation and examples to make it easier to understand. Sorry for the confusion! I've thought of them but these variables are just a little bit difficult to name. I'll put on a modified patch later.

Comment on attachment 8358269 [details] v1.3 patch Here's the revised version. Setting style.top and style.left simplified things a lot, we don't need to call rotation function twice anymore. (The second call was just used to calculate the displacement need to be compensated. Sorry for didn't make them clear enough again.) Also made several changes and comments to make the code easier to read. Since bug 949930 has been resolved on master, we can now test it right on nightly build. I'd send review after testing it.

> But I'm not sure why this blocks 1.3 CS. From the > discussion I guess there are reference devices for CS release that mounted > in 270 > degree? Then the original code won't apply. > (the original code works only for 90 degree devices). > Let's ask Inder if it is the case to make sure we really need to fix it > within > CS deadline. KH Lee, the front-facing camera sensor on our reference device is mounted @180 degrees and after we applied the fix I mentioned in Comment 24 fixed the preview upside down issue.

Comment on attachment 8358269 [details] v1.3 patch Tested with latest m-c, both Inari and Unagi (which didn't work before) works well now. So we can completely solve this issue once this patch check-in. Djf would you mind review this patch again? I think the code should become more straightforward now.

And this is patch for master. It's almost the same with v1.3 patch, but with a few changes to put it into refactored Camera app.

Comment on attachment 8358269 [details] v1.3 patch This patch is a lot better. But I'm still giving r- because I don't understand whether the preview size selection code will still work. As it stands, the code only works if the camera.capabilities.previewSizes array has objects whose width and height are related to sensor angle. On all the devices I know of, with a sensor angle of 90, the width is the longer dimension and the height is the smaller dimension. As if the portrait mode screen has been rotated by 90 degrees. On a device with a sensorAngle of 0 or 180, will the width of the preview size objects always be the smaller dimension and the height always be the smaller dimension? If so, the code is good. If not, the code is broken. (The code will get the orientation correct, and Inder has tested that but I'm worried about subtler bugs here like getting a preview that is larger or smaller than needed.) Setting needinfo for Mike and Inder. Mike might be able to answer this question based on his knowledge of gecko and the camera hardware interface. Inder might be able to give an answer based on having access to the actual CS device, whatever that is. We need the same information about other rectangle sizes as well, of course: the size of the photo itself and the size of the photo thumbnails. Are they all related to sensorAngle, or are they all reported in the form of long dimension x short dimension?

Comment on attachment 8359649 [details] master patch Reassigning the review of the 1.4 patch to Wilson since he did most of the refactoring for that release, and since it is only the 1.3 version of that patch that needs to land ASAP. Wilson: no need to review this until the 1.3 patch has r+. And feel free to reassign to Diego or Justin if they would be better reviewers.

So if I understand correctly, the problem here is: would it guarantee the orientation after rotating is identical to the screen? For example, on unagi(320x480 portrait), orientation of the previews(480x360 landscape, for example) after rotating(angle = 90deg, to 360x480 portrait) is identical to the screen. so preview size selection works correctly. And the most special case for me here is flatfish(1280x800 **landscape**), orientation of the previews(1280x960 landscape, for example) after rotating (angle = 0deg, to 1280x960 landscape) is also identical to the screen, so preview size selection also works. I think they need to be identical or things would go weird.. but let’s confirm about this.

I tested it again after changes in bug 949930 have landed. And, it looks like changes in this patch with the changes in bug 949930 are not working on our device. Here are my findings: - Patch in this bug (comment 24) with bug 949930 is now making back-facing camera upside down (both preview and picture taken). Although it's fixing the preview orientation of front-facing camera which is mounted @180 on our reference device. - I misspoke about need for "ro.moz.cam.0.sensor_offset=180". It doesn't make any difference. So, we still need fix for our front-facing camera preview upside down issue. But, the patch in this bug is definitely not the solution. Note the issue we have with front-facing camera is just the preview is upside down. The picture taken comes with correct orientation. MikeH, any ideas on the fix?

Inder: You need to use the latest patch (on comment 31). Did you tried on that?

Comment on attachment 8359649 [details] master patch bug 959890 just landed in master, which made big changed to the viewport sizing logic. As a result the master patch branch is going to need to be rebased and reworked to sit on top of the new logic. The camera app is still in a very fluid state as we attempt to provide solid foundations for the many approaching 1.4 features. Sorry about this :(

(In reply to KM Lee [:rexboy] from comment #37) > Inder: > You need to use the latest patch (on comment 31). Did you tried on that? The patch mentioned in comment 31 (https://github.com/rexboy7/gaia/commit/de4fc720d4b56ef9164c806c162d8655c208e5b3) doesn't apply cleanly on v1.3. Am i missing something?

(In reply to Inder from comment #39) > > The patch mentioned in comment 31 > (https://github.com/rexboy7/gaia/commit/ > de4fc720d4b56ef9164c806c162d8655c208e5b3) doesn't apply cleanly on v1.3. Am > i missing something? Use this (since there are 2 commits there): https://github.com/mozilla-b2g/gaia/pull/15188.diff

> Use this (since there are 2 commits there): > https://github.com/mozilla-b2g/gaia/pull/15188.diff Tested this and this fixes our front-facing camera inversion issue! Can we please get it reviewed quickly and land. We need it fixed asap. Thanks.

:djf and :mike I made some discussion on Github because I think the current isLarger should work... may you take a look and discuss about it? Thanks a lot!

> > Tested this and this fixes our front-facing camera inversion issue! Can we > please get it reviewed quickly and land. We need it fixed asap. > Thanks. Sorry, looks like I responded too early. We found out that this patch fixes the preview orientation but messes up the snapshot orientation :( As mentioned earlier, our issue is front-facing camera preview is upside down but the snapshot was correct orientation. This patch fixes the preview orientation but also makes the snapshot orientation upside down. KM Lee, any ideas what's going wrong here?

Do you mean taking picture from the front camera? This patch is not changing anything about taking picture. It can be some regression caused by another thing. Can you make sure it's caused by this patch? If not, we can open another bug for that.

(In reply to Inder from comment #20) > > M1, the inverted camera preview no longer reproduces in our builds because I > have temporarily landed the attachment 8345777 [details] patch from this bug > in our builds till it lands in gaia. > So, we definitely need this bug fixed to fix the inverted preview issue on > our device. > I think this bug should still be blocking our metabug. Just trying to track down where this patch mentioned in this comment is hiding: - old, incorrectly tagged as for bug 922006: https://github.com/mozilla-b2g/gaia/pull/14561 - new, currectly tagged for this bug: https://github.com/mozilla-b2g/gaia/pull/15188

Confirmed the following on BRANCH=v1.3/aurora nightlies: - hamachi: - 2014-01-22 nightly: OK - +PR 15188 (above): OK - inari: - 2014-01-22 nightly: OK - +PR 15188 (above): OK OK means the viewfinder and taken photos have the correct orientation. Attempting to test on Helix, but I can't seem to get nightlies to flash.

Results for Helix using the 2014-01-22 nightly: - gaia: 744fb691c2b2a25a07c5d19fabf5748ae9aba4d9 - gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/71a8786c3815 - BuildID: 20140122004001 - Version: 28.0a2 Both without and with the PR 15188 patch applies, results are the same: - back/forward-facing camera: OK in all cases - front/backward-facing camera: - phone in portrait orientation: OK - phone in landscape orientation: viewfinder is OK, pictures appear upside-down

(In reply to KM Lee [:rexboy] from comment #44) > Do you mean taking picture from the front camera? > This patch is not changing anything about taking picture. It can be some > regression caused by another thing. > Can you make sure it's caused by this patch? If not, we can open another bug > for that. KM Lee, you are right, looks like the picture inversion problem was due to something else. So, to summarize this patch: https://github.com/mozilla-b2g/gaia/pull/15188.diff fixes the front-facing camera preview upside-down issue on our device.

Thanks Inder for testing the patch again. Once the patch is r+ for this bug, we can go ahead retest and land. But we need to open another bug for tracking the inverted image issue for landscape mode on helix device. Adding Marcia as QA contact/opening a new bug.

Instrumenting nsGonkCameraControl::TakePictureImpl() on Helix, I see from the front/backward-facing camera: Phone upright: 11-01 08:50:02.229 4565 4635 I Gecko : setting picture rotation to 270 degrees (mapped from 0) - picture OK Phone landscape, 90-degrees CW from upright: 11-01 08:50:12.879 4565 4635 I Gecko : setting picture rotation to 0 degrees (mapped from 90) - picture upside down Phone upside down: 11-01 08:50:22.379 4565 4635 I Gecko : setting picture rotation to 90 degrees (mapped from 180) - picture OK Phone landscape, 90-degrees CCW from upright: 11-01 08:50:29.059 4565 4635 I Gecko : setting picture rotation to 180 degrees (mapped from 270) - picture upside down I moved the images off of the phone to a PC and confirmed that the images are in fact incorrectly oriented. Then I dug into the EXIF information in the images and found this: # exif -l IMG_0013.jpg | grep Orientation 0x0112 Orientation - - - - - # exif --tag=0x0112 IMG_0013.jpg 'IMG_0013.jpg' does not contain tag 'Orientation'. # exif --tag=0x0112 IMG_0014.jpg 'IMG_0014.jpg' does not contain tag 'Orientation'. # exif --tag=0x0112 IMG_0015.jpg 'IMG_0015.jpg' does not contain tag 'Orientation'. # exif --tag=0x0112 IMG_0016.jpg 'IMG_0016.jpg' does not contain tag 'Orientation'. It turns out that the back/forward-facing camera's images, which are correct in all orientations, also do not seem to have any Orientation tags, which means the driver is actually rotating the captured pixels. The difference is that the front/backward-facing camera image is flipped so that the viewfinder behaves like a mirror. Given that, I believe the fix is for camera.js to stick a minus sign in front of the rotation option when calling takePicture().

This (sample?) patch results in Helix taking all photographs with the correct orientation.

I've confirmed that attachment 8364089 [details] [diff] [review] applied on top of https://github.com/mozilla-b2g/gaia/pull/15188.diff results in Helix taking correctly-oriented photos on both cameras in all device orientations.

Comment on attachment 8364089 [details] [diff] [review] Fix for misoriented photos Review of attachment 8364089 [details] [diff] [review]: ----------------------------------------------------------------- The patch does fix helix and fugu, but it still have some problems on flatfish: tablet orientation 0: ok tablet orientation 90, 180, and 270: all differ from correct orientation by 90 degree. f+ for me but we still need to find why flatfish has 90deg difference in degree. Should it be a Gecko issue? ::: apps/camera/js/camera.js @@ +1408,5 @@ > this.takePicture(); > }, > > takePicture: function camera_takePicture() { > + if (this.camera == 0) { should be this._cameraNumber.

OK, Ben and I found that, if we don't use minus angle (complement the minus by adding 360 degree), then both Flatfish and Helix are good to go. That is, for front camera we can use: this._config.rotation = -this._phoneOrientation + 360; We may trace inside Gecko to ensure that minus angle works among all devices (Maybe Mike would have some idea about this?) but at least we can include this fix inside this Gaia patch to ensure camera works.

Comment on attachment 8358269 [details] v1.3 patch David would you do the review again for this patch? If you have concern on how it runs on different devices, I can demo how this patch runs on some devices we have in Taipei.

(In reply to KM Lee [:rexboy] from comment #54) > OK, Ben and I found that, if we don't use minus angle (complement the minus > by adding 360 degree), then both Flatfish and Helix are good to go. That is, > for front camera we can use: > > this._config.rotation = -this._phoneOrientation + 360; Okay, I now completely understand what's going on. The code in Gecko to round the angle up to the nearest 90-degree increment doesn't correctly handle negative values. Here's the math: uint32_t r = static_cast<uint32_t>(aTakePicture->mRotation); r += mCameraHw->GetSensorOrientation(GonkCameraHardware::OFFSET_SENSOR_ORIENTATION); r %= 360; r += 45; r /= 90; r *= 90; SetParameter(CameraParameters::KEY_ROTATION, nsPrintfCString("%u", r).get()); So if aTakePicture->mRotation = -90 and GetSensorOrientation() returns 0, then: r %= 360 == -90 r += 45 == -45 // this is the bug r /= 90 == 0 r *= 90 == 0 // INCORRECT The solution is to make sure the addition step reflects the sign of the argument, so: r %= 360 == -90 r -= 45 == -135 r /= 90 == -1 r *= 90 == -90 // correct With this change, the +360 should no longer be required, which I believe is the correct solution. I'll prepare a patch and attach it to a dependent bug.

The Gecko fix has landed on b2g-inbound; once it lands on central, it can be merged into b2g18_v1_3 and the "+360" hack above won't be required anymore. https://bugzilla.mozilla.org/show_bug.cgi?id=963142#c6

It turns out video recording has the same problem. This patch fixes all of the issues: https://bugzilla.mozilla.org/attachment.cgi?id=8364738

(In reply to KM Lee [:rexboy] from comment #53) > > ::: apps/camera/js/camera.js > @@ +1408,5 @@ > > this.takePicture(); > > }, > > > > takePicture: function camera_takePicture() { > > + if (this.camera == 0) { > > should be this._cameraNumber. Yes, it should be.

The video recording fix has also landed on b2g-inbound.

Great. Close as soon as possible please!

The Gecko patches in bug 963142 have now landed in v1.3/aurora.

Is anything pending here?

In reply to comment 63... I don't see an r+ on rexboy's patch (comment 55). djf will be back from Taipei on Monday to complete the review and we can mark this fixed. Thanks Hema

Comment on attachment 8358269 [details] v1.3 patch Removed 360deg hack per comment 57 & tested on my Helix.

Comment on attachment 8358269 [details] v1.3 patch r- but much closer to landing now. I've been convinced that the preview selection code I was concerned about is okay. But I've left more comments on github. Please check that changing the order of scale() and rotate() is okay when using the selfie camera. I'm worried about that. Also, I've made various suggestions to improve code clarity: there are comments I'd like to see added, comments that need to be fixed, and variable and function names that could be made clearer. Sorry this has taken so long. This is one of the hardest-to-understand patches that I've had to review in a while.

rexboy: if this runs up against the new year holiday for you, let me know and I'll try to take over and get this landed.

Comment on attachment 8358269 [details] v1.3 patch I'm going back and changing my r- to an r+ now. I've finally understood why we're changing the order of the rotate and scale here. I've convinced myself that the new code is correct, and see that Rexboy has independently explained it in the same way on github, so I'm not happy with this patch. Rexboy: please consider the comment changes and variable name changes before landing. Also, I notice that there is a jslint travis failure that appears to be coming from the tab characters on line 1536 of camera.js. This was introduced by a recent Qualcomm patch. Would you fix it in this one while you're at it?

Thanks for your patient! I’m sorry that I can’t explain things much better... all these should be more easier to explain with pictures and devices. I’ll land the patch after adding some comments and things addressed on Github to make these codes look more comfort.

v1.3: https://github.com/mozilla-b2g/gaia/commit/b0aa74619cffb7a39ee0e7da5e9361a98e69f3f6 Let’s close this bug and open another one to adapt this 1.3 patch to gaia-master, since the Camera app has been refactored a lot there.