932669 - [Flatfish][Camera] Preview is in wrong direction

Status: VERIFIED FIXED

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

Description

[Al Tsai [:atsai]]

There's 90 degrees rotate when previewing

Gaia:     b93de939502cff5dc7ef242a6ec9af34b0864f91
Gecko:    94801a471800e7edcce71fb4d02e89a38d1b99ef
BuildID   20131030064537
Version   28.0a1

** The case is by the testing on Flatfish devices. It might not be able to reproduce on other mobile devices **

Comment 1

[Francis Lee [:frlee]]

this is function fail, mark it koi+.

hi Marco, 

may you please help to loop in the right owner to fix this bug?

Comment 2

[Marco Chen [:mchen]]

This seems to be a general bug on different platform because Gaia didn't set CSS rotation transform according to real camera sensor mount angle.

Hi Vincent,

Could you help to provide the old bug discussed this issue? Thanks.

Comment 3

[Vincent Liu[:vliu]]

Bug 825826 had discussion with partner about wrong preview orientation. Bug 930961 also talked a way implemented in Android platform.

Comment 4

[Kai-Chih Hu [:khu]]

It looks like we can't provide a general solution on time. We can try to provide a workaround solution for flatfish.

Comment 5

[RaymondSu]

Justin already did the patch for our platform.  
The camera preview (either in landscape or portrait mode) are correct now. 
Please kindly check again.

Comment 6

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

This method can fix this bug and works normally on mobile. I will update test cases later.

Comment 7

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 827937 [details]
PR to master

Sorry, this pr is not for this bug.

Comment 8

[Francis Lee [:frlee]]

as we confirmed that Flatfish will use v1.3 and we need to respect current testing cycle, set target milestone to 12/6 since 1.3FC tag is not created yet.

Comment 9

[Francis Lee [:frlee]]

hi Marco,

are you able to assign an owner for this bug?

Comment 10

[Marco Chen [:mchen]]

Hi Mike,

Could you give some suggestion to the rotation issue of preview frames? Thanks.

1. I found that nsGonkCameraControl.cpp will set rotation degree for takePicuture or StartRecording function based on property - "ro.moz.cam.%d.sensor_offset".
   This can solve the issue about different devices may mount camera sensor in different angles.

2. But for preview frames, currently the gaia fixed the rotation transform by CSS attribute for correcting the orientation of preview frames. We think that it should get mounting angle in runtime then decide the CSS rotation transform.

Comment 11

[JustinLee]

Attachment: workaround-camera-preview-rotation-and-enabled-front-camera-mirror-for-flatfish.patch

I did a workaround using v1.3 on flatfish platform for the preview rotation issue. Please refer to attached patch. The patch alse enabled the mirror function of front-camera.

I don't know why should have the below code in camera.js.
var transform = 'rotate(90deg)';

Comment 12

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

(In reply to Marco Chen [:mchen] from comment #10)
> 
> 2. But for preview frames, currently the gaia fixed the rotation transform
> by CSS attribute for correcting the orientation of preview frames. We think
> that it should get mounting angle in runtime then decide the CSS rotation
> transform.

Hi Marco, yes, this is an old and ugly hack we need to clean up. We can expose the platform rotation to the DOM easily.

Comment 13

[Marco Chen [:mchen]]

Hi Mike,

Thanks for your reply and we can make sure the right direction.
May I know you have time for this or not?

Comment 14

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

Marco, I have time to do it.

Comment 15

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

I don't have a Flatfish to test this on: can someone change the following line from DOM_CAMERA_LOG*() to printf_stderr() and tell me what the logcat output shows? http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraHwMgr.cpp?from=GonkCameraHwMgr.cpp#182

Also, what is the correct 'rotate' value for Flatfish, with no other changes? https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L1023 (I'm guessing from comment 11 it's no rotation at all, but I want to make sure).

Comment 16

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

Further to comment 15, please confirm whether or not pictures are taken with the correct orientation, and whether or not videos are recorded with the correct orientation. Thanks!

Comment 17

[Marco Chen [:mchen]]

Hi Vincent,

Could you help to provide what information asked by MikeH from Comment 15 & 16?

Thanks.

Comment 18

[Vincent Liu[:vliu]]

(In reply to Mike Habicher [:mikeh] from comment #15)
> I don't have a Flatfish to test this on: can someone change the following
> line from DOM_CAMERA_LOG*() to printf_stderr() and tell me what the logcat
> output shows?
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraHwMgr.
> cpp?from=GonkCameraHwMgr.cpp#182
> 

I tried to dump the message on Flatfish and got the below result.

Sensor orientation: base=0, offset=0, final=0

> Also, what is the correct 'rotate' value for Flatfish, with no other
> changes?
> https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.
> js#L1023 (I'm guessing from comment 11 it's no rotation at all, but I want
> to make sure).

Currently Flatfish using the patch on comment 11 to let the preview to have correct direction. But even we don't have this patch, we still got the correct direction for taking picture and recorded file.

(In reply to Mike Habicher [:mikeh] from comment #16)
> Further to comment 15, please confirm whether or not pictures are taken with
> the correct orientation, and whether or not videos are recorded with the
> correct orientation. Thanks!

I confirmed that the picture and recorded file got the correct orientation.

Comment 19

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

Attachment: Expose sensor rotation offset to JS

Please try out this patch with the following change to camera.js:

diff --git a/apps/camera/js/camera.js b/apps/camera/js/camera.js
index 1ad56a0..91296cf 100644
--- a/apps/camera/js/camera.js
+++ b/apps/camera/js/camera.js
@@ -1015,17 +1015,17 @@ var Camera = {
       // Pick the smallest valid preview
       this._previewConfig = validPreviews.sort(function(a, b) {
         return a.width * a.height - b.width * b.height;
       }).shift();
     } else {
       this._previewConfig = camera.capabilities.previewSizes[0];
     }
 
-    var transform = 'rotate(90deg)';
+    var transform = 'rotate(' + -camera.sensorAngle + 'deg)';
     var width, height;
     var translateX = 0;
 
     // The preview should be larger than the screen, shrink it so that as
     // much as possible is on screen.
     if (screenAspectRatio < pictureAspectRatio) {
       width = screenWidth;
       height = screenWidth * pictureAspectRatio;

Comment 20

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

Comment on attachment 8340070 [details] [diff] [review]
Expose sensor rotation offset to JS

try-server push:
  https://tbpl.mozilla.org/?tree=Try&rev=b116364b2502

Comment 21

[Dave Hylands [:dhylands]]

Comment on attachment 8340070 [details] [diff] [review]
Expose sensor rotation offset to JS

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

Looks reasonable to me.

Comment 22

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

try-server re-push:
  https://tbpl.mozilla.org/?tree=Try&rev=5aeaa76a4332

Comment 23

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

Attachment: Expose sensor rotation offset to JS, v2

Update patch to fix try-server gotchas:
  https://tbpl.mozilla.org/?tree=Try&rev=8f8751cd2d20

Carrying r=dhylands forward.

Comment 24

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

Comment on attachment 8340151 [details] [diff] [review]
Expose sensor rotation offset to JS, v2

This patch is ready to go. Can someone with a Flatfish verify that it fixes the issue?

Comment 25

[Marco Chen [:mchen]]

Hi Mike,

Thanks for this quick and great work here.
We should help you to do verification.

Hi Vincent,

Could you help to verify this issue based on patch here if atsai didn't do it already?

Thanks.

Comment 26

[Vincent Liu[:vliu]]

Sure!! I can help on verification.

Comment 27

[steve]

Will this patch merge to master branch?

Comment 28

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

(In reply to steve from comment #27)
>
> Will this patch merge to master branch?

It should--it was written against master.

Comment 29

[steve]

After apply your patch, the preview orientation is controlled  by sensor, But also with a wrong orientation .When my UI is landscape,camera preview is Portrait, rotated 90 degree.

(In reply to Mike Habicher [:mikeh] from comment #24)
> Comment on attachment 8340151 [details] [diff] [review]
> Expose sensor rotation offset to JS, v2
> 
> This patch is ready to go. Can someone with a Flatfish verify that it fixes
> the issue?

Comment 30

[Vincent Liu[:vliu]]

(In reply to Mike Habicher [:mikeh] from comment #24)
> Comment on attachment 8340151 [details] [diff] [review]
> Expose sensor rotation offset to JS, v2
> 
> This patch is ready to go. Can someone with a Flatfish verify that it fixes
> the issue?

I tried your patch with the gaia modification you said in Comment 19, the preview got the whole black. I think the preview had rotated to out of screen.

Comment 31

[JustinLee]

(In reply to Vincent Liu[:vliu] from comment #30)
> (In reply to Mike Habicher [:mikeh] from comment #24)
> > Comment on attachment 8340151 [details] [diff] [review]
> > Expose sensor rotation offset to JS, v2
> > 
> > This patch is ready to go. Can someone with a Flatfish verify that it fixes
> > the issue?
> 
> I tried your patch with the gaia modification you said in Comment 19, the
> preview got the whole black. I think the preview had rotated to out of
> screen.

I got the same result (black screen) after applied the patch (Attachment #8340151 [details] [diff]).

Comment 32

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #30)
> (In reply to Mike Habicher [:mikeh] from comment #24)
> > Comment on attachment 8340151 [details] [diff] [review]
> > Expose sensor rotation offset to JS, v2
> > 
> > This patch is ready to go. Can someone with a Flatfish verify that it fixes
> > the issue?
> 
> I tried your patch with the gaia modification you said in Comment 19, the
> preview got the whole black. I think the preview had rotated to out of
> screen.

Rex had ever done some gaia works on setPreviewSize() and maybe he can help on this.

Comment 33

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

(In reply to JustinLee from comment #31)
> 
> I got the same result (black screen) after applied the patch (Attachment
> #8340151 [details] [diff]).

Justin or Vincent, can you confirm the value returned by .sensorAngle, and try setting different values of 'rotate' in transform to see what the correct value should be? Based on my reading of attachment 8333681 [details] [diff] [review], I think it should be 0deg.

Comment 34

[Vincent Liu[:vliu]]

(In reply to Mike Habicher [:mikeh] from comment #33)
> (In reply to JustinLee from comment #31)
> > 
> > I got the same result (black screen) after applied the patch (Attachment
> > #8340151 [details] [diff]).
> 
> Justin or Vincent, can you confirm the value returned by .sensorAngle, and

I modified the gaia code the same with you said in Comment 19.

diff --git a/apps/camera/js/camera.js b/apps/camera/js/camera.js
index acdd166..ede5775 100644
--- a/apps/camera/js/camera.js
+++ b/apps/camera/js/camera.js
@@ -1020,10 +1020,13 @@ var Camera = {
       this._previewConfig = camera.capabilities.previewSizes[0];
     }
 
-    var transform = 'rotate(90deg)';
+    // var transform = 'rotate(90deg)';
+
+    var transform = 'rotate(' + -camera.sensorAngle + 'deg)';
     var width, height;
     var translateX = 0;
 
+    dump(" camera.sensorAngle = " + camera.sensorAngle + " transform = " + transform); 
     // The preview should be larger than the screen, shrink it so that as
     // much as possible is on screen.
     if (screenAspectRatio < pictureAspectRatio) {

and got the result of camera.sensorAngle and transform below. The preview screen got black.

I/GeckoDump( 2237):  camera.sensorAngle = 0 transform = rotate(0deg)

> try setting different values of 'rotate' in transform to see what the
> correct value should be? Based on my reading of attachment 8333681 [details] [diff] [review]
> [diff] [review], I think it should be 0deg.

For the 90 and 180, I got the whole black screen.
I/GeckoDump( 1974):  camera.sensorAngle = 90 transform = rotate(-90deg)
I/GeckoDump( 1729):  camera.sensorAngle = 180 transform = rotate(-180deg)

For the 270, the preview screen is in Portrait and not for landscape. It has 90deg difference as we expected.
I/GeckoDump( 1718):  camera.sensorAngle = 270 transform = rotate(-270deg)

Comment 35

[KM Lee [:rexboy] (inactivate after Sep. 22)]

If we want to make all angles work,
for current CSS code, we need to compensate the displacement caused by rotation using translate().
Take 320x480 as example, the function should generate corresponding transform style as below:

For rear camera:
transform: rotate(90deg) translate(0, -480px);
transform: rotate(180deg) translate(-320px, -480px);
transform: rotate(270deg) translate(-320px, 0);

For front camera:
transform: rotate(90deg) scale(-1, 1) translate(-320px, -480px);
transform: rotate(180deg) scale(-1, 1) translate(0, -480px);
transform: rotate(270deg) scale(-1, 1) translate(0, 0);

But.... this is just a mess. I'm not sure if there are any math formula that can generate this.
I can investigate if changing transform origin works.

Comment 36

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

There's some follow-up work to be done in JS-land, but the new CameraControl property can land now.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b4d9c56e18a8

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4d9c56e18a8

Comment 39

[Michael Vines [:m1] [:evilmachines]]

Is there a bug for accompanying changes to the camera app?

Comment 40

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #39)
>
> Is there a bug for accompanying changes to the camera app?

Bug 947956.

Comment 41

[Al Tsai [:atsai]]

Ooops... Sorry for not seeing the request. I've tested on Flatfish and it works well.
Gaia      fbb6ce88ce8b7bd4d2efdb7a4a9f5a3c145f3eab  │
Gecko     d3a0e400918372970a8ece9936d3647bce9dcc7d  │
BuildID   20131210064521                            │
Version   28.0a1

Comment 42

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I found the patch doesn't work consistently between different devices. See the follow-up bug above.