<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 6, 2021 at 9:56 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Thu, May 06, 2021 at 09:14:52PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo,<br>
><br>
> On Thu, May 6, 2021 at 6:11 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> ><br>
> > Currently we construct V4L2ControlInfo passing in a<br>
> > v4l2_query_ext_ctrl.<br>
> ><br>
> > Can't we do the same and detect if we have to enumerate indices if the<br>
> > control type is MENU, instead of creating a new function here ?<br>
> ><br>
> ><br>
> To do that, we need to pass fd to call QUERYMENU.<br>
<br>
Right, but as we switch on the type, passing to the constructor an<br>
additional parameter doesn't seem that bad.<br>
<br>
> I think the direction is opposite. I would remove V4L2CtrlInfo because it<br>
> just calls a few functions to construct ControlInfo.<br>
<br>
And we do set/get controls in v4l2 device already...<br>
<br>
I see your point, and I would not mind that, v4l2_controls.cpp/h is<br>
pretty slim at the moment and I think it can be moved to the<br>
v4l2_device. I don't see drawbacks, and I would be pleased to see<br>
v4l2_controls gone tbh<br>
<br>
> What do you think?<br>
<br>
I like the idea, but I think you should first move everything to the<br>
v4l2_device, remove v4l2_controls.cpp/h and then add support for menu.<br>
<br>
Please have this yak which needs a fresh shave it seems<br>
<br>
_,,,_<br>
.-'` ( '.<br>
.-' ,_ ; \___ _,<br>
__.' ) \'.__.'(:;'.__.'/<br>
__..--"" ( '.__{':');}__.'<br>
.' ( ; ( .-|` ' |-.<br>
/ ( ) ) '-p q-'<br>
( ; ; ; ; |.---.|<br>
) ( ( ; \ o o)<br>
| ) ; | ) ) /'.__/<br>
) ; ) ; | ; //<br>
( ) _,\ ; //<br>
; ( ,_,,-~""~`"" \ ( //<br>
\_.'\\_ '. /<_<br>
\\_)--\ \ \--\<br>
)--\""` )--\"`<br>
`""`<br>
<br>
Joking aside, I don't want to throw more issues to solve before<br>
addresing the issue at hand with v4l2 menu controls, but having some<br>
controls constructed here and some others in v4l2 controls feels<br>
weird...<br>
<br></blockquote><div><br></div><div>Alright, I submitted the patch series of removing the classes.</div><div>With that, I expect this code will not be weird.</div><div><br></div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> -Hiro<br>
><br>
> > +{<br>
> > > + ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU ||<br>
> > > + ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU);<br>
> > > +<br>
> > > + std::vector<ControlValue> indices;<br>
> > > + v4l2_querymenu menu;<br>
> > > + memset(&menu, 0, sizeof(menu));<br>
> > > + <a href="http://menu.id" rel="noreferrer" target="_blank">menu.id</a> = <a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>;<br>
> > > +<br>
> > > + for (menu.index = ctrl.minimum;<br>
> > > + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) {<br>
> ><br>
> > I was about to complain that you could loop until you don't get<br>
> > EINVAL, but the V4L2 documentation supports what you have here<br>
> ><br>
> > "It is possible for VIDIOC_QUERYMENU to return an EINVAL error code<br>
> > for some indices between minimum and maximum. In that case that<br>
> > particular menu item is not supported by this driver. Also note that<br>
> > the minimum value is not necessarily 0."<br>
> ><br>
> > > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)<br>
> > > + continue;<br>
> ><br>
> > I would however be loud if the return code is !-EINVAL<br>
> ><br>
> > > +<br>
> > > + indices.emplace_back(static_cast<int32_t>(menu.index));<br>
> ><br>
> > Has emplicing any benefit compared to push_back for POD like an<br>
> > int32_t ?<br>
> ><br>
> > > + }<br>
> > > +<br>
> > > + if (indices.empty()) {<br>
> > > + LOG(V4L2, Error)<br>
> > > + << "No applicable value: " << utils::hex(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>);<br>
> > > +<br>
> > > + return false;<br>
> > > + }<br>
> > > +<br>
> > > + ctrlInfo = ControlInfo(indices);<br>
> ><br>
> > Can't you return the ctrlInfo directly and exploit RVO ? or would it<br>
> > become difficult to detect errors ? I would howerv consider try moving<br>
> > this to the V4L2ControlInfo constructor first.<br>
> ><br>
> > > +<br>
> > > + return true;<br>
> > > +}<br>
> > ><br>
> > > /*<br>
> > > * \brief List and store information about all controls supported by the<br>
> > > @@ -473,7 +511,6 @@ void V4L2Device::listControls()<br>
> > > ControlInfoMap::Map ctrls;<br>
> > > struct v4l2_query_ext_ctrl ctrl = {};<br>
> > ><br>
> > > - /* \todo Add support for menu controls. */<br>
> > > while (1) {<br>
> > > <a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a> |= V4L2_CTRL_FLAG_NEXT_CTRL |<br>
> > > V4L2_CTRL_FLAG_NEXT_COMPOUND;<br>
> > > @@ -484,15 +521,22 @@ void V4L2Device::listControls()<br>
> > > ctrl.flags & V4L2_CTRL_FLAG_DISABLED)<br>
> > > continue;<br>
> > ><br>
> > > + ControlInfo ctrlInfo;<br>
> > > switch (ctrl.type) {<br>
> > > case V4L2_CTRL_TYPE_INTEGER:<br>
> > > case V4L2_CTRL_TYPE_BOOLEAN:<br>
> > > - case V4L2_CTRL_TYPE_MENU:<br>
> > > case V4L2_CTRL_TYPE_BUTTON:<br>
> > > case V4L2_CTRL_TYPE_INTEGER64:<br>
> > > case V4L2_CTRL_TYPE_BITMASK:<br>
> > > - case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> > > case V4L2_CTRL_TYPE_U8:<br>
> > > + ctrlInfo = V4L2ControlInfo(ctrl);<br>
> > > + break;<br>
> > > +<br>
> > > + case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> > > + case V4L2_CTRL_TYPE_MENU:<br>
> > > + if (!createControlInfoForMenu(ctrl, ctrlInfo))<br>
> > > + continue;<br>
> > > +<br>
> > > break;<br>
> > > /* \todo Support other control types. */<br>
> > > default:<br>
> > > @@ -502,10 +546,13 @@ void V4L2Device::listControls()<br>
> > > continue;<br>
> > > }<br>
> > ><br>
> > > + LOG(V4L2, Debug) << "Control: " << <a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a><br>
> > > + << " (" << utils::hex(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>) << ")";<br>
> > > +<br>
> > ><br>
> > controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));<br>
> > > controlInfo_.emplace(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, ctrl);<br>
> > ><br>
> > > - ctrls.emplace(controlIds_.back().get(),<br>
> > V4L2ControlInfo(ctrl));<br>
> > > + ctrls.emplace(controlIds_.back().get(), ctrlInfo);<br>
> > > }<br>
> > ><br>
> > > controls_ = std::move(ctrls);<br>
> > > @@ -535,6 +582,10 @@ void V4L2Device::updateControls(ControlList *ctrls,<br>
> > > value.set<int64_t>(v4l2Ctrl->value64);<br>
> > > break;<br>
> > ><br>
> > > + case ControlTypeInteger32:<br>
> > > + value.set<int32_t>(v4l2Ctrl->value);<br>
> > > + break;<br>
> > > +<br>
> ><br>
> > We have this here below:<br>
> ><br>
> > default:<br>
> > /*<br>
> > * \todo To be changed when support for string<br>
> > controls<br>
> > * will be added.<br>
> > */<br>
> > value.set<int32_t>(v4l2Ctrl->value);<br>
> > break;<br>
> ><br>
> > wasn't it enough ?<br>
> ><br>
> > Thanks<br>
> > j<br>
> ><br>
> > > case ControlTypeByte:<br>
> > > /*<br>
> > > * No action required, the VIDIOC_[GS]_EXT_CTRLS<br>
> > ioctl<br>
> > > --<br>
> > > 2.31.1.607.g51e8a6a459-goog<br>
> > ><br>
> > > _______________________________________________<br>
> > > libcamera-devel mailing list<br>
> > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div>