[libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for setting android metadata from libcamera controls
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 9 09:52:55 CEST 2021
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 ?
> >
> > > + 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
> >
> > > + 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