[libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for setting android metadata from libcamera controls

[Jacopo Mondi]

Hi Paul,

On Fri, Jul 02, 2021 at 07:37:47PM +0900, Paul Elder wrote:
> Add helpers for setting android metadata from libcamera controls.
>
> There are two versions, for scalars and collections, both of which take
> a default value to fill in the android control if the libcamera control
> is not found. A version for scalars exists for no default, to not set
> the android control at all if it is not found in libcamera. The
> functions take two template parameters, the first for the android type,
> and the second for the libcamera type of the control. They can be
> different, for example, if the former is an enum and the latter is a
> boolean, or if the former is an enum (uint8_t) and the latter is an enum
> (int32_t).
>
> The versions that take a default value return the value that was set in
> the android metadata.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v3:
> - setMetadata for collection only works with vectors
> - change enum to enum class
> - add two template parameters for android type and libcamera type
> - add docs
>
> New in v2
>
> TODO: make ControlList versions so that we can use them in result
> metadata
> ---
>  src/android/camera_capabilities.cpp | 137 ++++++++++++++++++++++++++++
>  1 file changed, 137 insertions(+)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 54bd71da..1d4c44ce 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -124,6 +124,143 @@ hwLevelStrings = {
>  	{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, "EXTERNAL" },
>  };
>
> +enum class ControlRange {
> +	Min,
> +	Def,
> +	Max,
> +};
> +
> +/**

We don't parse the HAL with doxygen so you can spare the additional *
But it's more than fine to use the Doxygen syntax when documenting
things imho, mostly for consistency

> + * \brief Set android metadata from libcamera ControlInfo
> + * \tparam T Type of the control in android
> + * \tparam V Type of the control in libcamera
> + * \param[in] metadata Android metadata to add the control value to

s/Android metadata/Android metadata pack/

> + * \param[in] tag Android metadata tag to add

s/to add// ?

> + * \param[in] controlsInfo libcamera ControlInfoMap from which to find the control info
> + * \param[in] control libcamera ControlId to find from \a controlsInfo
> + * \param[in] controlRange Whether to use the min, def, or max value from the control info
> + *
> + * Set the android metadata entry in \a metadata with tag \a tag based on the

Set the android metadata \a tag in the \a metadata pack based on
the...

> + * control info found for the libcamera control \a control in the libcamera
> + * ControlInfoMap \a controlsInfo. If no libcamera ControlInfo is found, then
> + * the function returns without modifying anything.
> + *
> + * This function is for scalar values.
> + */
> +template<typename T,
> +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr,

You should include <type_traits>

Do you need this to solve which overload ? I see three functions here
with the following prototypes

void setMetadata(CameraMetadata *metadata, uint32_t tag,
		 const ControlInfoMap &controlsInfo, const ControlId *control,
		 enum ControlRange controlRange)

T setMetadata(CameraMetadata *metadata, uint32_t tag,
	      const ControlInfoMap &controlsInfo, const ControlId *control,
	      enum ControlRange controlRange, const V defaultValue)

std::vector<T> setMetadata(CameraMetadata *metadata, uint32_t tag,
			   const ControlInfoMap &controlsInfo,
			   const ControlId *control,
			   const std::vector<T> &defaultVector)

There doesn't seem to be any need to distinguish them on the template parameter
type.

> +	 typename V>
> +void setMetadata(CameraMetadata *metadata, uint32_t tag,
> +		 const ControlInfoMap &controlsInfo, const ControlId *control,
> +		 enum ControlRange controlRange)
> +{
> +	const auto &info = controlsInfo.find(control);
> +	if (info == controlsInfo.end())
> +		return;
> +
> +	T ret;
> +	switch (controlRange) {
> +	case ControlRange::Min:
> +		ret = info->second.min().get<V>();
> +		break;
> +	case ControlRange::Def:
> +		ret = info->second.def().get<V>();
> +		break;
> +	case ControlRange::Max:
> +		ret = info->second.max().get<V>();
> +		break;
> +	}
> +
> +	metadata->addEntry(tag, ret);
> +	return;
> +}
> +
> +/**
> + * \brief Set android metadata from libcamera ControlInfo or a default value
> + * \tparam T Type of the control in android
> + * \tparam U Type of the control in libcamera
> + * \param[in] metadata Android metadata to add the control value to
> + * \param[in] tag Android metadata tag to add
> + * \param[in] controlsInfo libcamera ControlInfoMap from which to find the control info
> + * \param[in] control libcamera ControlId to find from \a controlsInfo
> + * \param[in] controlRange Whether to use the min, def, or max value from the control info
> + * \param[in] defaultValue The value to set in \a metadata if \a control is not found
> + *
> + * Set the android metadata entry in \a metadata with tag \a tag based on the
> + * control info found for the libcamera control \a control in the libcamera
> + * ControlInfoMap \a controlsInfo. If no libcamera ControlInfo is found, then
> + * the android metadata entry is set to \a defaultValue.
> + *
> + * This function is for scalar values.
> + */
> +template<typename T,
> +	 typename U,
> +	 typename V,

Isn't V the same type as T ?

> +	 std::enable_if_t<std::is_arithmetic_v<V>> * = nullptr>

This is probably functionally equivalent for now, but in the previous
version you were checking for the type T to be an arithmetic type.

> +T setMetadata(CameraMetadata *metadata, uint32_t tag,
> +	      const ControlInfoMap &controlsInfo, const ControlId *control,
> +	      enum ControlRange controlRange, const V defaultValue)
> +{
> +	T ret = defaultValue;
> +
> +	const auto &info = controlsInfo.find(control);
> +	if (info != controlsInfo.end()) {
> +		switch (controlRange) {
> +		case ControlRange::Min:
> +			ret = info->second.min().get<U>();
> +			break;
> +		case ControlRange::Def:
> +			ret = info->second.def().get<U>();
> +			break;
> +		case ControlRange::Max:
> +			ret = info->second.max().get<U>();
> +			break;
> +		}
> +	}
> +
> +	metadata->addEntry(tag, ret);
> +	return ret;
> +}
> +
> +/**
> + * \brief Set android metadata from libcamera ControlInfo or a default value
> + * \tparam T Type of the control in android
> + * \tparam V Type of the control in libcamera
> + * \param[in] metadata Android metadata to add the control value to
> + * \param[in] tag Android metadata tag to add
> + * \param[in] controlsInfo libcamera ControlInfoMap from which to find the control info
> + * \param[in] control libcamera ControlId to find from \a controlsInfo
> + * \param[in] defaultVector The value to set in \a metadata if \a control is not found
> + *
> + * Set the android metadata entry in \a metadata with tag \a tag based on the
> + * control info found for the libcamera control \a control in the libcamera
> + * ControlInfoMap \a controlsInfo. If no libcamera ControlInfo is found, then
> + * the android metadata entry is set to \a defaultVector.
> + *
> + * This function is for vector values.
> + */
> +template<typename T,
> +	 typename V>

Fits on one line

> +std::vector<T> setMetadata(CameraMetadata *metadata, uint32_t tag,
> +			   const ControlInfoMap &controlsInfo,
> +			   const ControlId *control,
> +			   const std::vector<T> &defaultVector)
> +{
> +	std::vector<T> ret = {};
> +
> +	const auto &info = controlsInfo.find(control);
> +	if (info != controlsInfo.end()) {
> +		ret.reserve(info->second.values().size());
> +		for (const auto &value : info->second.values())
> +			ret.push_back(value.get<V>());
> +	} else {
> +		ret = defaultVector;

this is a copy that could be avoided by calling addEntry() in each
branch. Something like

	const auto &info = controlsInfo.find(control);
	if (info == controlsInfo.end()) {
                metadata->addEntry(tag, defaultVector);
                return defaultVector;
        }

        std::vector<T> ret(info->second.values().size());
        for (const auto &value : info->second.values())
                ret.push_back(value.get<V>());
        metadata->addEntry(tag, ret);

        return ret;


> +	}
> +
> +	metadata->addEntry(tag, ret);
> +	return ret;
> +}
> +
>  } /* namespace */
>
>  int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
> --
> 2.27.0
>