[libcamera-devel] [PATCH v2 3/6] android: camera_metadata: Add appendEntry helper

[Jacopo Mondi]

Hi Paul,

On Mon, Dec 20, 2021 at 05:26:26PM -0600, Paul Elder wrote:
> Add appendEntry() helper, that automatically detects if updateEntry() or
> addEntry() should be used.
>
> Note that updateEntry() will fail if the entry was not yet added, and
> addEntry() will fail is the entry was already added. They are silent
> failures that cause unexpected values to find their way into the android
> metadata instance.
>
> Previously this helper was not necessary, as (with respect to the
> current use case) the preview template generator would always add a key
> so the other template generators that used the preview template as
> boilerplate could reliably use updateEntry(). The preview template
> generator will soon decide based on capabilities whether or not to add
> keys, so the other template generators need a helper to decide whether
> to use updateEntry() or addEntry().
>
> For now only implement it for enums and arithmetic values, as they will
> mainly be used in populating templates.

I'm just not sure if doing this for enum and arithmetic types is a
good idea, or we would need a template specialization for each
specialization of addEntry() (and it seems to me updateEntry() and
addEntry() are already mis-aligned).

>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
> Changes in v2:
> - expand on correctness of updateEntry() vs addEntry(), and the
>   rationale for the patch
> ---
>  src/android/camera_metadata.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index e70f60af..f3b7c91e 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -33,6 +33,17 @@ public:
>
>  	bool hasEntry(uint32_t tag) const;
>
> +	template<typename T,
> +		 std::enable_if_t<std::is_arithmetic_v<T> ||
> +				  std::is_enum_v<T>> * = nullptr>
> +	bool appendEntry(uint32_t tag, const T &data)

Usual bikeshedding about name: is append the best name to explain what
this function does ? Isn't 'setEntry()' more appropriate ?

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

> +	{
> +		if (hasEntry(tag))
> +			return updateEntry(tag, &data, 1, sizeof(T));
> +		else
> +			return addEntry(tag, &data, 1, sizeof(T));
> +	}
> +
>  	template<typename T,
>  		 std::enable_if_t<std::is_arithmetic_v<T> ||
>  				  std::is_enum_v<T>> * = nullptr>
> --
> 2.27.0
>