| Message ID | 20190827095008.11405-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-08-27 11:50:06 +0200, Jacopo Mondi wrote: > Currently the selection API are implemented by wrappers of the > setSelection method. As we add support for more targets, adding new > wrapper does not scale well. Make the setSelection operation public and > implement getSelection, and require callers to specify the selection > target in the operation parameters list. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/include/v4l2_subdevice.h | 9 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > src/libcamera/v4l2_subdevice.cpp | 113 ++++++++++++++++--------- > 3 files changed, 78 insertions(+), 48 deletions(-) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 9c077674f997..b69b93280894 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -41,8 +41,10 @@ public: > > const MediaEntity *entity() const { return entity_; } > > - int setCrop(unsigned int pad, Rectangle *rect); > - int setCompose(unsigned int pad, Rectangle *rect); > + int getSelection(unsigned int pad, unsigned int target, > + Rectangle *rect); > + int setSelection(unsigned int pad, unsigned int target, > + Rectangle *rect); > > ImageFormats formats(unsigned int pad); > > @@ -60,9 +62,6 @@ private: > std::vector<SizeRange> enumPadSizes(unsigned int pad, > unsigned int code); > > - int setSelection(unsigned int pad, unsigned int target, > - Rectangle *rect); > - > const MediaEntity *entity_; > }; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 827906d5cd2e..bd027c7f10be 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1071,11 +1071,11 @@ int ImgUDevice::configureInput(const Size &size, > .w = inputFormat->size.width, > .h = inputFormat->size.height, > }; > - ret = imgu_->setCrop(PAD_INPUT, &rect); > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); > if (ret) > return ret; > > - ret = imgu_->setCompose(PAD_INPUT, &rect); > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); > if (ret) > return ret; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index a188298de34c..262dfa23ee57 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -127,25 +127,87 @@ int V4L2Subdevice::open() > */ > > /** > - * \brief Set a crop rectangle on one of the V4L2 subdevice pads > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > - * \param[inout] rect The rectangle describing crop target area > + * \brief Get the V4L2_SEL_TGT_* selection \a target rectangle on \a pad > + * \param[in] pad The 0-indexed pad number to get the selection target from > + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2 > + * selection API > + * \param[out] rect The rectangle describing the returned selection target > + * > + * This operations wraps the VIDIOC_SUBDEV_G_SELECTION ioctl, applied to one of > + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad. > + * The retrieved selection rectangle is returned in the output parameter \a > + * rect. > + * > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect) > +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target, > + Rectangle *rect) > { > - return setSelection(pad, V4L2_SEL_TGT_CROP, rect); > + struct v4l2_subdev_selection sel = {}; > + > + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sel.pad = pad; > + sel.target = target; > + > + int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Unable to get rectangle " << target << " on pad " > + << pad << ": " << strerror(-ret); > + return ret; > + } > + > + rect->x = sel.r.left; > + rect->y = sel.r.top; > + rect->w = sel.r.width; > + rect->h = sel.r.height; > + > + return 0; > } > > /** > - * \brief Set a compose rectangle on one of the V4L2 subdevice pads > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > - * \param[inout] rect The rectangle describing the compose target area > + * \brief Set the V4L2_SEL_TGT_* selection \a target rectangle on \a pad > + * \param[in] pad The 0-indexed pad number to set the selection target on > + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2 > + * selection API > + * \param[inout] rect The rectangle to be applied to the the selection \a target > + * > + * This operations wraps the VIDIOC_SUBDEV_S_SELECTION ioctl, applied to one of > + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad. > + * The selection rectangle to apply is described by the parameter \a rect, > + * which is updated to reflect what has been actually applied on the subdevice > + * when the method returns. > + * > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > + Rectangle *rect) > { > - return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > + struct v4l2_subdev_selection sel = {}; > + > + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sel.pad = pad; > + sel.target = target; > + > + sel.r.left = rect->x; > + sel.r.top = rect->y; > + sel.r.width = rect->w; > + sel.r.height = rect->h; > + > + int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Unable to set rectangle " << target << " on pad " > + << pad << ": " << strerror(-ret); > + return ret; > + } > + > + rect->x = sel.r.left; > + rect->y = sel.r.top; > + rect->w = sel.r.width; > + rect->h = sel.r.height; > + > + return 0; > } > > /** > @@ -329,35 +391,4 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad, > return sizes; > } > > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > - Rectangle *rect) > -{ > - struct v4l2_subdev_selection sel = {}; > - > - sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; > - sel.pad = pad; > - sel.target = target; > - sel.flags = 0; > - > - sel.r.left = rect->x; > - sel.r.top = rect->y; > - sel.r.width = rect->w; > - sel.r.height = rect->h; > - > - int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel); > - if (ret < 0) { > - LOG(V4L2, Error) > - << "Unable to set rectangle " << target << " on pad " > - << pad << ": " << strerror(-ret); > - return ret; > - } > - > - rect->x = sel.r.left; > - rect->y = sel.r.top; > - rect->w = sel.r.width; > - rect->h = sel.r.height; > - > - return 0; > -} > - > } /* namespace libcamera */ > -- > 2.23.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Tue, Aug 27, 2019 at 11:50:06AM +0200, Jacopo Mondi wrote: > Currently the selection API are implemented by wrappers of the > setSelection method. As we add support for more targets, adding new > wrapper does not scale well. Make the setSelection operation public and > implement getSelection, and require callers to specify the selection > target in the operation parameters list. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_subdevice.h | 9 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > src/libcamera/v4l2_subdevice.cpp | 113 ++++++++++++++++--------- > 3 files changed, 78 insertions(+), 48 deletions(-) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 9c077674f997..b69b93280894 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -41,8 +41,10 @@ public: > > const MediaEntity *entity() const { return entity_; } > > - int setCrop(unsigned int pad, Rectangle *rect); > - int setCompose(unsigned int pad, Rectangle *rect); > + int getSelection(unsigned int pad, unsigned int target, > + Rectangle *rect); > + int setSelection(unsigned int pad, unsigned int target, > + Rectangle *rect); > > ImageFormats formats(unsigned int pad); > > @@ -60,9 +62,6 @@ private: > std::vector<SizeRange> enumPadSizes(unsigned int pad, > unsigned int code); > > - int setSelection(unsigned int pad, unsigned int target, > - Rectangle *rect); > - > const MediaEntity *entity_; > }; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 827906d5cd2e..bd027c7f10be 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1071,11 +1071,11 @@ int ImgUDevice::configureInput(const Size &size, > .w = inputFormat->size.width, > .h = inputFormat->size.height, > }; > - ret = imgu_->setCrop(PAD_INPUT, &rect); > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); > if (ret) > return ret; > > - ret = imgu_->setCompose(PAD_INPUT, &rect); > + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); > if (ret) > return ret; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index a188298de34c..262dfa23ee57 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -127,25 +127,87 @@ int V4L2Subdevice::open() > */ > > /** > - * \brief Set a crop rectangle on one of the V4L2 subdevice pads > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > - * \param[inout] rect The rectangle describing crop target area > + * \brief Get the V4L2_SEL_TGT_* selection \a target rectangle on \a pad I would drop V4L2_SEL_TGT_* here as it's mentioned in the target parameter. > + * \param[in] pad The 0-indexed pad number to get the selection target from s/target/rectangle/ > + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2 > + * selection API > + * \param[out] rect The rectangle describing the returned selection target s/selection/selection rectangle/ or s/\a target/rectangle/ > + * > + * This operations wraps the VIDIOC_SUBDEV_G_SELECTION ioctl, applied to one of > + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad. > + * The retrieved selection rectangle is returned in the output parameter \a > + * rect. > + * > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect) > +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target, > + Rectangle *rect) > { > - return setSelection(pad, V4L2_SEL_TGT_CROP, rect); > + struct v4l2_subdev_selection sel = {}; > + > + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sel.pad = pad; > + sel.target = target; > + > + int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Unable to get rectangle " << target << " on pad " Maybe s/rectangle/selection rectangle/ ? All these comments apply to setSelection() below. With those small issues fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + << pad << ": " << strerror(-ret); > + return ret; > + } > + > + rect->x = sel.r.left; > + rect->y = sel.r.top; > + rect->w = sel.r.width; > + rect->h = sel.r.height; > + > + return 0; > } > > /** > - * \brief Set a compose rectangle on one of the V4L2 subdevice pads > - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to > - * \param[inout] rect The rectangle describing the compose target area > + * \brief Set the V4L2_SEL_TGT_* selection \a target rectangle on \a pad > + * \param[in] pad The 0-indexed pad number to set the selection target on > + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2 > + * selection API > + * \param[inout] rect The rectangle to be applied to the the selection \a target > + * > + * This operations wraps the VIDIOC_SUBDEV_S_SELECTION ioctl, applied to one of > + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad. > + * The selection rectangle to apply is described by the parameter \a rect, > + * which is updated to reflect what has been actually applied on the subdevice > + * when the method returns. > + * > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > + Rectangle *rect) > { > - return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > + struct v4l2_subdev_selection sel = {}; > + > + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sel.pad = pad; > + sel.target = target; > + > + sel.r.left = rect->x; > + sel.r.top = rect->y; > + sel.r.width = rect->w; > + sel.r.height = rect->h; > + > + int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel); > + if (ret < 0) { > + LOG(V4L2, Error) > + << "Unable to set rectangle " << target << " on pad " > + << pad << ": " << strerror(-ret); > + return ret; > + } > + > + rect->x = sel.r.left; > + rect->y = sel.r.top; > + rect->w = sel.r.width; > + rect->h = sel.r.height; > + > + return 0; > } > > /** > @@ -329,35 +391,4 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad, > return sizes; > } > > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > - Rectangle *rect) > -{ > - struct v4l2_subdev_selection sel = {}; > - > - sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; > - sel.pad = pad; > - sel.target = target; > - sel.flags = 0; > - > - sel.r.left = rect->x; > - sel.r.top = rect->y; > - sel.r.width = rect->w; > - sel.r.height = rect->h; > - > - int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel); > - if (ret < 0) { > - LOG(V4L2, Error) > - << "Unable to set rectangle " << target << " on pad " > - << pad << ": " << strerror(-ret); > - return ret; > - } > - > - rect->x = sel.r.left; > - rect->y = sel.r.top; > - rect->w = sel.r.width; > - rect->h = sel.r.height; > - > - return 0; > -} > - > } /* namespace libcamera */
diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h index 9c077674f997..b69b93280894 100644 --- a/src/libcamera/include/v4l2_subdevice.h +++ b/src/libcamera/include/v4l2_subdevice.h @@ -41,8 +41,10 @@ public: const MediaEntity *entity() const { return entity_; } - int setCrop(unsigned int pad, Rectangle *rect); - int setCompose(unsigned int pad, Rectangle *rect); + int getSelection(unsigned int pad, unsigned int target, + Rectangle *rect); + int setSelection(unsigned int pad, unsigned int target, + Rectangle *rect); ImageFormats formats(unsigned int pad); @@ -60,9 +62,6 @@ private: std::vector<SizeRange> enumPadSizes(unsigned int pad, unsigned int code); - int setSelection(unsigned int pad, unsigned int target, - Rectangle *rect); - const MediaEntity *entity_; }; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 827906d5cd2e..bd027c7f10be 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1071,11 +1071,11 @@ int ImgUDevice::configureInput(const Size &size, .w = inputFormat->size.width, .h = inputFormat->size.height, }; - ret = imgu_->setCrop(PAD_INPUT, &rect); + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect); if (ret) return ret; - ret = imgu_->setCompose(PAD_INPUT, &rect); + ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_COMPOSE, &rect); if (ret) return ret; diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index a188298de34c..262dfa23ee57 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -127,25 +127,87 @@ int V4L2Subdevice::open() */ /** - * \brief Set a crop rectangle on one of the V4L2 subdevice pads - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to - * \param[inout] rect The rectangle describing crop target area + * \brief Get the V4L2_SEL_TGT_* selection \a target rectangle on \a pad + * \param[in] pad The 0-indexed pad number to get the selection target from + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2 + * selection API + * \param[out] rect The rectangle describing the returned selection target + * + * This operations wraps the VIDIOC_SUBDEV_G_SELECTION ioctl, applied to one of + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad. + * The retrieved selection rectangle is returned in the output parameter \a + * rect. + * * \return 0 on success or a negative error code otherwise */ -int V4L2Subdevice::setCrop(unsigned int pad, Rectangle *rect) +int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target, + Rectangle *rect) { - return setSelection(pad, V4L2_SEL_TGT_CROP, rect); + struct v4l2_subdev_selection sel = {}; + + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; + sel.pad = pad; + sel.target = target; + + int ret = ioctl(VIDIOC_SUBDEV_G_SELECTION, &sel); + if (ret < 0) { + LOG(V4L2, Error) + << "Unable to get rectangle " << target << " on pad " + << pad << ": " << strerror(-ret); + return ret; + } + + rect->x = sel.r.left; + rect->y = sel.r.top; + rect->w = sel.r.width; + rect->h = sel.r.height; + + return 0; } /** - * \brief Set a compose rectangle on one of the V4L2 subdevice pads - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to - * \param[inout] rect The rectangle describing the compose target area + * \brief Set the V4L2_SEL_TGT_* selection \a target rectangle on \a pad + * \param[in] pad The 0-indexed pad number to set the selection target on + * \param[in] target The selection target V4L2_SEL_TGT_* as defined by the V4L2 + * selection API + * \param[inout] rect The rectangle to be applied to the the selection \a target + * + * This operations wraps the VIDIOC_SUBDEV_S_SELECTION ioctl, applied to one of + * the V4L2_SEL_TGT_* targets defined by the V4L2 API on the subdevice \a pad. + * The selection rectangle to apply is described by the parameter \a rect, + * which is updated to reflect what has been actually applied on the subdevice + * when the method returns. + * * \return 0 on success or a negative error code otherwise */ -int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) +int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, + Rectangle *rect) { - return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); + struct v4l2_subdev_selection sel = {}; + + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; + sel.pad = pad; + sel.target = target; + + sel.r.left = rect->x; + sel.r.top = rect->y; + sel.r.width = rect->w; + sel.r.height = rect->h; + + int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel); + if (ret < 0) { + LOG(V4L2, Error) + << "Unable to set rectangle " << target << " on pad " + << pad << ": " << strerror(-ret); + return ret; + } + + rect->x = sel.r.left; + rect->y = sel.r.top; + rect->w = sel.r.width; + rect->h = sel.r.height; + + return 0; } /** @@ -329,35 +391,4 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad, return sizes; } -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, - Rectangle *rect) -{ - struct v4l2_subdev_selection sel = {}; - - sel.which = V4L2_SUBDEV_FORMAT_ACTIVE; - sel.pad = pad; - sel.target = target; - sel.flags = 0; - - sel.r.left = rect->x; - sel.r.top = rect->y; - sel.r.width = rect->w; - sel.r.height = rect->h; - - int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel); - if (ret < 0) { - LOG(V4L2, Error) - << "Unable to set rectangle " << target << " on pad " - << pad << ": " << strerror(-ret); - return ret; - } - - rect->x = sel.r.left; - rect->y = sel.r.top; - rect->w = sel.r.width; - rect->h = sel.r.height; - - return 0; -} - } /* namespace libcamera */
Currently the selection API are implemented by wrappers of the setSelection method. As we add support for more targets, adding new wrapper does not scale well. Make the setSelection operation public and implement getSelection, and require callers to specify the selection target in the operation parameters list. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_subdevice.h | 9 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- src/libcamera/v4l2_subdevice.cpp | 113 ++++++++++++++++--------- 3 files changed, 78 insertions(+), 48 deletions(-)