Closed Bug 925192 Opened 11 years ago Closed 11 years ago

[Media] [Camera][User Story] Enable continuous auto focus mode if the camera supports the capability

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.3 Sprint 5 - 11/22

People

(Reporter: skasetti, Assigned: mikeh)

References

Details

(Whiteboard: [FT:Media, ucid:Media 48][1.3:p2])

Attachments

(3 files, 4 obsolete files)

Helix current consumption, focusMode = 'continuous-picture'
83.75 KB, image/png
Details
Helix current consumption, focusMode = 'auto'
81.08 KB, image/png
Details
Link to PR to enable continuous auto-focus mode
358 bytes, text/html
mikeh
: review+
Details 
User Story:
If the camera supports continous auto focus then the app should always have continuous auto focus option enabled

Acceptance Criteria:
- When the user taps the settings icon in the camera app, an option to set continuous auto focus should be displayed (this only occurs when the camera hardware does support continuous auto focus mode)
- By default continuous auto focus mode should be disabled
- When the user chooses the continuous auto focus option the camera should operate in that mode always (state should be remembered)
blocking-b2g: --- → 1.3?
Flags: in-moztrap?(mozillamarcia.knous)
On unagi, this doesn't seem to do anything. :(
To improve the user experience, it should give a setting item to user to choose the focus mode.
And it still need to implement the focus ring which can indicate the real time focus state.
Assignee: nobody → dmarcos
Mike,

The second part (in resumePreview()) has a bug. It should be this._cameraObj instead of camera.  But it seems to work if I just comment out all that code in resumePreview() anyway.

It works for me on the Helix. Continuous focus mode is quite nice. I can take pictures much more quickly than I can with autofocus.

Sometimes, though, I would tap the shutter button and nothing would happen for a while. Maybe it wasn't focused yet and wasn't allowing me to take a picture yet until focus was locked?

Mike: does the hardware tell you when continuous-picture mode has locked focus and when it has lost focus?  I think we could make this work better if we could tell the user when the camera is ready to take pictures.  And if the user clicks the shutter button when the camera is not focused, then maybe we call autoFocus() manually in that case.
(In reply to lecky from comment #4)
> To improve the user experience, it should give a setting item to user to
> choose the focus mode.
> And it still need to implement the focus ring which can indicate the real
> time focus state.

Mike's patch is just a proof of concept to see if it works, not a finished patch.

Note, however, that our UX designers would prefer to not have a control for this: if possible we just want to use continuous focus always.  If we can't do that, we'll have a switch.
Mike Lee,

UX has asked us to enable continuous focus mode for the camera. Before we make it the default, I suppose we should have some idea what the battery life implications are. Does anyone on your team have the time to measure the power consumption of the camera app with and without Mike H's patch above?  (It would also be interesting to check power consumption to ensure that the camera stops trying to focus when it is not in the foreground.  I assume it does this automatically, but don't know how we can know for sure without measuring.)
Flags: needinfo?(mlee)
David,
How soon would you like this?

Jon (ni'd) can help with this after 11.08, possibly before. Longer term we can provide a harness to the Media team so you can do this type of analysis yourselves. Jon's started putting together instructions for using the harness here: http://www.huv.com/harness/

Jon,
How long would it take to create and send a harness to either David/Mike H?
Flags: needinfo?(mlee) → needinfo?(jhylands)
I could drop a harness off for Mike some time next week. He should order an ammeter, since mine still isn't ready for prime time. If I go to the office, I can bring my yoctopuce ammeter so he can do some tests while I'm there.
Flags: needinfo?(jhylands)
Hi Jon, let me know when you'll be around. What was the lead time on ordering a Yoctopuce?
Right now I'm planning on coming in on Wednesday - I'll probably show up around 10:30-11:00 am.

Lead time on ordering is about a week, I believe.
So, it turns out this requires a Unagi, which I don't have a harness for at this point. I can build one in about a week, so if we can hold off a week I can do this...
Jon/Mike - It will be good to test the battery consumption on a production device - perhaps helix or a leo (instead of unagi). Also if you need to place order for the parts of the harness, please go ahead. 

We are targeting to get this feature in for 1.3.

Thanks!
Hema
(In reply to David Flanagan [:djf] from comment #5)
> 
> Sometimes, though, I would tap the shutter button and nothing would happen
> for a while. Maybe it wasn't focused yet and wasn't allowing me to take a
> picture yet until focus was locked?

The AOSP documentation here[1] makes a hash of explaining how the API is supposed to work. Probably easiest if you give it a quick read yourself.

> Mike: does the hardware tell you when continuous-picture mode has locked
> focus and when it has lost focus?  I think we could make this work better if
> we could tell the user when the camera is ready to take pictures.  And if
> the user clicks the shutter button when the camera is not focused, then
> maybe we call autoFocus() manually in that case.

djf, the way it's supposed to work is you call .autoFocus() and if the subject is already in focus (or autofocus failed), you'll get the callback right away indicating success or failure; if the autofocus scan is in progress, the callback will return when it completes.

According to the header I've linked to below, it looks like in order to _resume_ continuous autofocus after the call to .autoFocus() above, we need to call a function called cancelAutoFocus() (which is not currently exposed to JS).

IMHO, I think this API is kind of horrid. If you have any thoughts on the best way to expose this to JS, I'd love to hear it. I can handle the messiness in Gecko.

1. http://androidxref.com/4.0.4/xref/frameworks/base/include/camera/CameraParameters.h#628
I was curious about whether or not the AOSP 4.4 CameraParameters.h header had more information on how to use continuous auto-focus, or if the API had changed -- unfortunately no.
Whiteboard: [FT:Media, ucid:Media 48]
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Fixes cut and paste error pointed out by djf. Tested/works on Helix/v1.1.0hd.
Attachment #821192 - Attachment is obsolete: true
Average current with continuous autofocus enabled: ~380mA.
Current consumption: ~315mA.

So using continuous auto-focus increases the current consumption of the camera by ~65mA.

For anyone trying to reproduce this, it's important that the phone/camera be pointed at the same scene for both tests to get comparable results, as power draw seems to vary with the brightness of the scene. e.g. when the phone is lying flat on a desk and the scene is almost completely black, current draw drops below 300mA; when pointed at a bright light, current draw rises to ~340mA.
Thanks for the link to the Android doc, Mike.  I agree that it is a confusing API.  It sounds to me like we do need to expose the cancelAutoFocus() function. And also ensure that the boolean parameter to the autoFocus() callback is getting passed.  Also, it seems wrong to use the same autoFocus() method in auto mode and in continuous focus mode. And the API might be clearer if we separated the two cases.

In auto focus mode (what we have now), maybe we rename the existing autoFocus() method to focus() and not allow focus() to be called in continuous focus mode.

In continuous focus mode, maybe there should be a new pair of functions lockFocus() and unlockFocus() that can only be called in continuous mode.

I'm not certain that would help, but it seems to me that it is a start at simplifying the API.

It sounds like when we're in continuous focus mode, there are three possible focus states the camera can be in: 

 - focusing
 - sharp focus
 - focused, but not sharp

It is unclear to me how common that third state will be in practice.  But given these states, we might have a continuous focus UX like this:

- when the user touches the shutter button, we call lockFocus() and set a timer for some short (50ms?) interval. 

- when the user lifts their finger off the shutter button we take a picture no matter what the state of the focus is. After taking the picture we call unlockFocus() to return to continuous focus mode.

- if the timer triggers before the lockFocus() callback is called, then we draw a red focus ring or do something else to indicate that we are in the "focusing" state.  Actually instead of a static red focus ring, perhaps we animate a rotation or something like the Android camera does to show that something is happening.

- when the lock focus callback is called we cancel the timer if it has not already been triggered. Then:

  - if the callback argument is true we know we have sharp focus and we display a green focus ring or do something else to let the user know that focus is locked and the camera is ready to take a sharp picture.

  - if the callback argument is false, then we don't have sharp focus and we display a red focus ring.  At this point the simplest thing is to give up and let the user slide their finger off the shutter button and try to recompose the shot so that the camera can focus. If this kind of focus failure is rare then that is probably okay. But if it happens more often then maybe we need to "run a full auto focus cycle" as the Android docs suggest.  So we'd take the camera out of continuous focus mode, call focus(), perform some focusing animation and then wait for the user to release the shutter button.  We'd have to be careful here so that we do the right thing even if the user releases the shutter and takes a picture while the focus() call was pending. 

I think I'd want to sketch out a finite state machine for all of this to make sure we cover all the possible states and transitions between states for this kind of focus
My Android camera appears to do continuous autofocus.  Each time focus changes it displays a rotating focus ring, so I know it is focusing. So somehow it knows when focus is changing.  I'd like to be able to do the same thing, but it sounds like there may be an undocumented API involved.

Or, maybe what the android camera is doing is not using continuous autofocus but just calling autoFocus() every now and then and animating the focus ring until the callback is called.

Since we are alredy listening to the accelerometer to know our orientation, we could refocus the camera immediately when the user moves it. And then refocus every second or two in case the subject has moved even when the camera has not?

I wonder what impact that would have on power consumption? More or less than continuous focus mode? I suppose it depends on what happens when you call autoFocus() when the camera is already focused.  If it just does nothing and calls its callback, then this could be really good approach.  But if it "runs a full auto focus cycle" regardless of the current focus, then this is probably a wasteful approach.
Jon,

As a performance person, what do you think about using 65mA for continuous focus? Is that something we should have on by default with no way of disabling?  Or would you recommend that we allow the user to disable continuous focus to save power? UX would like us to just use continuous focus mode and not give the user any choice.
Flags: needinfo?(jhylands)
CC'ing Dave Huseby who's working with Jon on power measurement.

Dave,
Any comments on djf's comment #21?
Have you guys looked at or considered how auto-focus works with video recording?

I don't think 65 mA is that big of a deal, given that the camera baseline is 315 mA - users don't typically spend a huge amount of time in the camera app, unless they are taking a lot of pictures, and then I think there is an expectation that a reasonable amount of battery will be consumed. I suspect most users that spend that much time in the camera app are probably more interested in image quality than battery consumption.

It seems like a reasonable trade-off to me. I agree with UX's position.
Flags: needinfo?(jhylands)
djf, is your Android phone running Jelly Bean or later? If so, API 16[1] added a new function to tell when continuous autofocus is moving[2].

1. http://developer.android.com/guide/topics/manifest/uses-sdk-element.html#ApiLevels
2. http://developer.android.com/reference/android/hardware/Camera.AutoFocusMoveCallback.html

Regarding the impact on battery life, e.g. Helix's 1730mAh cell can run the camera in autofocus mode for 5.5 hours (to first-order--current consumption likely goes up as the battery discharges); but only 4.5 hours with continuous focus enabled. That's assuming the user is doing nothing other that using his/her phone as a camera.
Flags: needinfo?(dflanagan)
Further to comment 24, here is the new CAMERA_MSG_FOCUS_MOVE event[1] in 4.1.1 (JB) that is missing in 4.0.4(ICS)[2].

1. http://androidxref.com/4.1.1/xref/system/core/include/system/camera.h#90
2. http://androidxref.com/4.0.4/xref/system/core/include/system/camera.h#72
Thanks for the details, Mike.  It sounds like we have consensus that we won't worry about power consumption and will just go with continuous autofocus as the default.

I propose a three step plan:

1) Let's land Mike's totally straightforward patch to enable continous autofocus right away. If nothing else, that will meet our 1.3 goal here, if UX likes how it works.

2) Let's file a followup bug for the more sophisticated approach (possibly minus the "run a full autofocus cycle part" at the end) I outline in comment 19, so that we can give the user feedback with a focus ring. Once we have some camera refactoring done, this focus management stuff might be something for Justin to tackle. I'd say we should treat that as a nice-to-have for 1.3, but not something we commit to. Doing this will require Mike to expose the cancelAutoFocus() method (or unlockFocus() if we use the naming scheme from comment 19)

3) When we move to Android JB, let's revisit this and give more focusing feedback.
Flags: needinfo?(dflanagan)
Comment on attachment 828177 [details] [diff] [review]
camera.js patch to use continuous auto-focus in picture mode, v2 [v1.1.0hd]

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

This is a nice simple patch that seems to work well. r+ to land it if you address the review comments.

::: apps/camera/js/camera.js
@@ +890,5 @@
>          camera.capabilities.focusModes.indexOf('auto') !== -1;
> +      if (camera.capabilities.focusModes.indexOf('continuous-picture') !== -1) {
> +        camera.focusMode = 'continuous-picture';
> +        dump("mikeh: 'continuous-picture' focus mode enabled!");
> +        this._autoFocusSupported = 0;

Use false instead of 0, please.
And remove the debugging output.

@@ +893,5 @@
> +        dump("mikeh: 'continuous-picture' focus mode enabled!");
> +        this._autoFocusSupported = 0;
> +      } else {
> +        dump("mikeh: 'continuous-picture' focus mode not supported.");
> +      }

Remove the entire else clause

@@ +1073,5 @@
>      this._previewActive = true;
>      this.enableButtons();
> +    if (this._cameraObj.capabilities.focusModes.indexOf('continuous-picture') !== -1) {
> +      this._cameraObj.focusMode = 'continuous-picture';
> +      dump("mikeh: 'continuous-picture' focus mode re-enabled!");

Remove the debugging output.

Is setting focusMode actually necessary here, or is only used if we call autoFocus()?  Since you disable autoFocus() above, I wonder if we can just remove all of this.
Attachment #828177 - Flags: review+
I've done an unsolicited review of Mike's patch. Mike do you want to just steal this bug from Diego and land a modified version of your patch?  And if so, would you also file the followup bug described above?
Flags: needinfo?(mhabicher)
Assignee: dmarcos → mhabicher
Flags: needinfo?(mhabicher)
(In reply to David Flanagan [:djf] from comment #27)
>
> @@ +1073,5 @@
> >      this._previewActive = true;
> >      this.enableButtons();
> > +    if (this._cameraObj.capabilities.focusModes.indexOf('continuous-picture') !== -1) {
> > +      this._cameraObj.focusMode = 'continuous-picture';
> > +      dump("mikeh: 'continuous-picture' focus mode re-enabled!");
> 
> Remove the debugging output.
> 
> Is setting focusMode actually necessary here, or is only used if we call
> autoFocus()?  Since you disable autoFocus() above, I wonder if we can just
> remove all of this.

I've confirmed that it's not necessary and can be removed.
This PR incorporates earlier feedback, but also differentiates between continuous picture and video modes.
Attachment #828177 - Attachment is obsolete: true
Attachment #830365 - Flags: review?(dflanagan)
Comment on attachment 830365 [details]
Link to PR to enable continuous auto-focus mode

Since you've revised your patch to support both pictures and videos, we've got to also change focus mode when the user switches between picture and video.  Probably in the changeMode() function.

I think even the old version of the patch would have been wrong: it would have set focus mode to continuous-picture and then left it that way for videos.

In changeMode() if the continuous focus mode is not supported, you'll have to set focus to its default value, whatever that is. That is for the case where continuous focus is supported for photos but not videos or vice versa. I don't even know what the default mode is.

And if you are making those changes, let's not set autofocusSupported to false.  autofocus is still supported, but we're just not using it.  Better to have a flag instead for whether continuous focus is supported. And if that flag is set, then we don't call autoFocus().
Attachment #830365 - Flags: review?(dflanagan) → review-
Take 2.
Attachment #830365 - Attachment is obsolete: true
Attachment #830424 - Flags: review?(dflanagan)
Whiteboard: [FT:Media, ucid:Media 48] → [FT:Media, ucid:Media 48][1.3:p2]
Blocks: 933912
Comment on attachment 830424 [details]
Link to PR to enable continuous auto-focus mode

I have not tried it out, but the code looks like it should work.

I've left a number of suggestions on github. If you prefer, however, you could just land this as is, and address the suggestions as part of Justin's refactor.

In either case, do coordinate with Justin before landing.  At least to let him know that this change is coming.
Attachment #830424 - Flags: review?(dflanagan) → review+
Justin is aware of this. I think we should land this as soon as we can
Incorporating feedback from previous review, carrying r+ forward.
Attachment #830424 - Attachment is obsolete: true
Attachment #831029 - Flags: review+
The linter is complaining on your pull request. The following lines in your camera.js file are too long

Line 1077, E:0110: Line too long (104 characters).
Line 1078, E:0110: Line too long (102 characters).
Line 1079, E:0110: Line too long (100 characters).

I really don't like this 80 characters limit.
(In reply to Diego Marcos from comment #37)
> 
> I really don't like this 80 characters limit.

Do I really need to fix this??
Yes because it breaks the gaia test suite. We would be landing a red tree.
Merged.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Not a committed feature, so clearing nom.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: