Message ID | 20221124144553.3012369-1-kieran.bingham@ideasonboard.com |
---|---|
State | Accepted |
Commit | 96ed45b9711e5c70afc4f91eab0ed657ccfb9695 |
Headers | show |
Series |
|
Related | show |
Hi Kieran, Thank you for the patch. On Thu, Nov 24, 2022 at 02:45:53PM +0000, Kieran Bingham via libcamera-devel wrote: > Some UVC cameras have been identified that can provide V4L2 menu > controls without any menu items. > > This leads to a segfault where we try to construct a > ControlInfo(Span<>,default) with an empty span. > > Convert the v4l2ControlInfo and v4l2MenuControlInfo helper functions to > return std::optional<ControlInfo> to be able to account in the caller if > the control is valid, and only add acceptable controls to the supported > control list. > > Menu controls without a list of menu items are no longer added as a > valid control and a warning is logged. > > This also fixes a potential crash that would have occured in the > unlikely event that a ctrl.minimum was set to less than 0. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=167 > Reported-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > As I posted two v3 patches, I've gone straight to v5 on this. :-D > Now there's only the common path error report, and a couple of minor > text fixes. > > > include/libcamera/internal/v4l2_device.h | 4 ++-- > src/libcamera/v4l2_device.cpp | 28 +++++++++++++++++++----- > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index 75304be11f77..50d4adbc5f2b 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -70,8 +70,8 @@ protected: > private: > static ControlType v4l2CtrlType(uint32_t ctrlType); > static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl); > - ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl); > - ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl); > + std::optional<ControlInfo> v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl); > + std::optional<ControlInfo> v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl); > > void listControls(); > void updateControls(ControlList *ctrls, > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index c17b323f8af0..57a88d96b12c 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -529,7 +529,7 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl & > * \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control > * \return A ControlInfo that represents \a ctrl > */ > -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl) > +std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl) > { > switch (ctrl.type) { > case V4L2_CTRL_TYPE_U8: > @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl) > * > * \return A ControlInfo that represents \a ctrl > */ > -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > +std::optional<ControlInfo> V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > { > std::vector<ControlValue> indices; > struct v4l2_querymenu menu = {}; > menu.id = ctrl.id; > > if (ctrl.minimum < 0) > - return ControlInfo(); > + return std::nullopt; > > for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) { > menu.index = index; > @@ -583,6 +583,14 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct > indices.push_back(index); > } > > + /* > + * Some faulty UVC devices are known to return an empty menu control. > + * Controls without a menu option can not be set, or read, so they are > + * not exposed. > + */ > + if (indices.size() == 0) > + return std::nullopt; > + > return ControlInfo(indices, > ControlValue(static_cast<int32_t>(ctrl.default_value))); > } > @@ -631,7 +639,17 @@ void V4L2Device::listControls() > controlIdMap_[ctrl.id] = controlIds_.back().get(); > controlInfo_.emplace(ctrl.id, ctrl); > > - ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl)); > + std::optional<ControlInfo> info = v4l2ControlInfo(ctrl); > + > + if (!info) { > + LOG(V4L2, Error) > + << "Control " << ctrl.name > + << " cannot be registered"; > + > + continue; > + } > + > + ctrls.emplace(controlIds_.back().get(), *info); > } > > controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_); > @@ -670,7 +688,7 @@ void V4L2Device::updateControlInfo() > continue; > } > > - info = v4l2ControlInfo(ctrl); > + info = *v4l2ControlInfo(ctrl); > } > } >
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index 75304be11f77..50d4adbc5f2b 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -70,8 +70,8 @@ protected: private: static ControlType v4l2CtrlType(uint32_t ctrlType); static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl); - ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl); - ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl); + std::optional<ControlInfo> v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl); + std::optional<ControlInfo> v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl); void listControls(); void updateControls(ControlList *ctrls, diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index c17b323f8af0..57a88d96b12c 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -529,7 +529,7 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl & * \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control * \return A ControlInfo that represents \a ctrl */ -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl) +std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl) { switch (ctrl.type) { case V4L2_CTRL_TYPE_U8: @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl) * * \return A ControlInfo that represents \a ctrl */ -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl) +std::optional<ControlInfo> V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl) { std::vector<ControlValue> indices; struct v4l2_querymenu menu = {}; menu.id = ctrl.id; if (ctrl.minimum < 0) - return ControlInfo(); + return std::nullopt; for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) { menu.index = index; @@ -583,6 +583,14 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct indices.push_back(index); } + /* + * Some faulty UVC devices are known to return an empty menu control. + * Controls without a menu option can not be set, or read, so they are + * not exposed. + */ + if (indices.size() == 0) + return std::nullopt; + return ControlInfo(indices, ControlValue(static_cast<int32_t>(ctrl.default_value))); } @@ -631,7 +639,17 @@ void V4L2Device::listControls() controlIdMap_[ctrl.id] = controlIds_.back().get(); controlInfo_.emplace(ctrl.id, ctrl); - ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl)); + std::optional<ControlInfo> info = v4l2ControlInfo(ctrl); + + if (!info) { + LOG(V4L2, Error) + << "Control " << ctrl.name + << " cannot be registered"; + + continue; + } + + ctrls.emplace(controlIds_.back().get(), *info); } controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_); @@ -670,7 +688,7 @@ void V4L2Device::updateControlInfo() continue; } - info = v4l2ControlInfo(ctrl); + info = *v4l2ControlInfo(ctrl); } }