From b949b61103a14645ed6fda09dc626fc273bf49e3 Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Thu, 6 Feb 2020 11:08:41 -0800 Subject: [PATCH] Camera NDK: New lifecycle for ACameraMetadata_fromCameraMetadata Instead of requiring the user to call NewGlobalRef/DeleteGlobalRef for keeping the java object alive when creating an NDK view into it, reference count the real native data instead, so that there's no need to keep track of the Java object lifecycle. - Switch CameraMetadataNative to use std::shared_ptr internally - Switch ACameraMetadata to use std::shared_ptr internally - Always copy data in the ACameraMetadata copy constructor Test: New CTS tests pass, fail without this CL Bug: 148972471 Change-Id: I40a0ccb8b40c7a89ee7d3a6f7bac7c9c88d709f1 --- camera/ndk/NdkCameraMetadata.cpp | 14 +++++++------- camera/ndk/impl/ACameraMetadata.cpp | 19 +++++-------------- camera/ndk/impl/ACameraMetadata.h | 14 ++++++-------- camera/ndk/include/camera/NdkCameraMetadata.h | 9 ++++----- 4 files changed, 22 insertions(+), 34 deletions(-) diff --git a/camera/ndk/NdkCameraMetadata.cpp b/camera/ndk/NdkCameraMetadata.cpp index 1fec3e13d9..7d3a53efc0 100644 --- a/camera/ndk/NdkCameraMetadata.cpp +++ b/camera/ndk/NdkCameraMetadata.cpp @@ -81,15 +81,16 @@ bool InitJni(JNIEnv* env) { } // Given cameraMetadata, an instance of android.hardware.camera2.CameraMetadata, invokes -// cameraMetadata.getNativeMetadataPtr() and returns it as a CameraMetadata*. -CameraMetadata* CameraMetadata_getNativeMetadataPtr(JNIEnv* env, jobject cameraMetadata) { +// cameraMetadata.getNativeMetadataPtr() and returns it as a std::shared_ptr*. +std::shared_ptr* CameraMetadata_getNativeMetadataPtr(JNIEnv* env, + jobject cameraMetadata) { if (cameraMetadata == nullptr) { ALOGE("%s: Invalid Java CameraMetadata object.", __FUNCTION__); return nullptr; } jlong ret = env->CallLongMethod(cameraMetadata, android_hardware_camera2_CameraMetadata_getNativeMetadataPtr); - return reinterpret_cast(ret); + return reinterpret_cast* >(ret); } } // namespace @@ -179,10 +180,9 @@ ACameraMetadata* ACameraMetadata_fromCameraMetadata(JNIEnv* env, jobject cameraM return nullptr; } - CameraMetadata* src = CameraMetadata_getNativeMetadataPtr(env, - cameraMetadata); - ACameraMetadata* output = new ACameraMetadata(src, type); + auto sharedData = CameraMetadata_getNativeMetadataPtr(env, cameraMetadata); + ACameraMetadata* output = new ACameraMetadata(*sharedData, type); output->incStrong(/*id=*/(void*) ACameraMetadata_fromCameraMetadata); return output; } -#endif /* __ANDROID_VNDK__ */ \ No newline at end of file +#endif /* __ANDROID_VNDK__ */ diff --git a/camera/ndk/impl/ACameraMetadata.cpp b/camera/ndk/impl/ACameraMetadata.cpp index 18c5d3c3f3..1bbb9c22c5 100644 --- a/camera/ndk/impl/ACameraMetadata.cpp +++ b/camera/ndk/impl/ACameraMetadata.cpp @@ -28,33 +28,24 @@ using namespace android; * ACameraMetadata Implementation */ ACameraMetadata::ACameraMetadata(camera_metadata_t* buffer, ACAMERA_METADATA_TYPE type) : - mData(new CameraMetadata(buffer)), - mOwnsData(true), + mData(std::make_shared(buffer)), mType(type) { init(); } -ACameraMetadata::ACameraMetadata(CameraMetadata* cameraMetadata, ACAMERA_METADATA_TYPE type) : +ACameraMetadata::ACameraMetadata(const std::shared_ptr& cameraMetadata, + ACAMERA_METADATA_TYPE type) : mData(cameraMetadata), - mOwnsData(false), mType(type) { init(); } ACameraMetadata::ACameraMetadata(const ACameraMetadata& other) : - mOwnsData(other.mOwnsData), + mData(std::make_shared(*(other.mData))), mType(other.mType) { - if (other.mOwnsData) { - mData = new CameraMetadata(*(other.mData)); - } else { - mData = other.mData; - } } ACameraMetadata::~ACameraMetadata() { - if (mOwnsData) { - delete mData; - } } void @@ -373,7 +364,7 @@ ACameraMetadata::getConstEntry(uint32_t tag, ACameraMetadata_const_entry* entry) Mutex::Autolock _l(mLock); - camera_metadata_ro_entry rawEntry = static_cast(mData)->find(tag); + camera_metadata_ro_entry rawEntry = static_cast(mData.get())->find(tag); if (rawEntry.count == 0) { ALOGE("%s: cannot find metadata tag %d", __FUNCTION__, tag); return ACAMERA_ERROR_METADATA_NOT_FOUND; diff --git a/camera/ndk/impl/ACameraMetadata.h b/camera/ndk/impl/ACameraMetadata.h index a57b2ffa49..084a60ba38 100644 --- a/camera/ndk/impl/ACameraMetadata.h +++ b/camera/ndk/impl/ACameraMetadata.h @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -50,14 +51,13 @@ struct ACameraMetadata : public RefBase { // Constructs a ACameraMetadata that takes ownership of `buffer`. ACameraMetadata(camera_metadata_t* buffer, ACAMERA_METADATA_TYPE type); - // Constructs a ACameraMetadata that is a view of `cameraMetadata`. - // `cameraMetadata` will not be deleted by ~ACameraMetadata(). - ACameraMetadata(CameraMetadata* cameraMetadata, ACAMERA_METADATA_TYPE type); + // Constructs a ACameraMetadata that shares its data with something else, like a Java object + ACameraMetadata(const std::shared_ptr& cameraMetadata, + ACAMERA_METADATA_TYPE type); // Copy constructor. // - // If `other` owns its CameraMetadata, then makes a deep copy. - // Otherwise, the new instance is also a view of the same data. + // Always makes a deep copy. ACameraMetadata(const ACameraMetadata& other); ~ACameraMetadata(); @@ -125,9 +125,7 @@ struct ACameraMetadata : public RefBase { // Guard access of public APIs: get/update/getTags. mutable Mutex mLock; - CameraMetadata* mData; - // If true, has ownership of mData. Otherwise, mData is a view of an external instance. - bool mOwnsData; + std::shared_ptr mData; mutable Vector mTags; // Updated by `getTags()`, cleared by `update()`. const ACAMERA_METADATA_TYPE mType; diff --git a/camera/ndk/include/camera/NdkCameraMetadata.h b/camera/ndk/include/camera/NdkCameraMetadata.h index ee75610543..072bb0249a 100644 --- a/camera/ndk/include/camera/NdkCameraMetadata.h +++ b/camera/ndk/include/camera/NdkCameraMetadata.h @@ -274,10 +274,9 @@ bool ACameraMetadata_isLogicalMultiCamera(const ACameraMetadata* staticMetadata, *

The returned ACameraMetadata must be freed by the application by {@link ACameraMetadata_free} * after application is done using it.

* - *

This function does not affect the lifetime of {@link cameraMetadata}. Attempting to use the - * returned ACameraMetadata object after {@link cameraMetadata} has been garbage collected is - * unsafe. To manage the lifetime beyond the current JNI function call, use - * {@code env->NewGlobalRef()} and {@code env->DeleteGlobalRef()}. + *

The ACameraMetadata maintains a reference count to the underlying data, so + * it can be used independently of the Java object, and it remains valid even if + * the Java metadata is garbage collected. * * @param env the JNI environment. * @param cameraMetadata the source {@link android.hardware.camera2.CameraMetadata} from which the @@ -297,4 +296,4 @@ __END_DECLS #endif /* _NDK_CAMERA_METADATA_H */ -/** @} */ \ No newline at end of file +/** @} */