Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When not in continuousFocus mode, taking photo triggers focus #149

Closed
Raenar4k opened this issue Dec 8, 2017 · 7 comments
Closed

When not in continuousFocus mode, taking photo triggers focus #149

Raenar4k opened this issue Dec 8, 2017 · 7 comments
Milestone

Comments

@Raenar4k
Copy link
Contributor

Raenar4k commented Dec 8, 2017

What are you trying to achieve or the steps to reproduce?

When FA is initialized without continuous focus mode -> taking photo triggers focus.
I've implemented manual focus on tap, so user can try to get focus manually

fotoapparat.focus().whenAvailable {
            Timber.tag(TAG).d("focus state is $it")
        }

Even if user has taps and camera gets focused, taking a photo still triggers camera to focus again, which may lose focus that user wanted.

How did you initialize FA?

        return Fotoapparat
                .with(context)
                .into(renderer)
                .previewScaleType(ScaleType.CENTER_CROP)
                .photoSize(AspectRatioSelectors.wideRatio(SizeSelectors.biggestSize()))
                .lensPosition(LensPositionSelectors.lensPosition(LensPosition.BACK))
                .focusMode(Selectors.firstAvailable(
                        FocusModeSelectors.autoFocus(),
                        FocusModeSelectors.fixed()
                ))
                .flash(FlashSelectors.off())
                .logger(Loggers.loggers(Logger { Timber.tag(CameraImpl.TAG).d(it) }))
                .cameraErrorCallback({ errorSubject.onNext(it) })
                .build()

What was the result you received?

When FA is not in continuousFocus mode, taking photo triggers focus, even if fotoapparat.focus() method was called and camera is focused.

What did you expect?

If camera was already focused manually, by focus() method, then taking photo would not trigger another round of focus- like in continuous focus mode.

Context:

  • FA version: 1.5.0
  • Devices/APIs affected: Seems to be not device specific (Tested on Nexus 5x, Samsung A5, Xiaomi redmi 4, Samsung S5)
  • any other relevant information: All devices are API 21 and above

That decision to do away with continuous focus itself is tied to other issues -

  1. on some devices continuous focus stops working - camera stays unfocused -> which led me to adding manual focus
  2. manual focus does not work when in continuousFocus mode -> so i've removed that mode altogether and now i'm using autoFocus and fixed mode

Should i file separate issues for them too?

@jpribble
Copy link
Contributor

jpribble commented Dec 8, 2017

+1. It would be great if whether or not takePicture() triggered a refocus was configurable!

@Diolor
Copy link
Member

Diolor commented Dec 8, 2017

I see. Okay we can delete that logic from there and if a developer wants a focus before taking a picture could use FA. autoFocus().takePicture(). The reason we had this was camera2, which has a cumbersome focus routine. By deleting this I am unsure about the stability of v2 (which is not actively developed either way). Few people here prefer use v2 but we will not develop it further for now. @dmitry-zaitsev further input?

@Raenar4k
Copy link
Contributor Author

What about adding it as an option parameter to builder, like jpeg quality?
I think getting rid of shouldFocus method and making user call autoFocus().takePicture()can be problem to existing users of the api.

Also it can be done as overload for .takePicture() that has something like boolean shouldFocus in it or as separate method (cant think of a short name for that one), allowing to take picture without focusing.

I can help with PR, waiting for feedback :)

@Diolor
Copy link
Member

Diolor commented Dec 18, 2017

Ok, so plan for FA v2 is:

  • Removed shouldFocus in takePicture
  • If you want to just take picture with existing focus just call takePicture()
  • If you want a focus routine before taking a picture call autoFocus().takePicture()

I didn't like to add an overload shouldFocus in takePicture as in my head adds more mixed responsibility in the actions than the above structure.

Any objections, ideas, input, something I miss?

@Diolor Diolor added this to the 2.0.0 milestone Dec 18, 2017
@Diolor
Copy link
Member

Diolor commented Dec 18, 2017

And @Raenar4k regarding why continuous focus fails, here you go: #159

@Diolor Diolor closed this as completed Dec 31, 2017
@sefaaycicek
Copy link

Is there any improvement in beta5 version? We tried beta4 and some pictures are blurry.

@Diolor
Copy link
Member

Diolor commented Dec 31, 2017

Not on this topic. You can better open a new issue with more details about your configuration, etc if you think there is something wrong..

PS: Stable 2.0.0 is also available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants