[libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 7 14:27:10 CEST 2020
Hi Niklas,
On Tue, Apr 07, 2020 at 02:22:09PM +0200, Niklas Söderlund wrote:
> On 2020-04-04 04:34:02 +0300, Laurent Pinchart wrote:
> > On Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:
> > > The libcamera Rectangle struct has no constructor that allows to
> > > construct statically initialized instances. Furthermore, its class
> > > members that represents the rectangle horizontal and vertical sizes are
> > > named 'w' and 'h' compared to the Size and SizeRange classes which uses
> > > the 'width' and 'height', which every time results in having to look at
> > > class definition to know which name to use.
> >
> > Good point regarding w and h, it makes sense to standardize them. I'm
> > sure Niklas would ask for two separate patches though :-)
>
> In a perfect world, but I have given up on that ;-)
>
> > > Add a constructor that takes the rectangle 4 sizes and force generation
> > > of a default constructor, and while at there rationalize class members
> > > names to 'width' and 'height'.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/geometry.h | 10 ++++++++--
> > > src/libcamera/geometry.cpp | 22 ++++++++++++++++++----
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +------
> > > src/libcamera/v4l2_subdevice.cpp | 8 ++++----
> > > src/libcamera/v4l2_videodevice.cpp | 8 ++++----
> > > 5 files changed, 35 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 7f1b29fe8c19..5eb4e72e365c 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -13,10 +13,16 @@
> > > namespace libcamera {
> > >
> > > struct Rectangle {
> > > + Rectangle() = default;
> > > + Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)
> > > + : x(x_), y(y_), width(width_), height(height_)
> > > + {
> > > + }
> > > +
> > > int x;
> > > int y;
> > > - unsigned int w;
> > > - unsigned int h;
> > > + unsigned int width;
> > > + unsigned int height;
> > >
> > > const std::string toString() const;
> > > };
> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > > index 13f642be526f..97dbdcf301b9 100644
> > > --- a/src/libcamera/geometry.cpp
> > > +++ b/src/libcamera/geometry.cpp
> > > @@ -29,6 +29,20 @@ namespace libcamera {
> > > * refers to, are defined by the context were rectangle is used.
> > > */
> > >
> > > +/**
> > > + * \fn Rectangle::Rectangle
> > > + * \brief Construct a Rectangle with all sizes initialized to 0
> > > + */
> > > +
> > > +/**
> > > + * \fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)
> > > + * \brief Construct a Rectangle with its sizes initialized
> > > + * \param[in] x_: The Rectangle top-left corner horizontal displacement
> > > + * \param[in] y_: The Rectangle top-left corner vertical displacement
> > > + * \param[in] width_: The Rectangle horizontal size
> > > + * \param[in] height_: The Rectangle vertical size
> > > + */
> > > +
> > > /**
> > > * \var Rectangle::x
> > > * \brief The horizontal coordinate of the rectangle's top-left corner
> > > @@ -40,12 +54,12 @@ namespace libcamera {
> > > */
> > >
> > > /**
> > > - * \var Rectangle::w
> > > + * \var Rectangle::width
> > > * \brief The distance between the left and right sides
> > > */
> > >
> > > /**
> > > - * \var Rectangle::h
> > > + * \var Rectangle::height
> > > * \brief The distance between the top and bottom sides
> > > */
> > >
> > > @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const
> > > {
> > > std::stringstream ss;
> > >
> > > - ss << "(" << x << "x" << y << ")/" << w << "x" << h;
> > > + ss << "(" << x << "x" << y << ")/" << width << "x" << height;
> > >
> > > return ss.str();
> > > }
> > > @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const
> > > bool operator==(const Rectangle &lhs, const Rectangle &rhs)
> > > {
> > > return lhs.x == rhs.x && lhs.y == rhs.y &&
> > > - lhs.w == rhs.w && lhs.h == rhs.h;
> > > + lhs.width == rhs.width && lhs.height == rhs.height;
> > > }
> > >
> > > /**
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 1e114ca7ed10..7e15ad28bef2 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,
> > > * to configure the crop/compose rectangles, contradicting the
> > > * V4L2 specification.
> > > */
> > > - Rectangle rect = {
> > > - .x = 0,
> > > - .y = 0,
> > > - .w = inputFormat->size.width,
> > > - .h = inputFormat->size.height,
> > > - };
> > > + Rectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);
> >
> > Do we really need a constructor for this ? We can already do
> >
> > Rectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};
> >
> > and we can also do
> >
> > Rectangle rect{};
> >
> > to initialize everything to 0. What does an explicit constructor bring
> > us, are there benefits I would be overlooking at this time of the night
> > ?
>
> My preference would be a constructor or to keep the current way of
> creating the Rectangle with the struck fields named.
Designated initializers are unfortunately a C++20 feature :-( I've seen
some random breakages when using them before in our code base, with
different compiler versions. I haven't investigated what a reasonable
baseline for designated initializer support would be, but it would be
worth it.
As for using a constructor vs. non-designated initializers, it would
really be about
- Rectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};
+ Rectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);
The only important difference I can see is that the compiler will cast
signed integers to unsigned integers with (), but not with {}, forcing
us to explicitly handle sign conversion with static_cast. It's a bit
more code to type, but it also forces us to think about the conversion,
potentially avoiding bugs.
> > > ret = imgu_->setCrop(PAD_INPUT, &rect);
> > > if (ret)
> > > return ret;
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 8b9da81e8ab3..50fd26d99d9c 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >
> > > sel.r.left = rect->x;
> > > sel.r.top = rect->y;
> > > - sel.r.width = rect->w;
> > > - sel.r.height = rect->h;
> > > + sel.r.width = rect->width;
> > > + sel.r.height = rect->height;
> > >
> > > int ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);
> > > if (ret < 0) {
> > > @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > >
> > > rect->x = sel.r.left;
> > > rect->y = sel.r.top;
> > > - rect->w = sel.r.width;
> > > - rect->h = sel.r.height;
> > > + rect->width = sel.r.width;
> > > + rect->height = sel.r.height;
> > >
> > > return 0;
> > > }
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index eb33a68e50d6..6e8f230f593d 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)
> > >
> > > sel.r.left = rect->x;
> > > sel.r.top = rect->y;
> > > - sel.r.width = rect->w;
> > > - sel.r.height = rect->h;
> > > + sel.r.width = rect->width;
> > > + sel.r.height = rect->height;
> > >
> > > int ret = ioctl(VIDIOC_S_SELECTION, &sel);
> > > if (ret < 0) {
> > > @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)
> > >
> > > rect->x = sel.r.left;
> > > rect->y = sel.r.top;
> > > - rect->w = sel.r.width;
> > > - rect->h = sel.r.height;
> > > + rect->width = sel.r.width;
> > > + rect->height = sel.r.height;
> > >
> > > return 0;
> > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list