[libcamera-devel] [PATCH v8 2/6] libcamera: V4L2Device: Support v4l2 menu control
Hirokazu Honda
hiroh at chromium.org
Fri Jun 11 05:54:41 CEST 2021
Hi Jacopo,
On Fri, Jun 11, 2021 at 1:04 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Hiro,
>
> On Thu, Jun 10, 2021 at 05:25:35PM +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>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/internal/v4l2_device.h | 5 +
> > src/libcamera/v4l2_device.cpp | 186 +++++++++++++++--------
> > 2 files changed, 127 insertions(+), 64 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > index b82e2a14..c318e976 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -56,6 +56,11 @@ protected:
> > int fd() const { return fd_; }
> >
> > private:
> > + static ControlType v4l2CtrlType(uint32_t ctrlType);
> > + static std::unique_ptr<ControlId> v4l2ControlId(const
> v4l2_query_ext_ctrl &ctrl);
> > + ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
> > + 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 b39c6266..cda3769a 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -32,69 +32,6 @@ LOG_DEFINE_CATEGORY(V4L2)
> >
> > namespace {
> >
> > -ControlType v4l2CtrlType(uint32_t ctrlType)
> > -{
> > - switch (ctrlType) {
> > - case V4L2_CTRL_TYPE_U8:
> > - return ControlTypeByte;
> > -
> > - case V4L2_CTRL_TYPE_BOOLEAN:
> > - return ControlTypeBool;
> > -
> > - case V4L2_CTRL_TYPE_INTEGER:
> > - return ControlTypeInteger32;
> > -
> > - case V4L2_CTRL_TYPE_INTEGER64:
> > - return ControlTypeInteger64;
> > -
> > - case V4L2_CTRL_TYPE_MENU:
> > - case V4L2_CTRL_TYPE_BUTTON:
> > - case V4L2_CTRL_TYPE_BITMASK:
> > - case V4L2_CTRL_TYPE_INTEGER_MENU:
> > - /*
> > - * More precise types may be needed, for now use a 32-bit
> > - * integer type.
> > - */
> > - return ControlTypeInteger32;
> > -
> > - default:
> > - return ControlTypeNone;
> > - }
> > -}
> > -
> > -std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl
> &ctrl)
> > -{
> > - const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > - const std::string name(static_cast<const char *>(ctrl.name), len);
> > - const ControlType type = v4l2CtrlType(ctrl.type);
> > -
> > - return std::make_unique<ControlId>(ctrl.id, name, type);
> > -}
> > -
> > -ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> > -{
> > - switch (ctrl.type) {
> > - case V4L2_CTRL_TYPE_U8:
> > - return ControlInfo(static_cast<uint8_t>(ctrl.minimum),
> > - static_cast<uint8_t>(ctrl.maximum),
> > -
> static_cast<uint8_t>(ctrl.default_value));
> > -
> > - case V4L2_CTRL_TYPE_BOOLEAN:
> > - return ControlInfo(static_cast<bool>(ctrl.minimum),
> > - static_cast<bool>(ctrl.maximum),
> > - static_cast<bool>(ctrl.default_value));
> > -
> > - case V4L2_CTRL_TYPE_INTEGER64:
> > - return ControlInfo(static_cast<int64_t>(ctrl.minimum),
> > - static_cast<int64_t>(ctrl.maximum),
> > -
> static_cast<int64_t>(ctrl.default_value));
> > -
> > - default:
> > - return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > - static_cast<int32_t>(ctrl.maximum),
> > -
> static_cast<int32_t>(ctrl.default_value));
> > - }
> > -}
> > } /* namespace */
>
> Thanks for addressing my request, but now we have an empty namespace
> which could be dropped.
>
> I have a few more picky comments on the newly added documentation.
> With your ack they can be applied when merging.
>
> >
> > /**
> > @@ -524,6 +461,121 @@ int V4L2Device::ioctl(unsigned long request, void
> *argp)
> > * \return The V4L2 device file descriptor, -1 if the device node is
> not open
> > */
> >
> > +/**
> > + * \brief Retrieve ControlType for V4L2 control type
>
> Retrieve the libcamera control type associated with the V4L2 control type
>
> > + * \param[in] ctrlType V4L2 control type
> > + *
> > + * \return ControlType for \a ctrlType
>
> The ControlType corresponding to \a ctrlType
>
> > + */
> > +ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
> > +{
> > + switch (ctrlType) {
> > + case V4L2_CTRL_TYPE_U8:
> > + return ControlTypeByte;
> > +
> > + case V4L2_CTRL_TYPE_BOOLEAN:
> > + return ControlTypeBool;
> > +
> > + case V4L2_CTRL_TYPE_INTEGER:
> > + return ControlTypeInteger32;
> > +
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + return ControlTypeInteger64;
> > +
> > + case V4L2_CTRL_TYPE_MENU:
> > + case V4L2_CTRL_TYPE_BUTTON:
> > + case V4L2_CTRL_TYPE_BITMASK:
> > + case V4L2_CTRL_TYPE_INTEGER_MENU:
> > + /*
> > + * More precise types may be needed, for now use a 32-bit
> > + * integer type.
> > + */
> > + return ControlTypeInteger32;
> > +
> > + default:
> > + return ControlTypeNone;
> > + }
> > +}
> > +
> > +/**
> > + * \brief Create ControlId for v4l2_query_ext_ctrl
>
> \brief Create a ControlId for a V4L2 control
>
> > + * \param[in] ctrl v4l2_query_ext_ctrl
>
> Your below description sound better:
>
> \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control
>
> > + *
> > + * \return ControlId for \a ctrl
> > + */
> > +std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const
> v4l2_query_ext_ctrl &ctrl)
> > +{
> > + const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > + const std::string name(static_cast<const char *>(ctrl.name), len);
> > + const ControlType type = v4l2CtrlType(ctrl.type);
> > +
> > + return std::make_unique<ControlId>(ctrl.id, name, type);
> > +}
> > +
> > +/**
> > + * \brief Create ControlInfo from v4l2_query_ext_ctrl
> > + * \param[in] ctrl v4l2_query_ext_ctrl
>
> Your below description sound better:
>
> \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control
>
>
> > + *
> > + * \return ControlInfo from \a v4l2_query_ext_ctrl
>
> A ControlInfo created from \a ctrl
>
> > + */
> > +ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> > +{
> > + switch (ctrl.type) {
> > + case V4L2_CTRL_TYPE_U8:
> > + return ControlInfo(static_cast<uint8_t>(ctrl.minimum),
> > + static_cast<uint8_t>(ctrl.maximum),
> > +
> static_cast<uint8_t>(ctrl.default_value));
> > +
> > + case V4L2_CTRL_TYPE_BOOLEAN:
> > + return ControlInfo(static_cast<bool>(ctrl.minimum),
> > + static_cast<bool>(ctrl.maximum),
> > + static_cast<bool>(ctrl.default_value));
> > +
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + return ControlInfo(static_cast<int64_t>(ctrl.minimum),
> > + static_cast<int64_t>(ctrl.maximum),
> > +
> static_cast<int64_t>(ctrl.default_value));
> > +
> > + case V4L2_CTRL_TYPE_INTEGER_MENU:
> > + case V4L2_CTRL_TYPE_MENU:
> > + return v4l2MenuControlInfo(ctrl);
> > +
> > + default:
> > + return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > + static_cast<int32_t>(ctrl.maximum),
> > +
> static_cast<int32_t>(ctrl.default_value));
> > + }
> > +}
> > +
> > +/**
> > + * \brief Create ControlInfo for a V4L2 menu control
> > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
>
> .. a V4L2 menu control
>
> > + *
> > + * The created ControlInfo contains indices acquired by
> VIDIOC_QUERYMENU.
> > + *
> > + * \return ControlInfo for a V4L2 menu control
>
> I would apply the same suggestion given above
>
>
All of them sound good to me.
Thanks.
-Hiro
> > + */
> > +ControlInfo V4L2Device::v4l2MenuControlInfo(const struct
> v4l2_query_ext_ctrl &ctrl)
> > +{
> > + std::vector<ControlValue> indices;
> > + struct v4l2_querymenu menu = {};
> > + menu.id = ctrl.id;
> > +
> > + if (ctrl.minimum < 0)
> > + return ControlInfo();
> > +
> > + for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index)
> {
> > + menu.index = index;
> > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > + continue;
> > +
> > + indices.push_back(index);
> > + }
> > +
> > + return ControlInfo(indices,
> > +
> ControlValue(static_cast<int32_t>(ctrl.default_value)));
> > +}
> > +
> > /*
> > * \brief List and store information about all controls supported by the
> > * V4L2 device
> > @@ -533,7 +585,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;
> > @@ -562,6 +613,9 @@ void V4L2Device::listControls()
> > continue;
> > }
> >
> > + LOG(V4L2, Debug) << "Control: " << ctrl.name
> > + << " (" << utils::hex(ctrl.id) << ")";
> > +
> > controlIds_.emplace_back(v4l2ControlId(ctrl));
> > controlInfo_.emplace(ctrl.id, ctrl);
> >
> > @@ -630,6 +684,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> > value.set<int64_t>(v4l2Ctrl.value64);
> > break;
> >
> > + case ControlTypeInteger32:
> > + value.set<int32_t>(v4l2Ctrl.value);
> > + break;
> > +
> > case ControlTypeByte:
> > /*
> > * No action required, the VIDIOC_[GS]_EXT_CTRLS
> ioctl
> > --
> > 2.32.0.rc1.229.g3e70b5a671-goog
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210611/afa4cc65/attachment-0001.htm>
More information about the libcamera-devel
mailing list