Closed Bug 932669 Opened 6 years ago Closed 6 years ago

[Flatfish][Camera] Preview is in wrong direction

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+)

VERIFIED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+

People

(Reporter: atsai, Assigned: mikeh)

References

Details

(Whiteboard: [Flatfish][developer+])

Attachments

(1 file, 3 obsolete files)

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 **
this is function fail, mark it koi+.

hi Marco, 

may you please help to loop in the right owner to fix this bug?
blocking-b2g: --- → koi+
Flags: needinfo?(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.
Flags: needinfo?(mchen) → needinfo?(vliu)
Bug 825826 had discussion with partner about wrong preview orientation. Bug 930961 also talked a way implemented in Android platform.
Flags: needinfo?(vliu)
Blocks: flatfish
It looks like we can't provide a general solution on time. We can try to provide a workaround solution for flatfish.
Whiteboard: [Flatfish] → [Flatfish][developer+]
Justin already did the patch for our platform.  
The camera preview (either in landscape or portrait mode) are correct now. 
Please kindly check again.
Attached file PR to master (obsolete) —
This method can fix this bug and works normally on mobile. I will update test cases later.
Comment on attachment 827937 [details]
PR to master

Sorry, this pr is not for this bug.
Attachment #827937 - Attachment is obsolete: true
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.
blocking-b2g: koi+ → 1.3+
Target Milestone: --- → 1.3 Sprint 6 - 12/6
hi Marco,

are you able to assign an owner for this bug?
Flags: needinfo?(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.
Flags: needinfo?(mchen) → needinfo?(mhabicher)
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)';
(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.
Flags: needinfo?(mhabicher)
Hi Mike,

Thanks for your reply and we can make sure the right direction.
May I know you have time for this or not?
Marco, I have time to do it.
Summary: [Flatfish][Camera] Preview is in wrong direct → [Flatfish][Camera] Preview is in wrong direction
Assignee: nobody → mhabicher
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).
Flags: needinfo?(mchen)
Flags: needinfo?(hungche)
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!
Hi Vincent,

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

Thanks.
Flags: needinfo?(mchen) → needinfo?(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.
Flags: needinfo?(vliu)
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;
Attachment #8333681 - Attachment is obsolete: true
Attachment #8340070 - Flags: feedback?(hungche)
Attachment #8340070 - Flags: feedback?(atsai)
Comment on attachment 8340070 [details] [diff] [review]
Expose sensor rotation offset to JS

try-server push:
  https://tbpl.mozilla.org/?tree=Try&rev=b116364b2502
Attachment #8340070 - Flags: review?(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.
Attachment #8340070 - Flags: review?(dhylands) → review+
Update patch to fix try-server gotchas:
  https://tbpl.mozilla.org/?tree=Try&rev=8f8751cd2d20

Carrying r=dhylands forward.
Attachment #8340070 - Attachment is obsolete: true
Attachment #8340070 - Flags: feedback?(hungche)
Attachment #8340070 - Flags: feedback?(atsai)
Attachment #8340151 - Flags: review+
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?
Attachment #8340151 - Flags: feedback?(hungche)
Attachment #8340151 - Flags: feedback?(atsai)
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.
Flags: needinfo?(vliu)
Sure!! I can help on verification.
Flags: needinfo?(vliu)
Will this patch merge to master branch?
(In reply to steve from comment #27)
>
> Will this patch merge to master branch?

It should--it was written against master.
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?
(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.
(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]).
Flags: needinfo?(hungche)
(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.
(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.
(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)
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.
There's some follow-up work to be done in JS-land, but the new CameraControl property can land now.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b4d9c56e18a8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Is there a bug for accompanying changes to the camera app?
Flags: needinfo?(mhabicher)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #39)
>
> Is there a bug for accompanying changes to the camera app?

Bug 947956.
Flags: needinfo?(mhabicher)
Attachment #8340151 - Flags: feedback?(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
Status: RESOLVED → VERIFIED
I found the patch doesn't work consistently between different devices. See the follow-up bug above.
You need to log in before you can comment on or make changes to this bug.