Merge branch '720-camera-surface-illegal-state-exception' into 'master'

Don't crash if camera is reopened or surface is recreated

This branch fixes the crash is described in #720, which can be reproduced easily by scanning a QR code and failing to connect (for example, scan a screenshot of a QR code from a device that's no longer listening). When the camera view becomes visible again after trying to connect, its surfaceCreated() callback is called again with the same surface. An IllegalStateException added in !340 causes the crash.

Closes #720

See merge request !397

briar-android/src/org/briarproject/android/keyagreement/ShowQrCodeFragment.java

@@ -6,7 +6,6 @@ import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
 import android.graphics.Bitmap;
-import android.hardware.Camera;
 import android.os.AsyncTask;
 import android.os.Bundle;
 import android.support.annotation.Nullable;
@@ -58,7 +57,6 @@ import static android.view.View.INVISIBLE;
 import static android.view.View.VISIBLE;
 import static android.widget.Toast.LENGTH_LONG;
 import static java.util.logging.Level.INFO;
-import static java.util.logging.Level.WARNING;
 
 public class ShowQrCodeFragment extends BaseEventFragment
 		implements QrCodeDecoder.ResultCallback {
@@ -86,7 +84,6 @@ public class ShowQrCodeFragment extends BaseEventFragment
 	private ViewGroup mainProgressContainer;
 
 	private BluetoothStateReceiver receiver;
-	private QrCodeDecoder decoder;
 	private boolean gotRemotePayload, waitingForBluetooth;
 	private KeyAgreementTask task;
 
@@ -136,8 +133,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		super.onActivityCreated(savedInstanceState);
 
 		getActivity().setRequestedOrientation(SCREEN_ORIENTATION_NOSENSOR);
-
-		decoder = new QrCodeDecoder(this);
+		cameraView.setPreviewConsumer(new QrCodeDecoder(this));
 	}
 
 	@Override
@@ -163,8 +159,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		} else {
 			startListening();
 		}
-
-		openCamera();
+		cameraView.start();
 	}
 
 	@Override
@@ -172,7 +167,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		super.onStop();
 		stopListening();
 		if (receiver != null) getActivity().unregisterReceiver(receiver);
-		releaseCamera();
+		cameraView.stop();
 	}
 
 	@UiThread
@@ -200,45 +195,11 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		});
 	}
 
-	@SuppressWarnings("deprecation")
-	@UiThread
-	private void openCamera() {
-		LOG.info("Opening camera");
-		Camera camera;
-		try {
-			camera = Camera.open();
-		} catch (RuntimeException e) {
-			LOG.log(WARNING, e.toString(), e);
-			camera = null;
-		}
-		if (camera == null) {
-			LOG.log(WARNING, "Error opening camera");
-			Toast.makeText(getActivity(), R.string.could_not_open_camera,
-					LENGTH_LONG).show();
-			finish();
-			return;
-		}
-		cameraView.start(camera, decoder, 0);
-	}
-
-	@UiThread
-	private void releaseCamera() {
-		LOG.info("Releasing camera");
-		try {
-			cameraView.stop();
-		} catch (RuntimeException e) {
-			LOG.log(WARNING, "Error releasing camera", e);
-			// TODO better solution
-			finish();
-		}
-	}
-
 	@UiThread
 	private void reset() {
 		statusView.setVisibility(INVISIBLE);
 		cameraView.setVisibility(VISIBLE);
 		gotRemotePayload = false;
-		cameraView.startConsumer();
 		startListening();
 	}
 
@@ -380,7 +341,6 @@ public class ShowQrCodeFragment extends BaseEventFragment
 				LOG.info("Got result from decoder");
 				if (!gotRemotePayload) {
 					gotRemotePayload = true;
-					cameraView.stopConsumer();
 					qrCodeScanned(result.getText());
 				}
 			}

briar-android/src/org/briarproject/android/util/QrCodeDecoder.java

@@ -28,7 +28,7 @@ public class QrCodeDecoder implements PreviewConsumer, PreviewCallback {
 	private final Reader reader = new QRCodeReader();
 	private final ResultCallback callback;
 
-	private boolean stopped = false;
+	private Camera camera = null;
 
 	public QrCodeDecoder(ResultCallback callback) {
 		this.callback = callback;
@@ -36,37 +36,35 @@ public class QrCodeDecoder implements PreviewConsumer, PreviewCallback {
 
 	@Override
 	public void start(Camera camera) {
-		stopped = false;
-		askForPreviewFrame(camera);
+		this.camera = camera;
+		askForPreviewFrame();
 	}
 
 	@Override
 	public void stop() {
-		stopped = true;
+		camera = null;
 	}
 
 	@UiThread
-	private void askForPreviewFrame(Camera camera) {
-		if (!stopped) camera.setOneShotPreviewCallback(this);
+	private void askForPreviewFrame() {
+		if (camera != null) camera.setOneShotPreviewCallback(this);
 	}
 
 	@UiThread
 	@Override
 	public void onPreviewFrame(byte[] data, Camera camera) {
-		if (!stopped) {
+		if (camera == this.camera) {
 			Size size = camera.getParameters().getPreviewSize();
-			new DecoderTask(camera, data, size.width, size.height).execute();
+			new DecoderTask(data, size.width, size.height).execute();
 		}
 	}
 
 	private class DecoderTask extends AsyncTask<Void, Void, Void> {
 
-		private final Camera camera;
 		private final byte[] data;
 		private final int width, height;
 
-		DecoderTask(Camera camera, byte[] data, int width, int height) {
-			this.camera = camera;
+		DecoderTask(byte[] data, int width, int height) {
 			this.data = data;
 			this.width = width;
 			this.height = height;
@@ -84,7 +82,7 @@ public class QrCodeDecoder implements PreviewConsumer, PreviewCallback {
 			} catch (ReaderException e) {
 				return null; // No barcode found
 			} catch (RuntimeException e) {
-				return null; // Decoding failed due to bug in decoder
+				return null; // Preview data did not match width and height
 			} finally {
 				reader.reset();
 			}
@@ -97,7 +95,7 @@ public class QrCodeDecoder implements PreviewConsumer, PreviewCallback {
 
 		@Override
 		protected void onPostExecute(Void result) {
-			askForPreviewFrame(camera);
+			askForPreviewFrame();
 		}
 	}
 

briar-android/src/org/briarproject/android/view/CameraView.java

@@ -14,6 +14,8 @@ import android.view.SurfaceHolder;
 import android.view.SurfaceView;
 
 import org.briarproject.android.util.PreviewConsumer;
+import org.briarproject.api.nullsafety.MethodsNotNullByDefault;
+import org.briarproject.api.nullsafety.ParametersNotNullByDefault;
 
 import java.io.IOException;
 import java.util.List;
@@ -33,6 +35,8 @@ import static java.util.logging.Level.INFO;
 import static java.util.logging.Level.WARNING;
 
 @SuppressWarnings("deprecation")
+@MethodsNotNullByDefault
+@ParametersNotNullByDefault
 public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		AutoFocusCallback {
 
@@ -44,7 +48,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	private PreviewConsumer previewConsumer = null;
 	private Surface surface = null;
 	private int displayOrientation = 0, surfaceWidth = 0, surfaceHeight = 0;
-	private boolean autoFocus = false;
+	private boolean previewStarted = false, autoFocus = false;
 
 	public CameraView(Context context) {
 		super(context);
@@ -58,6 +62,12 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		super(context, attrs, defStyleAttr);
 	}
 
+	@UiThread
+	public void setPreviewConsumer(PreviewConsumer previewConsumer) {
+		LOG.info("Setting preview consumer");
+		this.previewConsumer = previewConsumer;
+	}
+
 	@Override
 	protected void onAttachedToWindow() {
 		super.onAttachedToWindow();
@@ -70,15 +80,18 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		super.onDetachedFromWindow();
 		setKeepScreenOn(false);
 		getHolder().removeCallback(this);
-		if (surface != null) surface.release();
 	}
 
 	@UiThread
-	public void start(Camera camera, PreviewConsumer previewConsumer,
-			int rotationDegrees) {
-		this.camera = camera;
-		this.previewConsumer = previewConsumer;
-		setDisplayOrientation(rotationDegrees);
+	public void start() {
+		try {
+			LOG.info("Opening camera");
+			camera = Camera.open();
+		} catch (RuntimeException e) {
+			LOG.log(WARNING, "Error opening camera", e);
+			return;
+		}
+		setDisplayOrientation(0);
 		// Use barcode scene mode if it's available
 		Parameters params = camera.getParameters();
 		params = setSceneMode(camera, params);
@@ -96,14 +109,17 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		enableAutoFocus(params.getFocusMode());
 		// Log the parameters that are being used (maybe not what we asked for)
 		logCameraParameters();
-		if (surface != null) startPreview(getHolder());
+		// Start the preview when the camera and the surface are both ready
+		if (surface != null && !previewStarted) startPreview(getHolder());
 	}
 
 	@UiThread
 	public void stop() {
+		if (camera == null) return;
 		stopPreview();
 		try {
-			if (camera != null) camera.release();
+			LOG.info("Releasing camera");
+			camera.release();
 		} catch (RuntimeException e) {
 			LOG.log(WARNING, "Error releasing camera", e);
 		}
@@ -112,9 +128,11 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 
 	@UiThread
 	private void startPreview(SurfaceHolder holder) {
+		LOG.info("Starting preview");
 		try {
 			camera.setPreviewDisplay(holder);
 			camera.startPreview();
+			previewStarted = true;
 			startConsumer();
 		} catch (IOException | RuntimeException e) {
 			LOG.log(WARNING, "Error starting camera preview", e);
@@ -123,24 +141,26 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 
 	@UiThread
 	private void stopPreview() {
+		LOG.info("Stopping preview");
 		try {
 			stopConsumer();
-			if (camera != null) camera.stopPreview();
+			camera.stopPreview();
 		} catch (RuntimeException e) {
 			LOG.log(WARNING, "Error stopping camera preview", e);
 		}
+		previewStarted = false;
 	}
 
 	@UiThread
-	public void startConsumer() {
+	private void startConsumer() {
 		if (autoFocus) camera.autoFocus(this);
 		previewConsumer.start(camera);
 	}
 
 	@UiThread
-	public void stopConsumer() {
-		if (previewConsumer != null) previewConsumer.stop();
+	private void stopConsumer() {
 		if (autoFocus) camera.cancelAutoFocus();
+		previewConsumer.stop();
 	}
 
 	@UiThread
@@ -295,9 +315,13 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	@UiThread
 	private void surfaceCreatedUi(SurfaceHolder holder) {
 		LOG.info("Surface created");
-		if (surface != null) throw new IllegalStateException();
+		if (surface != null && surface != holder.getSurface()) {
+			LOG.info("Releasing old surface");
+			surface.release();
+		}
 		surface = holder.getSurface();
-		if (camera != null) startPreview(holder);
+		// Start the preview when the camera and the surface are both ready
+		if (camera != null && !previewStarted) startPreview(holder);
 	}
 
 	@Override
@@ -314,9 +338,10 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	@UiThread
 	private void surfaceChangedUi(SurfaceHolder holder, int w, int h) {
 		if (LOG.isLoggable(INFO)) LOG.info("Surface changed: " + w + "x" + h);
-		// Release the previous surface if necessary
-		if (surface != null && surface != holder.getSurface())
+		if (surface != null && surface != holder.getSurface()) {
+			LOG.info("Releasing old surface");
 			surface.release();
+		}
 		surface = holder.getSurface();
 		surfaceWidth = w;
 		surfaceHeight = h;
@@ -346,8 +371,12 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	@UiThread
 	private void surfaceDestroyedUi(SurfaceHolder holder) {
 		LOG.info("Surface destroyed");
-		if (holder.getSurface() != surface) throw new IllegalStateException();
-		if (surface != null) surface.release();
+		if (surface != null && surface != holder.getSurface()) {
+			LOG.info("Releasing old surface");
+			surface.release();
+		}
+		surface = null;
+		holder.getSurface().release();
 	}
 
 	@Override