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

Manual focus #120

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@friendoye
Contributor

friendoye commented Oct 15, 2017

Part of #60. Add manual focus feature for Camera1 API.

@friendoye

This comment has been minimized.

Show comment
Hide comment
@friendoye

friendoye Oct 15, 2017

Contributor

As for now, this PR contains mostly my ideas about API for manual focus. If @dmitry-zaitsev @Diolor will be fine with my API, I'll add documentation and tests.

Contributor

friendoye commented Oct 15, 2017

As for now, this PR contains mostly my ideas about API for manual focus. If @dmitry-zaitsev @Diolor will be fine with my API, I'll add documentation and tests.

@dmitry-zaitsev dmitry-zaitsev self-requested a review Oct 18, 2017

@dmitry-zaitsev dmitry-zaitsev changed the base branch from develop to master Oct 18, 2017

@@ -250,6 +269,11 @@ public Fotoapparat autoFocus() {
return autoFocusRoutine.autoFocus();
}
public void setTapToFocusEnabled(boolean isEnabled) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

I believe it is not a responsibility of Fotoapparat to enabled or disable tap-to-focus feature. Instead, I would imagine that the app which uses the library would just not trigger manual focus if it does not want to.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

I believe it is not a responsibility of Fotoapparat to enabled or disable tap-to-focus feature. Instead, I would imagine that the app which uses the library would just not trigger manual focus if it does not want to.

This comment has been minimized.

@friendoye

friendoye Oct 18, 2017

Contributor

But what if we would like to enable/disable tap-to-focus during our interaction with CameraView? Switching Fotoapparat instances seems to be overhead for this.

@friendoye

friendoye Oct 18, 2017

Contributor

But what if we would like to enable/disable tap-to-focus during our interaction with CameraView? Switching Fotoapparat instances seems to be overhead for this.

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Maybe I did not yet understand the logic fully. I will finish review and then see if this comment still applies.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Maybe I did not yet understand the logic fully. I will finish review and then see if this comment still applies.

@dmitry-zaitsev

I am still reviewing it, but not to keep you waiting here is the first part of the review. Thanks for helping!

return getNormalizedCameraFocusRect(cameraSpaceCoords[0], cameraSpaceCoords[1]);
}
private Rect getNormalizedCameraFocusRect(float cameraSpaceX,

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

It is not really normalized rectangle though. How about naming it just toFocusRectangle instead?

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

It is not really normalized rectangle though. How about naming it just toFocusRectangle instead?

);
}
private int alignCameraSpaceCoord(float cameraSpaceCoord) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

It is called clamp operation. So let's name the method clamp respectively.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

It is called clamp operation. So let's name the method clamp respectively.

cameraToPreviewMatrix.setScale(1, cameraIsMirrored ? -1 : 1);
cameraToPreviewMatrix.postRotate(displayOrientationDegrees);
cameraToPreviewMatrix.postScale(cameraRect.width() / 2000f, cameraRect.height() / 2000f);

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

I think it worth extracting 2000f to a constant with a JavaDoc. I know where the value comes from but the next reader might not know that.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

I think it worth extracting 2000f to a constant with a JavaDoc. I know where the value comes from but the next reader might not know that.

} catch (InterruptedException e) {
// Do nothing
}
return FocusResult.successNoMeasurement();
}
@Override
public void cancelAutoFocus() {
camera.cancelAutoFocus();

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Since camera in Android is really glitchy we need to do the following:

  1. Log that method was called (similarly to how it is done in other public methods)
  2. Wrap method call into try/catch in order to not crash the app. Quite often camera throws a RuntimeException without any reasonable error message, so we have to swallow it.
@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Since camera in Android is really glitchy we need to do the following:

  1. Log that method was called (similarly to how it is done in other public methods)
  2. Wrap method call into try/catch in order to not crash the app. Quite often camera throws a RuntimeException without any reasonable error message, so we have to swallow it.
@@ -433,4 +443,26 @@ private void recordMethod() {
);
}
@Override
public void setFocusArea(Rect cameraViewRect, float x, float y) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

We should not use any Android classes in CameraDevice interface, otherwise, unit testing will not be possible. I suggest we create our own Rectangle class and add an utility converter function which maps it to Android's Rect.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

We should not use any Android classes in CameraDevice interface, otherwise, unit testing will not be possible. I suggest we create our own Rectangle class and add an utility converter function which maps it to Android's Rect.

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

After reading through this method I think it should be moved out of Camera1 into routine class as much as possible. Why - because this way it could be tested. In the end, it looks like we don't really need setFocusArea in CameraDevice itself as we can just update camera parameters from routine class.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

After reading through this method I think it should be moved out of Camera1 into routine class as much as possible. Why - because this way it could be tested. In the end, it looks like we don't really need setFocusArea in CameraDevice itself as we can just update camera parameters from routine class.

List<Camera.Area> focusingAreas = new ArrayList<>();
focusingAreas.add(new Camera.Area(previewSpaceRect, FOCUS_METERING_AREA_WEIGHT_DEFAULT));
FocusUpdater focusUpdater = new FocusUpdater(

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Generally it is better to not create classes which do some logic inside methods. Instead, it should be passed as a constructor parameter. In this case I would say that this class should be a parameter of routine which uses it.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Generally it is better to not create classes which do some logic inside methods. Instead, it should be passed as a constructor parameter. In this case I would say that this class should be a parameter of routine which uses it.

getCapabilities()
);
if (focusUpdater.setManualFocus(focusingAreas)) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Setters should do one thing which means it should not return boolean. I would instead write it like that:

if (focusUpdater.shouldUpdateManualFocus(getCapabilities())) {
    camera.setParameters(focusUpdater.buildParameters(focusingAreas).asCameraParameters());
}
@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Setters should do one thing which means it should not return boolean. I would instead write it like that:

if (focusUpdater.shouldUpdateManualFocus(getCapabilities())) {
    camera.setParameters(focusUpdater.buildParameters(focusingAreas).asCameraParameters());
}
@@ -142,6 +154,16 @@ public void setPreviewFpsRange(int min, int max) {
cameraParameters.setPreviewFpsRange(min, max);
}
// TODO: Wrap Camera.Area to run from explicit dependencies

This comment has been minimized.

@dmitry-zaitsev
@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

👍

@@ -128,6 +132,14 @@ public void setPreviewSize(int width, int height) {
cameraParameters.setPreviewSize(width, height);
}
public int getMaxNumFocusAreas() {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

That and getMaxMeteringAreas should be a part of Capabilities

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

That and getMaxMeteringAreas should be a part of Capabilities

return cameraParameters.getMaxNumFocusAreas();
}
public int getMaxNumMeteringAreas() {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Avoid shortcuts in the names. I would either call it getMaxNumberOfMeteringAreas or just getMaxMeteringAreas. Both are unambiguous, so I would go for a shorter one.

@dmitry-zaitsev

dmitry-zaitsev Oct 18, 2017

Member

Avoid shortcuts in the names. I would either call it getMaxNumberOfMeteringAreas or just getMaxMeteringAreas. Both are unambiguous, so I would go for a shorter one.

@dmitry-zaitsev

Done with the 2nd part of the review. In general looks good, we just need to move things around to make it more SOLID-ish.

import static io.fotoapparat.hardware.v1.capabilities.FocusCapability.toCode;
import static io.fotoapparat.hardware.v1.capabilities.FocusCapability.toFocusMode;
public class FocusUpdater {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Let's update the API as proposed in comments for Camera1

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Let's update the API as proposed in comments for Camera1

manualFocusResult.whenDone(new PendingResult.Callback<FocusResult>() {
@Override
public void onResult(FocusResult result) {
cameraDevice.cancelAutoFocus();

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Do we need this line? Since all camera operations are scheduled on single thread executor they will be executed sequentially. Which means that it should be safe to just submit another ManualFocusTask right away.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Do we need this line? Since all camera operations are scheduled on single thread executor they will be executed sequentially. Which means that it should be safe to just submit another ManualFocusTask right away.

}
@Override
public void onManualFocus(final Rect cameraViewRect, float focusX, float focusY) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

This callback should not be here. Routine is only responsible for performing the "business logic" on camera (exactly what your manualFocus method does). Reacting on taps is a responsibility of the view.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

This callback should not be here. Routine is only responsible for performing the "business logic" on camera (exactly what your manualFocus method does). Reacting on taps is a responsibility of the view.

cameraDevice.setFocusArea(cameraViewRect, focusX, focusY);
boolean isSucceeded = cameraDevice.autoFocus().succeeded;
if (isSucceeded) {
cameraDevice.cancelAutoFocus();

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Do we need to cancel it?

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Do we need to cancel it?

This comment has been minimized.

@friendoye

friendoye Oct 29, 2017

Contributor

Yeah, because it's still autofocusing without this.

@friendoye

friendoye Oct 29, 2017

Contributor

Yeah, because it's still autofocusing without this.

addView(rendererView);
focusMarkerLayout = new FocusMarkerLayout(getContext());

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

That's a correct approach, but we can make it even cooler. Instead of adding view here, let's allow user to decide whether he wants to use this functionality or not by adding it to XML layout. So it would look something like that:

<CameraView>
    <FocusMarkerLayout />
</CameraView>
@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

That's a correct approach, but we can make it even cooler. Instead of adding view here, let's allow user to decide whether he wants to use this functionality or not by adding it to XML layout. So it would look something like that:

<CameraView>
    <FocusMarkerLayout />
</CameraView>
public boolean onTouch(View v, MotionEvent motionEvent) {
if (motionEvent.getAction() == MotionEvent.ACTION_DOWN && isTapToFocusEnabled) {
focusMarkerLayout.showAt(motionEvent.getX(), motionEvent.getY());
rendererView.dispatchTouchEvent(motionEvent);

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

There is no need for this method if you will always return false from this callback.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

There is no need for this method if you will always return false from this callback.

@@ -66,4 +84,15 @@ public void attachCamera(CameraDevice camera) {
rendererView.attachCamera(camera);
}
@Override
public void enableTapToFocus(FocusCallback callback) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Those methods belong to FocusMarkerLayout

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

Those methods belong to FocusMarkerLayout

public class FocusMarkerLayout extends FrameLayout {
private View mFocusMarkerView;

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

We don't use mField notation

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

We don't use mField notation

@Override
public void enableTapToFocus(FocusCallback callback) {
isTapToFocusEnabled = true;
rendererView.enableTapToFocus(callback);

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

rendererView should have nothing to do with focus functionality.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

rendererView should have nothing to do with focus functionality.

viewRect.set(0, 0, w, h);
}
public void enableTapToFocus(final FocusCallback callback) {

This comment has been minimized.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

As already said, this belongs to FocusMarkerLayout.

@dmitry-zaitsev

dmitry-zaitsev Oct 29, 2017

Member

As already said, this belongs to FocusMarkerLayout.

@abodehBabelli

This comment has been minimized.

Show comment
Hide comment
@abodehBabelli

abodehBabelli Nov 6, 2017

I tested the sample app on Samsung S7 and Asus Zenfone 2, it seems that after manual focus finish it is not returning back to continuous focus, its still wait for my tap to focus.

abodehBabelli commented Nov 6, 2017

I tested the sample app on Samsung S7 and Asus Zenfone 2, it seems that after manual focus finish it is not returning back to continuous focus, its still wait for my tap to focus.

@friendoye

This comment has been minimized.

Show comment
Hide comment
@friendoye

friendoye Nov 6, 2017

Contributor

It is still WIP. I recently used source code from my branch and Fotoapparat was crashing every time I tried to manual focus after recreation of Fotoapparat instance. So yeah :(
Let's get back to your comment. You said that after manual focus Camera1 is not returning back to continuous focus. You mean that focus becoming fixed or that you expect focus return to continuous always after it?

Contributor

friendoye commented Nov 6, 2017

It is still WIP. I recently used source code from my branch and Fotoapparat was crashing every time I tried to manual focus after recreation of Fotoapparat instance. So yeah :(
Let's get back to your comment. You said that after manual focus Camera1 is not returning back to continuous focus. You mean that focus becoming fixed or that you expect focus return to continuous always after it?

@abodehBabelli

This comment has been minimized.

Show comment
Hide comment
@abodehBabelli

abodehBabelli Nov 6, 2017

I was expecting that focus should return back to Fotoapparat builder value( in my case is continuous focus)

abodehBabelli commented Nov 6, 2017

I was expecting that focus should return back to Fotoapparat builder value( in my case is continuous focus)

@friendoye

This comment has been minimized.

Show comment
Hide comment
@friendoye

friendoye Nov 6, 2017

Contributor

If i'm not wrong, changing focus mode to something different than continuous focus or autofocus, will cause losing result of manual focus. Anyway, focus mode should stay continuous after all.

Contributor

friendoye commented Nov 6, 2017

If i'm not wrong, changing focus mode to something different than continuous focus or autofocus, will cause losing result of manual focus. Anyway, focus mode should stay continuous after all.

@dmitry-zaitsev

This comment has been minimized.

Show comment
Hide comment
@dmitry-zaitsev

dmitry-zaitsev Nov 6, 2017

Member

We actually fixed exactly this issue in the very last release. Is it still reproducible?

Member

dmitry-zaitsev commented Nov 6, 2017

We actually fixed exactly this issue in the very last release. Is it still reproducible?

@friendoye

This comment has been minimized.

Show comment
Hide comment
@friendoye

friendoye Nov 6, 2017

Contributor

I think we need put some checks for current focus mode during manual focus task. These all "focus status" checks are annoying 😞

Contributor

friendoye commented Nov 6, 2017

I think we need put some checks for current focus mode during manual focus task. These all "focus status" checks are annoying 😞

/**
* @return whether or not camera should autoFocus after this
*/
public boolean setManualFocus(List<Camera.Area> focusingAreas) {

This comment has been minimized.

@jpribble

jpribble Dec 5, 2017

Contributor

Can you use a different term than “manual” focus for this feature? It appears to be using auto focus and setting custom focus areas. I would recommend reserving the term “manual” focus for a feature that manually sets the lens focus distance.

@jpribble

jpribble Dec 5, 2017

Contributor

Can you use a different term than “manual” focus for this feature? It appears to be using auto focus and setting custom focus areas. I would recommend reserving the term “manual” focus for a feature that manually sets the lens focus distance.

@Diolor

This comment has been minimized.

Show comment
Hide comment
@Diolor

Diolor Jan 7, 2018

Member

Will close for sake of #171

Member

Diolor commented Jan 7, 2018

Will close for sake of #171

@Diolor Diolor closed this Jan 7, 2018

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