Fixed memory leaks (cleaner version) #135
Conversation
… media locations like the gallery
… media locations like the gallery
…ce for CallbackBridge; upgraded version to 0.2.7
…google cameraview so comparisons are easier
| if (Build.VERSION.SDK_INT < 21) { | ||
| mImpl = new Camera1(mCallbacks, preview); | ||
| } else if (Build.VERSION.SDK_INT < 23) { | ||
| mImpl = new Camera2(mCallbacks, preview, context); | ||
| mImpl = new Camera2(mCallbacks, preview, context.getApplicationContext()); |
There was a problem hiding this comment.
Important for memory leak fix is to pass the application context here.
| } else { | ||
| mImpl = new Camera2Api23(mCallbacks, preview, context); | ||
| mImpl = new Camera2Api23(mCallbacks, preview, context.getApplicationContext()); |
There was a problem hiding this comment.
Important for memory leak fix is to pass the application context here.
| @@ -152,6 +153,9 @@ protected void onDetachedFromWindow() { | |||
| mDisplayOrientationDetector.disable(); | |||
| } | |||
| super.onDetachedFromWindow(); | |||
| if (mCallbacks != null && mCallbacks.mCallbacks != null) { | |||
| mCallbacks.mCallbacks.clear(); | |||
There was a problem hiding this comment.
Important for memory leak: clear out any callbacks to ensure no extra references are held
| @@ -407,13 +411,16 @@ public void takePicture() { | |||
| mImpl.takePicture(); | |||
| } | |||
|
|
|||
| private class CallbackBridge implements CameraViewImpl.Callback { | |||
| private static class CallbackBridge implements CameraViewImpl.Callback { | |||
There was a problem hiding this comment.
Important for memory leak: non-static inner classes implicitly hold references to their parent. This was causing a big leak. Making it static fixes that.
There was a problem hiding this comment.
Do you actually experience memory leaks due to this?
It seems to me CallbackBridge won't be an issue. It does hold a reference to CameraView, but it won't outlive the CameraView.
There was a problem hiding this comment.
My team uses LeakCanary to detect memory leaks. Unfortunately, the warnings were not false-positives. When I implemented these simple changes, the warnings went away.
It might be helpful to integrate LeakCanary into the demo app. It only takes a few minutes and run the app a few times to find out the leaks.
| private WeakReference<CameraView> cameraView; | ||
|
|
||
| CallbackBridge(CameraView cameraView) { | ||
| this.cameraView = new WeakReference<>(cameraView); |
There was a problem hiding this comment.
Important for memory leak: Since CallbackBridge is now static, we must pass the CameraView. Within CallbackBridge only hold a weak reference to it.
| @@ -9,6 +9,7 @@ buildscript { | |||
|
|
|||
| // NOTE: Do not place your application dependencies here; they belong | |||
| // in the individual module build.gradle files | |||
| classpath 'org.jfrog.buildinfo:build-info-extractor-gradle:4.1.0' | |||
There was a problem hiding this comment.
Can you suspend these changes in build scripts?
| @@ -407,13 +411,16 @@ public void takePicture() { | |||
| mImpl.takePicture(); | |||
| } | |||
|
|
|||
| private class CallbackBridge implements CameraViewImpl.Callback { | |||
| private static class CallbackBridge implements CameraViewImpl.Callback { | |||
There was a problem hiding this comment.
Do you actually experience memory leaks due to this?
It seems to me CallbackBridge won't be an issue. It does hold a reference to CameraView, but it won't outlive the CameraView.
As stated before, the biggest memory leak saver is converting the anonymous CallbackBridge class in CameraView to be static. And reference the CameraView within CallbackBridge to be a WeakReference.
Another is using the application context to get the camera manager, instead of the activity context. Memory leaks have been reported if you use the activity context to:
(CameraManager) context.getSystemService(Context.CAMERA_SERVICE);
So, we want the context to be the application context.
Other than that, I just cleaned up the code so there are as few differences as possible with google cameraview.