[{"id":4382,"web_url":"https://patchwork.libcamera.org/comment/4382/","msgid":"<20200404013402.GI9690@pendragon.ideasonboard.com>","date":"2020-04-04T01:34:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:\n> The libcamera Rectangle struct has no constructor that allows to\n> construct statically initialized instances. Furthermore, its class\n> members that represents the rectangle horizontal and vertical sizes are\n> named 'w' and 'h' compared to the Size and SizeRange classes which uses\n> the 'width' and 'height', which every time results in having to look at\n> class definition to know which name to use.\n\nGood point regarding w and h, it makes sense to standardize them. I'm\nsure Niklas would ask for two separate patches though :-)\n\n> Add a constructor that takes the rectangle 4 sizes and force generation\n> of a default constructor, and while at there rationalize class members\n> names to 'width' and 'height'.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/geometry.h | 10 ++++++++--\n> src/libcamera/geometry.cpp | 22 ++++++++++++++++++----\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +------\n> src/libcamera/v4l2_subdevice.cpp | 8 ++++----\n> src/libcamera/v4l2_videodevice.cpp | 8 ++++----\n> 5 files changed, 35 insertions(+), 20 deletions(-)\n> \n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 7f1b29fe8c19..5eb4e72e365c 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -13,10 +13,16 @@\n> namespace libcamera {\n> \n> struct Rectangle {\n> +\tRectangle() = default;\n> +\tRectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> +\t\t: x(x_), y(y_), width(width_), height(height_)\n> +\t{\n> +\t}\n> +\n> \tint x;\n> \tint y;\n> -\tunsigned int w;\n> -\tunsigned int h;\n> +\tunsigned int width;\n> +\tunsigned int height;\n> \n> \tconst std::string toString() const;\n> };\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index 13f642be526f..97dbdcf301b9 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -29,6 +29,20 @@ namespace libcamera {\n> * refers to, are defined by the context were rectangle is used.\n> */\n> \n> +/**\n> + * \\fn Rectangle::Rectangle\n> + * \\brief Construct a Rectangle with all sizes initialized to 0\n> + */\n> +\n> +/**\n> + * \\fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> + * \\brief Construct a Rectangle with its sizes initialized\n> + * \\param[in] x_: The Rectangle top-left corner horizontal displacement\n> + * \\param[in] y_: The Rectangle top-left corner vertical displacement\n> + * \\param[in] width_: The Rectangle horizontal size\n> + * \\param[in] height_: The Rectangle vertical size\n> + */\n> +\n> /**\n> * \\var Rectangle::x\n> * \\brief The horizontal coordinate of the rectangle's top-left corner\n> @@ -40,12 +54,12 @@ namespace libcamera {\n> */\n> \n> /**\n> - * \\var Rectangle::w\n> + * \\var Rectangle::width\n> * \\brief The distance between the left and right sides\n> */\n> \n> /**\n> - * \\var Rectangle::h\n> + * \\var Rectangle::height\n> * \\brief The distance between the top and bottom sides\n> */\n> \n> @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const\n> {\n> \tstd::stringstream ss;\n> \n> -\tss << \"(\" << x << \"x\" << y << \")/\" << w << \"x\" << h;\n> +\tss << \"(\" << x << \"x\" << y << \")/\" << width << \"x\" << height;\n> \n> \treturn ss.str();\n> }\n> @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const\n> bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> {\n> \treturn lhs.x == rhs.x && lhs.y == rhs.y &&\n> -\t lhs.w == rhs.w && lhs.h == rhs.h;\n> +\t lhs.width == rhs.width && lhs.height == rhs.height;\n> }\n> \n> /**\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 1e114ca7ed10..7e15ad28bef2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,\n> \t * to configure the crop/compose rectangles, contradicting the\n> \t * V4L2 specification.\n> \t */\n> -\tRectangle rect = {\n> -\t\t.x = 0,\n> -\t\t.y = 0,\n> -\t\t.w = inputFormat->size.width,\n> -\t\t.h = inputFormat->size.height,\n> -\t};\n> +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n\nDo we really need a constructor for this ? We can already do\n\n\tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n\nand we can also do\n\n\tRectangle rect{};\n\nto initialize everything to 0. What does an explicit constructor bring\nus, are there benefits I would be overlooking at this time of the night\n?\n\n> \tret = imgu_->setCrop(PAD_INPUT, &rect);\n> \tif (ret)\n> \t\treturn ret;\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 8b9da81e8ab3..50fd26d99d9c 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \n> \tsel.r.left = rect->x;\n> \tsel.r.top = rect->y;\n> -\tsel.r.width = rect->w;\n> -\tsel.r.height = rect->h;\n> +\tsel.r.width = rect->width;\n> +\tsel.r.height = rect->height;\n> \n> \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> \tif (ret < 0) {\n> @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \n> \trect->x = sel.r.left;\n> \trect->y = sel.r.top;\n> -\trect->w = sel.r.width;\n> -\trect->h = sel.r.height;\n> +\trect->width = sel.r.width;\n> +\trect->height = sel.r.height;\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index eb33a68e50d6..6e8f230f593d 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> \n> \tsel.r.left = rect->x;\n> \tsel.r.top = rect->y;\n> -\tsel.r.width = rect->w;\n> -\tsel.r.height = rect->h;\n> +\tsel.r.width = rect->width;\n> +\tsel.r.height = rect->height;\n> \n> \tint ret = ioctl(VIDIOC_S_SELECTION, &sel);\n> \tif (ret < 0) {\n> @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> \n> \trect->x = sel.r.left;\n> \trect->y = sel.r.top;\n> -\trect->w = sel.r.width;\n> -\trect->h = sel.r.height;\n> +\trect->width = sel.r.width;\n> +\trect->height = sel.r.height;\n> \n> \treturn 0;\n> }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A3D960409\n\tfor ;\n\tSat, 4 Apr 2020 03:34:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EDFDE321;\n\tSat, 4 Apr 2020 03:34:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BS1POqKB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585964051;\n\tbh=E7bIQTkDEWWUKDlTDH1/kGTGPDXXKW1eHqVop6Pq9Bs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BS1POqKBrX+gext6StfbvFJAGPyB8XLa4fXFk4oCDqmIOwMHl0srUmoE3pGx+aeZu\n\tFSa+AH3GxiD+n4RqIbXE5R8+W6nWA2DGsvtsFy+rjfcsXFu3tHii8eZNdPqQmpb7rG\n\tHbuNISHDxuBZWu6WvWlKYzNheNIm5t65bOWdrplQ=","Date":"Sat, 4 Apr 2020 04:34:02 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200404013402.GI9690@pendragon.ideasonboard.com>","References":"<20200403104327.3409564-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200403104327.3409564-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 04 Apr 2020 01:34:11 -0000"}},{"id":4388,"web_url":"https://patchwork.libcamera.org/comment/4388/","msgid":"<20200407122209.GB1716317@oden.dyn.berto.se>","date":"2020-04-07T12:22:09","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-04-04 04:34:02 +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:\n> > The libcamera Rectangle struct has no constructor that allows to\n> > construct statically initialized instances. Furthermore, its class\n> > members that represents the rectangle horizontal and vertical sizes are\n> > named 'w' and 'h' compared to the Size and SizeRange classes which uses\n> > the 'width' and 'height', which every time results in having to look at\n> > class definition to know which name to use.\n> \n> Good point regarding w and h, it makes sense to standardize them. I'm\n> sure Niklas would ask for two separate patches though :-)\n\nIn a perfect world, but I have given up on that ;-)\n\n> \n> > Add a constructor that takes the rectangle 4 sizes and force generation\n> > of a default constructor, and while at there rationalize class members\n> > names to 'width' and 'height'.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/geometry.h | 10 ++++++++--\n> > src/libcamera/geometry.cpp | 22 ++++++++++++++++++----\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +------\n> > src/libcamera/v4l2_subdevice.cpp | 8 ++++----\n> > src/libcamera/v4l2_videodevice.cpp | 8 ++++----\n> > 5 files changed, 35 insertions(+), 20 deletions(-)\n> > \n> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > index 7f1b29fe8c19..5eb4e72e365c 100644\n> > --- a/include/libcamera/geometry.h\n> > +++ b/include/libcamera/geometry.h\n> > @@ -13,10 +13,16 @@\n> > namespace libcamera {\n> > \n> > struct Rectangle {\n> > +\tRectangle() = default;\n> > +\tRectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> > +\t\t: x(x_), y(y_), width(width_), height(height_)\n> > +\t{\n> > +\t}\n> > +\n> > \tint x;\n> > \tint y;\n> > -\tunsigned int w;\n> > -\tunsigned int h;\n> > +\tunsigned int width;\n> > +\tunsigned int height;\n> > \n> > \tconst std::string toString() const;\n> > };\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index 13f642be526f..97dbdcf301b9 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -29,6 +29,20 @@ namespace libcamera {\n> > * refers to, are defined by the context were rectangle is used.\n> > */\n> > \n> > +/**\n> > + * \\fn Rectangle::Rectangle\n> > + * \\brief Construct a Rectangle with all sizes initialized to 0\n> > + */\n> > +\n> > +/**\n> > + * \\fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> > + * \\brief Construct a Rectangle with its sizes initialized\n> > + * \\param[in] x_: The Rectangle top-left corner horizontal displacement\n> > + * \\param[in] y_: The Rectangle top-left corner vertical displacement\n> > + * \\param[in] width_: The Rectangle horizontal size\n> > + * \\param[in] height_: The Rectangle vertical size\n> > + */\n> > +\n> > /**\n> > * \\var Rectangle::x\n> > * \\brief The horizontal coordinate of the rectangle's top-left corner\n> > @@ -40,12 +54,12 @@ namespace libcamera {\n> > */\n> > \n> > /**\n> > - * \\var Rectangle::w\n> > + * \\var Rectangle::width\n> > * \\brief The distance between the left and right sides\n> > */\n> > \n> > /**\n> > - * \\var Rectangle::h\n> > + * \\var Rectangle::height\n> > * \\brief The distance between the top and bottom sides\n> > */\n> > \n> > @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const\n> > {\n> > \tstd::stringstream ss;\n> > \n> > -\tss << \"(\" << x << \"x\" << y << \")/\" << w << \"x\" << h;\n> > +\tss << \"(\" << x << \"x\" << y << \")/\" << width << \"x\" << height;\n> > \n> > \treturn ss.str();\n> > }\n> > @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const\n> > bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> > {\n> > \treturn lhs.x == rhs.x && lhs.y == rhs.y &&\n> > -\t lhs.w == rhs.w && lhs.h == rhs.h;\n> > +\t lhs.width == rhs.width && lhs.height == rhs.height;\n> > }\n> > \n> > /**\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 1e114ca7ed10..7e15ad28bef2 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,\n> > \t * to configure the crop/compose rectangles, contradicting the\n> > \t * V4L2 specification.\n> > \t */\n> > -\tRectangle rect = {\n> > -\t\t.x = 0,\n> > -\t\t.y = 0,\n> > -\t\t.w = inputFormat->size.width,\n> > -\t\t.h = inputFormat->size.height,\n> > -\t};\n> > +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n> \n> Do we really need a constructor for this ? We can already do\n> \n> \tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n> \n> and we can also do\n> \n> \tRectangle rect{};\n> \n> to initialize everything to 0. What does an explicit constructor bring\n> us, are there benefits I would be overlooking at this time of the night\n> ?\n\nMy preference would be a constructor or to keep the current way of \ncreating the Rectangle with the struck fields named.\n\n> \n> > \tret = imgu_->setCrop(PAD_INPUT, &rect);\n> > \tif (ret)\n> > \t\treturn ret;\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 8b9da81e8ab3..50fd26d99d9c 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > \n> > \tsel.r.left = rect->x;\n> > \tsel.r.top = rect->y;\n> > -\tsel.r.width = rect->w;\n> > -\tsel.r.height = rect->h;\n> > +\tsel.r.width = rect->width;\n> > +\tsel.r.height = rect->height;\n> > \n> > \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> > \tif (ret < 0) {\n> > @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > \n> > \trect->x = sel.r.left;\n> > \trect->y = sel.r.top;\n> > -\trect->w = sel.r.width;\n> > -\trect->h = sel.r.height;\n> > +\trect->width = sel.r.width;\n> > +\trect->height = sel.r.height;\n> > \n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index eb33a68e50d6..6e8f230f593d 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > \n> > \tsel.r.left = rect->x;\n> > \tsel.r.top = rect->y;\n> > -\tsel.r.width = rect->w;\n> > -\tsel.r.height = rect->h;\n> > +\tsel.r.width = rect->width;\n> > +\tsel.r.height = rect->height;\n> > \n> > \tint ret = ioctl(VIDIOC_S_SELECTION, &sel);\n> > \tif (ret < 0) {\n> > @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > \n> > \trect->x = sel.r.left;\n> > \trect->y = sel.r.top;\n> > -\trect->w = sel.r.width;\n> > -\trect->h = sel.r.height;\n> > +\trect->width = sel.r.width;\n> > +\trect->height = sel.r.height;\n> > \n> > \treturn 0;\n> > }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8F59600F0\n\tfor ;\n\tTue, 7 Apr 2020 14:22:11 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id r24so3476518ljd.4\n\tfor ;\n\tTue, 07 Apr 2020 05:22:11 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\th3sm13647694lfk.30.2020.04.07.05.22.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 07 Apr 2020 05:22:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"ZjGNwpVX\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=6qSfYav06WdbdgOkDgi8jUZyFGU7jm7gDuHSgCx4yMc=;\n\tb=ZjGNwpVXR2rK9wmtLtmx+JDdcw6RBjE/xqP4Kzd6dokr8+BmaHyIl10pRZzFGG+hEW\n\tjuUS785uwqVBmHnKNzWTiJ+tiIuy2yTG6afzU8x4Omweek8zvRMDq4Cf/RkenCzmfo0s\n\tCxYjmA2n2UqFTNf190Wz9RyQ484le2P2xshvYX8QfyC0lZLQt7GHrnoYh3vW9OT5xfLx\n\tWrzY60cK392ffGUz9cQeEA3pZ4+DqOk/nZljVJfoep8eRZaCW9rD6f5CXZ8KTCV9G5zD\n\t6rTuyfTQ4cqlTQvxCBgJOcUiTe8C+QIOmSE7w2yWhzeDK8qIA6V893xYLNdcmKBi57Q4\n\tyl0Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=6qSfYav06WdbdgOkDgi8jUZyFGU7jm7gDuHSgCx4yMc=;\n\tb=XEbL4TmikowHdISsuWk1fwraXeu4yU1qVS2OGKZPPBisswVqxgGXRev2ahmeCIvr64\n\tN4qjayHvz7mytDor7Ok+aNjgqF2qxCkEOxIM8VY82j7sd2ocn+fN+oodiwJ5d2DklOY/\n\tMnXgqmTiDzmOMYSUIA3oUBVWr7yuo8/HywLNTWsLHlJeHt3V2jB8Oz3EaWWr0kr3FRe+\n\t3TxbshdxkNsWELU7v1VSbw31yfoTXO/y9CbYBvJFsx1N4l1T2/PgnOEWxc5p85J9XnTB\n\tnXmcI6I/ty9ZBpsh0fXRjjzA8l2wOkGAf7R6moCq/V7GnBzSb6rk0rCxzfwaLYlLk5A4\n\t6fhQ==","X-Gm-Message-State":"AGi0PubH3OLqZQNgxaQR4lwAkplVzFcKYCkCiopf9gm860aXEaixZXm/\n\tUSCnmXe7HdBINoLmoC8NRcif5A==","X-Google-Smtp-Source":"APiQypLBWrzaaGsfmxSKLdHeHBjIUCy3NRBlTEUBk1J57RKr5zxGZiKNybvNGHaDkNOljBE3dBunAA==","X-Received":"by 2002:a2e:989a:: with SMTP id\n\tb26mr1612259ljj.274.1586262130828; \n\tTue, 07 Apr 2020 05:22:10 -0700 (PDT)","Date":"Tue, 7 Apr 2020 14:22:09 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200407122209.GB1716317@oden.dyn.berto.se>","References":"<20200403104327.3409564-1-jacopo@jmondi.org>\n\t<20200404013402.GI9690@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200404013402.GI9690@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 07 Apr 2020 12:22:11 -0000"}},{"id":4389,"web_url":"https://patchwork.libcamera.org/comment/4389/","msgid":"<20200407122710.GE4751@pendragon.ideasonboard.com>","date":"2020-04-07T12:27:10","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Apr 07, 2020 at 02:22:09PM +0200, Niklas Söderlund wrote:\n> On 2020-04-04 04:34:02 +0300, Laurent Pinchart wrote:\n> > On Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:\n> > > The libcamera Rectangle struct has no constructor that allows to\n> > > construct statically initialized instances. Furthermore, its class\n> > > members that represents the rectangle horizontal and vertical sizes are\n> > > named 'w' and 'h' compared to the Size and SizeRange classes which uses\n> > > the 'width' and 'height', which every time results in having to look at\n> > > class definition to know which name to use.\n> > \n> > Good point regarding w and h, it makes sense to standardize them. I'm\n> > sure Niklas would ask for two separate patches though :-)\n> \n> In a perfect world, but I have given up on that ;-)\n> \n> > > Add a constructor that takes the rectangle 4 sizes and force generation\n> > > of a default constructor, and while at there rationalize class members\n> > > names to 'width' and 'height'.\n> > > \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/geometry.h | 10 ++++++++--\n> > > src/libcamera/geometry.cpp | 22 ++++++++++++++++++----\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +------\n> > > src/libcamera/v4l2_subdevice.cpp | 8 ++++----\n> > > src/libcamera/v4l2_videodevice.cpp | 8 ++++----\n> > > 5 files changed, 35 insertions(+), 20 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > index 7f1b29fe8c19..5eb4e72e365c 100644\n> > > --- a/include/libcamera/geometry.h\n> > > +++ b/include/libcamera/geometry.h\n> > > @@ -13,10 +13,16 @@\n> > > namespace libcamera {\n> > > \n> > > struct Rectangle {\n> > > +\tRectangle() = default;\n> > > +\tRectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> > > +\t\t: x(x_), y(y_), width(width_), height(height_)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > \tint x;\n> > > \tint y;\n> > > -\tunsigned int w;\n> > > -\tunsigned int h;\n> > > +\tunsigned int width;\n> > > +\tunsigned int height;\n> > > \n> > > \tconst std::string toString() const;\n> > > };\n> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > index 13f642be526f..97dbdcf301b9 100644\n> > > --- a/src/libcamera/geometry.cpp\n> > > +++ b/src/libcamera/geometry.cpp\n> > > @@ -29,6 +29,20 @@ namespace libcamera {\n> > > * refers to, are defined by the context were rectangle is used.\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn Rectangle::Rectangle\n> > > + * \\brief Construct a Rectangle with all sizes initialized to 0\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> > > + * \\brief Construct a Rectangle with its sizes initialized\n> > > + * \\param[in] x_: The Rectangle top-left corner horizontal displacement\n> > > + * \\param[in] y_: The Rectangle top-left corner vertical displacement\n> > > + * \\param[in] width_: The Rectangle horizontal size\n> > > + * \\param[in] height_: The Rectangle vertical size\n> > > + */\n> > > +\n> > > /**\n> > > * \\var Rectangle::x\n> > > * \\brief The horizontal coordinate of the rectangle's top-left corner\n> > > @@ -40,12 +54,12 @@ namespace libcamera {\n> > > */\n> > > \n> > > /**\n> > > - * \\var Rectangle::w\n> > > + * \\var Rectangle::width\n> > > * \\brief The distance between the left and right sides\n> > > */\n> > > \n> > > /**\n> > > - * \\var Rectangle::h\n> > > + * \\var Rectangle::height\n> > > * \\brief The distance between the top and bottom sides\n> > > */\n> > > \n> > > @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const\n> > > {\n> > > \tstd::stringstream ss;\n> > > \n> > > -\tss << \"(\" << x << \"x\" << y << \")/\" << w << \"x\" << h;\n> > > +\tss << \"(\" << x << \"x\" << y << \")/\" << width << \"x\" << height;\n> > > \n> > > \treturn ss.str();\n> > > }\n> > > @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const\n> > > bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> > > {\n> > > \treturn lhs.x == rhs.x && lhs.y == rhs.y &&\n> > > -\t lhs.w == rhs.w && lhs.h == rhs.h;\n> > > +\t lhs.width == rhs.width && lhs.height == rhs.height;\n> > > }\n> > > \n> > > /**\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 1e114ca7ed10..7e15ad28bef2 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,\n> > > \t * to configure the crop/compose rectangles, contradicting the\n> > > \t * V4L2 specification.\n> > > \t */\n> > > -\tRectangle rect = {\n> > > -\t\t.x = 0,\n> > > -\t\t.y = 0,\n> > > -\t\t.w = inputFormat->size.width,\n> > > -\t\t.h = inputFormat->size.height,\n> > > -\t};\n> > > +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n> > \n> > Do we really need a constructor for this ? We can already do\n> > \n> > \tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n> > \n> > and we can also do\n> > \n> > \tRectangle rect{};\n> > \n> > to initialize everything to 0. What does an explicit constructor bring\n> > us, are there benefits I would be overlooking at this time of the night\n> > ?\n> \n> My preference would be a constructor or to keep the current way of \n> creating the Rectangle with the struck fields named.\n\nDesignated initializers are unfortunately a C++20 feature :-( I've seen\nsome random breakages when using them before in our code base, with\ndifferent compiler versions. I haven't investigated what a reasonable\nbaseline for designated initializer support would be, but it would be\nworth it.\n\nAs for using a constructor vs. non-designated initializers, it would\nreally be about\n\n-\tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n+\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n\nThe only important difference I can see is that the compiler will cast\nsigned integers to unsigned integers with (), but not with {}, forcing\nus to explicitly handle sign conversion with static_cast. It's a bit\nmore code to type, but it also forces us to think about the conversion,\npotentially avoiding bugs.\n\n> > > \tret = imgu_->setCrop(PAD_INPUT, &rect);\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index 8b9da81e8ab3..50fd26d99d9c 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > \n> > > \tsel.r.left = rect->x;\n> > > \tsel.r.top = rect->y;\n> > > -\tsel.r.width = rect->w;\n> > > -\tsel.r.height = rect->h;\n> > > +\tsel.r.width = rect->width;\n> > > +\tsel.r.height = rect->height;\n> > > \n> > > \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> > > \tif (ret < 0) {\n> > > @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > \n> > > \trect->x = sel.r.left;\n> > > \trect->y = sel.r.top;\n> > > -\trect->w = sel.r.width;\n> > > -\trect->h = sel.r.height;\n> > > +\trect->width = sel.r.width;\n> > > +\trect->height = sel.r.height;\n> > > \n> > > \treturn 0;\n> > > }\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index eb33a68e50d6..6e8f230f593d 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > > \n> > > \tsel.r.left = rect->x;\n> > > \tsel.r.top = rect->y;\n> > > -\tsel.r.width = rect->w;\n> > > -\tsel.r.height = rect->h;\n> > > +\tsel.r.width = rect->width;\n> > > +\tsel.r.height = rect->height;\n> > > \n> > > \tint ret = ioctl(VIDIOC_S_SELECTION, &sel);\n> > > \tif (ret < 0) {\n> > > @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > > \n> > > \trect->x = sel.r.left;\n> > > \trect->y = sel.r.top;\n> > > -\trect->w = sel.r.width;\n> > > -\trect->h = sel.r.height;\n> > > +\trect->width = sel.r.width;\n> > > +\trect->height = sel.r.height;\n> > > \n> > > \treturn 0;\n> > > }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1398600F0\n\tfor ;\n\tTue, 7 Apr 2020 14:27:19 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31CFC59E;\n\tTue, 7 Apr 2020 14:27:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ia5BWm8x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1586262439;\n\tbh=FEH/6H0jM4npiqFh5II1OVoqqcleeNzoGStbJ5dpwec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ia5BWm8xT4Fiwk4W0UYqZ7fGpcIY1T+De7CD1B9QAb8id1453N1m7dir0IbQiuwrg\n\tjQ7bBH7h9KBS67wTesZbR8GKlb6YfBN9+IiZvxG6DnvPU4otBhmRYSGccp+cVAxi+0\n\tXPuQyRl3UY9inwvG8xWCBIQwfWdnESi8MHgFJtIA=","Date":"Tue, 7 Apr 2020 15:27:10 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200407122710.GE4751@pendragon.ideasonboard.com>","References":"<20200403104327.3409564-1-jacopo@jmondi.org>\n\t<20200404013402.GI9690@pendragon.ideasonboard.com>\n\t<20200407122209.GB1716317@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200407122209.GB1716317@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 07 Apr 2020 12:27:20 -0000"}},{"id":4471,"web_url":"https://patchwork.libcamera.org/comment/4471/","msgid":"<20200420081957.vsc7azbzxrxh5z7y@uno.localdomain>","date":"2020-04-20T08:19:57","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 07, 2020 at 03:27:10PM +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n>\n> On Tue, Apr 07, 2020 at 02:22:09PM +0200, Niklas Söderlund wrote:\n> > On 2020-04-04 04:34:02 +0300, Laurent Pinchart wrote:\n> > > On Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:\n> > > > The libcamera Rectangle struct has no constructor that allows to\n> > > > construct statically initialized instances. Furthermore, its class\n> > > > members that represents the rectangle horizontal and vertical sizes are\n> > > > named 'w' and 'h' compared to the Size and SizeRange classes which uses\n> > > > the 'width' and 'height', which every time results in having to look at\n> > > > class definition to know which name to use.\n> > >\n> > > Good point regarding w and h, it makes sense to standardize them. I'm\n> > > sure Niklas would ask for two separate patches though :-)\n> >\n> > In a perfect world, but I have given up on that ;-)\n> >\n> > > > Add a constructor that takes the rectangle 4 sizes and force generation\n> > > > of a default constructor, and while at there rationalize class members\n> > > > names to 'width' and 'height'.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/geometry.h | 10 ++++++++--\n> > > > src/libcamera/geometry.cpp | 22 ++++++++++++++++++----\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +------\n> > > > src/libcamera/v4l2_subdevice.cpp | 8 ++++----\n> > > > src/libcamera/v4l2_videodevice.cpp | 8 ++++----\n> > > > 5 files changed, 35 insertions(+), 20 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > > index 7f1b29fe8c19..5eb4e72e365c 100644\n> > > > --- a/include/libcamera/geometry.h\n> > > > +++ b/include/libcamera/geometry.h\n> > > > @@ -13,10 +13,16 @@\n> > > > namespace libcamera {\n> > > >\n> > > > struct Rectangle {\n> > > > +\tRectangle() = default;\n> > > > +\tRectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> > > > +\t\t: x(x_), y(y_), width(width_), height(height_)\n> > > > +\t{\n> > > > +\t}\n> > > > +\n> > > > \tint x;\n> > > > \tint y;\n> > > > -\tunsigned int w;\n> > > > -\tunsigned int h;\n> > > > +\tunsigned int width;\n> > > > +\tunsigned int height;\n> > > >\n> > > > \tconst std::string toString() const;\n> > > > };\n> > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > > index 13f642be526f..97dbdcf301b9 100644\n> > > > --- a/src/libcamera/geometry.cpp\n> > > > +++ b/src/libcamera/geometry.cpp\n> > > > @@ -29,6 +29,20 @@ namespace libcamera {\n> > > > * refers to, are defined by the context were rectangle is used.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\fn Rectangle::Rectangle\n> > > > + * \\brief Construct a Rectangle with all sizes initialized to 0\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> > > > + * \\brief Construct a Rectangle with its sizes initialized\n> > > > + * \\param[in] x_: The Rectangle top-left corner horizontal displacement\n> > > > + * \\param[in] y_: The Rectangle top-left corner vertical displacement\n> > > > + * \\param[in] width_: The Rectangle horizontal size\n> > > > + * \\param[in] height_: The Rectangle vertical size\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\var Rectangle::x\n> > > > * \\brief The horizontal coordinate of the rectangle's top-left corner\n> > > > @@ -40,12 +54,12 @@ namespace libcamera {\n> > > > */\n> > > >\n> > > > /**\n> > > > - * \\var Rectangle::w\n> > > > + * \\var Rectangle::width\n> > > > * \\brief The distance between the left and right sides\n> > > > */\n> > > >\n> > > > /**\n> > > > - * \\var Rectangle::h\n> > > > + * \\var Rectangle::height\n> > > > * \\brief The distance between the top and bottom sides\n> > > > */\n> > > >\n> > > > @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const\n> > > > {\n> > > > \tstd::stringstream ss;\n> > > >\n> > > > -\tss << \"(\" << x << \"x\" << y << \")/\" << w << \"x\" << h;\n> > > > +\tss << \"(\" << x << \"x\" << y << \")/\" << width << \"x\" << height;\n> > > >\n> > > > \treturn ss.str();\n> > > > }\n> > > > @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const\n> > > > bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> > > > {\n> > > > \treturn lhs.x == rhs.x && lhs.y == rhs.y &&\n> > > > -\t lhs.w == rhs.w && lhs.h == rhs.h;\n> > > > +\t lhs.width == rhs.width && lhs.height == rhs.height;\n> > > > }\n> > > >\n> > > > /**\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 1e114ca7ed10..7e15ad28bef2 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,\n> > > > \t * to configure the crop/compose rectangles, contradicting the\n> > > > \t * V4L2 specification.\n> > > > \t */\n> > > > -\tRectangle rect = {\n> > > > -\t\t.x = 0,\n> > > > -\t\t.y = 0,\n> > > > -\t\t.w = inputFormat->size.width,\n> > > > -\t\t.h = inputFormat->size.height,\n> > > > -\t};\n> > > > +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n> > >\n> > > Do we really need a constructor for this ? We can already do\n> > >\n> > > \tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n> > >\n> > > and we can also do\n> > >\n> > > \tRectangle rect{};\n> > >\n> > > to initialize everything to 0. What does an explicit constructor bring\n> > > us, are there benefits I would be overlooking at this time of the night\n> > > ?\n> >\n> > My preference would be a constructor or to keep the current way of\n> > creating the Rectangle with the struck fields named.\n>\n> Designated initializers are unfortunately a C++20 feature :-( I've seen\n> some random breakages when using them before in our code base, with\n> different compiler versions. I haven't investigated what a reasonable\n> baseline for designated initializer support would be, but it would be\n> worth it.\n>\n> As for using a constructor vs. non-designated initializers, it would\n> really be about\n>\n> -\tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n> +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n>\n> The only important difference I can see is that the compiler will cast\n> signed integers to unsigned integers with (), but not with {}, forcing\n> us to explicitly handle sign conversion with static_cast. It's a bit\n> more code to type, but it also forces us to think about the conversion,\n> potentially avoiding bugs.\n\nI wanted to avoid implicit conversion, let alone the fact I'm\npersonally not found of list initializiers for object initialization :)\n\nActually, not allowing implicit conversion gives us more headache than\nnot, considering most of our Control are initialized statically\nwith unsigned integers. I would be permissive here and let the\ncompiler help us, so I could here drop the constructor.\n\nThanks\n j\n\n\n>\n> > > > \tret = imgu_->setCrop(PAD_INPUT, &rect);\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > > index 8b9da81e8ab3..50fd26d99d9c 100644\n> > > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > > @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > >\n> > > > \tsel.r.left = rect->x;\n> > > > \tsel.r.top = rect->y;\n> > > > -\tsel.r.width = rect->w;\n> > > > -\tsel.r.height = rect->h;\n> > > > +\tsel.r.width = rect->width;\n> > > > +\tsel.r.height = rect->height;\n> > > >\n> > > > \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> > > > \tif (ret < 0) {\n> > > > @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > >\n> > > > \trect->x = sel.r.left;\n> > > > \trect->y = sel.r.top;\n> > > > -\trect->w = sel.r.width;\n> > > > -\trect->h = sel.r.height;\n> > > > +\trect->width = sel.r.width;\n> > > > +\trect->height = sel.r.height;\n> > > >\n> > > > \treturn 0;\n> > > > }\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index eb33a68e50d6..6e8f230f593d 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > > >\n> > > > \tsel.r.left = rect->x;\n> > > > \tsel.r.top = rect->y;\n> > > > -\tsel.r.width = rect->w;\n> > > > -\tsel.r.height = rect->h;\n> > > > +\tsel.r.width = rect->width;\n> > > > +\tsel.r.height = rect->height;\n> > > >\n> > > > \tint ret = ioctl(VIDIOC_S_SELECTION, &sel);\n> > > > \tif (ret < 0) {\n> > > > @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > > >\n> > > > \trect->x = sel.r.left;\n> > > > \trect->y = sel.r.top;\n> > > > -\trect->w = sel.r.width;\n> > > > -\trect->h = sel.r.height;\n> > > > +\trect->width = sel.r.width;\n> > > > +\trect->height = sel.r.height;\n> > > >\n> > > > \treturn 0;\n> > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70C9B603FF\n\tfor ;\n\tMon, 20 Apr 2020 10:16:52 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id F38241BF212;\n\tMon, 20 Apr 2020 08:16:50 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 20 Apr 2020 10:19:57 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200420081957.vsc7azbzxrxh5z7y@uno.localdomain>","References":"<20200403104327.3409564-1-jacopo@jmondi.org>\n\t<20200404013402.GI9690@pendragon.ideasonboard.com>\n\t<20200407122209.GB1716317@oden.dyn.berto.se>\n\t<20200407122710.GE4751@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200407122710.GE4751@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 20 Apr 2020 08:16:52 -0000"}},{"id":4472,"web_url":"https://patchwork.libcamera.org/comment/4472/","msgid":"<20200420083541.GC5860@pendragon.ideasonboard.com>","date":"2020-04-20T08:35:41","subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 20, 2020 at 10:19:57AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 07, 2020 at 03:27:10PM +0300, Laurent Pinchart wrote:\n> > On Tue, Apr 07, 2020 at 02:22:09PM +0200, Niklas Söderlund wrote:\n> >> On 2020-04-04 04:34:02 +0300, Laurent Pinchart wrote:\n> >>> On Fri, Apr 03, 2020 at 12:43:27PM +0200, Jacopo Mondi wrote:\n> >>>> The libcamera Rectangle struct has no constructor that allows to\n> >>>> construct statically initialized instances. Furthermore, its class\n> >>>> members that represents the rectangle horizontal and vertical sizes are\n> >>>> named 'w' and 'h' compared to the Size and SizeRange classes which uses\n> >>>> the 'width' and 'height', which every time results in having to look at\n> >>>> class definition to know which name to use.\n> >>>\n> >>> Good point regarding w and h, it makes sense to standardize them. I'm\n> >>> sure Niklas would ask for two separate patches though :-)\n> >>\n> >> In a perfect world, but I have given up on that ;-)\n> >>\n> >>>> Add a constructor that takes the rectangle 4 sizes and force generation\n> >>>> of a default constructor, and while at there rationalize class members\n> >>>> names to 'width' and 'height'.\n> >>>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> include/libcamera/geometry.h | 10 ++++++++--\n> >>>> src/libcamera/geometry.cpp | 22 ++++++++++++++++++----\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +------\n> >>>> src/libcamera/v4l2_subdevice.cpp | 8 ++++----\n> >>>> src/libcamera/v4l2_videodevice.cpp | 8 ++++----\n> >>>> 5 files changed, 35 insertions(+), 20 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> >>>> index 7f1b29fe8c19..5eb4e72e365c 100644\n> >>>> --- a/include/libcamera/geometry.h\n> >>>> +++ b/include/libcamera/geometry.h\n> >>>> @@ -13,10 +13,16 @@\n> >>>> namespace libcamera {\n> >>>>\n> >>>> struct Rectangle {\n> >>>> +\tRectangle() = default;\n> >>>> +\tRectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> >>>> +\t\t: x(x_), y(y_), width(width_), height(height_)\n> >>>> +\t{\n> >>>> +\t}\n> >>>> +\n> >>>> \tint x;\n> >>>> \tint y;\n> >>>> -\tunsigned int w;\n> >>>> -\tunsigned int h;\n> >>>> +\tunsigned int width;\n> >>>> +\tunsigned int height;\n> >>>>\n> >>>> \tconst std::string toString() const;\n> >>>> };\n> >>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> >>>> index 13f642be526f..97dbdcf301b9 100644\n> >>>> --- a/src/libcamera/geometry.cpp\n> >>>> +++ b/src/libcamera/geometry.cpp\n> >>>> @@ -29,6 +29,20 @@ namespace libcamera {\n> >>>> * refers to, are defined by the context were rectangle is used.\n> >>>> */\n> >>>>\n> >>>> +/**\n> >>>> + * \\fn Rectangle::Rectangle\n> >>>> + * \\brief Construct a Rectangle with all sizes initialized to 0\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\fn Rectangle::Rectangle(int x_, int y_, unsigned int width_, unsigned int height_)\n> >>>> + * \\brief Construct a Rectangle with its sizes initialized\n> >>>> + * \\param[in] x_: The Rectangle top-left corner horizontal displacement\n> >>>> + * \\param[in] y_: The Rectangle top-left corner vertical displacement\n> >>>> + * \\param[in] width_: The Rectangle horizontal size\n> >>>> + * \\param[in] height_: The Rectangle vertical size\n> >>>> + */\n> >>>> +\n> >>>> /**\n> >>>> * \\var Rectangle::x\n> >>>> * \\brief The horizontal coordinate of the rectangle's top-left corner\n> >>>> @@ -40,12 +54,12 @@ namespace libcamera {\n> >>>> */\n> >>>>\n> >>>> /**\n> >>>> - * \\var Rectangle::w\n> >>>> + * \\var Rectangle::width\n> >>>> * \\brief The distance between the left and right sides\n> >>>> */\n> >>>>\n> >>>> /**\n> >>>> - * \\var Rectangle::h\n> >>>> + * \\var Rectangle::height\n> >>>> * \\brief The distance between the top and bottom sides\n> >>>> */\n> >>>>\n> >>>> @@ -57,7 +71,7 @@ const std::string Rectangle::toString() const\n> >>>> {\n> >>>> \tstd::stringstream ss;\n> >>>>\n> >>>> -\tss << \"(\" << x << \"x\" << y << \")/\" << w << \"x\" << h;\n> >>>> +\tss << \"(\" << x << \"x\" << y << \")/\" << width << \"x\" << height;\n> >>>>\n> >>>> \treturn ss.str();\n> >>>> }\n> >>>> @@ -69,7 +83,7 @@ const std::string Rectangle::toString() const\n> >>>> bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> >>>> {\n> >>>> \treturn lhs.x == rhs.x && lhs.y == rhs.y &&\n> >>>> -\t lhs.w == rhs.w && lhs.h == rhs.h;\n> >>>> +\t lhs.width == rhs.width && lhs.height == rhs.height;\n> >>>> }\n> >>>>\n> >>>> /**\n> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> index 1e114ca7ed10..7e15ad28bef2 100644\n> >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> @@ -1129,12 +1129,7 @@ int ImgUDevice::configureInput(const Size &size,\n> >>>> \t * to configure the crop/compose rectangles, contradicting the\n> >>>> \t * V4L2 specification.\n> >>>> \t */\n> >>>> -\tRectangle rect = {\n> >>>> -\t\t.x = 0,\n> >>>> -\t\t.y = 0,\n> >>>> -\t\t.w = inputFormat->size.width,\n> >>>> -\t\t.h = inputFormat->size.height,\n> >>>> -\t};\n> >>>> +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n> >>>\n> >>> Do we really need a constructor for this ? We can already do\n> >>>\n> >>> \tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n> >>>\n> >>> and we can also do\n> >>>\n> >>> \tRectangle rect{};\n> >>>\n> >>> to initialize everything to 0. What does an explicit constructor bring\n> >>> us, are there benefits I would be overlooking at this time of the night\n> >>> ?\n> >>\n> >> My preference would be a constructor or to keep the current way of\n> >> creating the Rectangle with the struck fields named.\n> >\n> > Designated initializers are unfortunately a C++20 feature :-( I've seen\n> > some random breakages when using them before in our code base, with\n> > different compiler versions. I haven't investigated what a reasonable\n> > baseline for designated initializer support would be, but it would be\n> > worth it.\n> >\n> > As for using a constructor vs. non-designated initializers, it would\n> > really be about\n> >\n> > -\tRectangle rect{0, 0, inputFormat->size.width, inputFormat->size.height};\n> > +\tRectangle rect(0, 0, inputFormat->size.width, inputFormat->size.height);\n> >\n> > The only important difference I can see is that the compiler will cast\n> > signed integers to unsigned integers with (), but not with {}, forcing\n> > us to explicitly handle sign conversion with static_cast. It's a bit\n> > more code to type, but it also forces us to think about the conversion,\n> > potentially avoiding bugs.\n> \n> I wanted to avoid implicit conversion, let alone the fact I'm\n> personally not found of list initializiers for object initialization :)\n> \n> Actually, not allowing implicit conversion gives us more headache than\n> not, considering most of our Control are initialized statically\n> with unsigned integers.\n\nI'm really in two minds about the curly braces initialization and its\nrejection of implicit integer conversions. It can be annoying as you've\nmentioned, but it also forces us to think about the conversion when\nwriting the program, and thus hopefully avoid some of the sign\nconversion bugs. A bit painful, but maybe for our own good :-)\n\n> I would be permissive here and let the compiler help us, so I could\n> here drop the constructor.\n> \n> >>>> \tret = imgu_->setCrop(PAD_INPUT, &rect);\n> >>>> \tif (ret)\n> >>>> \t\treturn ret;\n> >>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> >>>> index 8b9da81e8ab3..50fd26d99d9c 100644\n> >>>> --- a/src/libcamera/v4l2_subdevice.cpp\n> >>>> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >>>> @@ -339,8 +339,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >>>>\n> >>>> \tsel.r.left = rect->x;\n> >>>> \tsel.r.top = rect->y;\n> >>>> -\tsel.r.width = rect->w;\n> >>>> -\tsel.r.height = rect->h;\n> >>>> +\tsel.r.width = rect->width;\n> >>>> +\tsel.r.height = rect->height;\n> >>>>\n> >>>> \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> >>>> \tif (ret < 0) {\n> >>>> @@ -352,8 +352,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >>>>\n> >>>> \trect->x = sel.r.left;\n> >>>> \trect->y = sel.r.top;\n> >>>> -\trect->w = sel.r.width;\n> >>>> -\trect->h = sel.r.height;\n> >>>> +\trect->width = sel.r.width;\n> >>>> +\trect->height = sel.r.height;\n> >>>>\n> >>>> \treturn 0;\n> >>>> }\n> >>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >>>> index eb33a68e50d6..6e8f230f593d 100644\n> >>>> --- a/src/libcamera/v4l2_videodevice.cpp\n> >>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >>>> @@ -1115,8 +1115,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> >>>>\n> >>>> \tsel.r.left = rect->x;\n> >>>> \tsel.r.top = rect->y;\n> >>>> -\tsel.r.width = rect->w;\n> >>>> -\tsel.r.height = rect->h;\n> >>>> +\tsel.r.width = rect->width;\n> >>>> +\tsel.r.height = rect->height;\n> >>>>\n> >>>> \tint ret = ioctl(VIDIOC_S_SELECTION, &sel);\n> >>>> \tif (ret < 0) {\n> >>>> @@ -1127,8 +1127,8 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> >>>>\n> >>>> \trect->x = sel.r.left;\n> >>>> \trect->y = sel.r.top;\n> >>>> -\trect->w = sel.r.width;\n> >>>> -\trect->h = sel.r.height;\n> >>>> +\trect->width = sel.r.width;\n> >>>> +\trect->height = sel.r.height;\n> >>>>\n> >>>> \treturn 0;\n> >>>> }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 604FF603FF\n\tfor ;\n\tMon, 20 Apr 2020 10:35:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF450528;\n\tMon, 20 Apr 2020 10:35:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FLBC0cKx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587371754;\n\tbh=RQFEoMMXr0Nw6kSm6svousPP0k/3+Gjyc/M+x+G1pkc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FLBC0cKxPisjMs1QODBWTpqPlvlmU7INlsnczxTXVLOAnAfHW/5IyEJdZfelQ969Y\n\t1gOw3+3uyBlYQWVZAkjuA47CWT1lgrEk0/TmZi+0+JHU2AKgr0XMjuANukP8uibHqX\n\tLN58furLXhTv/4UomLItcEKU8MJhU5VHVcxwJx4Q=","Date":"Mon, 20 Apr 2020 11:35:41 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200420083541.GC5860@pendragon.ideasonboard.com>","References":"<20200403104327.3409564-1-jacopo@jmondi.org>\n\t<20200404013402.GI9690@pendragon.ideasonboard.com>\n\t<20200407122209.GB1716317@oden.dyn.berto.se>\n\t<20200407122710.GE4751@pendragon.ideasonboard.com>\n\t<20200420081957.vsc7azbzxrxh5z7y@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200420081957.vsc7azbzxrxh5z7y@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: geometry: Rework Rectangle","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 20 Apr 2020 08:35:54 -0000"}}]