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

Hirokazu Honda hiroh at chromium.org
Thu May 6 08:12:12 CEST 2021 

Hi Jacopo and Laurent,

On Fri, Apr 30, 2021 at 8:44 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> On Fri, Apr 30, 2021 at 10:58:10AM +0200, Jacopo Mondi wrote:
> > 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 ?
>
> Agreed on all that. The semantics of "extraValues()" is very
> ill-defined, and it would be hard to fix that. It's a hack, and we don't
> want hacks in the public API, we want a properly designed solution.
>
> Menu strings are not needed in ControlList, and ControlValue is mostly
> meant as the storage for variant entries in ControlList. It is also
> reused in ControlInfo, but that's not its main purpose. If we want to
> handle menu strings, they should go to ControlInfo directly. Even then,
> it would need to be done in a clean way from a public API point of view.
> In particular, the issue of internationalisation needs to be taken into
> account, so I'm really, really not keen to have menu strings, anywhere.
>
>
Laurent, is the implementation of this patch what you suggested me?
I will abandon this and re-implement with Jacopo's idea. But I would like
to make sure if I didn't miss something.

-Hiro


> > 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 ?
> >


I agreed.
It sounds like the current control implementation is enough for menu
because we should only store the index.


>
> > > 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
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210506/b8661cfa/attachment-0001.htm>

More information about the libcamera-devel mailing list