1068559 - [KK] Continuous auto focus (CAF) not working in Camcorder

Status: VERIFIED FIXED

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

Description

[Pooja]

STR:
1. Open camera app
2. Switch to Camcorder
3. Try to focus on an object.

Actual behavior:
  CAF is not working

Expected behavior:
  CAF should work 

Device and build details:
Device msm8x26 QRD KK branch

Gaia : 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f
Gecko: a50444fdaa68fefefc3ecc08d97530e13a4742ab
OS Version: 2.1.0
Platform Version : 34.0a2
Build ID: 20140915184008

Comment 1

[Pooja]

[Blocking Requested - why for this release]:

Comment 2

[Jason Smith [:jsmith]]

QA Wanted - Is this possible to reproduce on a Flame KK 2.1 build?

Comment 3

[Pi Wei Cheng [:piwei] (inactive)]

Issue is reproducible on Flame 2.1, Flame 2.2 Master, and Open C 2.2 Master.

Observed behavior on Flame: Continuous auto focus is not enabled in video mode. User needs to manually tap on screen to make it focus.

Observed behavior on Open C: Continuous auto focus is not enabled in video mode. Tapping on screen doesn't make the camera focus either. Also it shows a band of artifacts on bottom of screen when in video mode.

Device: Flame 2.1
BuildID: 20140917073957
Gaia: 47939f4c41d0c941e5047e5d1af74a79b7d8e0d5
Gecko: d7ad9b5167d8
Version: 34.0a2 (2.1)
Firmware: v165
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master
BuildID: 20140916170759
Gaia: 50666fa8bbbf3d346faff24f92ad8140a44a49d0
Gecko: 8252eae8278c
Version: 35.0a1 (2.2 Master)
Firmware: v165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

--------------

Issue is NOT reproducible on Flame 2.0.

Observed behavior: Continuous auto focus is enabled in video mode. Device correctly adjusts its focus when moving device away or close to objects without user tapping on screen.

Device: Flame 2.0
BuildID: 20140917073857
Gaia: 31434a3949556171f3565ca47ac2b44e810e95e6
Gecko: e02fe140c0d5
Version: 32.0 (2.0)
Firmware: v165
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[Pi Wei Cheng [:piwei] (inactive)]

Forgot to post Open C environmental variables on comment 3:

Device: Open_C 2.2 Master
BuildID: 20140916170759
Gaia: 50666fa8bbbf3d346faff24f92ad8140a44a49d0
Gecko: 8252eae8278c
Version: 35.0a1 (2.2 Master)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 5

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20140625143731
Gaia: 515914905d40d171ea6b927f4c8cbbb93c5bde20
Gecko: ffa21babf90c
Version: 33.0a1
Firmware: V123

First Broken Environmental Variables:
Device: Flame
BuildID: 20140625144906
Gaia: 515914905d40d171ea6b927f4c8cbbb93c5bde20
Gecko: 7a84bd4bb3da
Version: 33.0a1
Firmware: V123

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=ffa21babf90c&tochange=7a84bd4bb3da

Caused by Bug 1025197.

Comment 6

[Joshua Mitchell (Inactive)]

broken by  Bug 1025197 - can you take a look Mike?

Comment 7

[Hema Koka [:hema]]

Blocking Reason: Regression

Andrew, please investigate this one (Mike is out till 22nd)

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

While it is not 100% reproducible for me, it does happen often enough that I think I've captured the problem. When auto focus stops moving in continuous-video mode, the camera app attempts to auto focus explicitly. This is permitted behaviour according to the AOSP documentation [1]. While the driver appears to kick off the focus, it would not return until it was very close to an object, and even then the focus would fail in the end.

The AOSP documentation [2] suggests that it is intended for UI updates and its camera application [3] follows this behaviour. I changed our application to follow this and the performance is much more satisfactory on flame.

[1] http://developer.android.com/reference/android/hardware/Camera.Parameters.html#FOCUS_MODE_CONTINUOUS_VIDEO
[2] http://developer.android.com/reference/android/hardware/Camera.AutoFocusMoveCallback.html
[3] https://android.googlesource.com/platform/packages/apps/Camera2.git/+/master/src/com/android/camera/FocusOverlayManager.java line 305

It is possible that setting the recording hint aggravated the driver somehow (the change marked as introducing the regression). Previously we would let the driver set it internally, and only reset it after ending a recording. But my belief is that that code is correct :).

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gaia pull request, v1

piwei: Could you try out the pull request and let me know if resolves the problem for you? The bug *should* affect 2.0 (and 1.4) but since I couldn't reproduce 100% of the time, so I want to make sure I am fixing the right problem :).

Comment 10

[Pi Wei Cheng [:piwei] (inactive)]

Comment on attachment 8491783 [details] [review]
Gaia pull request, v1

This patch fixes the continuous auto focus issue. I'm now seeing the video recorder continuously making effort to focus.

However I am seeing another issue where sometimes when it's having difficulty focusing, the viewfinder/screen would continuously go very dark and very bright until it successfully focuses, which seems a bit odd to me. I could add a video if needed.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Pi Wei Cheng [:piwei] from comment #10)
> Comment on attachment 8491783 [details] [review]
> Gaia pull request, v1
> 
> This patch fixes the continuous auto focus issue. I'm now seeing the video
> recorder continuously making effort to focus.
> 
> However I am seeing another issue where sometimes when it's having
> difficulty focusing, the viewfinder/screen would continuously go very dark
> and very bright until it successfully focuses, which seems a bit odd to me.
> I could add a video if needed.

I've seen this before independent of my change, but only on KK; v165 is what I have been working with. Did you test this on JB or KK?

Comment 12

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Andrew Osmond [:aosmond] from comment #11)
> I've seen this before independent of my change, but only on KK; v165 is what
> I have been working with. Did you test this on JB or KK?

I was on KK (v180) base when testing the patch.

Comment 13

[Diego Marcos [:dmarcos]]

We currently call autofocus when the onAutoFocusMoving callback is invoked. It's the only way we had to determine if CAF succeded or failed to focus a subject and show a red or green focus ring accordingly.

bug 994912 is introducing events instead of callbacks and will make onAutoFocusMoving unnecessary solving this bug as well.

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Diego Marcos [:dmarcos] from comment #13)
> We currently call autofocus when the onAutoFocusMoving callback is invoked.
> It's the only way we had to determine if CAF succeded or failed to focus a
> subject and show a red or green focus ring accordingly.
> 
> bug 994912 is introducing events instead of callbacks and will make
> onAutoFocusMoving unnecessary solving this bug as well.

bug 994912 will not land in time for 2.1, so we need a way to resolve this regression with  minimal risk

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gecko, add support for auto focus completed cb, v1

Bug 994912 will be committed on Monday but maybe it is a bit large / risky to pull into 2.1. Created gecko patch to add necessary functionality for apps to get desired auto focus state. Updated gaia pull request to follow.

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

gecko try: https://tbpl.mozilla.org/?tree=Try&rev=ff61842e7f31

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gaia pull request, v2

Updated gaia pull request to rely upon the new auto focus completed method added in the gecko patch.

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gecko, add support for auto focus completed cb, v2

gecko try: https://tbpl.mozilla.org/?tree=Try&rev=89247df58b68

Don't be hasty. I reused a previous debugging hg patch queue and forgot there was still stuff in the tree. Mea culpa.

Comment 19

[Dave Hylands [:dhylands]]

Comment on attachment 8492455 [details] [diff] [review]
Gecko, add support for auto focus completed cb, v2

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

r=me with the test issue resolved.

::: dom/camera/test/test_bug1022766.html
@@ +39,2 @@
>    checkForDone: function test_checkForDone() {
> +    if (Camera.firstCallFailed && Camera.secondCallSucceeded && Camera.callbackTriggered) {

This says callbackTriggered but a couple lines above this you set callbackTrigger (no ed).

You should add a test for the callback not being called and make sure that calbackTriggered is still false.

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gecko, add support for auto focus completed cb, v3

(In reply to Dave Hylands [:dhylands] from comment #19)
> Comment on attachment 8492455 [details] [diff] [review]
> Gecko, add support for auto focus completed cb, v2
> 
> Review of attachment 8492455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the test issue resolved.
> 
> ::: dom/camera/test/test_bug1022766.html
> @@ +39,2 @@
> >    checkForDone: function test_checkForDone() {
> > +    if (Camera.firstCallFailed && Camera.secondCallSucceeded && Camera.callbackTriggered) {
> 
> This says callbackTriggered but a couple lines above this you set
> callbackTrigger (no ed).
> 
> You should add a test for the callback not being called and make sure that
> calbackTriggered is still false.

Nit fixed.

gecko try: https://tbpl.mozilla.org/?tree=Try&rev=e060b4f64641

Comment 21

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

Comment on attachment 8493035 [details] [diff] [review]
Gecko, add support for auto focus completed cb, v3

Andrew, if we want to land this, you'll need a DOM-peer to review the WebIDL change.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8493035 [details] [diff] [review]
Gecko, add support for auto focus completed cb, v3

bz: This should be a quick review compared to bug 994912 :). This is a temporary addition which that bug deprecates (because ideally we won't put 994912 in 2.1 since it is so big).

Comment 23

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

aosmond, dmarcos: with the above Gecko and Gaia patches applied to a Nexus 4, I see CAF working properly, with no spurious flash fires in varying lighting conditions.

I do, however, still see an initial CAF-moving circle appear and go green when I switch to video mode; after it goes away, CAF still works but the circle never reappears.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493035 [details] [diff] [review]
Gecko, add support for auto focus completed cb, v3

r=me

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

The gecko change is safe to check in on its own, but the gaia change needs to go in after it.

Comment 26

[Carsten Book [:Tomcat]]

landed in https://hg.mozilla.org/integration/b2g-inbound/rev/7ea7a02ce2be

and gaia master: https://github.com/mozilla-b2g/gaia/commit/d9106e0e0b50e262cc6b16b98a068c65e5880342

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ea7a02ce2be

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Please add Gaia v2.1 and aurora approval requests when you get a chance :)

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8493035 [details] [diff] [review]
Gecko, add support for auto focus completed cb, v3

Approval Request Comment
[Feature/regressing bug #]: 1068559
[User impact if declined]: Continuous auto focus performance will be very poor. User may see flash turn on when unexpected.
[Describe test coverage new/current, TBPL]: Augmented test case which verifies we receive new callback added when expected with the emulated camera. Verified manually on flame and nexus4 that it works.
[Risks and why]: It is possible that some drivers behave differently from flame-jb, flame-kk and nexus4 and we won't receive the driver event; we didn't rely upon receiving this event in this context in the past.
[String/UUID change made/needed]: None

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8492449 [details] [review]
Gaia pull request, v2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1068559
[User impact] if declined: Continuous auto focus performance will be very poor. User may see flash turn on when unexpected.
[Testing completed]: Added/updated unit test cases for affected modules. Verified manually on flame and nexus4 that CAF works as expected now.
[Risk to taking this patch] (and alternatives if risky): It is possible that some drivers behave differently from flame-jb, flame-kk and nexus4 and we won't receive the driver event; we didn't rely upon receiving this event in this context in the past. That would leave the focus ring displayed on the application some of the time.
[String changes made]: None

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/38ad67d1f185

v2.1: https://github.com/mozilla-b2g/gaia/commit/fd3e01dc3699518b32a2ac130d54c40cff6e5413

Comment 33

[Pooja]

Hi Andrew,

We still see the issue in our latest build in camcorder CAF. 
Triggering for CAF occurs however the focus doesn't happen.

Comment 34

[Pooja]

Hi Andrew,

We still see the issue in our latest build in camcorder CAF. 
Triggering for CAF occurs however the focus doesn't happen.


Kindly confirm once and re-open the bug

Comment 35

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Pooja from comment #34)
> Hi Andrew,
> 
> We still see the issue in our latest build in camcorder CAF. 
> Triggering for CAF occurs however the focus doesn't happen.
> 
> 
> Kindly confirm once and re-open the bug

I am unable to confirm on flame v188 with latest b2g-inbound and 2.1 builds. 2.0 does not perform as well as 2.1/b2g-inbound but it still autofocuses, just less frequently. Has the STR changed at all or is it still as in the original description? Could you also let me know the gaia/gecko/etc versions you are using?

Comment 36

[Hema Koka [:hema]]

Adding qawanted to see if this is reproducible with latest 2.1 build on flame with KK.

Thanks
Hema

Comment 37

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Hema Koka [:hema] from comment #36)
> Adding qawanted to see if this is reproducible with latest 2.1 build on
> flame with KK.

Issue is NOT reproducible on 2.1 KK Flame. Camcorder automatically and continuously makes effort to focus and would show a green ring once it successfully focuses.

Device: Flame (full flash, 512MB mem)
BuildID: 20141028001203
Gaia: a0174f7166745256aaca1cb3aa9f894033fbffa6
Gecko: 43bda3541f6b
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 38

[Hema Koka [:hema]]

Pooja, please check. We are not able to reproduce on Flame KK. 

Thanks
Hema

Comment 39

[Pooja]

Yeas, its not reproducible on flame. 
We debugged it as internal issue.

Thanks everyone for your time and support :)

Comment 40

[sarsenyev]

Verified the bug is fixed on 2.2 and 2.1 Flame builds'
Camcorder automatically and continuously makes effort to focus and shows a green ring once it successfully focuses.

Device: Flame 2.2 Master KK
BuildID: 20141030040201
Gaia: 0dfc1996eb583c8b507a82bf6b8319624bba23ea
Gecko: 80e18ff7c7b2
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1 KK
BuildID: 20141030001201
Gaia: 3e585d8be5e2dffc376f83313299c9b6d53c3db4
Gecko: b643d78a23c6
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0