| Message ID | 20210519075941.1337388-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Wed, May 19, 2021 at 04:59:37PM +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 | 64 ++++++++++++++++++++++-- > 2 files changed, 63 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index 5ba9b23b..34080b07 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -56,6 +56,9 @@ protected: > int fd() const { return fd_; } > > private: > + bool createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl, > + ControlInfo &ctrlInfo); > + We create ControlInfo from a v4l2_query_ext_ctrl with V4L2Device::v4l2ControlInfo() I would name this V4L2Device::v4l2MenuControlInfo() or maybe just expand V4L2Device::v4l2ControlInfo() with support for menu controls > 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 515cbdfe..9f7873f1 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -464,6 +464,49 @@ int V4L2Device::ioctl(unsigned long request, void *argp) > * \return The V4L2 device file descriptor, -1 if the device node is not open > */ > > +/** > + * \brief Create ControlInfo for v4l2 menu ctrl. No '.' at the end of briefs > + * \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. I don't think this applies in full anymore > + * > + * \return True on success or false otherwise > + */ > +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl, > + ControlInfo &ctrlInfo) ctrlInfo is an output parameter, a * is better suited > +{ > + ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU || > + ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU); This is internal code, the assertion makes only sure this patch does what you intend, does it bring any value ? > + > + std::vector<ControlValue> indices; > + v4l2_querymenu menu; > + memset(&menu, 0, sizeof(menu)); I don't recall if we already had a discussion about using menu{}; > + menu.id = ctrl.id; > + > + for (menu.index = ctrl.minimum; > + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) { Do you need the cast ? Here it compiles fine without (clang 11.1.0) > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0) > + continue; > + > + indices.emplace_back(static_cast<int32_t>(menu.index)); > + } > + > + if (indices.empty()) { Can this happen ? More in general, can this function fail at all ? I don't like we have ctrlInfo = v4l2ControlInfo(ctrl) and a few lines below if (!createControlInfoForMenu(ctrl, ctrlInfo); which are basically doing the same thing (create a ControlInfo from a v4l2_query_ext_ctrl) I would suggest either ctrlInfo = v4l2ControlInfo(ctrl) .... ctrlInfo = v4l2MenuControlInfo(ctrl) or just support menu in v4l2ControlInfo() > + 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 > * V4L2 device > @@ -473,7 +516,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 +526,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 +551,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); > @@ -572,6 +624,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.31.1.751.gd2f1c929bd-goog >
Hi Jacopo, thank you for reviewing. On Thu, May 27, 2021 at 5:58 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Hiro, > > On Wed, May 19, 2021 at 04:59:37PM +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 | 64 ++++++++++++++++++++++-- > > 2 files changed, 63 insertions(+), 4 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_device.h > b/include/libcamera/internal/v4l2_device.h > > index 5ba9b23b..34080b07 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -56,6 +56,9 @@ protected: > > int fd() const { return fd_; } > > > > private: > > + bool createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl, > > + ControlInfo &ctrlInfo); > > + > > We create ControlInfo from a v4l2_query_ext_ctrl with > V4L2Device::v4l2ControlInfo() > > I would name this V4L2Device::v4l2MenuControlInfo() or maybe just > expand V4L2Device::v4l2ControlInfo() with support for menu controls > > > 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 515cbdfe..9f7873f1 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -464,6 +464,49 @@ int V4L2Device::ioctl(unsigned long request, void > *argp) > > * \return The V4L2 device file descriptor, -1 if the device node is > not open > > */ > > > > +/** > > + * \brief Create ControlInfo for v4l2 menu ctrl. > > No '.' at the end of briefs > > > + * \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. > > I don't think this applies in full anymore > > > + * > > + * \return True on success or false otherwise > > + */ > > +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl > &ctrl, > > + ControlInfo &ctrlInfo) > > ctrlInfo is an output parameter, a * is better suited > > > +{ > > + ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU || > > + ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU); > > This is internal code, the assertion makes only sure this patch does > what you intend, does it bring any value ? > > > + > > + std::vector<ControlValue> indices; > > + v4l2_querymenu menu; > > + memset(&menu, 0, sizeof(menu)); > > I don't recall if we already had a discussion about using menu{}; > > I use memset for C structure. -Hiro > > + menu.id = ctrl.id; > > + > > + for (menu.index = ctrl.minimum; > > + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) { > > Do you need the cast ? Here it compiles fine without (clang 11.1.0) > > > > + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0) > > + continue; > > + > > + indices.emplace_back(static_cast<int32_t>(menu.index)); > > + } > > + > > + if (indices.empty()) { > > Can this happen ? > More in general, can this function fail at all ? > > I don't like we have > ctrlInfo = v4l2ControlInfo(ctrl) > and a few lines below > if (!createControlInfoForMenu(ctrl, ctrlInfo); > > which are basically doing the same thing (create a ControlInfo from a > v4l2_query_ext_ctrl) > > I would suggest either > ctrlInfo = v4l2ControlInfo(ctrl) > .... > ctrlInfo = v4l2MenuControlInfo(ctrl) > > or just support menu in v4l2ControlInfo() > > > > + 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 > > * V4L2 device > > @@ -473,7 +516,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 +526,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 +551,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); > > @@ -572,6 +624,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.31.1.751.gd2f1c929bd-goog > > >
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index 5ba9b23b..34080b07 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -56,6 +56,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 515cbdfe..9f7873f1 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -464,6 +464,49 @@ int V4L2Device::ioctl(unsigned long request, void *argp) * \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 * V4L2 device @@ -473,7 +516,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 +526,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 +551,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); @@ -572,6 +624,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 | 64 ++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 4 deletions(-)