Closed Bug 1152260 Opened 5 years ago Closed 5 years ago

[Nexus 5][Camera]The camera can't autofocus, white focus circle always exists and does not change color.

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S10 (17apr)
blocking-b2g 2.2+
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: yi.zou, Assigned: aosmond)

References

Details

(Whiteboard: [v2.2-nexus-5-l])

Attachments

(6 files, 4 obsolete files)

Attached video 1654.MP4
[1.Description]:
[Nexus5 v2.2 & v3.0]The camera can't autofocus, white focus circle always exists and does not change color.
Time:16:54
See attachment:1654.3gp,logcat-1654.txt

[2.Testing Steps]: 
1. Open camera app.
2. Move the camera around to change focus
3. Waiting for the camera with auto focus

[3.Expected Result]: 
3. A white focus circle reappears, and after locking the focus, it turns green and disappears.

[4.Actual Result]: 
3. The camera can't autofocus, white focus circle always exists and does not change color.

[5.Reproduction build]: 
Device: N5 2.2 (affected)
Build ID               20150329002502
Gaia Revision          473cd63f53c855299b719285d9b95e3f2910782f
Gaia Date              2015-03-27 20:14:43
Gecko Revision         https://hg.mozilla.org/releases/mozilla-

b2g37_v2_2/rev/4b13c4254e2f
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150329.035946
Firmware Date          Sun Mar 29 03:59:59 EDT 2015
Bootloader             HHZ12d

Device: N5 3.0 (affected)
Build ID               20150407160201
Gaia Revision          84cbd4391fb7175d5380fa72c04d68873ce77e6d
Gaia Date              2015-04-07 17:33:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/078128c2600a
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150407.192944
Firmware Date          Tue Apr  7 19:30:00 EDT 2015
Bootloader             HHZ12d

Device: Flame 2.2 (unaffected)
Build ID               20150407162504
Gaia Revision          ea735c21bfb0d78333213ff0376fce1eac89ead6
Gaia Date              2015-04-07 20:58:15
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f86ddb7f719
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150407.195227
Firmware Date          Tue Apr  7 19:52:39 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (unaffected)
Build ID               20150407160201
Gaia Revision          84cbd4391fb7175d5380fa72c04d68873ce77e6d
Gaia Date              2015-04-07 17:33:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/078128c2600a
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150407.193600
Firmware Date          Tue Apr  7 19:36:12 EDT 2015
Bootloader             L1TC000118D0


[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

[8.Note]:
This issue doesn't exist on flame.
Attached file logcat-1654.txt
regressionwindow on V2.2, thanks.
Flags: needinfo?(yi.zou)
Attached video RegressionVideo.mp4
Hi Eric,
The N5 v2.2 oldest nightly build is ‘20150224162516’ and problem can be reproduced on this build by the STR in comment 0.
See attachment: RegressionVideo.mp4

Device: Nexus5 v2.2
Build ID               20150224162516
Gaia Revision          ca64f2fe145909f31af266b1730874051ba76c78
Gaia Date              2015-02-24 22:06:53
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/16804008c29f
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150224.201149
Firmware Date          Tue Feb 24 20:12:04 EST 2015
Bootloader             HHZ12d
Flags: needinfo?(yi.zou)
QA Whiteboard: [MGSEI-Triage+]
Autofocus not working on Nexus, not sure this is an L issue or Nexus only issue, ni RD for clarification.
Flags: needinfo?(dflanagan)
I can partially reproduce this on 2.2. (The 3.0 build for nexus5-l is badly broken today with graphics bugs, so I couldn't test on that.)

For me, the camera does successfully auto-focus, but it does so slowly.  And I see that the focus circle never turns green, and the white circle just sits there. Touch-to-focus does work and does produce a green circle. I see the same thing in both photo and video modes.

Eric: I can't answer your question, myself, but I bet Mike or Andrew can. If this is an L issue then obviously we need to fix it right away. But if it is specific to the nexus-5 camera driver, then we don't need to fix it, right?

Mike or Andrew: do you know if this is a nexus5 thing or a Lollipop thing? Has the auto-focus API changed in Lollipop?  It seems to me like it is autofocusing, but we're not responding correctly in Gaia, so I'm wondering if something has changed in the way that the camera driver sends notification of its autofocus results.
Flags: needinfo?(mhabicher)
Flags: needinfo?(dflanagan)
Flags: needinfo?(aosmond)
To my knowledge the API hasn't changed. They added a new version of the camera APIs and declared the old ones (i.e. that which we use) as legacy, but they still should work as before.

The video shows that in the continuous auto focus case, we get an auto focus moving event from the driver, but we don't receive an auto focus complete event (which would turn the circle green or red depending on the result of the focus operation). Touch to focus generates an auto focus complete event as one would expect.

Empirical testing last summer indicated that we always get a completed event for continuous auto focus attempts. However it is not clearly spelled out in the API documentation that the driver *must* supply that event in that circumstance.

The documentation does claim that if we call auto focus in continuous-picture/video mode, we can force a complete event without triggering a new focus attempt (i.e. query the state). We took this out because it was problematic (appeared to trigger an explicit auto focus) and we got the event anyways. But it is possible we called it at the wrong time (code shows it was when the focus was moving, but it should be called when the focus *stops* moving). I can provide a patch to test out this theory (but no nexus 5 device handy to test against).
Flags: needinfo?(aosmond)
Andrew: I can't tell whether you're proposing a gaia patch or a gecko patch. If gaia, I'd be happy to test it.  (And Mike presumably has a nexus5 that he should pass on to you...)

I think this is urgent only if we have reason to believe that it will affect other L-based builds.  If the nexus 5 is the only one we've got then I guess we just may not know. If it is easy to prepare a patch, then maybe it would be a good idea to have it ready in case we need it.
Flags: needinfo?(aosmond)
Eric: from Andrew's response it sounds like we don't know if this is specific to the Nexus 5 or is a general L problem. Do we have access to any other Lollipop-based devices to try this out on? Or are there partners we can ask to test this for us on their devices?
Flags: needinfo?(echang)
David: Err forgive my obtuseness :). It will have to be a gecko patch as the required signal doesn't make it up to the DOM layer. While I cannot be certain, I would expect it is rooted in the driver. I'll get the nexus 5 from Mike tomorrow or Monday.
Clear ni per comment 9. 
Just a note, we have Nexus 5, let me check if I can get another type of device for reference.
Flags: needinfo?(echang)
Blocks: 1152257
Attached patch bug1152260.patch, v1 (obsolete) — Splinter Review
Mochitests are broken so hopefully the test works. Tested on flame and the nexus 5, and it is a workaround for the problem.

If you look at the AOSP camera application, it doesn't actually use the auto focus API in the way described in the documentation. When it gets an auto focus moving event where the focus has stopped moving, it just assumes success. I have done something very similar here where if we don't get the auto focus complete notification within 1 second of receiving the auto focus moving notification (normally it follows immediately), it will generate the auto focus complete notification on the driver's behalf. If that happens three times, it eliminates the 1 second delay.

This is actually a concern we had when we originally fixed the focus issues on flame (that we may not get the auto focus complete notification at all for CAF), but all of the devices we tested worked fine. This is a more robust solution (if sadly not ideal).
Assignee: nobody → aosmond
Flags: needinfo?(mhabicher)
Flags: needinfo?(aosmond)
Attachment #8591909 - Flags: review?(mhabicher)
Flags: in-testsuite+
Attached patch bug1152260.patch, v2 (obsolete) — Splinter Review
Move this to GonkCameraControl where it belongs.
Attachment #8591909 - Attachment is obsolete: true
Attachment #8591909 - Flags: review?(mhabicher)
Attachment #8592285 - Flags: review?(mhabicher)
Attached patch bug1152260.patch, v2.1 (obsolete) — Splinter Review
Fix how mAutoFocusCompleteExpired doesn't need to be atomic (was an earlier attempt to avoid the lock).
Attachment #8592285 - Attachment is obsolete: true
Attachment #8592285 - Flags: review?(mhabicher)
Attachment #8592286 - Flags: review?(mhabicher)
Comment on attachment 8592286 [details] [diff] [review]
bug1152260.patch, v2.1

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

r+ with the following issues addressed.

::: dom/camera/GonkCameraControl.cpp
@@ +59,5 @@
>      }                                                                     \
>    } while(0)
>  
> +static const unsigned long kAutoFocusCompleteTimeoutMs = 1000;
> +static const int32_t kAutoFocusCompleteThreshold = 3;

I think 'kAutoFocusCompleteTimeoutLimit' would be more descriptive.

@@ +1361,5 @@
> +    }
> +
> +    if (expiredCount >= kAutoFocusCompleteThreshold) {
> +      OnAutoFocusComplete(true, true);
> +    }

Could this logic be simplfied a little bit?
  if (mAutoFocusCompleteTimer) {
    // ...
    return;
  }
  if (!mAutoFocusPending) {
    expiredCount = mAutoFocusCompleteExpired;
  }

@@ +1406,5 @@
> +          --mAutoFocusCompleteExpired;
> +        }
> +
> +        if (mAutoFocusCompleteExpired >= kAutoFocusCompleteThreshold ||
> +            mAutoFocusCompleteExpired <= -kAutoFocusCompleteThreshold)

Are you trying to catch overflows here?

::: dom/camera/GonkCameraControl.h
@@ +19,5 @@
>  
>  #include "base/basictypes.h"
>  #include "nsRefPtrHashtable.h"
>  #include "mozilla/ReentrantMonitor.h"
> +#include "nsITimer.h"

Since we only need nsITimer as a pointer in this header, please move this #include to GonkCameraControl.cpp and add a forward reference to 'class nsITimer' below.

@@ +203,5 @@
>  
> +  class AutoFocusMovingTimerCallback : public nsITimerCallback
> +  {
> +  public:
> +    NS_DECL_ISUPPORTS

Is this class accessed from multiple threads? If so, this should be NS_DECL_ISUPPORTS_THREADSAFE.
Attachment #8592286 - Flags: review?(mhabicher) → review+
Attached patch bug1152260.patch, v3 (obsolete) — Splinter Review
(In reply to Mike Habicher [:mikeh] from comment #14)
> Comment on attachment 8592286 [details] [diff] [review]
> bug1152260.patch, v2.1
> 
> Review of attachment 8592286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the following issues addressed.
> 
> @@ +1406,5 @@
> > +          --mAutoFocusCompleteExpired;
> > +        }
> > +
> > +        if (mAutoFocusCompleteExpired >= kAutoFocusCompleteThreshold ||
> > +            mAutoFocusCompleteExpired <= -kAutoFocusCompleteThreshold)
> 
> Are you trying to catch overflows here?
> 

Yes it was just a bit of paranoia. I switched them to ==.

> ::: dom/camera/GonkCameraControl.cpp
> @@ +59,5 @@
> >      }                                                                     \
> >    } while(0)
> >  
> > +static const unsigned long kAutoFocusCompleteTimeoutMs = 1000;
> > +static const int32_t kAutoFocusCompleteThreshold = 3;
> 
> I think 'kAutoFocusCompleteTimeoutLimit' would be more descriptive.
> 
> @@ +1361,5 @@
> > +    }
> > +
> > +    if (expiredCount >= kAutoFocusCompleteThreshold) {
> > +      OnAutoFocusComplete(true, true);
> > +    }
> 
> Could this logic be simplfied a little bit?
>   if (mAutoFocusCompleteTimer) {
>     // ...
>     return;
>   }
>   if (!mAutoFocusPending) {
>     expiredCount = mAutoFocusCompleteExpired;
>   }
> 
> ::: dom/camera/GonkCameraControl.h
> @@ +19,5 @@
> >  
> >  #include "base/basictypes.h"
> >  #include "nsRefPtrHashtable.h"
> >  #include "mozilla/ReentrantMonitor.h"
> > +#include "nsITimer.h"
> 
> Since we only need nsITimer as a pointer in this header, please move this
> #include to GonkCameraControl.cpp and add a forward reference to 'class
> nsITimer' below.
> 
> @@ +203,5 @@
> >  
> > +  class AutoFocusMovingTimerCallback : public nsITimerCallback
> > +  {
> > +  public:
> > +    NS_DECL_ISUPPORTS
> 
> Is this class accessed from multiple threads? If so, this should be
> NS_DECL_ISUPPORTS_THREADSAFE.

Done.
Attachment #8592286 - Attachment is obsolete: true
Attachment #8592358 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9688ef4c6f16
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
Are we going to nom this a 2.2 blocker to land the fix to 2.2?
Flags: needinfo?(echang)
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1111890
User impact if declined: Focus rings on lollipop builds for nexus 5 will remain persistently on screen.
Testing completed: Verified focus rings disappear on nexus 5 on 2.2 with patch applied.
Risk to taking this patch (and alternatives if risky):  Risk is low. If the runtime detection of missing the auto complete event is faulty, we may post an extra auto focus complete event unexpectedly. The gaia camera application ignores this.
String or UUID changes made by this patch: None.
Attachment #8594785 - Flags: review+
Attachment #8594785 - Flags: approval-mozilla-b2g37?
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → 2.2+
Attachment #8594785 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Flags: needinfo?(echang)
According to the STR of Comment 0,this bug has been successfully verified on latest Nightly Flame v2.2&3.0 and Nexus 5 v2.2&3.0.

Actual results:The camera can focus automatically, and the white focus circle can change to green highlight when camera is focusing successfully.

See attachment: verified_v2.2&3.0.mp4
Reproduce rate: 0/6


-----------------------------------------------------------------------------------------
Device: Flame 2.2 (Pass)
Build ID               20150504002502
Gaia Revision          8d14361337e608c8cdf165ea5034db5eda23b618
Gaia Date              2015-05-01 18:23:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7cb6597c91
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150504.041436
Firmware Date          Mon May  4 04:14:48 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (Pass)
Build ID               20150504160201
Gaia Revision          70077825aab2c7a79611befb40a5fe7e610d5443
Gaia Date              2015-05-04 18:09:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/102d0e9aa9e1
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150504.193019
Firmware Date          Mon May  4 19:30:30 EDT 2015
Bootloader             L1TC000118D0

Device: N5_2.2 (Pass)
Build ID               20150504002502
Gaia Revision          8d14361337e608c8cdf165ea5034db5eda23b618
Gaia Date              2015-05-01 18:23:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7cb6597c91
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150504.041256
Firmware Date          Mon May  4 04:13:14 EDT 2015
Bootloader             HHZ12f

Device: N5_3.0 (Pass)
Build ID               20150504160201
Gaia Revision          70077825aab2c7a79611befb40a5fe7e610d5443
Gaia Date              2015-05-04 18:09:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/102d0e9aa9e1
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150504.193136
Firmware Date          Mon May  4 19:31:54 EDT 2015
Bootloader             HHZ12f
Duplicate of this bug: 1094747
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.