[libcamera-devel,v5,6/8] android: camera_device: Fix handling of request template
diff mbox series

Message ID 20190904141825.20697-7-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: Rework metadata tags
Related show

Commit Message

Jacopo Mondi Sept. 4, 2019, 2:18 p.m. UTC 
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@jmondi.org>
---
 src/android/camera_device.cpp | 98 +++++++++++++++++------------------
 src/android/camera_device.h   |  2 +-
 2 files changed, 49 insertions(+), 51 deletions(-)

Comments

Laurent Pinchart Sept. 4, 2019, 5:47 p.m. UTC | #1 
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@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@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_;
>  };
>
Laurent Pinchart Sept. 4, 2019, 5:55 p.m. UTC | #2 
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@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@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_;
> >  };
> >
Laurent Pinchart Sept. 4, 2019, 11:05 p.m. UTC | #3 
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@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@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_;
> > >  };
> > >

Patch
diff mbox series 

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_()
 {
 	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();
 }
 
 /*
@@ -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];
 
 	/*
 	 * \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);
 
-	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_;
 };