[libcamera-devel] [PATCH 06/10] libcamera: v4l2_controls: Replace V4L2ControlInfo with V4L2ControlRange
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 15 21:18:52 CEST 2019
Hi Jacopo,
On Tue, Oct 15, 2019 at 07:50:10PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 14, 2019 at 02:27:52AM +0300, Laurent Pinchart wrote:
> > The V4L2ControlInfo class only stores a ControlRange. Make it inherit
> > from ControlRange to provide a convenience constructor from a struct
> > v4l2_query_ext_ctrl and rename it to V4L2ControlRange.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/rkisp1.cpp | 8 ++---
> > src/libcamera/include/v4l2_controls.h | 43 ++++++++++++---------------
> > src/libcamera/pipeline/uvcvideo.cpp | 4 +--
> > src/libcamera/pipeline/vimc.cpp | 4 +--
> > src/libcamera/v4l2_controls.cpp | 30 ++++++++-----------
> > src/libcamera/v4l2_device.cpp | 2 +-
> > test/v4l2_videodevice/controls.cpp | 24 +++++++--------
> > 7 files changed, 52 insertions(+), 63 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 13059d99036c..d64c334cbf0c 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -83,12 +83,12 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> >
> > autoExposure_ = true;
> >
> > - minExposure_ = std::max<uint32_t>(itExp->second.range().min().get<int32_t>(), 1);
> > - maxExposure_ = itExp->second.range().max().get<int32_t>();
> > + minExposure_ = std::max<uint32_t>(itExp->second.min().get<int32_t>(), 1);
> > + maxExposure_ = itExp->second.max().get<int32_t>();
> > exposure_ = minExposure_;
> >
> > - minGain_ = std::max<uint32_t>(itGain->second.range().min().get<int32_t>(), 1);
> > - maxGain_ = itGain->second.range().max().get<int32_t>();
> > + minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1);
> > + maxGain_ = itGain->second.max().get<int32_t>();
> > gain_ = minGain_;
> >
> > LOG(IPARkISP1, Info)
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > index ca7217501256..741569824b68 100644
> > --- a/src/libcamera/include/v4l2_controls.h
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -25,38 +25,33 @@ public:
> > V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);
> > };
> >
> > -class V4L2ControlInfo
> > +class V4L2ControlRange : public ControlRange
> > {
> > public:
> > - V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> > -
> > - const ControlRange &range() const { return range_; }
> > -
> > -private:
> > - ControlRange range_;
> > + V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl);
> > };
> >
> > -class V4L2ControlInfoMap : private std::map<const ControlId *, V4L2ControlInfo>
> > +class V4L2ControlInfoMap : private std::map<const ControlId *, V4L2ControlRange>
> > {
> > public:
> > - V4L2ControlInfoMap &operator=(std::map<const ControlId *, V4L2ControlInfo> &&info);
> > + V4L2ControlInfoMap &operator=(std::map<const ControlId *, V4L2ControlRange> &&info);
> >
> > - using std::map<const ControlId *, V4L2ControlInfo>::key_type;
> > - using std::map<const ControlId *, V4L2ControlInfo>::mapped_type;
> > - using std::map<const ControlId *, V4L2ControlInfo>::value_type;
> > - using std::map<const ControlId *, V4L2ControlInfo>::size_type;
> > - using std::map<const ControlId *, V4L2ControlInfo>::iterator;
> > - using std::map<const ControlId *, V4L2ControlInfo>::const_iterator;
> > + using std::map<const ControlId *, V4L2ControlRange>::key_type;
> > + using std::map<const ControlId *, V4L2ControlRange>::mapped_type;
> > + using std::map<const ControlId *, V4L2ControlRange>::value_type;
> > + using std::map<const ControlId *, V4L2ControlRange>::size_type;
> > + using std::map<const ControlId *, V4L2ControlRange>::iterator;
> > + using std::map<const ControlId *, V4L2ControlRange>::const_iterator;
> >
> > - using std::map<const ControlId *, V4L2ControlInfo>::begin;
> > - using std::map<const ControlId *, V4L2ControlInfo>::cbegin;
> > - using std::map<const ControlId *, V4L2ControlInfo>::end;
> > - using std::map<const ControlId *, V4L2ControlInfo>::cend;
> > - using std::map<const ControlId *, V4L2ControlInfo>::at;
> > - using std::map<const ControlId *, V4L2ControlInfo>::empty;
> > - using std::map<const ControlId *, V4L2ControlInfo>::size;
> > - using std::map<const ControlId *, V4L2ControlInfo>::count;
> > - using std::map<const ControlId *, V4L2ControlInfo>::find;
> > + using std::map<const ControlId *, V4L2ControlRange>::begin;
> > + using std::map<const ControlId *, V4L2ControlRange>::cbegin;
> > + using std::map<const ControlId *, V4L2ControlRange>::end;
> > + using std::map<const ControlId *, V4L2ControlRange>::cend;
> > + using std::map<const ControlId *, V4L2ControlRange>::at;
> > + using std::map<const ControlId *, V4L2ControlRange>::empty;
> > + using std::map<const ControlId *, V4L2ControlRange>::size;
> > + using std::map<const ControlId *, V4L2ControlRange>::count;
> > + using std::map<const ControlId *, V4L2ControlRange>::find;
> >
> > mapped_type &at(unsigned int key);
> > const mapped_type &at(unsigned int key) const;
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 4d76b5fd9347..7356585b982e 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -337,7 +337,7 @@ int UVCCameraData::init(MediaEntity *entity)
> > /* Initialise the supported controls. */
> > const V4L2ControlInfoMap &controls = video_->controls();
> > for (const auto &ctrl : controls) {
> > - const V4L2ControlInfo &info = ctrl.second;
> > + const V4L2ControlRange &range = ctrl.second;
> > const ControlId *id;
> >
> > switch (ctrl.first->id()) {
> > @@ -362,7 +362,7 @@ int UVCCameraData::init(MediaEntity *entity)
> >
> > controlInfo_.emplace(std::piecewise_construct,
> > std::forward_as_tuple(id),
> > - std::forward_as_tuple(info.range()));
> > + std::forward_as_tuple(range));
> > }
> >
> > return 0;
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 78c0fe5ae509..87e7e54e964f 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -413,7 +413,7 @@ int VimcCameraData::init(MediaDevice *media)
> > /* Initialise the supported controls. */
> > const V4L2ControlInfoMap &controls = sensor_->controls();
> > for (const auto &ctrl : controls) {
> > - const V4L2ControlInfo &info = ctrl.second;
> > + const V4L2ControlRange &range = ctrl.second;
> > const ControlId *id;
> >
> > switch (ctrl.first->id()) {
> > @@ -432,7 +432,7 @@ int VimcCameraData::init(MediaDevice *media)
> >
> > controlInfo_.emplace(std::piecewise_construct,
> > std::forward_as_tuple(id),
> > - std::forward_as_tuple(info.range()));
> > + std::forward_as_tuple(range));
> > }
> >
> > return 0;
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 9a5e4830db91..4aab34b9a2b4 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -104,14 +104,14 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> > }
> >
> > /**
> > - * \class V4L2ControlInfo
> > + * \class V4L2ControlRange
> > * \brief Information on a V4L2 control
> > *
> > - * The V4L2ControlInfo class represents all the information related to a V4L2
> > + * The V4L2ControlRange class represents all the information related to a V4L2
> > * control, such as its ID, its type, its user-readable name and the expected
> > * size of its value data.
>
> This is not actual anymore I guess
>
> Trying to re-use the ControlRange documentation:
>
> /**
> * \class V4L2ControlRange
> * \brief Describe the limits of valid values for a V4L2 control
> *
> * The V4L2 ControlRange expresses the constraints on valid values for a
> * V4L2 control. A V4L2ControlRange is created inspecting the fields
> * of a struct v4l2_query_ext_ctrl as returned by the kernel.
> */
Oops. I'll update the documentation with
* The V4L2ControlRange class is a specialisation of the ControlRange for V4L2
* controls.
to match V4L2ControlId.
> > *
> > - * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> > + * V4L2ControlRange instances are created by inspecting the fieldS of a struct
>
> s/fieldS/fields/
>
> With the comment updated and the the small nit fixed
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > * v4l2_query_ext_ctrl structure, after it has been filled by the device driver
> > * as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> > *
> > @@ -121,28 +121,22 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> > */
> >
> > /**
> > - * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > + * \brief Construct a V4L2ControlRange from a struct v4l2_query_ext_ctrl
> > * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > */
> > -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > +V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
> > {
> > if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > - range_ = ControlRange(static_cast<int64_t>(ctrl.minimum),
> > - static_cast<int64_t>(ctrl.maximum));
> > + ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
> > + static_cast<int64_t>(ctrl.maximum)));
> > else
> > - range_ = ControlRange(static_cast<int32_t>(ctrl.minimum),
> > - static_cast<int32_t>(ctrl.maximum));
> > + ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
> > + static_cast<int32_t>(ctrl.maximum)));
> > }
> >
> > -/**
> > - * \fn V4L2ControlInfo::range()
> > - * \brief Retrieve the control value range
> > - * \return The V4L2 control value range
> > - */
> > -
> > /**
> > * \class V4L2ControlInfoMap
> > - * \brief A map of controlID to V4L2ControlInfo
> > + * \brief A map of controlID to V4L2ControlRange
> > */
> >
> > /**
> > @@ -156,9 +150,9 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > *
> > * \return The populated V4L2ControlInfoMap
> > */
> > -V4L2ControlInfoMap &V4L2ControlInfoMap::operator=(std::map<const ControlId *, V4L2ControlInfo> &&info)
> > +V4L2ControlInfoMap &V4L2ControlInfoMap::operator=(std::map<const ControlId *, V4L2ControlRange> &&info)
> > {
> > - std::map<const ControlId *, V4L2ControlInfo>::operator=(std::move(info));
> > + std::map<const ControlId *, V4L2ControlRange>::operator=(std::move(info));
> >
> > idmap_.clear();
> > for (const auto &ctrl : *this)
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 4bb7d5950f3a..133f8abc8f13 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -342,7 +342,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> > */
> > void V4L2Device::listControls()
> > {
> > - std::map<const ControlId *, V4L2ControlInfo> ctrls;
> > + std::map<const ControlId *, V4L2ControlRange> ctrls;
> > struct v4l2_query_ext_ctrl ctrl = {};
> >
> > /* \todo Add support for menu and compound controls. */
> > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> > index 3add6e67d2cf..d4b7588eb362 100644
> > --- a/test/v4l2_videodevice/controls.cpp
> > +++ b/test/v4l2_videodevice/controls.cpp
> > @@ -41,9 +41,9 @@ protected:
> > return TestFail;
> > }
> >
> > - const V4L2ControlInfo &brightness = info.find(V4L2_CID_BRIGHTNESS)->second;
> > - const V4L2ControlInfo &contrast = info.find(V4L2_CID_CONTRAST)->second;
> > - const V4L2ControlInfo &saturation = info.find(V4L2_CID_SATURATION)->second;
> > + const V4L2ControlRange &brightness = info.find(V4L2_CID_BRIGHTNESS)->second;
> > + const V4L2ControlRange &contrast = info.find(V4L2_CID_CONTRAST)->second;
> > + const V4L2ControlRange &saturation = info.find(V4L2_CID_SATURATION)->second;
> >
> > /* Test getting controls. */
> > V4L2ControlList ctrls(info);
> > @@ -65,9 +65,9 @@ protected:
> > }
> >
> > /* Test setting controls. */
> > - ctrls.set(V4L2_CID_BRIGHTNESS, brightness.range().min());
> > - ctrls.set(V4L2_CID_CONTRAST, contrast.range().max());
> > - ctrls.set(V4L2_CID_SATURATION, saturation.range().min());
> > + ctrls.set(V4L2_CID_BRIGHTNESS, brightness.min());
> > + ctrls.set(V4L2_CID_CONTRAST, contrast.max());
> > + ctrls.set(V4L2_CID_SATURATION, saturation.min());
> >
> > ret = capture_->setControls(&ctrls);
> > if (ret) {
> > @@ -76,9 +76,9 @@ protected:
> > }
> >
> > /* Test setting controls outside of range. */
> > - ctrls.set(V4L2_CID_BRIGHTNESS, brightness.range().min().get<int32_t>() - 1);
> > - ctrls.set(V4L2_CID_CONTRAST, contrast.range().max().get<int32_t>() + 1);
> > - ctrls.set(V4L2_CID_SATURATION, saturation.range().min().get<int32_t>() + 1);
> > + ctrls.set(V4L2_CID_BRIGHTNESS, brightness.min().get<int32_t>() - 1);
> > + ctrls.set(V4L2_CID_CONTRAST, contrast.max().get<int32_t>() + 1);
> > + ctrls.set(V4L2_CID_SATURATION, saturation.min().get<int32_t>() + 1);
> >
> > ret = capture_->setControls(&ctrls);
> > if (ret) {
> > @@ -86,9 +86,9 @@ protected:
> > return TestFail;
> > }
> >
> > - if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.range().min() ||
> > - ctrls.get(V4L2_CID_CONTRAST) != contrast.range().max() ||
> > - ctrls.get(V4L2_CID_SATURATION) != saturation.range().min().get<int32_t>() + 1) {
> > + if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() ||
> > + ctrls.get(V4L2_CID_CONTRAST) != contrast.max() ||
> > + ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) {
> > cerr << "Controls not updated when set" << endl;
> > return TestFail;
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list