| Message ID | 20190928152238.23752-9-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sat, Sep 28, 2019 at 06:22:34PM +0300, Laurent Pinchart wrote: > Use the ControlValue class to replace the manually crafted data storage > in V4L2Control. This will help sharing code when more data types will be > supported. > Happy indeed to see that at third attempt we might be getting there > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_controls.h | 15 +++++++++------ > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > src/libcamera/pipeline/vimc.cpp | 2 +- > src/libcamera/v4l2_controls.cpp | 19 ++++++++++--------- > src/libcamera/v4l2_device.cpp | 8 ++++---- > 5 files changed, 25 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > index 10b726504951..f2b67c5d44e1 100644 > --- a/src/libcamera/include/v4l2_controls.h > +++ b/src/libcamera/include/v4l2_controls.h > @@ -16,6 +16,8 @@ > #include <linux/v4l2-controls.h> > #include <linux/videodev2.h> > > +#include <libcamera/controls.h> > + > namespace libcamera { > > class V4L2ControlInfo > @@ -46,17 +48,18 @@ using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; > class V4L2Control > { > public: > - V4L2Control(unsigned int id, int value = 0) > - : id_(id), value_(value) {} > - > - int64_t value() const { return value_; } > - void setValue(int64_t value) { value_ = value; } > + V4L2Control(unsigned int id, const ControlValue &value = ControlValue()) > + : id_(id), value_(value) The default copy constructor is fine so far, as it copies the union content. With compound controls this will fall short I fear. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + { > + } > > unsigned int id() const { return id_; } > + const ControlValue &value() const { return value_; } > + ControlValue &value() { return value_; } > > private: > unsigned int id_; > - int64_t value_; > + ControlValue value_; > }; > > class V4L2ControlList > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 3635ea83a7b1..a80f8a43a116 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -252,7 +252,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > LOG(UVC, Debug) > << "Setting control 0x" > << std::hex << std::setw(8) << ctrl.id() << std::dec > - << " to " << ctrl.value(); > + << " to " << ctrl.value().toString(); > > int ret = data->video_->setControls(&controls); > if (ret) { > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index d9dd031e173c..5d6909f58cf9 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -300,7 +300,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) > LOG(VIMC, Debug) > << "Setting control 0x" > << std::hex << std::setw(8) << ctrl.id() << std::dec > - << " to " << ctrl.value(); > + << " to " << ctrl.value().toString(); > > int ret = data->sensor_->setControls(&controls); > if (ret) { > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > index 84258d9954d0..64f0555fff7c 100644 > --- a/src/libcamera/v4l2_controls.cpp > +++ b/src/libcamera/v4l2_controls.cpp > @@ -143,6 +143,16 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * \param value The control value > */ > > +/** > + * \fn V4L2Control::value() const > + * \brief Retrieve the value of the control > + * > + * This method is a const version of V4L2Control::value(), returning a const > + * reference to the value. > + * > + * \return The V4L2 control value > + */ > + > /** > * \fn V4L2Control::value() > * \brief Retrieve the value of the control > @@ -154,15 +164,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > * \return The V4L2 control value > */ > > -/** > - * \fn V4L2Control::setValue() > - * \brief Set the value of the control > - * \param value The new V4L2 control value > - * > - * This method stores the control value, which will be applied to the > - * device when calling V4L2Device::setControls(). > - */ > - > /** > * \fn V4L2Control::id() > * \brief Retrieve the control ID this instance refers to > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 349bf2d29704..fd4b9c6d5c62 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -264,14 +264,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > /* Set the v4l2_ext_control value for the write operation. */ > switch (info->type()) { > case V4L2_CTRL_TYPE_INTEGER64: > - v4l2Ctrls[i].value64 = ctrl->value(); > + v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>(); > break; > default: > /* > * \todo To be changed when support for string and > * compound controls will be added. > */ > - v4l2Ctrls[i].value = ctrl->value(); > + v4l2Ctrls[i].value = ctrl->value().get<int32_t>(); > break; > } > } > @@ -393,14 +393,14 @@ void V4L2Device::updateControls(V4L2ControlList *ctrls, > > switch (info->type()) { > case V4L2_CTRL_TYPE_INTEGER64: > - ctrl->setValue(v4l2Ctrl->value64); > + ctrl->value().set<int64_t>(v4l2Ctrl->value64); > break; > default: > /* > * \todo To be changed when support for string and > * compound controls will be added. > */ > - ctrl->setValue(v4l2Ctrl->value); > + ctrl->value().set<int32_t>(v4l2Ctrl->value); > break; > } > } > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Sun, Sep 29, 2019 at 12:10:25PM +0200, Jacopo Mondi wrote: > On Sat, Sep 28, 2019 at 06:22:34PM +0300, Laurent Pinchart wrote: > > Use the ControlValue class to replace the manually crafted data storage > > in V4L2Control. This will help sharing code when more data types will be > > supported. > > Happy indeed to see that at third attempt we might be getting there It's nice to share code, but I'd like to make it clear that my first focus is a good API towards applications, my second focus a good API towards pipeline handlers and IPAs, and internal code sharing is only my third focus. As long as libcamera and V4L2 controls are similar enough that code can be shared, I have no issue with that. If we figure out later that differences are needed, I will choose separate implementations over any regression in the public API for the purpose of code sharing. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/include/v4l2_controls.h | 15 +++++++++------ > > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > > src/libcamera/pipeline/vimc.cpp | 2 +- > > src/libcamera/v4l2_controls.cpp | 19 ++++++++++--------- > > src/libcamera/v4l2_device.cpp | 8 ++++---- > > 5 files changed, 25 insertions(+), 21 deletions(-) > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h > > index 10b726504951..f2b67c5d44e1 100644 > > --- a/src/libcamera/include/v4l2_controls.h > > +++ b/src/libcamera/include/v4l2_controls.h > > @@ -16,6 +16,8 @@ > > #include <linux/v4l2-controls.h> > > #include <linux/videodev2.h> > > > > +#include <libcamera/controls.h> > > + > > namespace libcamera { > > > > class V4L2ControlInfo > > @@ -46,17 +48,18 @@ using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; > > class V4L2Control > > { > > public: > > - V4L2Control(unsigned int id, int value = 0) > > - : id_(id), value_(value) {} > > - > > - int64_t value() const { return value_; } > > - void setValue(int64_t value) { value_ = value; } > > + V4L2Control(unsigned int id, const ControlValue &value = ControlValue()) > > + : id_(id), value_(value) > > The default copy constructor is fine so far, as it copies the union > content. With compound controls this will fall short I fear. Yes, we will then need an explicit copy constructor. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + { > > + } > > > > unsigned int id() const { return id_; } > > + const ControlValue &value() const { return value_; } > > + ControlValue &value() { return value_; } > > > > private: > > unsigned int id_; > > - int64_t value_; > > + ControlValue value_; > > }; > > > > class V4L2ControlList > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index 3635ea83a7b1..a80f8a43a116 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -252,7 +252,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > > LOG(UVC, Debug) > > << "Setting control 0x" > > << std::hex << std::setw(8) << ctrl.id() << std::dec > > - << " to " << ctrl.value(); > > + << " to " << ctrl.value().toString(); > > > > int ret = data->video_->setControls(&controls); > > if (ret) { > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index d9dd031e173c..5d6909f58cf9 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -300,7 +300,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) > > LOG(VIMC, Debug) > > << "Setting control 0x" > > << std::hex << std::setw(8) << ctrl.id() << std::dec > > - << " to " << ctrl.value(); > > + << " to " << ctrl.value().toString(); > > > > int ret = data->sensor_->setControls(&controls); > > if (ret) { > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp > > index 84258d9954d0..64f0555fff7c 100644 > > --- a/src/libcamera/v4l2_controls.cpp > > +++ b/src/libcamera/v4l2_controls.cpp > > @@ -143,6 +143,16 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > * \param value The control value > > */ > > > > +/** > > + * \fn V4L2Control::value() const > > + * \brief Retrieve the value of the control > > + * > > + * This method is a const version of V4L2Control::value(), returning a const > > + * reference to the value. > > + * > > + * \return The V4L2 control value > > + */ > > + > > /** > > * \fn V4L2Control::value() > > * \brief Retrieve the value of the control > > @@ -154,15 +164,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) > > * \return The V4L2 control value > > */ > > > > -/** > > - * \fn V4L2Control::setValue() > > - * \brief Set the value of the control > > - * \param value The new V4L2 control value > > - * > > - * This method stores the control value, which will be applied to the > > - * device when calling V4L2Device::setControls(). > > - */ > > - > > /** > > * \fn V4L2Control::id() > > * \brief Retrieve the control ID this instance refers to > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 349bf2d29704..fd4b9c6d5c62 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -264,14 +264,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) > > /* Set the v4l2_ext_control value for the write operation. */ > > switch (info->type()) { > > case V4L2_CTRL_TYPE_INTEGER64: > > - v4l2Ctrls[i].value64 = ctrl->value(); > > + v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>(); > > break; > > default: > > /* > > * \todo To be changed when support for string and > > * compound controls will be added. > > */ > > - v4l2Ctrls[i].value = ctrl->value(); > > + v4l2Ctrls[i].value = ctrl->value().get<int32_t>(); > > break; > > } > > } > > @@ -393,14 +393,14 @@ void V4L2Device::updateControls(V4L2ControlList *ctrls, > > > > switch (info->type()) { > > case V4L2_CTRL_TYPE_INTEGER64: > > - ctrl->setValue(v4l2Ctrl->value64); > > + ctrl->value().set<int64_t>(v4l2Ctrl->value64); > > break; > > default: > > /* > > * \todo To be changed when support for string and > > * compound controls will be added. > > */ > > - ctrl->setValue(v4l2Ctrl->value); > > + ctrl->value().set<int32_t>(v4l2Ctrl->value); > > break; > > } > > }
diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h index 10b726504951..f2b67c5d44e1 100644 --- a/src/libcamera/include/v4l2_controls.h +++ b/src/libcamera/include/v4l2_controls.h @@ -16,6 +16,8 @@ #include <linux/v4l2-controls.h> #include <linux/videodev2.h> +#include <libcamera/controls.h> + namespace libcamera { class V4L2ControlInfo @@ -46,17 +48,18 @@ using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>; class V4L2Control { public: - V4L2Control(unsigned int id, int value = 0) - : id_(id), value_(value) {} - - int64_t value() const { return value_; } - void setValue(int64_t value) { value_ = value; } + V4L2Control(unsigned int id, const ControlValue &value = ControlValue()) + : id_(id), value_(value) + { + } unsigned int id() const { return id_; } + const ControlValue &value() const { return value_; } + ControlValue &value() { return value_; } private: unsigned int id_; - int64_t value_; + ControlValue value_; }; class V4L2ControlList diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 3635ea83a7b1..a80f8a43a116 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -252,7 +252,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) LOG(UVC, Debug) << "Setting control 0x" << std::hex << std::setw(8) << ctrl.id() << std::dec - << " to " << ctrl.value(); + << " to " << ctrl.value().toString(); int ret = data->video_->setControls(&controls); if (ret) { diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index d9dd031e173c..5d6909f58cf9 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -300,7 +300,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) LOG(VIMC, Debug) << "Setting control 0x" << std::hex << std::setw(8) << ctrl.id() << std::dec - << " to " << ctrl.value(); + << " to " << ctrl.value().toString(); int ret = data->sensor_->setControls(&controls); if (ret) { diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp index 84258d9954d0..64f0555fff7c 100644 --- a/src/libcamera/v4l2_controls.cpp +++ b/src/libcamera/v4l2_controls.cpp @@ -143,6 +143,16 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * \param value The control value */ +/** + * \fn V4L2Control::value() const + * \brief Retrieve the value of the control + * + * This method is a const version of V4L2Control::value(), returning a const + * reference to the value. + * + * \return The V4L2 control value + */ + /** * \fn V4L2Control::value() * \brief Retrieve the value of the control @@ -154,15 +164,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * \return The V4L2 control value */ -/** - * \fn V4L2Control::setValue() - * \brief Set the value of the control - * \param value The new V4L2 control value - * - * This method stores the control value, which will be applied to the - * device when calling V4L2Device::setControls(). - */ - /** * \fn V4L2Control::id() * \brief Retrieve the control ID this instance refers to diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 349bf2d29704..fd4b9c6d5c62 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -264,14 +264,14 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) /* Set the v4l2_ext_control value for the write operation. */ switch (info->type()) { case V4L2_CTRL_TYPE_INTEGER64: - v4l2Ctrls[i].value64 = ctrl->value(); + v4l2Ctrls[i].value64 = ctrl->value().get<int64_t>(); break; default: /* * \todo To be changed when support for string and * compound controls will be added. */ - v4l2Ctrls[i].value = ctrl->value(); + v4l2Ctrls[i].value = ctrl->value().get<int32_t>(); break; } } @@ -393,14 +393,14 @@ void V4L2Device::updateControls(V4L2ControlList *ctrls, switch (info->type()) { case V4L2_CTRL_TYPE_INTEGER64: - ctrl->setValue(v4l2Ctrl->value64); + ctrl->value().set<int64_t>(v4l2Ctrl->value64); break; default: /* * \todo To be changed when support for string and * compound controls will be added. */ - ctrl->setValue(v4l2Ctrl->value); + ctrl->value().set<int32_t>(v4l2Ctrl->value); break; } }
Use the ControlValue class to replace the manually crafted data storage in V4L2Control. This will help sharing code when more data types will be supported. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/include/v4l2_controls.h | 15 +++++++++------ src/libcamera/pipeline/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- src/libcamera/v4l2_controls.cpp | 19 ++++++++++--------- src/libcamera/v4l2_device.cpp | 8 ++++---- 5 files changed, 25 insertions(+), 21 deletions(-)