966829 - [User story] Support for continuous auto focus

Status: RESOLVED FIXED

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

Comment 1

[Youngjun Kim]

As I know, Mozilla engineer is working on it. Right?

Comment 2

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

Outstanding questions for Acceptance Criteria:
1a. To support this, we may need to be able to differentiate between "face detection has failed" and "haven't detected any faces YET"--from reading the API, I'm not sure this is possible.
1b/c/d. The visual indicator is only possible on JB- and KK-based builds.

Comment 3

[Jyothiprasad]

Once Gaia has information about face tracking failed or no faces detected from Gecko, it is possible to switch the focus mode from Face tracking  to Continuous Auto Focus mode.

Comment 4

[Hema Koka [:hema]]

Youngjun/jyothi,

What is the status of the this feature?

Thanks
Hema

Comment 5

[Jyothiprasad]

Hello Hema,

You can expect a PR for continuous Autofocus by Today.

Thanks

Comment 6

[Youngjun Kim]

Prasad

Comment 7

[Jyothiprasad]

Attachment: Pointer to pull request

Hello Diego,

This is a just work-in-progress patch to test Continuous Auto Focus (C-AF) functionality.
As per the plan, C-AF is part of focus priority and we can't test stand alone C-AF feature. So, I am enabling C-AF when viewfinder is clicked.

Later, I will submit patch for focus priority to schedule C-AF, touch focus and Face tracking modes.

Thanks.

Comment 8

[Jyothiprasad]

Attachment: Pointer to pull request

Hello Diego,

I have uploaded a patch for Continuous auto Focus. In this patch I am setting C-AF as default focus mode. Once the facetracking is landed then we switch the default mode as facetracking.

Please review the patch.

Regards,
Prasad

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8388046 [details] [review]
Pointer to pull request

r- for these reasons:

I don't actually know what the mozCamera.onAutoFocusMoving callback does, but giving fake focus feedback with timeouts seems completely unacceptable.

Also, this patch sets mozCamera.focusMode to continuous-picture, but does not modify the existing code that focuses the camera before taking a picture.  The primiary benefit of having CAF on is that the user can take pictures more quickly because the camera is already focused. But if the code is still attempting to focus before taking a picture, then we don't get the benefit from the feature.

Also there doesn't seem to be any code to determine whether continuous focus mode is supported. We should only turn it on as the default mode on hardware that supports it.

Also: there are no tests in this PR.

As a general note, I'd add that I think that doing this right is pretty hard.  I also expect that there will be a lot of interaction between CAF and touch focus, so it might make sense to combine those two features in to a single pull request.

Finally, we are trying to clear out the camera-dev branch and stop using it, so in the future, you should probably base your pull requests on camera-new-features.

Comment 10

[David Flanagan [:djf]]

I've learned more about the onAutoFocusMoving callback, and can add to the previous comment.  That callback gets a boolean argument. If true, we know that the camera is trying to refocus.  If false we know that it has stopped trying to refocus.  We do not know whether the focus has succeeded or not.

So, when the callback is called with the argument true, then we should display a focus ring animation to indicate that the focus is changing.  When the callback is called with the argument false, then we should hide the focus ring.  We should not make the ring green or red because we do not know whehter the focus succeeded or failed, only that it was in transition.

There should be no need to use setTimeout to respond to the onAutoFocusMoving callback.

Comment 11

[Jyothiprasad]

(In reply to David Flanagan [:djf] from comment #10)
> I've learned more about the onAutoFocusMoving callback, and can add to the
> previous comment.  That callback gets a boolean argument. If true, we know
> that the camera is trying to refocus.  If false we know that it has stopped
> trying to refocus.  We do not know whether the focus has succeeded or not.
> 
> So, when the callback is called with the argument true, then we should
> display a focus ring animation to indicate that the focus is changing.  When
> the callback is called with the argument false, then we should hide the
> focus ring.  We should not make the ring green or red because we do not know
> whehter the focus succeeded or failed, only that it was in transition.
> 
> There should be no need to use setTimeout to respond to the
> onAutoFocusMoving callback.

Hi David, Please check bug 966830, where I have uploaded a merged patch with this information.

Comment 12

[Wilson Page [:wilsonpage]]

Comment on attachment 8388046 [details] [review]
Pointer to pull request

r-

Comments on GH.

Comment 13

[singhashish1887]

Attachment: Pull Request(Camera-new-feature) Work in progress code

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8394715 [details] [review]
Pull Request(Camera-new-feature) Work in progress code

Clearing the review flag and setting feedback-. I've left a few comments on github.  If you're taking over the focus work from Prasad, please ask Prasad to forward all of the recent emails discussing the feature.

Also, note that at Mozilla we are currently assuming that the focus features will not make the deadline to be included in CS testing, and are prioritizing other features and bug fixing over focus-related work.

In general, please keep in mind that I've got a very heavy work load for camera and other FirefoxOS features.  If your code is marked "work in progress" please don't ask me to review it. Instead, if you'd like my feedback on a work in progress, set the feedback? flag and include a comment explaining what you have questions about or what parts of the code you'd particularly like feedback on.

Comment 15

[singhashish1887]

As discussed with Wilson, I am changing the code , Removing the 'focus.js' from controller and moving the code in camera.js library .We have uploaded the small part of code for review and based on your feedback will progress.

Comment 16

[Wilson Page [:wilsonpage]]

Comment on attachment 8394715 [details] [review]
Pull Request(Camera-new-feature) Work in progress code

This is a scary complex patch. I haven't spent the time to work through it properly, just a quick skim. But I'm r-'ing. This should probably be a 'feedback' request not 'review' as the patch is still a work in progress.

INITIAL FEEDBACK:

FocusRingView should be a direct 'sub-view' dependency of ViewfinderView, there is no reason for it to be floating around in `app` scope.

This is prime unit-test territory. You (the developer, not a separate team) need to be writing unit-tests as you go, to prove to yourself and reviewers that the code is conforming to the agreed spec. I'm worried unit-testing is still an afterthought, and simply there to 'tick a box'.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8394715 [details] [review]
Pull Request(Camera-new-feature) Work in progress code

Wilson has offered feedback so I'm clearing the feedback request on myself.

Comment 18

[singhashish1887]

Attachment: Pull Request

Comment 19

[Wayne Chang [:wchang]]

This is no longer in 1.4 scope, removing meta bug tracking dependency.

Comment 20

[David Flanagan [:djf]]

Ashish,

I'm not going to have time to look at this patch today.  But please note that I landed bug 995432 in master to enable simple C-AF.  Please look at that patch and make sure that this one is compatible with it. In particular, note the new resumeContinuousFocus() method that is needed after calling autoFocus() when in C-AF mode.

Comment 21

[Youngjun Kim]

Ashish is working on it. So I assigned it.

Comment 22

[Hema Koka [:hema]]

This is still happening in the 2.0 timeframe (just not marked blocker since it is a feature)

Comment 23

[Hema Koka [:hema]]

Youngjun, 

Any update?

Thanks
Hema

Comment 24

[singhashish1887]

We have Updated the code for Continues Focus along with Bug 966828, 966830.

Comment 25

[singhashish1887]

There is no separate patch for this. This code is part of 966828 (face tracking) as well 966830 (touch focus).

Comment 26

[Diego Marcos [:dmarcos]]

mike, Do we have a callback to know when continuous auto focus is focusing, when it has failed or succeded?

Comment 27

[Hema Koka [:hema]]

(In reply to Diego Marcos [:dmarcos] from comment #26)
> mike, Do we have a callback to know when continuous auto focus is focusing,
> when it has failed or succeded?

See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=965421

Comment 28

[Hema Koka [:hema]]

Diego, please review and update the status.

Comment 29

[Diego Marcos [:dmarcos]]

The continuous callbacks are now enabled by default on master:

https://bugzilla.mozilla.org/show_bug.cgi?id=1011101

Comment 30

[Diego Marcos [:dmarcos]]

We need to make a decision from the UX perspective if this is something we still want or not.

Comment 31

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

(In reply to Diego Marcos [:dmarcos] from comment #26)
>
> mike, Do we have a callback to know when continuous auto focus is focusing,
> when it has failed or succeded?

See bug 965421, which Hema linked to in comment 27.

This callback only tells you whether or not the camera is focusing. To determine whether or not the camera -is- focused, you need to call .autoFocus() and get the status from its callback.

Comment 32

[Tiffanie Shakespeare [:tif]]

Hey guys! We (Rob, Amy, and I) reviewed the focus work that's in master so far and have came up with some feedback:

1. We noticed that when hitting the shutter button, the camera refocuses for both auto and touch. This extra focus should be eliminated if auto and touch focus are working. It doesn't make sense to re-focus when already focused and the added delay will cause frustration.

2. Assuming #1 can be done (remove extra focus), auto-focus should have an indicator when focusing. So you're holding your phone, move it around and when you pause and the camera tries to focus there should be an indicator. This will give the user feedback that focus is working as well as what is being focused on. 

3. We also noticed auto-focus didn't always "resume". This was seen especially with touch-focus. Tapping on the screen would focus on the point, but when the phone was moved around auto-focus would not take over. The same seemed to be true after taking a photo; the focus point seemed to be stuck where you had tapped. Auto-focus should take over and when face-tracking is implemented, that should as well. 

Hopefully this is clear. Thanks Diego!

Comment 33

[Hema Koka [:hema]]

Thanks Tif, Rob and Amy!

Diego, assigning this to you.

Thanks
Hema

Comment 34

[Diego Marcos [:dmarcos]]

Comment on attachment 8406108 [details] [review]
Pull Request

This patch is obsolete. Removing review flags

Comment 35

[Hema Koka [:hema]]

Closing this one - Continuous auto focus is on;

visual feedback for C-AF is tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=1019965