[libcamera-devel] [PATCH v3 7/8] android: camera_device: Cache request metadata

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Jan 23, 2021 at 02:17:03PM +0900, Paul Elder wrote:
> The settings in an android capture request may be null, in which case
> the settings from the most recently submitted capture request should be
> used. Cache the request settings to achieve this.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Changes in v3:
> - rebase on "android: camera device and metatada improvements", so it's
>   a bit cleaner
> 
> New in v2
> ---
>  src/android/camera_device.cpp | 9 ++++++---
>  src/android/camera_device.h   | 2 ++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index d0922db7..a2484fdd 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -343,7 +343,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
>  
>  CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
>  	: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> +	  facing_(CAMERA_FACING_FRONT), orientation_(0),
> +	  lastSettings_(CameraMetadata(0, 0))

CameraMetadata has a default constructor, wouldn't it be enough ? You
could then drop this hunk completely.

>  {
>  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
>  
> @@ -1688,10 +1689,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * The descriptor and the associated memory reserved here are freed
>  	 * at request complete time.
>  	 */
> -	/* \todo Handle null request settings */
> -	CameraMetadata settings(camera3Request->settings);
>  	Camera3RequestDescriptor *descriptor =
>  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
> +	if (camera3Request->settings)
> +		lastSettings_ = camera3Request->settings;
> +	else
> +		descriptor->settings_ = lastSettings_;

We will likely need something a bit more elaborate in the future. For
instance, if a request contains ANDROID_JPEG_THUMBNAIL_SIZE and the next
one doesn't, we should use the previous thumbnail size. The cache should
thus be populated incrementaly. This is good enough for now, but maybe
we should record a \todo ? It should mention checking whether
incremental handling of the cache is required, maybe the Android camera
service does so already and guarantees either null or fully populated
request settings.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  
>  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
>  			<< descriptor->numBuffers_ << " HAL streams";
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 058a3f9a..fa4fb544 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -134,6 +134,8 @@ private:
>  	int orientation_;
>  
>  	unsigned int maxJpegBufferSize_;
> +
> +	CameraMetadata lastSettings_;
>  };
>  
>  #endif /* __ANDROID_CAMERA_DEVICE_H__ */

-- 
Regards,

Laurent Pinchart