[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:55:18 CEST 2019
Hi Jacopo,
On Wed, Sep 04, 2019 at 08:47:15PM +0300, Laurent Pinchart wrote:
> 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;
This leak isn't addressed in 7/8. If you use unique_ptr as proposed, it
will be fixed. I would then move the parts of 7/8 that deal with other
methods than constructDefaultRequestSettings() before this patch, and
squash the part that deals with constructDefaultRequestSettings() with
this patch to avoid a bisection regression.
> > + }
> > + 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