| Message ID | 20190817105937.29353-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-08-17 12:59:36 +0200, Jacopo Mondi wrote: > Add a private operation to perform G_SELECTION ioctl on the v4l2 > subdevice and create two public methods to retrieve the crop bound > rectangle and the subdevice native area size. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/include/v4l2_subdevice.h | 4 +++ > src/libcamera/v4l2_subdevice.cpp | 48 ++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 9c077674f997..5316617c6873 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -43,6 +43,8 @@ public: > > int setCrop(unsigned int pad, Rectangle *rect); > int setCompose(unsigned int pad, Rectangle *rect); > + int getCropBounds(unsigned int pad, Rectangle *rect); > + int getNativeSize(unsigned int pad, Rectangle *rect); > > ImageFormats formats(unsigned int pad); > > @@ -62,6 +64,8 @@ private: > > int setSelection(unsigned int pad, unsigned int target, > Rectangle *rect); > + int getSelection(unsigned int pad, unsigned int target, > + Rectangle *rect); > > const MediaEntity *entity_; > }; > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index a188298de34c..5e661e2ef1fc 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > } > > +/** > + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads > + * \param[in] pad The 0-indexed pad number the rectangle is retrived from > + * \param[out] rect The rectangle describing the crop bounds area > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect) > +{ > + return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect); > +} > + > +/** > + * \brief Get the native area size on one of the V4L2 subdevice pads > + * \param[in] pad The 0-indexed pad number the native area is retrieved from > + * \param[out] rect The rectangle describing the native size area > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect) > +{ > + return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect); > +} > + > /** > * \brief Enumerate all media bus codes and frame sizes on a \a pad > * \param[in] pad The 0-indexed pad number to enumerate formats on > @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > return 0; > } > > +int V4L2Subdevice::getSelection(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; > + > + 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; > +} > + > } /* namespace libcamera */ > -- > 2.22.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 Sat, Aug 17, 2019 at 12:59:36PM +0200, Jacopo Mondi wrote: > Add a private operation to perform G_SELECTION ioctl on the v4l2 > subdevice and create two public methods to retrieve the crop bound > rectangle and the subdevice native area size. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_subdevice.h | 4 +++ > src/libcamera/v4l2_subdevice.cpp | 48 ++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > index 9c077674f997..5316617c6873 100644 > --- a/src/libcamera/include/v4l2_subdevice.h > +++ b/src/libcamera/include/v4l2_subdevice.h > @@ -43,6 +43,8 @@ public: > > int setCrop(unsigned int pad, Rectangle *rect); > int setCompose(unsigned int pad, Rectangle *rect); > + int getCropBounds(unsigned int pad, Rectangle *rect); > + int getNativeSize(unsigned int pad, Rectangle *rect); Should these two methods be named cropBounds() and nativeSize() ? They don't map directly to a VIDIOC_SUBDEV_G_... ioctl. And should they return a Rectangle instance instead of taking it through an argument ? Finally, should we remove the pad argument ? The native size is the native size of the sensor, not the size of a pad. > > ImageFormats formats(unsigned int pad); > > @@ -62,6 +64,8 @@ private: > > int setSelection(unsigned int pad, unsigned int target, > Rectangle *rect); > + int getSelection(unsigned int pad, unsigned int target, > + Rectangle *rect); I would move getSelection() above setSelection(). > > const MediaEntity *entity_; > }; > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index a188298de34c..5e661e2ef1fc 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > } > > +/** > + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads I think this method shouldn't use V4L2 terminology, but explain in clearer terms what it retrieves. It should thus likely be renamed. > + * \param[in] pad The 0-indexed pad number the rectangle is retrived from > + * \param[out] rect The rectangle describing the crop bounds area > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect) > +{ > + return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect); > +} > + > +/** > + * \brief Get the native area size on one of the V4L2 subdevice pads If you rename the method to nativeSize(), I would write the brief as "Retrieve the ..." as we usually do. > + * \param[in] pad The 0-indexed pad number the native area is retrieved from > + * \param[out] rect The rectangle describing the native size area > + * \return 0 on success or a negative error code otherwise > + */ > +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect) Should this be renamted to pixelArraySize() and return a Size ? > +{ > + return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect); > +} > + > /** > * \brief Enumerate all media bus codes and frame sizes on a \a pad > * \param[in] pad The 0-indexed pad number to enumerate formats on > @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > return 0; > } > > +int V4L2Subdevice::getSelection(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; You can drop this as you initialise sel to 0. > + > + 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; > +} > + > } /* namespace libcamera */
Hi Jacopo, On Sat, Aug 17, 2019 at 07:19:43PM +0300, Laurent Pinchart wrote: > On Sat, Aug 17, 2019 at 12:59:36PM +0200, Jacopo Mondi wrote: > > Add a private operation to perform G_SELECTION ioctl on the v4l2 > > subdevice and create two public methods to retrieve the crop bound > > rectangle and the subdevice native area size. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_subdevice.h | 4 +++ > > src/libcamera/v4l2_subdevice.cpp | 48 ++++++++++++++++++++++++++ > > 2 files changed, 52 insertions(+) > > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > index 9c077674f997..5316617c6873 100644 > > --- a/src/libcamera/include/v4l2_subdevice.h > > +++ b/src/libcamera/include/v4l2_subdevice.h > > @@ -43,6 +43,8 @@ public: > > > > int setCrop(unsigned int pad, Rectangle *rect); > > int setCompose(unsigned int pad, Rectangle *rect); > > + int getCropBounds(unsigned int pad, Rectangle *rect); > > + int getNativeSize(unsigned int pad, Rectangle *rect); > > Should these two methods be named cropBounds() and nativeSize() ? > They don't map directly to a VIDIOC_SUBDEV_G_... ioctl. And should they > return a Rectangle instance instead of taking it through an argument ? > Finally, should we remove the pad argument ? The native size is the > native size of the sensor, not the size of a pad. My bad, for some reason I though I was reviewing the CameraSensor class. Please ignore all comments about the getCropBounds and getNativeSize methods. I wonder, however, if we shouldn't make the get/set selection methods public (and remove setCrop() and setCompose()), otherwise we'll keep adding accessors for all targets, and I'm not sure it's worth it. If you really think dedicated accessors are best I would at least make them inline and move the get methods before the set methods. > > > > ImageFormats formats(unsigned int pad); > > > > @@ -62,6 +64,8 @@ private: > > > > int setSelection(unsigned int pad, unsigned int target, > > Rectangle *rect); > > + int getSelection(unsigned int pad, unsigned int target, > > + Rectangle *rect); > > I would move getSelection() above setSelection(). > > > > > const MediaEntity *entity_; > > }; > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > index a188298de34c..5e661e2ef1fc 100644 > > --- a/src/libcamera/v4l2_subdevice.cpp > > +++ b/src/libcamera/v4l2_subdevice.cpp > > @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > > return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > > } > > > > +/** > > + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads > > I think this method shouldn't use V4L2 terminology, but explain in > clearer terms what it retrieves. It should thus likely be renamed. > > > + * \param[in] pad The 0-indexed pad number the rectangle is retrived from > > + * \param[out] rect The rectangle describing the crop bounds area > > + * \return 0 on success or a negative error code otherwise > > + */ > > +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect) > > +{ > > + return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect); > > +} > > + > > +/** > > + * \brief Get the native area size on one of the V4L2 subdevice pads > > If you rename the method to nativeSize(), I would write the brief as > "Retrieve the ..." as we usually do. > > > + * \param[in] pad The 0-indexed pad number the native area is retrieved from > > + * \param[out] rect The rectangle describing the native size area > > + * \return 0 on success or a negative error code otherwise > > + */ > > +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect) > > Should this be renamted to pixelArraySize() and return a Size ? > > > +{ > > + return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect); > > +} > > + > > /** > > * \brief Enumerate all media bus codes and frame sizes on a \a pad > > * \param[in] pad The 0-indexed pad number to enumerate formats on > > @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > > return 0; > > } > > > > +int V4L2Subdevice::getSelection(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; > > You can drop this as you initialise sel to 0. > > > + > > + 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; > > +} > > + > > } /* namespace libcamera */
Hi Laurent, On Sat, Aug 17, 2019 at 07:25:47PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Sat, Aug 17, 2019 at 07:19:43PM +0300, Laurent Pinchart wrote: > > On Sat, Aug 17, 2019 at 12:59:36PM +0200, Jacopo Mondi wrote: > > > Add a private operation to perform G_SELECTION ioctl on the v4l2 > > > subdevice and create two public methods to retrieve the crop bound > > > rectangle and the subdevice native area size. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/include/v4l2_subdevice.h | 4 +++ > > > src/libcamera/v4l2_subdevice.cpp | 48 ++++++++++++++++++++++++++ > > > 2 files changed, 52 insertions(+) > > > > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > > > index 9c077674f997..5316617c6873 100644 > > > --- a/src/libcamera/include/v4l2_subdevice.h > > > +++ b/src/libcamera/include/v4l2_subdevice.h > > > @@ -43,6 +43,8 @@ public: > > > > > > int setCrop(unsigned int pad, Rectangle *rect); > > > int setCompose(unsigned int pad, Rectangle *rect); > > > + int getCropBounds(unsigned int pad, Rectangle *rect); > > > + int getNativeSize(unsigned int pad, Rectangle *rect); > > > > Should these two methods be named cropBounds() and nativeSize() ? > > They don't map directly to a VIDIOC_SUBDEV_G_... ioctl. And should they > > return a Rectangle instance instead of taking it through an argument ? > > Finally, should we remove the pad argument ? The native size is the > > native size of the sensor, not the size of a pad. > > My bad, for some reason I though I was reviewing the CameraSensor class. > Please ignore all comments about the getCropBounds and getNativeSize > methods. I wonder, however, if we shouldn't make the get/set selection > methods public (and remove setCrop() and setCompose()), otherwise we'll > keep adding accessors for all targets, and I'm not sure it's worth it. > If you really think dedicated accessors are best I would at least make > them inline and move the get methods before the set methods. > I had the same concern.. We'll keep adding wrappers and wrappers. However this is not too bad, the other way around would be what you said, but this will then require user of the subdev API to use V4L2 defined flags (which is not a big issue, as they're already using a -v4l2-_subdevice instance after all) > > > > > > ImageFormats formats(unsigned int pad); > > > > > > @@ -62,6 +64,8 @@ private: > > > > > > int setSelection(unsigned int pad, unsigned int target, > > > Rectangle *rect); > > > + int getSelection(unsigned int pad, unsigned int target, > > > + Rectangle *rect); > > > > I would move getSelection() above setSelection(). > > OK > > > > > > const MediaEntity *entity_; > > > }; > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > > index a188298de34c..5e661e2ef1fc 100644 > > > --- a/src/libcamera/v4l2_subdevice.cpp > > > +++ b/src/libcamera/v4l2_subdevice.cpp > > > @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > > > return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > > > } > > > > > > +/** > > > + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads > > > > I think this method shouldn't use V4L2 terminology, but explain in > > clearer terms what it retrieves. It should thus likely be renamed. > > > > > + * \param[in] pad The 0-indexed pad number the rectangle is retrived from > > > + * \param[out] rect The rectangle describing the crop bounds area > > > + * \return 0 on success or a negative error code otherwise > > > + */ > > > +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect) > > > +{ > > > + return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect); > > > +} > > > + > > > +/** > > > + * \brief Get the native area size on one of the V4L2 subdevice pads > > > > If you rename the method to nativeSize(), I would write the brief as > > "Retrieve the ..." as we usually do. > > Ignoring the renaming part, but I should probably use the usual "Retrieve" here. > > > + * \param[in] pad The 0-indexed pad number the native area is retrieved from > > > + * \param[out] rect The rectangle describing the native size area > > > + * \return 0 on success or a negative error code otherwise > > > + */ > > > +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect) > > > > Should this be renamted to pixelArraySize() and return a Size ? > > > > > +{ > > > + return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect); > > > +} > > > + > > > /** > > > * \brief Enumerate all media bus codes and frame sizes on a \a pad > > > * \param[in] pad The 0-indexed pad number to enumerate formats on > > > @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > > > return 0; > > > } > > > > > > +int V4L2Subdevice::getSelection(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; > > > > You can drop this as you initialise sel to 0. > > > > > + > > > + 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; > > > +} > > > + > > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Aug 19, 2019 at 09:37:26AM +0200, Jacopo Mondi wrote: > On Sat, Aug 17, 2019 at 07:25:47PM +0300, Laurent Pinchart wrote: > > On Sat, Aug 17, 2019 at 07:19:43PM +0300, Laurent Pinchart wrote: > >> On Sat, Aug 17, 2019 at 12:59:36PM +0200, Jacopo Mondi wrote: > >>> Add a private operation to perform G_SELECTION ioctl on the v4l2 > >>> subdevice and create two public methods to retrieve the crop bound > >>> rectangle and the subdevice native area size. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> src/libcamera/include/v4l2_subdevice.h | 4 +++ > >>> src/libcamera/v4l2_subdevice.cpp | 48 ++++++++++++++++++++++++++ > >>> 2 files changed, 52 insertions(+) > >>> > >>> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h > >>> index 9c077674f997..5316617c6873 100644 > >>> --- a/src/libcamera/include/v4l2_subdevice.h > >>> +++ b/src/libcamera/include/v4l2_subdevice.h > >>> @@ -43,6 +43,8 @@ public: > >>> > >>> int setCrop(unsigned int pad, Rectangle *rect); > >>> int setCompose(unsigned int pad, Rectangle *rect); > >>> + int getCropBounds(unsigned int pad, Rectangle *rect); > >>> + int getNativeSize(unsigned int pad, Rectangle *rect); > >> > >> Should these two methods be named cropBounds() and nativeSize() ? > >> They don't map directly to a VIDIOC_SUBDEV_G_... ioctl. And should they > >> return a Rectangle instance instead of taking it through an argument ? > >> Finally, should we remove the pad argument ? The native size is the > >> native size of the sensor, not the size of a pad. > > > > My bad, for some reason I though I was reviewing the CameraSensor class. > > Please ignore all comments about the getCropBounds and getNativeSize > > methods. I wonder, however, if we shouldn't make the get/set selection > > methods public (and remove setCrop() and setCompose()), otherwise we'll > > keep adding accessors for all targets, and I'm not sure it's worth it. > > If you really think dedicated accessors are best I would at least make > > them inline and move the get methods before the set methods. > > I had the same concern.. We'll keep adding wrappers and wrappers. > However this is not too bad, the other way around would be what you > said, but this will then require user of the subdev API to use V4L2 > defined flags (which is not a big issue, as they're already using a > -v4l2-_subdevice instance after all) Exactly, this class is specific to V4L2 subdevs, so I think we can use the V4L2 API :-) > >>> > >>> ImageFormats formats(unsigned int pad); > >>> > >>> @@ -62,6 +64,8 @@ private: > >>> > >>> int setSelection(unsigned int pad, unsigned int target, > >>> Rectangle *rect); > >>> + int getSelection(unsigned int pad, unsigned int target, > >>> + Rectangle *rect); > >> > >> I would move getSelection() above setSelection(). > > OK > > >>> > >>> const MediaEntity *entity_; > >>> }; > >>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > >>> index a188298de34c..5e661e2ef1fc 100644 > >>> --- a/src/libcamera/v4l2_subdevice.cpp > >>> +++ b/src/libcamera/v4l2_subdevice.cpp > >>> @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) > >>> return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); > >>> } > >>> > >>> +/** > >>> + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads > >> > >> I think this method shouldn't use V4L2 terminology, but explain in > >> clearer terms what it retrieves. It should thus likely be renamed. > >> > >>> + * \param[in] pad The 0-indexed pad number the rectangle is retrived from > >>> + * \param[out] rect The rectangle describing the crop bounds area > >>> + * \return 0 on success or a negative error code otherwise > >>> + */ > >>> +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect) > >>> +{ > >>> + return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect); > >>> +} > >>> + > >>> +/** > >>> + * \brief Get the native area size on one of the V4L2 subdevice pads > >> > >> If you rename the method to nativeSize(), I would write the brief as > >> "Retrieve the ..." as we usually do. > > Ignoring the renaming part, but I should probably use the usual > "Retrieve" here. If you turn these functions into getSelection and setSelection, I think "get" and "set" are good for the documentation. > >>> + * \param[in] pad The 0-indexed pad number the native area is retrieved from > >>> + * \param[out] rect The rectangle describing the native size area > >>> + * \return 0 on success or a negative error code otherwise > >>> + */ > >>> +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect) > >> > >> Should this be renamted to pixelArraySize() and return a Size ? > >> > >>> +{ > >>> + return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect); > >>> +} > >>> + > >>> /** > >>> * \brief Enumerate all media bus codes and frame sizes on a \a pad > >>> * \param[in] pad The 0-indexed pad number to enumerate formats on > >>> @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, > >>> return 0; > >>> } > >>> > >>> +int V4L2Subdevice::getSelection(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; > >> > >> You can drop this as you initialise sel to 0. > >> > >>> + > >>> + 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; > >>> +} > >>> + > >>> } /* namespace libcamera */
diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h index 9c077674f997..5316617c6873 100644 --- a/src/libcamera/include/v4l2_subdevice.h +++ b/src/libcamera/include/v4l2_subdevice.h @@ -43,6 +43,8 @@ public: int setCrop(unsigned int pad, Rectangle *rect); int setCompose(unsigned int pad, Rectangle *rect); + int getCropBounds(unsigned int pad, Rectangle *rect); + int getNativeSize(unsigned int pad, Rectangle *rect); ImageFormats formats(unsigned int pad); @@ -62,6 +64,8 @@ private: int setSelection(unsigned int pad, unsigned int target, Rectangle *rect); + int getSelection(unsigned int pad, unsigned int target, + Rectangle *rect); const MediaEntity *entity_; }; diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index a188298de34c..5e661e2ef1fc 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -148,6 +148,28 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect) return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect); } +/** + * \brief Get the crop bounds rectangle on one of the V4L2 subdevice pads + * \param[in] pad The 0-indexed pad number the rectangle is retrived from + * \param[out] rect The rectangle describing the crop bounds area + * \return 0 on success or a negative error code otherwise + */ +int V4L2Subdevice::getCropBounds(unsigned int pad, Rectangle *rect) +{ + return getSelection(pad, V4L2_SEL_TGT_CROP_BOUNDS, rect); +} + +/** + * \brief Get the native area size on one of the V4L2 subdevice pads + * \param[in] pad The 0-indexed pad number the native area is retrieved from + * \param[out] rect The rectangle describing the native size area + * \return 0 on success or a negative error code otherwise + */ +int V4L2Subdevice::getNativeSize(unsigned int pad, Rectangle *rect) +{ + return getSelection(pad, V4L2_SEL_TGT_NATIVE_SIZE, rect); +} + /** * \brief Enumerate all media bus codes and frame sizes on a \a pad * \param[in] pad The 0-indexed pad number to enumerate formats on @@ -360,4 +382,30 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target, return 0; } +int V4L2Subdevice::getSelection(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; + + 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; +} + } /* namespace libcamera */
Add a private operation to perform G_SELECTION ioctl on the v4l2 subdevice and create two public methods to retrieve the crop bound rectangle and the subdevice native area size. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_subdevice.h | 4 +++ src/libcamera/v4l2_subdevice.cpp | 48 ++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)