[libcamera-devel] [PATCH 4/6] android: camera_metadata: Add method to update an entry

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 24, 2020 at 04:21:18PM +0200, Jacopo Mondi wrote:
> Add a method to update an existing metadata tag entry, by wrapping
> the update_metadata_entry() function provided by the Android
> metadata library.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_metadata.cpp | 27 +++++++++++++++++++++++++++
>  src/android/camera_metadata.h   |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> index ea33e9c2de25..e48c987fb0fb 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -46,6 +46,33 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
>  	return false;
>  }
>  
> +bool CameraMetadata::updateEntry(uint32_t tag, const void *data, size_t count)
> +{
> +	if (!valid_)
> +		return false;
> +
> +	const char *name = get_camera_metadata_tag_name(tag);

As pointed out by Kieran, I would move this to the error cases below to
avoid unnecessary lookups.

> +
> +	camera_metadata_entry_t entry;
> +	int ret = find_camera_metadata_entry(metadata_, tag, &entry);
> +	if (ret) {
> +		LOG(CameraMetadata, Error)
> +			<< "Failed to update tag "
> +			<< (name ? name : "unknown") << ": not present";

s/"unknown"/"<unknown>"/

or something similar, otherwise the message will be

	Failed to update tag unknown: not present

and will be confusing.

> +		return false;
> +	}
> +
> +	ret = update_camera_metadata_entry(metadata_, entry.index, data,
> +					   count, nullptr);
> +	if (ret) {
> +		LOG(CameraMetadata, Error)
> +			<< "Failed to update tag " << (name ? name : "unknown");

Same here.

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

> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  camera_metadata_t *CameraMetadata::get()
>  {
>  	return valid_ ? metadata_ : nullptr;
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index f16dd27bbf44..9d047b1bb534 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -19,6 +19,7 @@ public:
>  
>  	bool isValid() const { return valid_; }
>  	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> +	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
>  
>  	camera_metadata_t *get();
>  	const camera_metadata_t *get() const;

-- 
Regards,

Laurent Pinchart