[libcamera-devel] [PATCH v5 2/9] android: metadata: Add hasEntry and entryContains

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:
> Add convenience functions for checking if an entry is present in a
> CameraMetadata instance, and to check if an array entry includes a
> specific value.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Changes in v5:
> - refactor template code to reduce the footprint of specialized template
>   code
> 
> New in v4
> ---
>  src/android/camera_metadata.cpp | 18 ++++++++++++++++++
>  src/android/camera_metadata.h   | 14 ++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> index 3fc7cf27..ac86c5fc 100644
> --- a/src/android/camera_metadata.cpp
> +++ b/src/android/camera_metadata.cpp
> @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)
>  	return true;
>  }
>  
> +template<>
> +bool CameraMetadata::entryContainsOne<uint8_t>(const camera_metadata_ro_entry_t &entry,
> +					       uint8_t value) const
> +{
> +	for (unsigned int i = 0; i < entry.count; i++) {
> +		if (entry.data.u8[i] == value)
> +			return true;
> +	}
> +
> +	return false;
> +}

You'll get a linker error if you call entryContains() with a type
different than uint8_t. Is there a use case for that ?

> +
> +bool CameraMetadata::hasEntry(uint32_t tag) const
> +{
> +	camera_metadata_ro_entry_t entry;
> +	return getEntry(tag, &entry);
> +}
> +
>  bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,
>  			      size_t elementSize)
>  {
> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> index 3b7c9e24..72ba4db2 100644
> --- a/src/android/camera_metadata.h
> +++ b/src/android/camera_metadata.h
> @@ -29,6 +29,20 @@ public:
>  	bool isValid() const { return valid_; }
>  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
>  
> +	template<typename T>
> +	bool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;

Unless you intend for this to be called directly, let's make it private.

> +
> +	template<typename T> bool entryContains(uint32_t tag, T value) const
> +	{
> +		camera_metadata_ro_entry_t entry;
> +		if (!getEntry(tag, &entry))
> +			return false;
> +
> +		return entryContainsOne<T>(entry, value);
> +	}

So the part you inline is the one that doesn't depend on the type T, and
the part you don't inline is that one that does ? That seems a bit
backwards, I'd move merge the two functions and move all the code to the
.cpp file.

> +
> +	bool hasEntry(uint32_t tag) const;
> +
>  	template<typename T,
>  		 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
>  	bool addEntry(uint32_t tag, const T &data)

-- 
Regards,

Laurent Pinchart