[libcamera-devel] [PATCH v4 2/6] libcamera: V4L2Device: Support v4l2 menu control
Jacopo Mondi
jacopo at jmondi.org
Thu May 6 14:56:56 CEST 2021
Hi Hiro,
On Thu, May 06, 2021 at 09:14:52PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Thu, May 6, 2021 at 6:11 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> >
> > 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.
Right, but as we switch on the type, passing to the constructor an
additional parameter doesn't seem that bad.
> I think the direction is opposite. I would remove V4L2CtrlInfo because it
> just calls a few functions to construct ControlInfo.
And we do set/get controls in v4l2 device already...
I see your point, and I would not mind that, v4l2_controls.cpp/h is
pretty slim at the moment and I think it can be moved to the
v4l2_device. I don't see drawbacks, and I would be pleased to see
v4l2_controls gone tbh
> What do you think?
I like the idea, but I think you should first move everything to the
v4l2_device, remove v4l2_controls.cpp/h and then add support for menu.
Please have this yak which needs a fresh shave it seems
_,,,_
.-'` ( '.
.-' ,_ ; \___ _,
__.' ) \'.__.'(:;'.__.'/
__..--"" ( '.__{':');}__.'
.' ( ; ( .-|` ' |-.
/ ( ) ) '-p q-'
( ; ; ; ; |.---.|
) ( ( ; \ o o)
| ) ; | ) ) /'.__/
) ; ) ; | ; //
( ) _,\ ; //
; ( ,_,,-~""~`"" \ ( //
\_.'\\_ '. /<_
\\_)--\ \ \--\
)--\""` )--\"`
`""`
Joking aside, I don't want to throw more issues to solve before
addresing the issue at hand with v4l2 menu controls, but having some
controls constructed here and some others in v4l2 controls feels
weird...
>
> -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
> >
More information about the libcamera-devel
mailing list