[libcamera-devel] [PATCH v5 6/8] android: camera_device: Fix handling of request template
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 5 01:05:31 CEST 2019
Hi Jacopo,
On Wed, Sep 04, 2019 at 08:55:18PM +0300, Laurent Pinchart wrote:
> 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);
Didn't checkstyle.py catch this ?
> > > + 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