[libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle

[Laurent Pinchart]

Hi Jacopo,

On Mon, Apr 20, 2020 at 10:19:57AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 07, 2020 at 03:27:10PM +0300, Laurent Pinchart wrote:
> > 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.
> 
> I wanted to avoid implicit conversion, let alone the fact I'm
> personally not found of list initializiers for object initialization :)
> 
> Actually, not allowing implicit conversion gives us more headache than
> not, considering most of our Control<int> are initialized statically
> with unsigned integers.

I'm really in two minds about the curly braces initialization and its
rejection of implicit integer conversions. It can be annoying as you've
mentioned, but it also forces us to think about the conversion when
writing the program, and thus hopefully avoid some of the sign
conversion bugs. A bit painful, but maybe for our own good :-)

> I would be permissive here and let the compiler help us, so I could
> here drop the constructor.
> 
> >>>>  	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