966830 - [User story] Add touch focus to the camera

Status: RESOLVED FIXED

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

Comment 1

[Youngjun Kim]

Prasad! Please check the user stories and update the current status.

Comment 2

[Jyothiprasad]

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.

Comment 3

[Jyothiprasad]

Attachment: Pointer to Pull Request.html

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.

Comment 4

[Jyothiprasad]

This patch code doesn't have the test codes. This is just a work in progress patch

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8379598 [details]
Pointer to Pull Request.html

Adding Diego and Wilson

Comment 6

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8379598 [details]
Pointer to Pull Request.html

See GitHub PR for feedback comments.

Comment 7

[Jyothiprasad]

Attachment: Pointer to pull request

Hello Marcos,

Updated the touch focus code as per the comments from Justin. Please check

Comment 8

[Justin D'Arcangelo [:justindarc]]

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!

Comment 9

[David Flanagan [:djf]]

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.

Comment 10

[David Flanagan [:djf]]

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!

Comment 11

[Jyothiprasad]

Attachment: Pointer to pull request on Diego's Branch

Comment 12

[Wilson Page [:wilsonpage]]

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?

Comment 13

[Hema Koka [:hema]]

please update with correct PR for david to review

Comment 14

[Diego Marcos [:dmarcos]]

This hasn't landed on camera-dev yet. I will create a PR after merging

Comment 15

[David Flanagan [:djf]]

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.

Comment 16

[Jyothiprasad]

Attachment: Pointer to pull request for both Touch and Continuous Focus

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.

Comment 17

[David Flanagan [:djf]]

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.

Comment 18

[singhashish1887]

Attachment: Pull Request

Comment 19

[Wayne Chang [:wchang]]

This is no longer part of 1.4, removing meta bug tracking dependency

Comment 21

[Youngjun Kim]

Ashish is working on it. So I assigned it.

Comment 22

[Diego Marcos [:dmarcos]]

Attachment: Rebased pull request

Comment 23

[Hema Koka [:hema]]

Adding the target milestone to current sprint for tracking purpose

Comment 24

[Hema Koka [:hema]]

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

Comment 25

[Diego Marcos [:dmarcos]]

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.

Comment 26

[Wilson Page [: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.

Comment 27

[Hema Koka [:hema]]

Ashish/Youngjun,

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

Thanks
Hema

Comment 28

[Wilson Page [:wilsonpage]]

The Flame doesn't appear to focus when tapped and then doesn't appear to focus when capture button is pressed either.

Comment 29

[Wilson Page [:wilsonpage]]

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 30

[Wilson Page [:wilsonpage]]

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.

Comment 31

[Diego Marcos [:dmarcos]]

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.

Comment 32

[Diego Marcos [:dmarcos]]

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?

Comment 33

[Wilson Page [:wilsonpage]]

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.

Comment 34

[Diego Marcos [:dmarcos]]

(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?

Comment 35

[Wilson Page [: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" ]

Comment 36

[Diego Marcos [:dmarcos]]

(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?

Comment 37

[Diego Marcos [:dmarcos]]

What exactly doesn't for for you on Nexus 4?

- Touch to focus
- Continuous auto focus
- Focus on picture taken

Comment 38

[Diego Marcos [:dmarcos]]

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.

Comment 39

[Justin D'Arcangelo [:justindarc]]

It also did not work for me yesterday on both Flame and Nexus 4 using latest Gecko with this patch.

Comment 40

[Diego Marcos [:dmarcos]]

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?

Comment 41

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

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.

Comment 42

[Diego Marcos [:dmarcos]]

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.

Comment 43

[Justin D'Arcangelo [:justindarc]]

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.

Comment 44

[Wilson Page [:wilsonpage]]

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.

Comment 45

[Wilson Page [:wilsonpage]]

Attachment: intent to focus left

Comment 46

[Wilson Page [:wilsonpage]]

Attachment: intent to focus right

Comment 47

[Wilson Page [:wilsonpage]]

(Manual tests run with latest Gecko on Nexus4)

Comment 48

[Diego Marcos [:dmarcos]]

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

Comment 49

[Diego Marcos [:dmarcos]]

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...

Comment 50

[Justin D'Arcangelo [:justindarc]]

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.

Comment 51

[Diego Marcos [:dmarcos]]

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/802281be1a1e013627de7fd88dc0cd93688bcc97

Comment 52

[Diego Marcos [:dmarcos]]

Landed this to keep the ball rolling. We can polish and iterate over it in follow up bugs. UX, Looking forward for your criticism.

Comment 53

[Tiffanie Shakespeare [:tif]]

Adding Amy - Check out the patch on mozilla/master

Comment 54

[Tiffanie Shakespeare [:tif]]

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.

Comment 55

[Amy Lee [:amylee] UX]

(In reply to Tiffanie Shakespeare from comment #53)
> Adding Amy - Check out the patch on mozilla/master