[libcamera-devel] [PATCH v5 6/8] android: camera_device: Fix handling of request template
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 4 19:47:15 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Wed, Sep 04, 2019 at 04:18:23PM +0200, Jacopo Mondi wrote:
> According to the Android camera HALv3 documentation, the request
> template metadata pack should not be modified after it is returned to
> the camera stack from the HAL.
>
> Currently, the same metadata pack is used for all types of template
> request, without updating the capture intent there contained to match
> the requested template type, as correctly reported by the
> cros_camera_test test application.
>
> In order to avoid modifying the single request template already returned
> to the camera stack in order to update the capture intent it contains,
> create a map that associates a dedicated template to each supported
> capture type.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_device.cpp | 98 +++++++++++++++++------------------
> src/android/camera_device.h | 2 +-
> 2 files changed, 49 insertions(+), 51 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 5f8d19b9ef3d..c4f11e91bcf1 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -51,7 +51,7 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
>
> CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> : running_(false), camera_(camera), staticMetadata_(nullptr),
> - requestTemplate_(nullptr)
> + requestTemplates_()
The default map constructor creates an empty map, so you can drop this.
> {
> camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> }
> @@ -62,9 +62,9 @@ CameraDevice::~CameraDevice()
> free_camera_metadata(staticMetadata_);
> staticMetadata_ = nullptr;
>
> - if (requestTemplate_)
> - free_camera_metadata(requestTemplate_);
> - requestTemplate_ = nullptr;
> + for (auto &it : requestTemplates_)
> + free_camera_metadata(it.second);
> + requestTemplates_.clear();
The clear() is not needed as requestTemplates_ is getting destroyed.
> }
>
> /*
> @@ -515,119 +515,117 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> {
> int ret;
>
> - /*
> - * \todo Inspect type and pick the right metadata pack.
> - * As of now just use a single one for all templates.
> - */
> - uint8_t captureIntent;
> - switch (type) {
> - case CAMERA3_TEMPLATE_PREVIEW:
> - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> - break;
> - case CAMERA3_TEMPLATE_STILL_CAPTURE:
> - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> - break;
> - case CAMERA3_TEMPLATE_VIDEO_RECORD:
> - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> - break;
> - case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> - break;
> - case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> - break;
> - case CAMERA3_TEMPLATE_MANUAL:
> - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> - break;
> - default:
> - LOG(HAL, Error) << "Invalid template request type: " << type;
> - return nullptr;
> - }
> -
> - if (requestTemplate_)
> - return requestTemplate_;
> + if (requestTemplates_.find(type) != requestTemplates_.end())
> + return requestTemplates_[type];
You could make this slightly more efficient by avoiding the double
lookup:
auto it = requestTemplates_.find(type);
if (it != requestTemplates_.end())
return it.second;
>
> /*
> * \todo Keep this in sync with the actual number of entries.
> * Currently: 12 entries, 15 bytes
> */
> - requestTemplate_ = allocate_camera_metadata(15, 20);
> - if (!requestTemplate_) {
> + camera_metadata_t * requestTemplate = allocate_camera_metadata(15, 20);
> + if (!requestTemplate) {
> LOG(HAL, Error) << "Failed to allocate template metadata";
> return nullptr;
> }
>
> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AE_MODE,
> &aeMode, 1);
> METADATA_ASSERT(ret);
>
> int32_t aeExposureCompensation = 0;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> &aeExposureCompensation, 1);
> METADATA_ASSERT(ret);
>
> uint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> &aePrecaptureTrigger, 1);
> METADATA_ASSERT(ret);
>
> uint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AE_LOCK,
> &aeLock, 1);
> METADATA_ASSERT(ret);
>
> uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AF_TRIGGER,
> &afTrigger, 1);
> METADATA_ASSERT(ret);
>
> uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AWB_MODE,
> &awbMode, 1);
> METADATA_ASSERT(ret);
>
> uint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_AWB_LOCK,
> &awbLock, 1);
> METADATA_ASSERT(ret);
>
> uint8_t flashMode = ANDROID_FLASH_MODE_OFF;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_FLASH_MODE,
> &flashMode, 1);
> METADATA_ASSERT(ret);
>
> uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_STATISTICS_FACE_DETECT_MODE,
> &faceDetectMode, 1);
> METADATA_ASSERT(ret);
>
> uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_OFF;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_NOISE_REDUCTION_MODE, &noiseReduction, 1);
> METADATA_ASSERT(ret);
>
> uint8_t aberrationMode = ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF;
> - ret = add_camera_metadata_entry(requestTemplate_,
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_COLOR_CORRECTION_ABERRATION_MODE,
> &aberrationMode, 1);
> METADATA_ASSERT(ret);
>
> - ret = add_camera_metadata_entry(requestTemplate_,
> + /* Use the capture intent matching the requested template type. */
> + uint8_t captureIntent;
> + switch (type) {
> + case CAMERA3_TEMPLATE_PREVIEW:
> + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> + break;
> + case CAMERA3_TEMPLATE_STILL_CAPTURE:
> + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> + break;
> + case CAMERA3_TEMPLATE_VIDEO_RECORD:
> + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> + break;
> + case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> + break;
> + case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> + break;
> + case CAMERA3_TEMPLATE_MANUAL:
> + captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> + break;
> + default:
> + LOG(HAL, Error) << "Invalid template request type: " << type;
> + return nullptr;
> + }
> + ret = add_camera_metadata_entry(requestTemplate,
> ANDROID_CONTROL_CAPTURE_INTENT,
> &captureIntent, 1);
> METADATA_ASSERT(ret);
You're leaking requestTemplate in all error paths, but you know that
already as you addressed it in the next patch, so with the issues above
fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> - return requestTemplate_;
> + requestTemplates_[type] = requestTemplate;
> +
> + return requestTemplate;
> }
>
> /*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 7897ba9dc5c7..64382bbac76a 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -66,7 +66,7 @@ private:
> std::unique_ptr<libcamera::CameraConfiguration> config_;
>
> camera_metadata_t *staticMetadata_;
> - camera_metadata_t *requestTemplate_;
> + std::map<unsigned int, camera_metadata_t *> requestTemplates_;
> const camera3_callback_ops_t *callbacks_;
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list