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

15174 diff mbox series

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.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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(+)

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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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@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
>