[Camera] Preview position changes for ontouch event and autofocussing

RESOLVED FIXED in Firefox 25

Status

defect
P1
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: leo.bugzilla.gaia, Assigned: pchang)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [TD-44960][POVB])

Attachments

(2 attachments, 1 obsolete attachment)

1. Title : Camera Preview position shakes for ontouch event and autofocussing
2. Precondition : Camera app should be working
3. Tester's Action : 1. Open camera
                     2. touch on camera preview or try to take a picture
 
4. Detailed Symptom (ENG.) : there are two scenarios for this issue:
1. Open camera app -> When user touches the camera preview -> filmstrip will be shown: during this time camera preview moves towards right side (in landscape mode) to align the postion after filmstrip.
2. Take picture -> auto focus is running: camera preview jumps up and down during focussing.

5. Expected : Camera preview should work normally without changing its preview position.
6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: a25fb6bd1b0284ce3e197e88567d433c7815ee52
9.Personal email id: gjyothiprasad@gmail.com
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
Whiteboard: [TD-44960]
Target Milestone: --- → 1.1 QE3 (24jun)
Priority: -- → P3
Priority: P3 → P1
This is a bug specific to the leo device (and quite a strange one)
ni?pchang to check on this. leo device uses a hardware composer.
Flags: needinfo?(pchang)
Right now HWComposer is enabled on leo device by default.
Rename the hwcomposer and restart b2g could fix the problem.

Looks like the layer configuration for hwcomposer has problem.

adb shell mv /system/lib/hw/hwcomposer.msm7627a.so /system/lib/hw/hwcomposer.msm7627a.so_bak
Flags: needinfo?(pchang)
camera preview shift problem could be fixed after comment below line.
But still found the flash light icon shift problem.
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#185
(In reply to Dale Harvey (:daleharvey) from comment #1)
> This is a bug specific to the leo device (and quite a strange one)

Marking as POVB in that case, since this is device specific. We may fix on our side, but this points to a device issue.
Whiteboard: [TD-44960] → [TD-44960][POVB]
Assignee: nobody → pchang
Component: Gaia::Camera → General
The issue was caused by incorrect crop region for Image Layer (camera frame).
The crop region of all layers was shifted with the aBufferRect.TopLeft().
Therefore, it caused problem for ImageLayer.
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#188

Since there is no buffer offset(state.mHasOwnOffset is false) for Image Layer (but ThebesLayer always does), set the whole surface size as buffer bound to calculate the correct crop region and display position.
Attachment #765648 - Flags: review?(dwilson)
Attachment #765648 - Flags: review?(ncameron)
Comment on attachment 765648 [details] [diff] [review]
Set correct buffer bounds for HWC

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

I can't vouch for the logic here, but if dwilson says it's ok, then it's ok with me :-) If this is correct, then do we want to use 0,0 for the location of bufferRect in the fillColor case too? (i.e., use 0,0 but the size of the visibleRect)?

In future please use eight lines of context in your patches, it makes it easier to review.
Attachment #765648 - Flags: review?(ncameron) → review+
We checked your patch.
The issue seems to be fixed. thanks.

But we have a question. 
The HwcComposer2D.cpp file is different a little.
In your patch, "nsIntRect(visibleRect.x, visibleRect.y, state.mSize.width, state.mSize.height);" is not same with our source code.
In our source code, there is a "nsIntRect(visibleRect.x, visibleRect.y, surfaceSize.width, surfaceSize.height);" at same line.
Doesn't it matter?
(In reply to leo.bugzilla.fw from comment #8)
> We checked your patch.
> The issue seems to be fixed. thanks.
> 
> But we have a question. 
> The HwcComposer2D.cpp file is different a little.
> In your patch, "nsIntRect(visibleRect.x, visibleRect.y, state.mSize.width,
> state.mSize.height);" is not same with our source code.
> In our source code, there is a "nsIntRect(visibleRect.x, visibleRect.y,
> surfaceSize.width, surfaceSize.height);" at same line.
> Doesn't it matter?
The patch was based on latest m-c branch. After review+, I will upload patches for b2g18 too.
leo working correctly
(In reply to Nick Cameron [:nrc] from comment #7)
> Comment on attachment 765648 [details] [diff] [review]
> Set correct buffer bounds for HWC
> 
> Review of attachment 765648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't vouch for the logic here, but if dwilson says it's ok, then it's ok
> with me :-) If this is correct, then do we want to use 0,0 for the location
> of bufferRect in the fillColor case too? (i.e., use 0,0 but the size of the
> visibleRect)?
> 
  I'm not sure we need to use 0,0 for the bufferRect of color layer or not. Because color layer is just a solid color and you may not see the difference with/without cropping.

  Diego, any comment on this?

> In future please use eight lines of context in your patches, it makes it
> easier to review.
Sure, will do it.
Flags: needinfo?(dwilson)
Comment on attachment 765648 [details] [diff] [review]
Set correct buffer bounds for HWC

LGTM!
Attachment #765648 - Flags: review?(dwilson) → review+
Uploaded m-c patch with reviewers
Attachment #765648 - Attachment is obsolete: true
Attachment #768712 - Flags: review+
uploaded patch for b2g18
Attachment #768714 - Flags: review+
Keywords: checkin-needed
Flags: needinfo?(dwilson)
Ryan,
  We also need to land this fix(comment 13) on m-c.
Flags: needinfo?(ryanvm)
m-c is currently closed. It will be automatically merged from birch once it reopens. You don't need to ping me about it.
Flags: needinfo?(ryanvm)
m-c may be closed for awhile, so I've gone ahead and pushed to b2g18 based on being green on birch.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/291ef0dadd88
https://hg.mozilla.org/mozilla-central/rev/b08889ab5f00
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.