[{"id":2598,"web_url":"https://patchwork.libcamera.org/comment/2598/","msgid":"<20190904175208.GB5433@pendragon.ideasonboard.com>","date":"2019-09-04T17:52:08","subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 04, 2019 at 04:18:24PM +0200, Jacopo Mondi wrote:\n> The current assertion macro on camera metadata handling operations\n> does not free the allocated metadata pack in case of errors.\n> \n> Fix this by freeing the metadata pack in the error path.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/android/camera_device.cpp | 140 +++++++++++++++++-----------------\n>  src/android/camera_device.h   |   3 +-\n>  2 files changed, 72 insertions(+), 71 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index c4f11e91bcf1..a153972fbef0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -141,7 +141,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,\n>  \t\t\taberrationModes.data(), aberrationModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Control static metadata. */\n>  \tstd::vector<uint8_t> aeAvailableAntiBandingModes = {\n> @@ -154,7 +154,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n>  \t\t\taeAvailableAntiBandingModes.data(),\n>  \t\t\taeAvailableAntiBandingModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> aeAvailableModes = {\n>  \t\tANDROID_CONTROL_AE_MODE_ON,\n> @@ -162,7 +162,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AE_AVAILABLE_MODES,\n>  \t\t\taeAvailableModes.data(), aeAvailableModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<int32_t> availableAeFpsTarget = {\n>  \t\t15, 30,\n> @@ -171,7 +171,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n>  \t\t\tavailableAeFpsTarget.data(),\n>  \t\t\tavailableAeFpsTarget.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<int32_t> aeCompensationRange = {\n>  \t\t0, 0,\n> @@ -180,7 +180,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_CONTROL_AE_COMPENSATION_RANGE,\n>  \t\t\taeCompensationRange.data(),\n>  \t\t\taeCompensationRange.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tconst camera_metadata_rational_t aeCompensationStep[] = {\n>  \t\t{ 0, 1 }\n> @@ -188,7 +188,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AE_COMPENSATION_STEP,\n>  \t\t\taeCompensationStep, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> availableAfModes = {\n>  \t\tANDROID_CONTROL_AF_MODE_OFF,\n> @@ -196,7 +196,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AF_AVAILABLE_MODES,\n>  \t\t\tavailableAfModes.data(), availableAfModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> availableEffects = {\n>  \t\tANDROID_CONTROL_EFFECT_MODE_OFF,\n> @@ -204,7 +204,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AVAILABLE_EFFECTS,\n>  \t\t\tavailableEffects.data(), availableEffects.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> availableSceneModes = {\n>  \t\tANDROID_CONTROL_SCENE_MODE_DISABLED,\n> @@ -212,7 +212,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AVAILABLE_SCENE_MODES,\n>  \t\t\tavailableSceneModes.data(), availableSceneModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> availableStabilizationModes = {\n>  \t\tANDROID_CONTROL_VIDEO_STABILIZATION_MODE_OFF,\n> @@ -221,7 +221,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,\n>  \t\t\tavailableStabilizationModes.data(),\n>  \t\t\tavailableStabilizationModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> availableAwbModes = {\n>  \t\tANDROID_CONTROL_AWB_MODE_OFF,\n> @@ -229,7 +229,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AWB_AVAILABLE_MODES,\n>  \t\t\tavailableAwbModes.data(), availableAwbModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<int32_t> availableMaxRegions = {\n>  \t\t0, 0, 0,\n> @@ -237,7 +237,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_MAX_REGIONS,\n>  \t\t\tavailableMaxRegions.data(), availableMaxRegions.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> sceneModesOverride = {\n>  \t\tANDROID_CONTROL_AE_MODE_ON,\n> @@ -247,25 +247,25 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n>  \t\t\tsceneModesOverride.data(), sceneModesOverride.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AE_LOCK_AVAILABLE,\n>  \t\t\t&aeLockAvailable, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>  \t\t\t&awbLockAvailable, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tchar availableControlModes = ANDROID_CONTROL_MODE_AUTO;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_CONTROL_AVAILABLE_MODES,\n>  \t\t\t&availableControlModes, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* JPEG static metadata. */\n>  \tstd::vector<int32_t> availableThumbnailSizes = {\n> @@ -275,7 +275,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,\n>  \t\t\tavailableThumbnailSizes.data(),\n>  \t\t\tavailableThumbnailSizes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Sensor static metadata. */\n>  \tint32_t pixelArraySize[] = {\n> @@ -284,7 +284,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n>  \t\t\t\t&pixelArraySize, 2);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tint32_t sensorSizes[] = {\n>  \t\t0, 0, 2560, 1920,\n> @@ -292,7 +292,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n>  \t\t\t\t&sensorSizes, 4);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tint32_t sensitivityRange[] = {\n>  \t\t32, 2400,\n> @@ -300,13 +300,13 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n>  \t\t\t\t&sensitivityRange, 2);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n>  \t\t\t\t&filterArr, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tint64_t exposureTimeRange[] = {\n>  \t\t100000, 200000000,\n> @@ -314,13 +314,13 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n>  \t\t\t\t&exposureTimeRange, 2);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tint32_t orientation = 0;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_ORIENTATION,\n>  \t\t\t\t&orientation, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<int32_t> testPatterModes = {\n>  \t\tANDROID_SENSOR_TEST_PATTERN_MODE_OFF,\n> @@ -328,7 +328,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n>  \t\t\t\ttestPatterModes.data(), testPatterModes.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<float> physicalSize = {\n>  \t\t2592, 1944,\n> @@ -336,39 +336,39 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,\n>  \t\t\t\tphysicalSize.data(), physicalSize.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n>  \t\t\t&timestampSource, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Statistics static metadata. */\n>  \tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n>  \t\t\t&faceDetectMode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tint32_t maxFaceCount = 0;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_STATISTICS_INFO_MAX_FACE_COUNT,\n>  \t\t\t&maxFaceCount, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Sync static metadata. */\n>  \tint32_t maxLatency = ANDROID_SYNC_MAX_LATENCY_UNKNOWN;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_SYNC_MAX_LATENCY, &maxLatency, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Flash static metadata. */\n>  \tchar flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_FLASH_INFO_AVAILABLE,\n>  \t\t\t&flashAvailable, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Lens static metadata. */\n>  \tstd::vector<float> lensApertures = {\n> @@ -377,12 +377,12 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_LENS_INFO_AVAILABLE_APERTURES,\n>  \t\t\t\tlensApertures.data(), lensApertures.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint8_t lensFacing = ANDROID_LENS_FACING_FRONT;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_LENS_FACING, &lensFacing, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<float> lensFocalLenghts = {\n>  \t\t1,\n> @@ -391,7 +391,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\t\tANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,\n>  \t\t\t\tlensFocalLenghts.data(),\n>  \t\t\t\tlensFocalLenghts.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> opticalStabilizations = {\n>  \t\tANDROID_LENS_OPTICAL_STABILIZATION_MODE_OFF,\n> @@ -400,33 +400,33 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\t\tANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,\n>  \t\t\t\topticalStabilizations.data(),\n>  \t\t\t\topticalStabilizations.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tfloat hypeFocalDistance = 0;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,\n>  \t\t\t\t&hypeFocalDistance, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tfloat minFocusDistance = 0;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,\n>  \t\t\t\t&minFocusDistance, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Noise reduction modes. */\n>  \tuint8_t noiseReductionModes = ANDROID_NOISE_REDUCTION_MODE_OFF;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\t\tANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n>  \t\t\t\t&noiseReductionModes, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Scaler static metadata. */\n>  \tfloat maxDigitalZoom = 1;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,\n>  \t\t\t&maxDigitalZoom, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint32_t> availableStreamFormats = {\n>  \t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB,\n> @@ -437,7 +437,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_SCALER_AVAILABLE_FORMATS,\n>  \t\t\tavailableStreamFormats.data(),\n>  \t\t\tavailableStreamFormats.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint32_t> availableStreamConfigurations = {\n>  \t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,\n> @@ -451,7 +451,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n>  \t\t\tavailableStreamConfigurations.data(),\n>  \t\t\tavailableStreamConfigurations.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<int64_t> availableStallDurations = {\n>  \t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> @@ -460,7 +460,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_SCALER_AVAILABLE_STALL_DURATIONS,\n>  \t\t\tavailableStallDurations.data(),\n>  \t\t\tavailableStallDurations.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<int64_t> minFrameDurations = {\n>  \t\tANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,\n> @@ -470,12 +470,12 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n>  \t\t\tminFrameDurations.data(), minFrameDurations.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \t/* Info static metadata. */\n>  \tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> @@ -488,13 +488,13 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_REQUEST_PARTIAL_RESULT_COUNT,\n>  \t\t\t&partialResultCount, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tuint8_t maxPipelineDepth = 2;\n>  \tret = add_camera_metadata_entry(staticMetadata_,\n>  \t\t\tANDROID_REQUEST_PIPELINE_MAX_DEPTH,\n>  \t\t\t&maxPipelineDepth, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \tstd::vector<uint8_t> availableCapabilities = {\n>  \t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,\n> @@ -503,7 +503,7 @@ camera_metadata_t *CameraDevice::getStaticMetadata()\n>  \t\t\tANDROID_REQUEST_AVAILABLE_CAPABILITIES,\n>  \t\t\tavailableCapabilities.data(),\n>  \t\t\tavailableCapabilities.size());\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, staticMetadata_);\n>  \n>  \treturn staticMetadata_;\n>  }\n> @@ -532,66 +532,66 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AE_MODE,\n>  \t\t\t&aeMode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tint32_t aeExposureCompensation = 0;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n>  \t\t\t&aeExposureCompensation, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,\n>  \t\t\t&aePrecaptureTrigger, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AE_LOCK,\n>  \t\t\t&aeLock, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AF_TRIGGER,\n>  \t\t\t&afTrigger, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AWB_MODE,\n>  \t\t\t&awbMode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_AWB_LOCK,\n>  \t\t\t&awbLock, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t flashMode = ANDROID_FLASH_MODE_OFF;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_FLASH_MODE,\n>  \t\t\t&flashMode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_STATISTICS_FACE_DETECT_MODE,\n>  \t\t\t&faceDetectMode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_OFF;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_NOISE_REDUCTION_MODE, &noiseReduction, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \tuint8_t aberrationMode = ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF;\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_COLOR_CORRECTION_ABERRATION_MODE,\n>  \t\t\t&aberrationMode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \t/* Use the capture intent matching the requested template type. */\n>  \tuint8_t captureIntent;\n> @@ -621,7 +621,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)\n>  \tret = add_camera_metadata_entry(requestTemplate,\n>  \t\t\tANDROID_CONTROL_CAPTURE_INTENT,\n>  \t\t\t&captureIntent, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, requestTemplate);\n>  \n>  \trequestTemplates_[type] = requestTemplate;\n>  \n> @@ -887,35 +887,35 @@ camera_metadata_t *CameraDevice::getResultMetadata(int frame_number,\n>  \tconst uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;\n>  \tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,\n>  \t\t\t\t\t&ae_state, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tconst uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;\n>  \tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,\n>  \t\t\t\t\t&ae_lock, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tuint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;\n>  \tret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,\n>  \t\t\t\t\t&af_state, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tconst uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_CONTROL_AWB_STATE,\n>  \t\t\t\t\t&awb_state, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tconst uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_CONTROL_AWB_LOCK,\n>  \t\t\t\t\t&awb_lock, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tconst uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_LENS_STATE,\n>  \t\t\t\t\t&lens_state, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tint32_t sensorSizes[] = {\n>  \t\t0, 0, 2560, 1920,\n> @@ -923,39 +923,39 @@ camera_metadata_t *CameraDevice::getResultMetadata(int frame_number,\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_SCALER_CROP_REGION,\n>  \t\t\t\t\tsensorSizes, 4);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_SENSOR_TIMESTAMP,\n>  \t\t\t\t\t&timestamp, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \t/* 33.3 msec */\n>  \tconst int64_t rolling_shutter_skew = 33300000;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_SENSOR_ROLLING_SHUTTER_SKEW,\n>  \t\t\t\t\t&rolling_shutter_skew, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \t/* 16.6 msec */\n>  \tconst int64_t exposure_time = 16600000;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_SENSOR_EXPOSURE_TIME,\n>  \t\t\t\t\t&exposure_time, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tconst uint8_t lens_shading_map_mode =\n>  \t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_STATISTICS_LENS_SHADING_MAP_MODE,\n>  \t\t\t\t\t&lens_shading_map_mode, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \tconst uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;\n>  \tret = add_camera_metadata_entry(resultMetadata,\n>  \t\t\t\t\tANDROID_STATISTICS_SCENE_FLICKER,\n>  \t\t\t\t\t&scene_flicker, 1);\n> -\tMETADATA_ASSERT(ret);\n> +\tMETADATA_ASSERT(ret, resultMetadata);\n>  \n>  \treturn resultMetadata;\n>  }\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 64382bbac76a..9880168a7581 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -19,10 +19,11 @@\n>  \n>  #include \"message.h\"\n>  \n> -#define METADATA_ASSERT(_r)\t\t\\\n> +#define METADATA_ASSERT(_r, _p)\t\t\\\n>  \tdo {\t\t\t\t\\\n>  \t\tif (!(_r)) break;\t\\\n>  \t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> +\t\tfree_camera_metadata((_p));\t\t\t\t     \\\n>  \t\treturn nullptr;\t\t\\\n>  \t} while(0);\n\nThat's really ugly :-(\n\nHow about using unique pointers instead ? After allocating the metadata,\ncreate a unique pointer on the stack\n\n\tstd::unique_ptr<camera_metadata_t> deleter(metadata);\n\nAnd to return in the *normal* path,\n\n\treturn deleter.release();\n\nOr, if you prefer,\n\n\tdeleter.release();\n\treturn metadata;\n\nThe two options are equivalent, but the latter may be more readable.","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78BF160BB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  4 Sep 2019 19:52:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C63FD440;\n\tWed,  4 Sep 2019 19:52:14 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567619535;\n\tbh=jjZ+W3GLNnEWBtrx3azMBgHyP5uvqLZv97AEB2MBStw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sbdTq5/NfMW0H4zCv26lSzME4GifDYObqh/qJZxt7KhP/3D71rDXtNiJNCmvbP4pm\n\tNTzL+mqQ1CzlkVqE6FRIrsyGG3nY7GihL9AXbc3twMASlDc4frIEXHKz52eRyqtF/f\n\t5trxUuYQn3GkZv13h75V9FmhPJS6wUAOk9TR+wdg=","Date":"Wed, 4 Sep 2019 20:52:08 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org, jcliang@google.com, wtlee@google.com","Message-ID":"<20190904175208.GB5433@pendragon.ideasonboard.com>","References":"<20190904141825.20697-1-jacopo@jmondi.org>\n\t<20190904141825.20697-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190904141825.20697-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 04 Sep 2019 17:52:15 -0000"}},{"id":2608,"web_url":"https://patchwork.libcamera.org/comment/2608/","msgid":"<20190904210317.xptbpuxr2ocnwlo2@uno.localdomain>","date":"2019-09-04T21:03:17","subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Sep 04, 2019 at 08:52:08PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n\n[snip]\n\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 64382bbac76a..9880168a7581 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -19,10 +19,11 @@\n> >\n> >  #include \"message.h\"\n> >\n> > -#define METADATA_ASSERT(_r)\t\t\\\n> > +#define METADATA_ASSERT(_r, _p)\t\t\\\n> >  \tdo {\t\t\t\t\\\n> >  \t\tif (!(_r)) break;\t\\\n> >  \t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> > +\t\tfree_camera_metadata((_p));\t\t\t\t     \\\n> >  \t\treturn nullptr;\t\t\\\n> >  \t} while(0);\n>\n> That's really ugly :-(\n\nThe whole METADATA_ASSERT thing is ugly. It has been there since my\nearly camera hal hacking days, and it stayed there on the assumption\nthe metadata handling code should be replaced by a proper Metadata\nclass, but since it has to be changed I agree it should probably be\nremoved.\n\n>\n> How about using unique pointers instead ? After allocating the metadata,\n> create a unique pointer on the stack\n>\n> \tstd::unique_ptr<camera_metadata_t> deleter(metadata);\n>\n> And to return in the *normal* path,\n>\n> \treturn deleter.release();\n>\n> Or, if you prefer,\n>\n> \tdeleter.release();\n> \treturn metadata;\n>\n> The two options are equivalent, but the latter may be more readable.\n\nGnn, I'm not a fan of those C++ implicities... I would rather goto to\na label where delete the metadata pack and return... Is this to C-ish ?\n\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B4B8260BB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  4 Sep 2019 23:01:45 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id BEC09240004;\n\tWed,  4 Sep 2019 21:01:44 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 4 Sep 2019 23:03:17 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, jcliang@google.com, wtlee@google.com","Message-ID":"<20190904210317.xptbpuxr2ocnwlo2@uno.localdomain>","References":"<20190904141825.20697-1-jacopo@jmondi.org>\n\t<20190904141825.20697-8-jacopo@jmondi.org>\n\t<20190904175208.GB5433@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mof5z4amko442rej\"","Content-Disposition":"inline","In-Reply-To":"<20190904175208.GB5433@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 04 Sep 2019 21:01:45 -0000"}},{"id":2609,"web_url":"https://patchwork.libcamera.org/comment/2609/","msgid":"<20190904214636.GL5433@pendragon.ideasonboard.com>","date":"2019-09-04T21:46:36","subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 04, 2019 at 11:03:17PM +0200, Jacopo Mondi wrote:\n> On Wed, Sep 04, 2019 at 08:52:08PM +0300, Laurent Pinchart wrote:\n> \n> [snip]\n> \n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 64382bbac76a..9880168a7581 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -19,10 +19,11 @@\n> > >\n> > >  #include \"message.h\"\n> > >\n> > > -#define METADATA_ASSERT(_r)\t\t\\\n> > > +#define METADATA_ASSERT(_r, _p)\t\t\\\n> > >  \tdo {\t\t\t\t\\\n> > >  \t\tif (!(_r)) break;\t\\\n> > >  \t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> > > +\t\tfree_camera_metadata((_p));\t\t\t\t     \\\n> > >  \t\treturn nullptr;\t\t\\\n> > >  \t} while(0);\n> >\n> > That's really ugly :-(\n> \n> The whole METADATA_ASSERT thing is ugly. It has been there since my\n> early camera hal hacking days, and it stayed there on the assumption\n> the metadata handling code should be replaced by a proper Metadata\n> class, but since it has to be changed I agree it should probably be\n> removed.\n> \n> >\n> > How about using unique pointers instead ? After allocating the metadata,\n> > create a unique pointer on the stack\n> >\n> > \tstd::unique_ptr<camera_metadata_t> deleter(metadata);\n> >\n> > And to return in the *normal* path,\n> >\n> > \treturn deleter.release();\n> >\n> > Or, if you prefer,\n> >\n> > \tdeleter.release();\n> > \treturn metadata;\n> >\n> > The two options are equivalent, but the latter may be more readable.\n> \n> Gnn, I'm not a fan of those C++ implicities... I would rather goto to\n> a label where delete the metadata pack and return... Is this to C-ish ?\n\nIt is, but that part that would be really bad about that would be the\ngoto hidden inside the METADATA_ASSERT() macro.","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2BF860BB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  4 Sep 2019 23:46:43 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F1309440;\n\tWed,  4 Sep 2019 23:46:42 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567633603;\n\tbh=WPII9HeavuHL6DSu9oddE+mlvPDKk1Dzh1/SQPLYnRo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=emZnoCzdcFOUgtGDi+LN9mpEH+c6YhSRKLhdzzd+YxbV0dH0Dd2sfVBwodjlQ1NAt\n\tTArGbzg0WHtSOMrnHHbZM7e1KtXaVm8nYsVshCEEggCqOfY7Gxnoojht2166InDLuY\n\tJeFWAKkBuI6NZ9K0XDKk0vfklu0gNRvmqFKji1UM=","Date":"Thu, 5 Sep 2019 00:46:36 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org, jcliang@google.com, wtlee@google.com","Message-ID":"<20190904214636.GL5433@pendragon.ideasonboard.com>","References":"<20190904141825.20697-1-jacopo@jmondi.org>\n\t<20190904141825.20697-8-jacopo@jmondi.org>\n\t<20190904175208.GB5433@pendragon.ideasonboard.com>\n\t<20190904210317.xptbpuxr2ocnwlo2@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190904210317.xptbpuxr2ocnwlo2@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 04 Sep 2019 21:46:43 -0000"}},{"id":2610,"web_url":"https://patchwork.libcamera.org/comment/2610/","msgid":"<20190904220118.ecsqxzwslxkavknl@uno.localdomain>","date":"2019-09-04T22:01:18","subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Thu, Sep 05, 2019 at 12:46:36AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Sep 04, 2019 at 11:03:17PM +0200, Jacopo Mondi wrote:\n> > On Wed, Sep 04, 2019 at 08:52:08PM +0300, Laurent Pinchart wrote:\n> >\n> > [snip]\n> >\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index 64382bbac76a..9880168a7581 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -19,10 +19,11 @@\n> > > >\n> > > >  #include \"message.h\"\n> > > >\n> > > > -#define METADATA_ASSERT(_r)\t\t\\\n> > > > +#define METADATA_ASSERT(_r, _p)\t\t\\\n> > > >  \tdo {\t\t\t\t\\\n> > > >  \t\tif (!(_r)) break;\t\\\n> > > >  \t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> > > > +\t\tfree_camera_metadata((_p));\t\t\t\t     \\\n> > > >  \t\treturn nullptr;\t\t\\\n> > > >  \t} while(0);\n> > >\n> > > That's really ugly :-(\n> >\n> > The whole METADATA_ASSERT thing is ugly. It has been there since my\n> > early camera hal hacking days, and it stayed there on the assumption\n> > the metadata handling code should be replaced by a proper Metadata\n> > class, but since it has to be changed I agree it should probably be\n> > removed.\n> >\n> > >\n> > > How about using unique pointers instead ? After allocating the metadata,\n> > > create a unique pointer on the stack\n> > >\n> > > \tstd::unique_ptr<camera_metadata_t> deleter(metadata);\n> > >\n> > > And to return in the *normal* path,\n> > >\n> > > \treturn deleter.release();\n> > >\n> > > Or, if you prefer,\n> > >\n> > > \tdeleter.release();\n> > > \treturn metadata;\n> > >\n> > > The two options are equivalent, but the latter may be more readable.\n> >\n> > Gnn, I'm not a fan of those C++ implicities... I would rather goto to\n> > a label where delete the metadata pack and return... Is this to C-ish ?\n>\n> It is, but that part that would be really bad about that would be the\n> goto hidden inside the METADATA_ASSERT() macro.\n>\n\nNot hidden, and explicit 'goto' replacing the macro.\n\nAnyway, I'll immediately take it back:\n../src/android/camera_device.cpp:390:3: error: cannot jump from this goto statement to its label\n                goto error;\n\nOne thing I realized bothers me is loosing the printout that tells you\nwhich tag has failed.. true, that code will go away but it will be\nreasonably shaken up a bit as soon as we have controls and properties,\nso it might be worth to keep it to spot errors generated by more\ndynamic control values around.\n\nA proper helper class would of course take care of that, but for the\nsake of this series, and as long as this code will be used, I would\nrather have that debug information available.\n\nI can add a LOG line to each if (ret), but really, 4 lines of\nidentical error handline code for each\nmetadata tag, goto or unique_ptr<> it doesn't matter, bother me a bit.\n\nSuddenly the ugly macro seems less uglier to me. It might be the late\ntime though..\n\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 357E060BB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  4 Sep 2019 23:59:46 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 6A9D2E0004;\n\tWed,  4 Sep 2019 21:59:45 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 5 Sep 2019 00:01:18 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, jcliang@google.com, wtlee@google.com","Message-ID":"<20190904220118.ecsqxzwslxkavknl@uno.localdomain>","References":"<20190904141825.20697-1-jacopo@jmondi.org>\n\t<20190904141825.20697-8-jacopo@jmondi.org>\n\t<20190904175208.GB5433@pendragon.ideasonboard.com>\n\t<20190904210317.xptbpuxr2ocnwlo2@uno.localdomain>\n\t<20190904214636.GL5433@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ipl4zbfd2fehve7p\"","Content-Disposition":"inline","In-Reply-To":"<20190904214636.GL5433@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 04 Sep 2019 21:59:46 -0000"}},{"id":2611,"web_url":"https://patchwork.libcamera.org/comment/2611/","msgid":"<20190904222211.GA2413@pendragon.ideasonboard.com>","date":"2019-09-04T22:22:11","subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 05, 2019 at 12:01:18AM +0200, Jacopo Mondi wrote:\n> On Thu, Sep 05, 2019 at 12:46:36AM +0300, Laurent Pinchart wrote:\n> > On Wed, Sep 04, 2019 at 11:03:17PM +0200, Jacopo Mondi wrote:\n> > > On Wed, Sep 04, 2019 at 08:52:08PM +0300, Laurent Pinchart wrote:\n> > >\n> > > [snip]\n> > >\n> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > index 64382bbac76a..9880168a7581 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -19,10 +19,11 @@\n> > > > >\n> > > > >  #include \"message.h\"\n> > > > >\n> > > > > -#define METADATA_ASSERT(_r)\t\t\\\n> > > > > +#define METADATA_ASSERT(_r, _p)\t\t\\\n> > > > >  \tdo {\t\t\t\t\\\n> > > > >  \t\tif (!(_r)) break;\t\\\n> > > > >  \t\tLOG(HAL, Error) << \"Error: \" << __func__ << \":\" << __LINE__; \\\n> > > > > +\t\tfree_camera_metadata((_p));\t\t\t\t     \\\n> > > > >  \t\treturn nullptr;\t\t\\\n> > > > >  \t} while(0);\n> > > >\n> > > > That's really ugly :-(\n> > >\n> > > The whole METADATA_ASSERT thing is ugly. It has been there since my\n> > > early camera hal hacking days, and it stayed there on the assumption\n> > > the metadata handling code should be replaced by a proper Metadata\n> > > class, but since it has to be changed I agree it should probably be\n> > > removed.\n> > >\n> > > > How about using unique pointers instead ? After allocating the metadata,\n> > > > create a unique pointer on the stack\n> > > >\n> > > > \tstd::unique_ptr<camera_metadata_t> deleter(metadata);\n> > > >\n> > > > And to return in the *normal* path,\n> > > >\n> > > > \treturn deleter.release();\n> > > >\n> > > > Or, if you prefer,\n> > > >\n> > > > \tdeleter.release();\n> > > > \treturn metadata;\n> > > >\n> > > > The two options are equivalent, but the latter may be more readable.\n> > >\n> > > Gnn, I'm not a fan of those C++ implicities... I would rather goto to\n> > > a label where delete the metadata pack and return... Is this to C-ish ?\n> >\n> > It is, but that part that would be really bad about that would be the\n> > goto hidden inside the METADATA_ASSERT() macro.\n> \n> Not hidden, and explicit 'goto' replacing the macro.\n> \n> Anyway, I'll immediately take it back:\n> ../src/android/camera_device.cpp:390:3: error: cannot jump from this goto statement to its label\n>                 goto error;\n> \n> One thing I realized bothers me is loosing the printout that tells you\n> which tag has failed.. true, that code will go away but it will be\n> reasonably shaken up a bit as soon as we have controls and properties,\n> so it might be worth to keep it to spot errors generated by more\n> dynamic control values around.\n> \n> A proper helper class would of course take care of that, but for the\n> sake of this series, and as long as this code will be used, I would\n> rather have that debug information available.\n> \n> I can add a LOG line to each if (ret), but really, 4 lines of\n> identical error handline code for each\n> metadata tag, goto or unique_ptr<> it doesn't matter, bother me a bit.\n> \n> Suddenly the ugly macro seems less uglier to me. It might be the late\n> time though..\n\nI haven't explained what I meant clearly enough. I think you can keep\nthe macro for now, but instead of adding free_camera_metadata() to the\nmacro, use a std::unique_ptr<> as a deleter. This will keep the LOG()\nand handle the leak in the request type switch().","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4288E60BB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Sep 2019 00:22:18 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84F18440;\n\tThu,  5 Sep 2019 00:22:17 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567635737;\n\tbh=EC2VtaFaJR7Mklst4pRcz8hSNQP6HD2sU/Ep7pX0c/s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OjucOj1CUvj5uX3VqdGLJwnrpECd5of8OGcB/UfPgmxqsBxVgeIzvB6KfvOsdxWYr\n\t7lHvYO1Da9mE2jyV+QWeKBjhteEED1pOsWSpwJVz5bPPqo3hzYjiL1k6PgXD3DfK7U\n\tQqCYmKXFJ4+/oXvPTAco7mObtYum0fEYATQeVnts=","Date":"Thu, 5 Sep 2019 01:22:11 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org, jcliang@google.com, wtlee@google.com","Message-ID":"<20190904222211.GA2413@pendragon.ideasonboard.com>","References":"<20190904141825.20697-1-jacopo@jmondi.org>\n\t<20190904141825.20697-8-jacopo@jmondi.org>\n\t<20190904175208.GB5433@pendragon.ideasonboard.com>\n\t<20190904210317.xptbpuxr2ocnwlo2@uno.localdomain>\n\t<20190904214636.GL5433@pendragon.ideasonboard.com>\n\t<20190904220118.ecsqxzwslxkavknl@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190904220118.ecsqxzwslxkavknl@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 7/8] android: camera_device: Free\n\tmetadata in error path","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 04 Sep 2019 22:22:18 -0000"}}]