No way to handle ErrorCallback from Camera

[arekolek]

At the moment, in io.fotoapparat.hardware.v1.Camera1 there is an error callback setup like this:

camera.setErrorCallback(new Camera.ErrorCallback() {
    @Override
    public void onError(int error, Camera camera) {
        if (lastStacktrace != null) {
            lastStacktrace.printStackTrace();
        }

        throw new IllegalStateException("Camera error code: " + error);
    }
});

This results in crashes like this one on Xiaomi Redmi Note 3 (Android 6.0.1):

Fatal Exception: java.lang.IllegalStateException: Camera error code: 100
       at io.fotoapparat.hardware.v1.Camera1$1.onError(SourceFile:81)
       at android.hardware.Camera$EventHandler.handleMessage(Camera.java:1172)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:148)
       at android.app.ActivityThread.main(ActivityThread.java:5441)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:738)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:628)

Or this one on HUAWEI P8 (Android 5.0.1):

Fatal Exception: java.lang.IllegalStateException: Camera error code: 3
       at io.fotoapparat.hardware.v1.Camera1$1.onError(SourceFile:73)
       at android.hardware.Camera$EventHandler.handleMessage(Camera.java:1198)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:135)
       at android.app.ActivityThread.main(ActivityThread.java:5538)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:958)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:753)

Logs got obfuscated:

 0 | 2017-07-08T12:53:45.133Z | D/Fotoapparat a
 1 | 2017-07-08T12:53:45.351Z | D/Fotoapparat j
 2 | 2017-07-08T12:53:45.359Z | D/Fotoapparat a
 3 | 2017-07-08T12:53:45.365Z | D/Fotoapparat a
 4 | 2017-07-08T12:53:45.367Z | D/Fotoapparat h
 5 | 2017-07-08T12:53:45.369Z | D/Fotoapparat Renderer parameters are: RendererParameters{previewSize=Size{width=1280, height=960}, frameRotation=90}
 6 | 2017-07-08T12:53:45.369Z | D/Fotoapparat a
 7 | 2017-07-08T12:53:45.370Z | D/Fotoapparat b
 8 | 2017-07-08T12:54:46.284Z | D/Fotoapparat d
 9 | 2017-07-08T12:54:49.769Z | D/Fotoapparat f
10 | 2017-07-08T12:54:50.270Z | D/Fotoapparat b

Because of how logging is implemented in Fotoapparat:

private void recordMethod() {
    lastStacktrace = new Exception();

    logger.log(
            lastStacktrace.getStackTrace()[1].getMethodName()
    );
}

I don't know what error codes 3 and 100 mean, because Android doesn't have them documented, but a library like Fotoapparat should leave developers with choices other than crashing the app when errors it can't handle happen.

[dmitry-zaitsev]

That is a valid point. However, there are some complications:

• Fotoapparat performs all of the operations asynchronously. Which means that such error callback would be disconnected from the actual operation which failed.
• If we do not know how to fix the problem (especially given those cryptic error codes), we do not expect our clients to do so.

With that said, I would like to postpone creation of such error handler. Instead, I will remove IllegalStateException from our current implementation so that it won't crash the app.

[arekolek]

@dmitry-zaitsev you also make valid points, however:

• By "handling the error in the client" I don't mean trying to make the Camera to work even though Fotoapparat couldn't. I mean reacting to the error in the app in a reasonable way, like maybe logging the error so the developers at least are aware that it happened at all, and also indicating to the user that he's not going to be able to take that photo instead of just hanging the UI like there's still some hope.
• It's good that those operations are done asynchronously. But I don't see how the disconnection is a problem here. If you look at other asynchronous APIs, like starting activity for result, requesting permissions, calling network operations (say with Retrofit for example), or the Camera API itself (see OP), there's always an error callback. Without it, I don't think it's possible to write an app that behaves the right way when an error happens, resulting in unhappy users.

I will remove IllegalStateException from our current implementation

I guess the question here is how that will affect existing clients. What will happen instead of a crash? Will the UI just freeze? Will the user know what's going on? Will the developers be aware that there is a problem?

[dmitry-zaitsev]

Indicating to the user that he's not going to be able to take that photo instead of just hanging the UI like there's still some hope.

Good point. I was thinking about adding a general error callback, which would indicate that camera could not be started, with possibly some (but not all) details.

I don't see how the disconnection is a problem here.

The problem is rather not in implementation on the library side. The problem is for the clients to try to react to those problems in a meaningful way. For example, handling failed autoFocus is possible (with options being: retry, ignore, notify user), but that direction would quickly turn client's code into a huge pile of error handling code.

So, instead, as said above, I suggest we do the following:

• Try to not crash the app in the first place. That is good enough for some not critical operations, such as autoFocus or (to some extent) takePicture, which can be safely retried. But, as you said, it is not enough in case if failure can't be ignored (for example, when camera failed to open at all).
• Add a general error callback, which would basically just tell that "Sorry, the camera does not seem to work, here is some diagnostics info, but you probably won't fix it anyway".

[dmitry-zaitsev]

Done. The callback will be included into the next release (expect it until the end of this week) and will look like that:

Fotoapparat.with(context)
    .cameraErrorCallback(cameraException -> {
        // Parameter is CameraException. This method is always called on the main thread.
    })

[devangichhatbar]

In Fotoapparat.with(context) .cameraErrorCallback(cameraException -> { // Parameter is CameraException. This method is always called on the main thread. })

Can not able to reopen camera. If I reOpen camera then the screen is just blank.