[libcamera-devel] [PATCH v4 2/6] libcamera: V4L2Device: Support v4l2 menu control
Hirokazu Honda
hiroh at chromium.org
Thu May 6 14:14:52 CEST 2021
Hi Jacopo,
On Thu, May 6, 2021 at 6:11 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Hiro,
>
> On Thu, May 06, 2021 at 04:54:45PM +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 | 3 ++
> > src/libcamera/v4l2_device.cpp | 65 +++++++++++++++++++++---
> > 2 files changed, 61 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > index 087f07e7..c7a2539c 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -54,6 +54,9 @@ protected:
> > int fd() const { return fd_; }
> >
> > private:
> > + bool createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,
> > + ControlInfo &ctrlInfo);
> > +
> > 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 397029ac..7056c71a 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -452,6 +452,7 @@ int V4L2Device::ioctl(unsigned long request, void
> *argp)
> > return 0;
> > }
> >
> > +
>
> Addional empty line
>
> Doesn't checkstyle.py warn you ?
>
> > /**
> > * \fn V4L2Device::deviceNode()
> > * \brief Retrieve the device node path
> > @@ -459,10 +460,47 @@ int V4L2Device::ioctl(unsigned long request, void
> *argp)
> > */
> >
> > /**
> > - * \fn V4L2Device::fd()
> > - * \brief Retrieve the V4L2 device file descriptor
> > - * \return The V4L2 device file descriptor, -1 if the device node is
> not open
>
> Where has the fd() documentation gone ?
>
> > + * \brief Create ControlInfo for v4l2 menu ctrl.
> > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> > + * \param[out] ctrlInfo The created controlInfo
> > + *
> > + * The created ControlInfo contains not only values and also extra
> values, which
> > + * are indices and name/value acquired by VIDIOC_QUERYMENU,
> respectively. The
> > + * extra values contains std::string if the type of \a ctrl is
> > + * V4L2_CTRL_TYPE_MENU or int64_t otherwise.
>
> This part of the documentation doesn't apply anymore
>
> > + *
> > + * \return True on success or false otherwise
> > */
> > +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl
> &ctrl,
> > + ControlInfo &ctrlInfo)
>
> Currently we construct V4L2ControlInfo passing in a
> v4l2_query_ext_ctrl.
>
> Can't we do the same and detect if we have to enumerate indices if the
> control type is MENU, instead of creating a new function here ?
>
>
To do that, we need to pass fd to call QUERYMENU.
I think the direction is opposite. I would remove V4L2CtrlInfo because it
just calls a few functions to construct ControlInfo.
What do you think?
-Hiro
> +{
> > + ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU ||
> > + ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU);
> > +
> > + std::vector<ControlValue> indices;
> > + v4l2_querymenu menu;
> > + memset(&menu, 0, sizeof(menu));
> > + menu.id = ctrl.id;
> > +
> > + for (menu.index = ctrl.minimum;
> > + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) {
>
> I was about to complain that you could loop until you don't get
> EINVAL, but the V4L2 documentation supports what you have here
>
> "It is possible for VIDIOC_QUERYMENU to return an EINVAL error code
> for some indices between minimum and maximum. In that case that
> particular menu item is not supported by this driver. Also note that
> the minimum value is not necessarily 0."
>
> > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > + continue;
>
> I would however be loud if the return code is !-EINVAL
>
> > +
> > + indices.emplace_back(static_cast<int32_t>(menu.index));
>
> Has emplicing any benefit compared to push_back for POD like an
> int32_t ?
>
> > + }
> > +
> > + if (indices.empty()) {
> > + LOG(V4L2, Error)
> > + << "No applicable value: " << utils::hex(ctrl.id);
> > +
> > + return false;
> > + }
> > +
> > + ctrlInfo = ControlInfo(indices);
>
> Can't you return the ctrlInfo directly and exploit RVO ? or would it
> become difficult to detect errors ? I would howerv consider try moving
> this to the V4L2ControlInfo constructor first.
>
> > +
> > + return true;
> > +}
> >
> > /*
> > * \brief List and store information about all controls supported by the
> > @@ -473,7 +511,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;
> > @@ -484,15 +521,22 @@ void V4L2Device::listControls()
> > ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> > continue;
> >
> > + ControlInfo ctrlInfo;
> > 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:
> > + if (!createControlInfoForMenu(ctrl, ctrlInfo))
> > + continue;
> > +
> > break;
> > /* \todo Support other control types. */
> > default:
> > @@ -502,10 +546,13 @@ void V4L2Device::listControls()
> > continue;
> > }
> >
> > + LOG(V4L2, Debug) << "Control: " << ctrl.name
> > + << " (" << utils::hex(ctrl.id) << ")";
> > +
> >
> controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));
> > controlInfo_.emplace(ctrl.id, ctrl);
> >
> > - ctrls.emplace(controlIds_.back().get(),
> V4L2ControlInfo(ctrl));
> > + ctrls.emplace(controlIds_.back().get(), ctrlInfo);
> > }
> >
> > controls_ = std::move(ctrls);
> > @@ -535,6 +582,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> > value.set<int64_t>(v4l2Ctrl->value64);
> > break;
> >
> > + case ControlTypeInteger32:
> > + value.set<int32_t>(v4l2Ctrl->value);
> > + break;
> > +
>
> We have this here below:
>
> default:
> /*
> * \todo To be changed when support for string
> controls
> * will be added.
> */
> value.set<int32_t>(v4l2Ctrl->value);
> break;
>
> wasn't it enough ?
>
> Thanks
> j
>
> > case ControlTypeByte:
> > /*
> > * No action required, the VIDIOC_[GS]_EXT_CTRLS
> ioctl
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210506/4fe0e752/attachment-0001.htm>
More information about the libcamera-devel
mailing list