[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 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.

>
> 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
-------------- 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/1e58cdd4/attachment.sig>