| Message ID | 20210506075449.1761752-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > +{ > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Thu, May 6, 2021 at 6:11 PM Jacopo Mondi <jacopo@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
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@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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Thu, May 6, 2021 at 9:56 PM Jacopo Mondi <jacopo@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@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
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; } + /** * \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 + * \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. + * + * \return True on success or false otherwise */ +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl, + ControlInfo &ctrlInfo) +{ + 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++) { + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0) + continue; + + indices.emplace_back(static_cast<int32_t>(menu.index)); + } + + if (indices.empty()) { + LOG(V4L2, Error) + << "No applicable value: " << utils::hex(ctrl.id); + + return false; + } + + ctrlInfo = ControlInfo(indices); + + 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; + case ControlTypeByte: /* * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
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@chromium.org> --- include/libcamera/internal/v4l2_device.h | 3 ++ src/libcamera/v4l2_device.cpp | 65 +++++++++++++++++++++--- 2 files changed, 61 insertions(+), 7 deletions(-)