[libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for setting android metadata from libcamera controls
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Jul 9 06:43:06 CEST 2021
Hi Jacopo,
On Mon, Jul 05, 2021 at 05:44:58PM +0200, Jacopo Mondi wrote:
> 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.
I added the template parameters for consistency. The latter two V and T,
respectively, can be inferred, but the first one cannot. And the first
one cannot be merged into the second (with default value) as then we
have no way of differentiating between "unspecified" and zero.
>
> > + 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 ?
It is, but then as I mentioned earlier, this call would then be missing
a template parameter while the other one won'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.
I was wondering if the template parameters should be in alphabetical
order or not...
Thanks,
Paul
>
> > +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
> >
More information about the libcamera-devel
mailing list