[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 10:08:42 CEST 2021
Hi Jacopo,
On Fri, Jul 09, 2021 at 09:52:55AM +0200, Jacopo Mondi wrote:
> Hi Paul,
>
> On Fri, Jul 09, 2021 at 01:43:06PM +0900, paul.elder at ideasonboard.com wrote:
> > 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.
> >
>
> What I meant was specificially regarding enable_if<> as it doesn't
> seem to me you need it. Couldn't you get to the same result with a regular
> function overload that does not involve template metaprogramming ?
I guess the first one doesn't need it.
The second and third I think do? Otherwise we could get V = std::vector<*> ?
> > >
> > > > + 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.
> >
>
> I see three template parameters here and 2 above.
> Also
> <typename T, .. , typename V>
> T setMetadata(..., V defaultValue)
> {
> T ret = defaultValue;
> ...
> }
>
> Again feels like T == V
My intention was to allow T != V as long as they can be implicitly
converted, like enum -> uint8_t, or uint8_t -> bool. It's quite common
that the android uses and enum while libcamera uses int32_t, for
example.
Which does mean that the template parameter needs || std::is_enum_v<V>.
Thanks,
Paul
>
> > >
> > > > + 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...
>
> Whatever, as long as you do your type checks on the same type as you
> have:
>
> std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> in one version, and
> std::enable_if_t<std::is_arithmetic_v<V>> * = nullptr>
> in this version
>
> This seems to reinforce to me T==V, but if you could just drop
> enable_if<> I think it would be better (iirc I compiled this patch
> with enable_if<> removed, the firs time I reviewed it)
>
> Thanks
> j
>
> >
> >
> > 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