Closed Bug 966830 Opened 6 years ago Closed 6 years ago

[User story] Add touch focus to the camera

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S2 (23may)
tracking-b2g backlog

People

(Reporter: skasetti, Assigned: dmarcos)

References

Details

(Whiteboard: [fxos:media] [m+])

User Story

User Stories:

1. As a user, while in camera mode I want auto focus be enabled by touch if the device doesn't support either face tracking or continuous focus
2. As a user, while in video mode I want auto focus be enabled by touch if the device doesn't support either face tracking or continuous focus

Acceptance Criteria:

1. Camera mode
a) The user can touch the viewfinder to focus on object of interest
b) A visual indicator will appear over the object of interest
c) Visual indicator will change color indicating that the focus was succesful

2. Video recording mode
a) The user can touch the viewfinder to focus on object of interest
b) A visual indicator will appear over the object of interest
c) Visual indicator will change color indicating that the focus was succesful

Attachments

(5 files, 3 obsolete files)

No description provided.
User Story: (updated)
Prasad! Please check the user stories and update the current status.
Assignee: nobody → gjyothiprasad
This can be achieved by:
1. Getting the touch coordinates when camera preview is in camera mode.
2. Use SetFocusAreas (x,y) web API to set focus area and call the autofocus webAPI.
Blocks: 966764
Depends on: 971456
Attached file Pointer to Pull Request.html (obsolete) —
Hi David,

We have created a pull request for touch focus. Here is the scenario of touch Focus:
1. when user touches the viewfinder, get touch coords and emit an event
2. call a function which calculates the focus and metering areas.
3. Enable autofocus to camera to focus on that area.
Attachment #8379598 - Flags: review?(dflanagan)
This patch code doesn't have the test codes. This is just a work in progress patch
Comment on attachment 8379598 [details]
Pointer to Pull Request.html

Adding Diego and Wilson
Attachment #8379598 - Flags: feedback?(wilsonpage)
Attachment #8379598 - Flags: feedback?(dmarcos)
Comment on attachment 8379598 [details]
Pointer to Pull Request.html

See GitHub PR for feedback comments.
Attachment #8379598 - Flags: feedback-
Attached file Pointer to pull request (obsolete) —
Hello Marcos,

Updated the touch focus code as per the comments from Justin. Please check
Attachment #8381085 - Flags: review?(dmarcos)
Comment on attachment 8381085 [details] [review]
Pointer to pull request

Giving this "feedback+" given that my last few nits are addressed. I believe wilsonpage also had 2 minor comments for you to address as well. If you can update your PR, then David (djf) will review. Nice work!
Attachment #8381085 - Flags: review?(dmarcos)
Attachment #8381085 - Flags: review?(dflanagan)
Attachment #8381085 - Flags: feedback+
Comment on attachment 8381085 [details] [review]
Pointer to pull request

This pull request was not ready for review. In situations like that, setting the feedback? flag would be better.

I've left lots of feedback on github.

In addition, there's my usual complaint about changing file permissions and my reminder about squashing your commits.

And: why are there SVG files in the patch when the patch seems to be using an icon font.  Does the font reference the files somehow?

Also, the SVG files are not pure SVG but have Illustrator crap in them.  Who supplied these? If they are from Peter and Amy, we need to let them know that they're giving us bad files.
Attachment #8381085 - Flags: review?(dflanagan) → review-
I just noticed that the PR I reviewed was a PR against Diego's personal branch and not the camera-new-features branch. I suppose that is why the review request was originally set for Diego. Sorry to steal it. I hope the feedback helps!
Attachment #8382826 - Flags: review?(dmarcos)
Comment on attachment 8379598 [details]
Pointer to Pull Request.html

I don't really want to see any DOM inside lib/camera.js. I don't think we should be doing very much calculation here, instead camera.js should just be a simple interface to MozCamera hardware. Let's try to have the values finalized before passing them to camera.js, similar to setting flashMode etc.

camera.setFlashMode('on');

camera.focus(x, y);

Does this make sense?
Attachment #8379598 - Flags: feedback?(wilsonpage) → feedback-
please update with correct PR for david to review
Flags: needinfo?(dmarcos)
This hasn't landed on camera-dev yet. I will create a PR after merging
Whiteboard: [fxos:media] → [fxos:media] PR ETA 3/7
Target Milestone: --- → 1.4 S3 (14mar)
Attachment #8382826 - Flags: review?(dmarcos) → review?(dflanagan)
Flags: needinfo?(dmarcos)
Comment on attachment 8382826 [details]
Pointer to pull request on Diego's Branch

I'm assuming that you're asking me to review this commit: https://github.com/jyothiprasad/gaia/commit/cd680f30aa9114b02c662ea732d05926c3d7dbf7

I've left most of my comments on that commit on github.  I think there is still substantial work to be done on this patch before it can land.

Some comments here, though: 

- config/camera.js is unchanged except for the file mode. See if you can remove that from the commit

- the commit says it has 6029 added lines. That is way too many, and is caused by the .svg files, I think. It looks like the svg has been incorporated into the font so you don't actually need it.  Please remove those files that are bloating this commit.
Attachment #8382826 - Flags: review?(dflanagan) → review-
Hi David,

I have merged the PRs of Touch Focus and continuous auto focus into one PR and made changes as you commented.

Please review the patch.
Attachment #8379598 - Attachment is obsolete: true
Attachment #8381085 - Attachment is obsolete: true
Attachment #8382826 - Attachment is obsolete: true
Attachment #8379598 - Flags: review?(dflanagan)
Attachment #8379598 - Flags: feedback?(dmarcos)
Attachment #8389200 - Flags: review?(dflanagan)
Comment on attachment 8389200 [details] [review]
Pointer to pull request for both Touch and Continuous Focus

Prasad,

It is very nice to see a clean pull request like this. So much easier to work with!

I've put just a couple of quick comments here, but do not have time for a full review right now. 

I'd like to spend some time next week discussing the overall focus architecture for the app since all of the focus related feature are so closely intertwined.
Attachment #8389200 - Flags: review?(dflanagan) → review-
Blocks: 995911
Attached file Pull Request
Attachment #8406128 - Flags: review?(wilsonpage)
This is no longer part of 1.4, removing meta bug tracking dependency
No longer blocks: 966764
Whiteboard: [fxos:media] PR ETA 3/7 → [fxos:media] [m+]
Attachment #8406128 - Flags: review?(wilsonpage)
Attachment #8406128 - Flags: review?(dmarcos)
Attachment #8406128 - Flags: review?(dflanagan)
blocking-b2g: --- → 2.0?
Target Milestone: 1.4 S3 (14mar) → ---
Duplicate of this bug: 835375
Ashish is working on it. So I assigned it.
Assignee: gjyothiprasad → singhashish1887
Attached file Rebased pull request
Adding the target milestone to current sprint for tracking purpose
Target Milestone: --- → 2.0 S1 (9may)
blocking-b2g: 2.0? → backlog
This is still happening in the 2.0 timeframe (just not marked blocker since it is a feature)
Comment on attachment 8414309 [details] [review]
Rebased pull request

MVP. Still no unit tests. Touch two focus should be already functional on Nexus 4. I need validation of the approach.
Attachment #8414309 - Flags: feedback?(wilsonpage)
Comment on attachment 8414309 [details] [review]
Rebased pull request

I'm really pleased with this patch, I think it's heading in the right direction :)

I really like how you have kept the Focus logic separate and not bloated camera.js any further. Some small nits, but I think we're on the way to a clean, testable solution.
Attachment #8414309 - Flags: feedback?(wilsonpage) → feedback+
Ashish/Youngjun,

Diego is helping rework the patch here. Please co-ordinate with him for this feature. 

Thanks
Hema
Blocks: 966829
Attachment #8414309 - Flags: review?(jdarcangelo)
Attachment #8414309 - Flags: review?(wilsonpage)
Attachment #8414309 - Flags: feedback+ → feedback?(mhabicher)
The Flame doesn't appear to focus when tapped and then doesn't appear to focus when capture button is pressed either.
Nexus4:

- Focus ring goes green, but the camera doesn't appear to actually focus on the area I tap. Focus remains as it was before the tap.
- When I 'capture' the scene is never focused. There is no continuous-auto-focus or auto-focus on 'capture'. This results in blurry pictures.
Comment on attachment 8414309 [details] [review]
Rebased pull request

+ Code looks good
+ Lots of lovely unit-test coverage

- Touch focus doesn't appear to work (Flame & Nexus4).
- Normal focus on capture doesn't seem to be working (Flame & Nexus).
- Few tiny formatting nits highlighted to keep code in-line with existing style.
Attachment #8414309 - Flags: review?(wilsonpage) → review-
Flame doesn't support touch to focus. There's tons of quirks and strange behavior on camera for that device. We shouldn't be evaluating the patches on it. You can file separate bugs once this patch lands. 

Touch to focus doesn't select the point you touch. It selects a region. The part of the scene on focus doesn't have too match exactly where your finger is. I think we should show a square that represents more accurately what the camera does. This is the main reason I flagged mike for feedback. 

(In reply to Wilson Page [:wilsonpage] from comment #30)
> Comment on attachment 8414309 [details] [review]
> Rebased pull request
> 
> + Code looks good
> + Lots of lovely unit-test coverage
> 
> - Touch focus doesn't appear to work (Flame & Nexus4).
> - Normal focus on capture doesn't seem to be working (Flame & Nexus).
> - Few tiny formatting nits highlighted to keep code in-line with existing
> style.
Mike. I think touch to focus is still not good from the UX perspective. The current implementation is misleading. We show a circle exactly where the user touches but the camera rarely focuses on that point. My understating is that the camera has different squared regions where it can focus. With the current API we can query the number of regions but we don't know their size or how they're positioned on the screen. I think we should provide feedback to the user by indicating exactly the area of the scene where the camera will try to focus. What are your thoughts?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(mhabicher)
dmarcos: It wasn't that the region was off, it was just that I never saw the camera even attempt to focus; on capture or on tap-to-focus.
Flags: needinfo?(wilsonpage)
(In reply to Wilson Page [:wilsonpage] from comment #33)
> dmarcos: It wasn't that the region was off, it was just that I never saw the
> camera even attempt to focus; on capture or on tap-to-focus.

I haven't tried Flame with this patch. When I played with it my feeling was that it's not mature from the drivers/firmware/gonk point of view. Do you know if modes other than fixed focus is currently available on flame? Can you debug and query for mozCamera.capabilities.focusModes?
Flags: needinfo?(wilsonpage)
(In reply to Diego Marcos [:dmarcos] from comment #34)

> I haven't tried Flame with this patch. When I played with it my feeling was
> that it's not mature from the drivers/firmware/gonk point of view. Do you
> know if modes other than fixed focus is currently available on flame? Can
> you debug and query for mozCamera.capabilities.focusModes?

I'm talking about Nexus4 as well as Flame.

Flame focus modes (mozCamera.capabilities.focusModes):
[ "auto", "infinity", "macro", "continuous-video", "continuous-picture" ]
Flags: needinfo?(wilsonpage)
(In reply to Wilson Page [:wilsonpage] from comment #35)
> (In reply to Diego Marcos [:dmarcos] from comment #34)
> 
> > I haven't tried Flame with this patch. When I played with it my feeling was
> > that it's not mature from the drivers/firmware/gonk point of view. Do you
> > know if modes other than fixed focus is currently available on flame? Can
> > you debug and query for mozCamera.capabilities.focusModes?
> 
> I'm talking about Nexus4 as well as Flame.
> 
> Flame focus modes (mozCamera.capabilities.focusModes):
> [ "auto", "infinity", "macro", "continuous-video", "continuous-picture" ]

What gecko do you have?
Flags: needinfo?(wilsonpage)
What exactly doesn't for for you on Nexus 4?

- Touch to focus
- Continuous auto focus
- Focus on picture taken
One more thing. Just give up with flame for the moment. It's still not reliable to evaluate correct behavior. Nexus 4 is the standard to validate this.
It also did not work for me yesterday on both Flame and Nexus 4 using latest Gecko with this patch.
Mike. With focusMode === 'continuous-picture' the camera properly keeps focusing the scene as expected. If I set this.mozCamera.meteringAreas and this.mozCamera.focusAreas continuous auto focus stops working even if I call this.mozCamera.resumeContinuousFocus(). Is this expected behaviour or a bug?
As discussed in IRC, setting focusAreas stops continuous auto-focus. The correct solution seems to be to set the focus areas and then call a manual auto-focus cycle. See bug 1008283.
Flags: needinfo?(mhabicher)
Comment on attachment 8414309 [details] [review]
Rebased pull request

I changed how touch to focus behave when continuous auto focus is enabled simultaneously. When touching the screen continuous auto focus is disabled and touch to focus takes priority. If the user doesn't touch the screen after 10 seconds continuous autofocus is renabled.
Attachment #8414309 - Flags: review- → review?(wilsonpage)
Comment on attachment 8414309 [details] [review]
Rebased pull request

I'm ok with this patch conditionally -- read my comments on GitHub because I believe there was a minor typo in one of the variable names (doesn't adversely affect patch's ability to function properly though). Overall, very nice job of handling the complexities of all the focus modes.
Attachment #8414309 - Flags: review?(jdarcangelo) → review+
Comment on attachment 8414309 [details] [review]
Rebased pull request

+ The code changes look a lot better :)

Although I can't r+ this yet as I'm still having difficulty using the focus:

- Touch to focus performs a focus round trip then always turns the focus ring green on the area you tapped, even if the lens isn't currently focused there.

Expected: If the camera wasn't able to focus on the tapped area, the focus ring should turn red. Although in the test cases I ran, I don't think there was any reason for the camera not to be able to focus on the area I tapped. I would have expected the focus to adjust to the tapped area and focus ring to turn green.

- Camera seems to purely be relying on continuous-auto-focus. When I press the capture button the camera no longer attempts to focus, it just takes the picture instantly. This results in 90% of my shots being out-of-focus.

Expected: Camera should only take shot instantly if it is sure that the subject is in focus, else it should do another focus run.
Attachment #8414309 - Flags: review?(wilsonpage) → review-
Flags: needinfo?(wilsonpage)
(Manual tests run with latest Gecko on Nexus4)
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Comment on attachment 8414309 [details] [review]
Rebased pull request

Origin of camera coordinates is on the top-left corner of the screen but with the device on landscape mode. This fixes the issue when the user touches a region of the screen to focus. 

There are still issues but I need more understanding about how gecko works. I realized that the flash goes off (if it's in auto or on modes) when tapping to focus on the screen on low light conditions. Is the behavior intentional? 

Also touch to focus is not very reliable compared to android and ios devices? When trying to get my finger on focus sometimes It works and sometimes it doesn't and I don't understand why. I need more insight about the area that the camera driver is actually trying to focus on. Flagging
Attachment #8414309 - Flags: review?(wilsonpage)
Attachment #8414309 - Flags: review-
Attachment #8414309 - Flags: feedback?(mhabicher)
Attachment #8414309 - Flags: feedback?(aosmond)
Flags: in-moztrap?(mozillamarcia.knous)
Attachment #8414309 - Flags: feedback?(aosmond) → feedback+
The pull request is pretty mature and ready to ship. Touch to focus is still very unreliable and I've been holding the landing back because I wanted to understand better what was going on. Touch to focus still doesn't provide consistent results: It doesn't always focus on the area you tap, sometimes it focuses but the camera returns a focus failure, other times it just focuses as expected. I've verified that the coordinates passed to gecko follow the android conventions(search for coordinates on the link below) and properly match the region of the screen tapped:

http://developer.android.com/guide/topics/media/camera.html

I've installed a stock android (jelly bean) on my nexus 4 and touch to focus is also pretty inconsistent. We can bring more eyes on to this. On my side I'm starting to think that the problem comes from the driver, the firmware or just lack of quality of the camera hardware. If someone has an idea on how to improve this I'm looking forward to listening. If no improvements can be done on the gaia side I suggest landing this patch and file following bugs to improve gecko/driver/firmware...
From what I understand, the mozCamera API is simply exposing methods from the AOSP Camera API. It amazes me how incredibly inconsistent the behaviors are from device to device. Even with zoom, there are inconsistencies. I may try and see if I can find a good example of an Android Camera app implementation to study the Java source code and see if these variances are being compensated for at the application-level or if not, then maybe something is wrong in our mozCamera implementation.
Landed in master:

https://github.com/mozilla-b2g/gaia/commit/802281be1a1e013627de7fd88dc0cd93688bcc97
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Landed this to keep the ball rolling. We can polish and iterate over it in follow up bugs. UX, Looking forward for your criticism.
Flags: needinfo?(tshakespeare)
Attachment #8414309 - Flags: review?(wilsonpage)
Assignee: singhashish1887 → dmarcos
Adding Amy - Check out the patch on mozilla/master
Flags: needinfo?(amlee)
Attachment #8406128 - Flags: review?(dmarcos)
Attachment #8406128 - Flags: review?(dflanagan)
Hey guys - talked with Rob and Amy this morning and have some feedback. I put some in Bug 966829 which might be applicable here too. Additionally we noticed that there seemed to be some lag with the animation.
Flags: needinfo?(tshakespeare)
(In reply to Tiffanie Shakespeare from comment #53)
> Adding Amy - Check out the patch on mozilla/master
Flags: needinfo?(amlee)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.