Closed Bug 995432 Opened 10 years ago Closed 10 years ago

[Camera] implement basic support for continuous auto focus in refactored camera.

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: djf, Assigned: djf)

Details

(Keywords: regression)

Attachments

(2 files)

We added basic C-AF support to the v1.3 camera app at the same time that we were refactoring the app for 1.4. Somehow, the C-AF support never made it into 1.4, so now we are in a situation where we dropped an fairly important feature.

Bug 966829 is about a full-featured C-AF implementation with user feedback for focus changes and a green or red focus ring when the user presses the shutter button.

This bug is just about bringing the 1.4 camera up to feature parity with the 1.3 camera.  I've discussed with Tiffany and Amy and they agree that it is okay to lose the focus ring in exchange for getting pictures more quickly.
Marking this as a regression and a 1.4+ blocker and assigning to myself.
Assignee: nobody → dflanagan
blocking-b2g: --- → 1.4+
Keywords: regression
Justin,

This is fairly close to what we do in 1.3.

The changes in config/settings.js and in main.js are just to make it so we can disable it.  The change in lib/sounds.js is required because we can now take pictures so fast that the shutter sound wasn't playing all the time.

In 1.3 we use continuous-video mode for videos, but in my testing regular 'auto' mode gave better C-AF for videos than continous-video did. 1.4 was already using 'auto' by default for videos, so this only changes  still photos.

The front camera (on the nexus 4 at least) has fixed focus, so I think we were not calling autoFocus() for selfies. That is still true with this patch, but now we at least display the focus ring in that case.
Attachment #8405676 - Flags: review?(jdarcangelo)
Comment on attachment 8405676 [details] [review]
link to patch on github

Tif,

Here's the C-AF patch. It works on the nexus 4, and now we can take pictures really, really fast.
Attachment #8405676 - Flags: ui-review?(tshakespeare)
The pics are wicked fast. It is me or do they seem more out of focus especially the closer ones?
David: Nothing that I can see is really wrong with your patch. However, I'm seeing where it seems like C-AF stops doing its thing after the first picture is taken. After you take one picture, it doesn't seem like the camera ever re-focuses on anything, it just stays where its at. I don't think this is anything you're doing in the patch though. I'm setting NI? for mikeh, maybe he can shed some light on this.
Flags: needinfo?(mhabicher)
Talked with Justin a bit about this too and I agree. It seems like the focusing isn't totally working. It's super fast but everything seems to come out fuzzy. I've gotta run now, but I'll check back in later.
Justin, Tif,

Thanks for the comments. I had times where I thought it wasn't focusing either, but then I tried some more and noticed that it was focusing, it just took a while.  I've only tested it on the Nexus 4, and I know that's what Tif is using too. Justin: did you try any other devices?

I'll see if I can run it on my hamachi or helix and see what happens there.

The ideal situation is to combine C-AF mode with explicit calls to autoFocus() that ensure that we're in focus. That should be faster when the camera is already focused, but should also take time to get focus if it isn't focused, and it allows us to give green or red focus ring feedback.  The problem is that in order to get back into C-AF mode after calling autoFocus() there is a method that needs to be exposed in gecko.  Mike is on it, but probably not in time for 1.4...

I'll do some experiements and see if I can figure out a way around the limitation. (Like switching the focus mode to auto and then back to continuous)
After further test, I find:

- the nexus 4 does CAF well, until I take a picture, then it stops auto focusing.  If I go to the homescreen and open the camera again, it auto focuses again. If I don't actually call autoFocus(), then it is still possible to take a picture while it is between focuses and get a blurry picture.  So I really need to call autoFocus() in CAF mode.  If I do, it is much (10x) quicker than a full auto-focus, but, again, CAF doesn't resume after taking a picture.  I really need the reset method, I suppose.

- The helix works much better, which we already know from 1.3.  It even works if I do the autoFocus() can in CAF mode.  CAF resumes after the picture even though I don't have a reset method to call.

- The hamachi is a fixed focus camera. Somehow I had the idea that it could do autofocus.

I'm sort of stuck here. It seems that CAF on Nexus 4 does not work well enough to land this patch. (Unless maybe the cancelAutoFocus() method that mike is exposing fixes the problem).

On the other hand, the Helix has good CAF in 1.3, and I'd like it to be an option for 1.4. So maybe we need to land this patch disabled in settings so there is at least a build-time option to enable CAF on devices that support it.
I had hoped I could workaround the lack of a resetAutoFocus method by setting the camera to some other focus mode waiting (or running an autofocus cycle) and then returning to CAF mode.  None of this was necessary on the Helix, and none of it worked on the Nexus 4.  (Though CAF was broken the Nexus4 even if I never called autoFocus.)
"On the other hand, the Helix has good CAF in 1.3, and I'd like it to be an option for 1.4. So maybe we need to land this patch disabled in settings so there is at least a build-time option to enable CAF on devices that support it."

This might be possible - it sucks though. I like the speediness of the patch but having a whole bunch of blurry photos negates that. :-/

Do we have any idea what the target device will most likely be able to handle?
I'll have to try building Mike's patch in bug 986024 and see if that saves us, I guess.  Its awfully close to the wire though!
(In reply to Justin D'Arcangelo [:justindarc] from comment #5)
>
> After you take one picture, it doesn't seem like the
> camera ever re-focuses on anything, it just stays where its at. I don't
> think this is anything you're doing in the patch though. I'm setting NI? for
> mikeh, maybe he can shed some light on this.

This sounds like the behaviour expected without having called cancelAutoFocus().

(In reply to David Flanagan [:djf] from comment #9)
>
> I had hoped I could workaround the lack of a resetAutoFocus method by
> setting the camera to some other focus mode waiting (or running an autofocus
> cycle) and then returning to CAF mode.  None of this was necessary on the
> Helix, and none of it worked on the Nexus 4.  (Though CAF was broken the
> Nexus4 even if I never called autoFocus.)

I was going to suggest this, but it sounds like it's not a solution. :(

(In reply to David Flanagan [:djf] from comment #11)
>
> I'll have to try building Mike's patch in bug 986024 and see if that saves
> us, I guess.  Its awfully close to the wire though!

Please let me know if it works for you. I'm going to line up reviews for this patch so we can get it in soon.
Flags: needinfo?(mhabicher)
After discussing with Hema, we think there may not be any commercial devices that support C-AF in 1.3 that will also be using the 1.4 release. So there may not be any users that see a regression if we don't get C-AF into 1.4.

On the other hand, it is still a really nice feature and the patch is simple. In order to get it work correctly on the nexus 4, we need the resumeContinuousFocus() method that is exposed by bug 986024. I've updated my patch to only use C-AF for hardware that supports it and supports resumeContinuousFocus(), so if Mike is able to land his gecko feature, then this patch will be ready to use it.
Attachment #8405676 - Flags: review?(jdarcangelo) → review+
Landed to master: https://github.com/mozilla-b2g/gaia/commit/289f131cc65dbf35f69924254a71e76628d2e36d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Uplifted to v1.4: https://github.com/mozilla-b2g/gaia/commit/9b877e7b63c61f9f5b27253aa208519cb9eb2acd

If the gecko patch in bug 986024 lands in 1.4 as well, then we'll have simple C-AF in the camera
Comment on attachment 8405676 [details] [review]
link to patch on github

Clearing the ui-review request because there wasn't actually anything to review in this patch.
Attachment #8405676 - Flags: ui-review?(tshakespeare)
Travis is not happy: https://travis-ci.org/mozilla-b2g/gaia/jobs/22997865
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Waiting for travis before re-landing the patch
Travis is green this time.
Relanded to master: https://github.com/mozilla-b2g/gaia/commit/35ed2ac8d17d373d5e34037bb24e60686561fa84
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S5 (11apr)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: