[libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls: Construct from a list of ids

[Jacopo Mondi]

Hi Niklas,

On Thu, Aug 29, 2019 at 11:08:25AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2019-08-29 10:56:57 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote:
> > > On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote:
> > > > Hi Niklas,
> > > >
> > > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thanks for your work.
> > > > >
> > > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:
> > > > > > Add a constructor for the V4L2ControlList class that accepts a list of
> > > > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList
> > > > > > instance to be used for reading controls only.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > ---
> > > > > >  src/libcamera/include/v4l2_controls.h |  3 +++
> > > > > >  src/libcamera/v4l2_controls.cpp       | 17 +++++++++++++++++
> > > > > >  2 files changed, 20 insertions(+)
> > > > > >
> > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > > > > > index 10b726504951..86c84e06d741 100644
> > > > > > --- a/src/libcamera/include/v4l2_controls.h
> > > > > > +++ b/src/libcamera/include/v4l2_controls.h
> > > > > > @@ -65,6 +65,9 @@ public:
> > > > > >  	using iterator = std::vector<V4L2Control>::iterator;
> > > > > >  	using const_iterator = std::vector<V4L2Control>::const_iterator;
> > > > > >
> > > > > > +	V4L2ControlList() {}
> > > > > > +	V4L2ControlList(std::vector<unsigned int> ids);
> > > > > > +
> > > > > >  	iterator begin() { return controls_.begin(); }
> > > > > >  	const_iterator begin() const { return controls_.begin(); }
> > > > > >  	iterator end() { return controls_.end(); }
> > > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > > > > > index 84258d9954d0..eeb325cbfff9 100644
> > > > > > --- a/src/libcamera/v4l2_controls.cpp
> > > > > > +++ b/src/libcamera/v4l2_controls.cpp
> > > > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > > > > >   * and prepare to be re-used for a new control write/read sequence.
> > > > > >   */
> > > > > >
> > > > > > +/**
> > > > > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers
> > > > > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*)
> > > > > > + *
> > > > > > + * Construct a V4L2ControlList from a list of control identifiers without any
> > > > > > + * value associated. This constructor is particularly useful to create a
> > > > > > + * V4L2ControlList that is used to read the values of all controls in the
> > > > > > + * \a ids list. The created V4L2ControlList should not be used to write control
> > > > > > + * values unless it is cleared first and then controls with an associated value
> > > > > > + * are manually added to it.
> > > > > > + */
> > > > >
> > > > > Do you think it would be worth it to add some code to enforce that no
> > > > > controls can be written if it's initialized with this constructor?
> > > > >
> > > >
> > > > How would you do so? The most naive implementation that comes to mind
> > > > it's to add a 'writable' flag to initialize to true only if the
> > > > control has a value assigned and check for the flag
> > > > V4L2Device::setControls()
> > >
> > > I'm not sure how to best implement it, but the constraint is so specific
> > > it looked like something that could be described in code. Now that I dig
> > > thru the code with fresh eyes I can't find the rational for the
> > > V4L2ControlList needs to be cleared before it's attempted to be used for
> > > writing.
> > >
> > > The control values are initialized to 0 using this new constructor and
> > > if used for writing V4L2Device::setControls() will validate that the
> > > control exists on the device and bail before writing anything if one the
> > > controls do not exist.
> >
> > Yes, it validates the control exists
> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251
> >
> > but it will silently set the value of the v4l2 control to write to 0
> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265
> >
> > One could
> >         V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... };
> >         device->setControls(&list);
> >
> > and if the controls in the list accept 0 as a value, set all controls
> > to 0 maybe unintentionally.
>
> Absolutely, that's what I'm saying :-) We already have this feature with
> the following code:
>
>      V4L2ControlList list;
>      list.add(V4L2_CID_...);
>      list.add(V4L2_CID_...);
>      device->setControls(&list);
>
> So this new constructor is just a more convenient way to do the above
> and I see no real reason to block this use-case as the behaviour is well
> defined. If V4L2ControlList::add() value argument did not have an
> default value set I agree this would be a slightly different change.
>
> If we add logic to prevent a V4L2ControlList initialized with this new
> constructor from being written we should also block V4L2ControlList
> where controls where added with just an id an no explicit value from
> being written.
>

Indeed, this applies to all controls added without a value, not just
the lists constructed with the newly introduced constructor, which is
just a shortcut to what you have here described...

> >
> > >
> > > What reason do you think there is for the need to manually add controls
> > > with a specific value before writing? This is just a specialisation
> > > where all values are initialized to 0, at least in my mind I might be
> > > missing something ;-)
> > >
> > We have no guarantee the controls in the list accept 0, or that they
> > even accepts integers at all (even if, we only support integers
> > controls at the moment).
> >
> > We cannot prevent anyone from shooting on his own foot, but to
> > me documenting that the list should only be used for reading, or
> > enforcing it in the code as you suggested, is a good thing...
> >
> > > >
> > > > > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids)
> > > > > > +{
> > > > > > +	for (auto id : ids)
> > > > > > +		add(id);
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * \typedef V4L2ControlList::iterator
> > > > > >   * \brief Iterator on the V4L2 controls contained in the instance
> > > > > > --
> > > > > > 2.23.0
> > > > > >
> > > > > > _______________________________________________
> > > > > > libcamera-devel mailing list
> > > > > > libcamera-devel at lists.libcamera.org
> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Niklas Söderlund
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190829/14ae4469/attachment-0001.sig>