[libcamera-devel] [PATCH v4 2/6] libcamera: V4L2Device: Support v4l2 menu control
Hirokazu Honda
hiroh at chromium.org
Fri May 7 04:19:13 CEST 2021
Hi Jacopo,
On Thu, May 6, 2021 at 9:56 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> 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...
>
>
Alright, I submitted the patch series of removing the classes.
With that, I expect this code will not be weird.
-Hiro
>
> > -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/20210507/563eb598/attachment-0001.htm>
More information about the libcamera-devel
mailing list