[libcamera-devel] [PATCH v6 2/6] libcamera: V4L2Device: Support v4l2 menu control
Hirokazu Honda
hiroh at chromium.org
Mon Jun 7 02:54:25 CEST 2021
Hi Laurent,
On Mon, Jun 7, 2021 at 9:46 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Hiro,
>
> On Mon, Jun 07, 2021 at 09:14:21AM +0900, Hirokazu Honda wrote:
> > On Mon, Jun 7, 2021 at 8:22 AM Laurent Pinchart wrote:
> > > On Fri, May 28, 2021 at 12:05:27PM +0900, Hirokazu Honda wrote:
> > > > This adds a support of v4l2 menu. The control info for v4l2 menu
> > > > contains indices without names and 64bit values of querymenu.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > > include/libcamera/internal/v4l2_device.h | 2 +
> > > > src/libcamera/v4l2_device.cpp | 47
> ++++++++++++++++++++++--
> > > > 2 files changed, 45 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > > > index b82e2a14..687be9b2 100644
> > > > --- a/include/libcamera/internal/v4l2_device.h
> > > > +++ b/include/libcamera/internal/v4l2_device.h
> > > > @@ -56,6 +56,8 @@ protected:
> > > > int fd() const { return fd_; }
> > > >
> > > > private:
> > > > + ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl
> &ctrl);
> > > > +
> > > > void listControls();
> > > > void updateControls(ControlList *ctrls,
> > > > Span<const v4l2_ext_control> v4l2Ctrls);
> > > > diff --git a/src/libcamera/v4l2_device.cpp
> b/src/libcamera/v4l2_device.cpp
> > > > index caafbc2d..032252bb 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -530,6 +530,33 @@ int V4L2Device::ioctl(unsigned long request,
> void *argp)
> > > > * \return The V4L2 device file descriptor, -1 if the device node
> is not open
> > > > */
> > > >
> > > > +/**
> > > > + * \brief Create ControlInfo for v4l2 menu ctrl
> > >
> > > s/for v4l2 menu ctrl/for a V4L2 menu control/
> > >
> > > > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> > > > + * \param[out] ctrlInfo The created controlInfo
> > >
> > > It's not an out parameter, it's the return value.
> > >
> > > > + *
> > > > + * The created ControlInfo contains indices acquired by
> VIDIOC_QUERYMENU.
> > > > + *
> > > > + * \return ControlInfo for v4l2 menu ctrl.
> > > > + */
> > > > +ControlInfo V4L2Device::v4l2MenuControlInfo(const
> v4l2_query_ext_ctrl &ctrl)
> > > > +{
> > > > + std::vector<ControlValue> indices;
> > > > + v4l2_querymenu menu;
> > >
> > > We usually prefix the V4L2 structures with struct. While not strictly
> > > mandatory, it emphasizes that there are C structs from V4L2.
> > >
> > > > + memset(&menu, 0, sizeof(menu));
> > >
> > > You could also write this
> > >
> > > struct v4l2_querymenu menu = { };
> > >
> > > (I really wish designated initializers were available before C++20
> :-().
> > >
> > > > + menu.id = ctrl.id;
> > >
> > > As a sanity check, should we add
> > >
> > > if (ctrl.minimum <= 0)
> > > return ControlInfo();
> > >
> > > ?
> > >
> > > > +
> > > > + for (menu.index = ctrl.minimum;
> > > > + static_cast<int32_t>(menu.index) <= ctrl.maximum;
> menu.index++) {
> > >
> > > for (int32_t index = ctrl.minimum; index <= ctrl.maximum;
> ++index)
> > > }
> > > menu.index = index;
> > >
> > > would allow dropping the cast. Up to you.
> > >
> > > > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > > > + continue;
> > > > +
> > > > + indices.emplace_back(static_cast<int32_t>(menu.index));
> > >
> > > This could be
> > >
> > > indices.emplace_back(index);
> > >
> > > or even
> > >
> > > indices.push_back(index);
> > >
> > > as it won't make much of a difference.
> > >
> > > > + }
> > > > +
> > > > + return ControlInfo(indices);
> > >
> > > Would it be useful to also pass the default value to the ControlInfo
> > > constructor ?
> >
> > You mean, I should set ctrl.default_value or some value in indices like
> > indices.begin()?
>
> I mean (untested)
>
> return ControlInfo(indices, ControlValue<int32_t>(ctrl.dev));
>
>
I guess you mean ctrl.default_value. Filled so if querymenu for the value
is successful.
> > > > +}
> > > > +
> > > > /*
> > > > * \brief List and store information about all controls supported
> by the
> > > > * V4L2 device
> > > > @@ -539,7 +566,6 @@ void V4L2Device::listControls()
> > > > ControlInfoMap::Map ctrls;
> > > > struct v4l2_query_ext_ctrl ctrl = {};
> > > >
> > > > - /* \todo Add support for menu controls. */
> > > > while (1) {
> > > > ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL |
> > > > V4L2_CTRL_FLAG_NEXT_COMPOUND;
> > > > @@ -550,16 +576,22 @@ void V4L2Device::listControls()
> > > > ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> > > > continue;
> > > >
> > > > + ControlInfo ctrlInfo;
> > >
> > > I'd add a blank line here.
> > >
> > > > switch (ctrl.type) {
> > > > case V4L2_CTRL_TYPE_INTEGER:
> > > > case V4L2_CTRL_TYPE_BOOLEAN:
> > > > - case V4L2_CTRL_TYPE_MENU:
> > > > case V4L2_CTRL_TYPE_BUTTON:
> > > > case V4L2_CTRL_TYPE_INTEGER64:
> > > > case V4L2_CTRL_TYPE_BITMASK:
> > > > - case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > > case V4L2_CTRL_TYPE_U8:
> > > > + ctrlInfo = v4l2ControlInfo(ctrl);
> > > > + break;
> > > > +
> > > > + case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > > + case V4L2_CTRL_TYPE_MENU:
> > > > + ctrlInfo = v4l2MenuControlInfo(ctrl);
> > > > break;
> > > > +
> > > > /* \todo Support other control types. */
> > > > default:
> > > > LOG(V4L2, Debug)
> > > > @@ -568,10 +600,13 @@ void V4L2Device::listControls()
> > > > continue;
> > > > }
> > > >
> > > > + LOG(V4L2, Debug) << "Control: " << ctrl.name
> > > > + << " (" << utils::hex(ctrl.id) <<
> ")";
> > > > +
> > > > controlIds_.emplace_back(v4l2ControlId(ctrl));
> > > > controlInfo_.emplace(ctrl.id, ctrl);
> > > >
> > > > - ctrls.emplace(controlIds_.back().get(),
> v4l2ControlInfo(ctrl));
> > > > + ctrls.emplace(controlIds_.back().get(),
> std::move(ctrlInfo));
> > > > }
> > > >
> > > > controls_ = std::move(ctrls);
> > > > @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList
> *ctrls,
> > > > value.set<int64_t>(v4l2Ctrl->value64);
> > > > break;
> > > >
> > > > + case ControlTypeInteger32:
> > > > + value.set<int32_t>(v4l2Ctrl->value);
> > >
> > > This needs a rebase, it's now v4l2Ctrl.value.
> >
> > By which patch? On top of tree, v4l2Ctrl is still a const pointer.
>
> By commit abdb11d9ccad ("libcamera: V4L2Device: Remove the controls
> order assumption in updateControls()") :-)
>
>
Okay, I saw it was merged just while ago. Thanks for merging.
-Hiro
> > > With these minor issues addressed, the patch looks good to me. Thanks
> > > for your continuous effort.
> > >
> > > > + break;
> > > > +
> > > > case ControlTypeByte:
> > > > /*
> > > > * No action required, the
> VIDIOC_[GS]_EXT_CTRLS ioctl
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210607/c1f3f1f6/attachment-0001.htm>
More information about the libcamera-devel
mailing list