[libcamera-devel] [PATCH v3 2/7] libcamera: controls: Add extra control values to ControlInfo

[Jacopo Mondi]

Hi Hiro,
 + Laurent for a digression at the end of the email

On Wed, Apr 28, 2021 at 04:36:12PM +0900, Hirokazu Honda wrote:
> The v4l2 menu contains not only index (int32_t) but also either
> name (string) or value (int64_t). To support it keeping
> ControlValue simple, this adds the extra ControlValues to
> ControlInfo. With the ControlInfo, indeices are stored in
> ControlInfo::values, and names (or values) are stored in
> ControlInfo::extraValues.
>

I feel it will be hard to assign a proper semantic to what "values"
and "extra values" are for a control.

Let's take a step back and look at the issue at hand and ask ourselves
if we really care about strings that are used to describe a test
pattern to users.

What do we care about ?
1) Being able to enumerate all the test patter modes supported by a
sensor
2) Map the sensor test patterns to a libcamera test pattern
3) Map the libcamera test pattern to the Android one

Looking at the rest of the series it seems the string test pattern
description is used to map the sensor's test pattern mode to the
libcamera one. Is this necessary ? Can't we map indexes to indexes and
leave strings out ?

This would allow us to:
1) Augment V4L2Device::listControls() to add support for menu controls
by only collecting indexes. This would work for menu and int_menu
controls. The indexes can be collected one by one calls to
VIDIOC_QUERYMENU. Once we have a list of indexes, V4L2ControlInfo
can be augmented with a contructor similar to

        ControlInfo::ControlInfo(Span<const ControlValue> values,
                                 const ControlValue &def)

So that it can transport a list of int32 (the indexes)

2) The sensor db requires a bit more work, not that much actually.
In your series you add entries like:

         { "Disabled", controls::draft::TestPatternModeOff },

Could this just be

         { 0, controls::draft::TestPatternModeOff },
?

Of course we are now tying the driver's patter listing order to the
libcamera's control, if a driver changes the ordering (why would it ?)
the sensor db will have to be updated. But this is not different.. if
the driver changes a pattern name (why would it?) we'll have to update
the db anyhow (possibly with versioning \o/ )

Setting a menu control goes by index anyhow, so it won't really make a
difference (when and if we'll have to support setting menu controls).

This might work for test pattern modes, as we don't actually care
about the mode name, if not for debugging purposes. It won't work that
well with INT_MENU controls, the most notable being CID_LINK_FREQ
(even if its usage from userspace is limited, even not required
possibly). In that case the index is less representitive than its
content, so might chose for INT_MENU to collect the actual values
instead of the indexes, and in that case we'll need a way to reverse
the value->index association when we'll need to set an INT_MENU
control as I assume ph will want to say "set LINK_FREQ to 192000000"
and not "set LINK_FREQ to index 0")

--------- Digressions not related to this patch ----------------------
--------- Mostly for discussions and not blocking -------------------

A question remains when setting a menu control: where does the
value->index reversing happens ?

Applications/Android will tell "set TEST_PATTERN to
libcamera::controls::TestPatternXYZ" to the pipeline
handler that will receive a control list with

        { TestPatternMode, TestPatternModeOff }

and it will have to set it in the video device or the camera sensor.

For video devices, we have a pure v4l2 controls based interface, so
the ph will have to do the reversing:

        TestPatternModeOff = index 0

by itself as it knows its video devices and can create a map for that
purpose.

For camera sensor we have a v4l2 controls based interface, but that's
not really finalized, and if we want the ph to do the value->index
reversing it mean we'll have to expose the sensor db. Otherwise we
introduce a libcamera::Controls based interface in CameraSensor and
keep the reversing internal, but this ofc has other drawbacks (the
most notable will be the fact we'll have two interfaces (one v4l2, the
other libcamera) or we drop the v4l2 interface from CameraSensor
(which will require IPAs to speak libcamera::controls when they send
sensor's controls to the ph).

What do you think ?

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/controls.h |  5 +++++
>  src/libcamera/controls.cpp   | 22 ++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1a5690a5..a8deb16a 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -271,11 +271,15 @@ public:
>  			     const ControlValue &def = 0);
>  	explicit ControlInfo(Span<const ControlValue> values,
>  			     const ControlValue &def = {});
> +	explicit ControlInfo(Span<const ControlValue> values,
> +			     Span<const ControlValue> extraValues,
> +			     const ControlValue &def = {});
>
>  	const ControlValue &min() const { return min_; }
>  	const ControlValue &max() const { return max_; }
>  	const ControlValue &def() const { return def_; }
>  	const std::vector<ControlValue> &values() const { return values_; }
> +	const std::vector<ControlValue> &extraValues() const { return extraValues_; }
>
>  	std::string toString() const;
>
> @@ -294,6 +298,7 @@ private:
>  	ControlValue max_;
>  	ControlValue def_;
>  	std::vector<ControlValue> values_;
> +	std::vector<ControlValue> extraValues_;
>  };
>
>  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index c58ed394..e2e8619a 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -513,6 +513,28 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
>  		values_.push_back(value);
>  }
>
> +/**
> + * \brief Construct a ControlInfo from the list of valid values and extra values
> + * \param[in] values The control valid values
> + * \param[in] extraValues The control valid extra values associated with \a values
> + * \param[in] def The control default value
> + *
> + * Construct a ControlInfo from a list of valid values and extra values. The
> + * ControlInfo minimum and maximum values are set to the first and last members
> + * of the values list respectively. The default value is set to \a def if
> + * provided, or to the minimum value otherwise. The extra values are associated
> + * with \a values and in the same order as \a values.
> + *
> + */
> +ControlInfo::ControlInfo(Span<const ControlValue> values,
> +			 Span<const ControlValue> extraValues,
> +			 const ControlValue &def)
> +	: ControlInfo(values, def)
> +{
> +	for (const ControlValue &extraValue : extraValues)
> +		extraValues_.push_back(extraValue);
> +}
> +
>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel