[libcamera-devel,v4,7/8] android: camera_device: Cache request metadata

10985 diff mbox series

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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v4:
- use default CameraMetadata constructor for lastSettings_ (so no
  initialization in CameraDevice constructor)
- add todo for incremental caching of android request settings

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 | 6 +++++-
 src/android/camera_device.h   | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

Hi Paul,

Thank you for the patch.

On Mon, Jan 25, 2021 at 04:14:43PM +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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ---
> Changes in v4:
> - use default CameraMetadata constructor for lastSettings_ (so no
>   initialization in CameraDevice constructor)
> - add todo for incremental caching of android request settings
> 
> 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 | 6 +++++-
>  src/android/camera_device.h   | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3068f89f..9330db39 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1688,9 +1688,13 @@ 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 */
>  	Camera3RequestDescriptor *descriptor =
>  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
> +	/* \todo Set cache incrementally? */

I'm not sure we'll remember what this means in a few months :-)

	/*
	 * \todo The Android request model is incremental, settings passed in
	 * previous requests are to be effective until overridden explicitly in
	 * a new request. Do we need to cache settings incrementally here, or is
	 * it handled by the Android camera service ?
	 */

> +	if (camera3Request->settings)
> +		lastSettings_ = camera3Request->settings;
> +	else
> +		descriptor->settings_ = lastSettings_;
>  
>  	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__ */

Hi Paul

On Mon, Jan 25, 2021 at 04:14:43PM +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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This scares me a bit, as it's not totally clear to me what the
semantic of a request without control impacts the other metadata
contructed using the values passed to us as settings.

For the time being though, I think it's ok
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> ---
> Changes in v4:
> - use default CameraMetadata constructor for lastSettings_ (so no
>   initialization in CameraDevice constructor)
> - add todo for incremental caching of android request settings
>
> 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 | 6 +++++-
>  src/android/camera_device.h   | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3068f89f..9330db39 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1688,9 +1688,13 @@ 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 */
>  	Camera3RequestDescriptor *descriptor =
>  		new Camera3RequestDescriptor(camera_.get(), camera3Request);
> +	/* \todo Set cache incrementally? */
> +	if (camera3Request->settings)
> +		lastSettings_ = camera3Request->settings;
> +	else
> +		descriptor->settings_ = lastSettings_;
>
>  	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__ */
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel