[{"id":1887,"web_url":"https://patchwork.libcamera.org/comment/1887/","msgid":"<20190613145117.azgn3ud4kns53juh@uno.localdomain>","date":"2019-06-13T14:51:17","subject":"Re: [libcamera-devel] [PATCH v2 04/16] libcamera: geometry:\n\tSizeRange: Add contains()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jun 12, 2019 at 02:43:47AM +0200, Niklas Söderlund wrote:\n> Add a method to check if a Size can fit inside a SizeRange. When\n> determining if a size is containable take step values into account if\n> they are not set to 0.\n\nNow they're initialized to 1 by default\n\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/geometry.h | 2 ++\n> src/libcamera/geometry.cpp | 17 +++++++++++++++++\n> 2 files changed, 19 insertions(+)\n>\n> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> index 03962d7d09fac2b0..52f4d010de76f840 100644\n> --- a/include/libcamera/geometry.h\n> +++ b/include/libcamera/geometry.h\n> @@ -92,6 +92,8 @@ public:\n> \t{\n> \t}\n>\n> +\tbool contains(const Size &size) const;\n> +\n> \tstd::string toString() const;\n>\n> \tSize min;\n> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> index 6a1d1d78870b23ec..a4d472d9fce2e226 100644\n> --- a/src/libcamera/geometry.cpp\n> +++ b/src/libcamera/geometry.cpp\n> @@ -107,6 +107,7 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> * \\brief The Size height\n> */\n>\n> +\n\nUps\n\n> /**\n> * \\brief Assemble and return a string describing the size\n> * \\return A string describing the size\n> @@ -261,6 +262,22 @@ bool operator<(const Size &lhs, const Size &rhs)\n> * \\brief The vertical step\n> */\n>\n> +/**\n> + * \\brief Test if a size can be contained in the range\n\ns/can be/is\n\n> + * \\param[in] size Size to check\n> + * \\returns True if \\a size can be contained\n> + */\n> +bool SizeRange::contains(const Size &size) const\n> +{\n> +\tif (size.width < min.width || size.width > max.width ||\n> +\t size.height < min.height || size.height > max.height ||\n> +\t (hStep && (size.width - min.width) % hStep) ||\n> +\t (vStep && (size.height - min.height) % vStep))\n\nThis returns false for ranges that describes a single size, which is\nfine (even if we could decide that a size is \"contained\" in a range if\nboth width and height are minor). In any case, I would document that.\n\nApart from this\nReviewed-by: Jacopo Mondi \n\n\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> /**\n> * \\brief Assemble and return a string describing the size range\n> * \\return A string describing the SizeRange\n> --\n> 2.21.0\n>\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 relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE8CA61B3C\n\tfor ;\n\tThu, 13 Jun 2019 16:50:05 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 7A5581C0008;\n\tThu, 13 Jun 2019 14:50:03 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 13 Jun 2019 16:51:17 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190613145117.azgn3ud4kns53juh@uno.localdomain>","References":"<20190612004359.15772-1-niklas.soderlund@ragnatech.se>\n\t<20190612004359.15772-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fs4tsim4y6jvvbwb\"","Content-Disposition":"inline","In-Reply-To":"<20190612004359.15772-5-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 04/16] libcamera: geometry:\n\tSizeRange: Add contains()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 13 Jun 2019 14:50:06 -0000"}},{"id":1897,"web_url":"https://patchwork.libcamera.org/comment/1897/","msgid":"<20190616125106.GB20468@bigcity.dyn.berto.se>","date":"2019-06-16T12:51:06","subject":"Re: [libcamera-devel] [PATCH v2 04/16] libcamera: geometry:\n\tSizeRange: Add contains()","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 feedback.\n\nOn 2019-06-13 16:51:17 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Jun 12, 2019 at 02:43:47AM +0200, Niklas Söderlund wrote:\n> > Add a method to check if a Size can fit inside a SizeRange. When\n> > determining if a size is containable take step values into account if\n> > they are not set to 0.\n> \n> Now they're initialized to 1 by default\n\nYes, but what they are initialized as 1 by the consturctors which do not \ninitialize the step values. So one still need to consider the case\n\n SizeRange(unsigned int minW, unsigned int minH,\n unsigned int maxW, unsigned int maxH,\n unsigned int hstep, unsigned int vstep)\n : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)\n\nWhere steps might be set to 0 do avoid a division by zero problem.\n\n> \n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/geometry.h | 2 ++\n> > src/libcamera/geometry.cpp | 17 +++++++++++++++++\n> > 2 files changed, 19 insertions(+)\n> >\n> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > index 03962d7d09fac2b0..52f4d010de76f840 100644\n> > --- a/include/libcamera/geometry.h\n> > +++ b/include/libcamera/geometry.h\n> > @@ -92,6 +92,8 @@ public:\n> > \t{\n> > \t}\n> >\n> > +\tbool contains(const Size &size) const;\n> > +\n> > \tstd::string toString() const;\n> >\n> > \tSize min;\n> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > index 6a1d1d78870b23ec..a4d472d9fce2e226 100644\n> > --- a/src/libcamera/geometry.cpp\n> > +++ b/src/libcamera/geometry.cpp\n> > @@ -107,6 +107,7 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> > * \\brief The Size height\n> > */\n> >\n> > +\n> \n> Ups\n> \n> > /**\n> > * \\brief Assemble and return a string describing the size\n> > * \\return A string describing the size\n> > @@ -261,6 +262,22 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > * \\brief The vertical step\n> > */\n> >\n> > +/**\n> > + * \\brief Test if a size can be contained in the range\n> \n> s/can be/is\n> \n> > + * \\param[in] size Size to check\n> > + * \\returns True if \\a size can be contained\n> > + */\n> > +bool SizeRange::contains(const Size &size) const\n> > +{\n> > +\tif (size.width < min.width || size.width > max.width ||\n> > +\t size.height < min.height || size.height > max.height ||\n> > +\t (hStep && (size.width - min.width) % hStep) ||\n> > +\t (vStep && (size.height - min.height) % vStep))\n> \n> This returns false for ranges that describes a single size, which is\n> fine (even if we could decide that a size is \"contained\" in a range if\n> both width and height are minor). In any case, I would document that.\n\nDoes it ? If a size describes a single range 100x100 with step values of \neither 1 or 0 this will return true if compared to a size of 100x100, \nright?\n\n size.width < min.width || -> 100 < 100 -> false\n size.width > max.width || -> 100 > 100 -> false\n size.height < min.height || -> 100 < 100 -> false\n size.height > max.height || -> 100 > 100 -> false\n\n < if we have step values of 0 >\n (hStep && (size.width - min.width) % hStep) || -> (0 && ...) -> false\n (vStep && (size.height - min.height) % vStep) -> (0 && ...) -> false\n\n < if we have step values of 1 >\n (hStep && (size.width - min.width) % hStep) || -> (1 && (100 - 100) % 1) -> false\n (vStep && (size.height - min.height) % vStep) -> (1 && (100 - 100) % 1) -> false\n\n> \n> Apart from this\n> Reviewed-by: Jacopo Mondi \n\nI corrected all comments but the last as I don't fully understand what \nyou are trying to say. I will hold of collecting your tag until we can \nsort that comment out.\n\n> \n> \n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > /**\n> > * \\brief Assemble and return a string describing the size range\n> > * \\return A string describing the SizeRange\n> > --\n> > 2.21.0\n> >\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-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 139886464D\n\tfor ;\n\tSun, 16 Jun 2019 14:51:08 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id r9so6713185ljg.5\n\tfor ;\n\tSun, 16 Jun 2019 05:51:08 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tt21sm1262579lfd.85.2019.06.16.05.51.06\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 16 Jun 2019 05:51:07 -0700 (PDT)"],"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\t:user-agent; bh=uJ3uBA7QorvRf6vBMVy9rEDjTRN2cgixOH7vlS/d/co=;\n\tb=x/FNrFZn0gxhUROLLUmcAefJ5otALSc5sNpkTVIDQLSnfQ7N60CBIVxKbeoWr2wJDn\n\tae1tkbglYaeeyd9GOiYlyWua68yGNADyVtrtVim3m174LUhQz9VjXWQhxsdm8lOJkL7l\n\tGKY+6fqbR/CER/XBExZBB53kaqSRFYY+TjtQhjtFzKBOr6J0PitDZUsmvOc7TMQaCZmH\n\t3n/wwLZ+Pol8RzQwIlzNe89k0h9bdaVoaQ0vXnb/NUoKjqx6VoLvW4GRqAV5ngIOzMvI\n\t54sLPn/PkZwgxc428njcHDo5ubcF+bug4TVvKWE0R0m7D8h19Z8+gekbFGTG5grIfBDc\n\tfwfw==","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:user-agent;\n\tbh=uJ3uBA7QorvRf6vBMVy9rEDjTRN2cgixOH7vlS/d/co=;\n\tb=P47r9CuXWNVXkzOa9IRrjGAeja57xRJueox/87n4gbG2zYABr+SNlApQ+WSelTdyio\n\tr4Cp1pP2bS8pJ/XJhe5TASakz30tmiCJXKuxfJQ8nVvCMGBM/kpYzHD944pbOvnjvZOv\n\tZacGixlrVbiKkvQzCaAJtoWMs3MGBq5VDjUoa0TtysvrmjYrrugFMYhw/EagD/X7T4Kf\n\tcVDPuJjhKbdZ6ckSO2E5ZFZnn3tMFDtOPN8xe++PdC66Oi37bDKOGTCl7wi8Y1pD3Txp\n\t8hsJpmvkFgzXe+GIH6/5VmlBVAW1oQBYYdfPuEc9zYMx5SsB8iG6nf8Ks4IGYeOAKfyI\n\tjrOw==","X-Gm-Message-State":"APjAAAW3KIsQz8E5VE8GYaRIDo27x/WKqlyMOyQa5/0HPxpSVu48m8zs\n\tS3BeGpJ6xzlMMEXT59HJxTiriHsN32s=","X-Google-Smtp-Source":"APXvYqz1awUknzTRh56EvZTXZXDW+ophTclbWdvvJGJaHsK3vQAp3/uM15ejdDICVxrg8zhn+kblpg==","X-Received":"by 2002:a2e:96c3:: with SMTP id d3mr45360957ljj.68.1560689468206;\n\tSun, 16 Jun 2019 05:51:08 -0700 (PDT)","Date":"Sun, 16 Jun 2019 14:51:06 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190616125106.GB20468@bigcity.dyn.berto.se>","References":"<20190612004359.15772-1-niklas.soderlund@ragnatech.se>\n\t<20190612004359.15772-5-niklas.soderlund@ragnatech.se>\n\t<20190613145117.azgn3ud4kns53juh@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190613145117.azgn3ud4kns53juh@uno.localdomain>","User-Agent":"Mutt/1.12.0 (2019-05-25)","Subject":"Re: [libcamera-devel] [PATCH v2 04/16] libcamera: geometry:\n\tSizeRange: Add contains()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 16 Jun 2019 12:51:09 -0000"}},{"id":1900,"web_url":"https://patchwork.libcamera.org/comment/1900/","msgid":"<20190616172943.rok65qkog6bvopkr@uno.localdomain>","date":"2019-06-16T17:32:12","subject":"Re: [libcamera-devel] [PATCH v2 04/16] libcamera: geometry:\n\tSizeRange: Add contains()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sun, Jun 16, 2019 at 02:51:06PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2019-06-13 16:51:17 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Wed, Jun 12, 2019 at 02:43:47AM +0200, Niklas Söderlund wrote:\n> > > Add a method to check if a Size can fit inside a SizeRange. When\n> > > determining if a size is containable take step values into account if\n> > > they are not set to 0.\n> >\n> > Now they're initialized to 1 by default\n>\n> Yes, but what they are initialized as 1 by the consturctors which do not\n> initialize the step values. So one still need to consider the case\n>\n> SizeRange(unsigned int minW, unsigned int minH,\n> unsigned int maxW, unsigned int maxH,\n> unsigned int hstep, unsigned int vstep)\n> : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)\n>\n> Where steps might be set to 0 do avoid a division by zero problem.\n>\n> >\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > include/libcamera/geometry.h | 2 ++\n> > > src/libcamera/geometry.cpp | 17 +++++++++++++++++\n> > > 2 files changed, 19 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h\n> > > index 03962d7d09fac2b0..52f4d010de76f840 100644\n> > > --- a/include/libcamera/geometry.h\n> > > +++ b/include/libcamera/geometry.h\n> > > @@ -92,6 +92,8 @@ public:\n> > > \t{\n> > > \t}\n> > >\n> > > +\tbool contains(const Size &size) const;\n> > > +\n> > > \tstd::string toString() const;\n> > >\n> > > \tSize min;\n> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp\n> > > index 6a1d1d78870b23ec..a4d472d9fce2e226 100644\n> > > --- a/src/libcamera/geometry.cpp\n> > > +++ b/src/libcamera/geometry.cpp\n> > > @@ -107,6 +107,7 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs)\n> > > * \\brief The Size height\n> > > */\n> > >\n> > > +\n> >\n> > Ups\n> >\n> > > /**\n> > > * \\brief Assemble and return a string describing the size\n> > > * \\return A string describing the size\n> > > @@ -261,6 +262,22 @@ bool operator<(const Size &lhs, const Size &rhs)\n> > > * \\brief The vertical step\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Test if a size can be contained in the range\n> >\n> > s/can be/is\n> >\n> > > + * \\param[in] size Size to check\n> > > + * \\returns True if \\a size can be contained\n> > > + */\n> > > +bool SizeRange::contains(const Size &size) const\n> > > +{\n> > > +\tif (size.width < min.width || size.width > max.width ||\n> > > +\t size.height < min.height || size.height > max.height ||\n> > > +\t (hStep && (size.width - min.width) % hStep) ||\n> > > +\t (vStep && (size.height - min.height) % vStep))\n> >\n> > This returns false for ranges that describes a single size, which is\n> > fine (even if we could decide that a size is \"contained\" in a range if\n> > both width and height are minor). In any case, I would document that.\n>\n> Does it ? If a size describes a single range 100x100 with step values of\n> either 1 or 0 this will return true if compared to a size of 100x100,\n> right?\n\nA Size will always describe a single size. My point was that a Range\ncan describe a single size and in that case what happens is ambiguous.\n>\n> size.width < min.width || -> 100 < 100 -> false\n> size.width > max.width || -> 100 > 100 -> false\n> size.height < min.height || -> 100 < 100 -> false\n> size.height > max.height || -> 100 > 100 -> false\n\n What I meant to ask was: \"is 99x99 contained in 100x100\" ?\n\n (99 < 100 -> true ||\n 99 < 100 -> false ||\n 99 < 100 -> true ||\n 99 < 199 -> false)\n -> contained = false\n\nI think it's fine if we consider Ranges that represent a single size\nnot to contain any Size, or if we decide that a Size is contained in\na Range that represent a single size if both width and height are\nminors, but we should document it.\n\n>\n> < if we have step values of 0 >\n> (hStep && (size.width - min.width) % hStep) || -> (0 && ...) -> false\n> (vStep && (size.height - min.height) % vStep) -> (0 && ...) -> false\n>\n> < if we have step values of 1 >\n> (hStep && (size.width - min.width) % hStep) || -> (1 && (100 - 100) % 1) -> false\n> (vStep && (size.height - min.height) % vStep) -> (1 && (100 - 100) % 1) -> false\n>\n> >\n> > Apart from this\n> > Reviewed-by: Jacopo Mondi \n>\n> I corrected all comments but the last as I don't fully understand what\n> you are trying to say. I will hold of collecting your tag until we can\n> sort that comment out.\n>\n\nPlease go ahead and include it with the comment clarified.\n\nThanks\n j\n\n> >\n> >\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Assemble and return a string describing the size range\n> > > * \\return A string describing the SizeRange\n> > > --\n> > > 2.21.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDFBE64634\n\tfor ;\n\tSun, 16 Jun 2019 19:31:04 +0200 (CEST)","from uno.localdomain (unknown [5.91.106.249])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 2B3D760002;\n\tSun, 16 Jun 2019 17:31:03 +0000 (UTC)"],"X-Originating-IP":"5.91.106.249","Date":"Sun, 16 Jun 2019 19:32:12 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190616172943.rok65qkog6bvopkr@uno.localdomain>","References":"<20190612004359.15772-1-niklas.soderlund@ragnatech.se>\n\t<20190612004359.15772-5-niklas.soderlund@ragnatech.se>\n\t<20190613145117.azgn3ud4kns53juh@uno.localdomain>\n\t<20190616125106.GB20468@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jlyc2birk2cogsyy\"","Content-Disposition":"inline","In-Reply-To":"<20190616125106.GB20468@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 04/16] libcamera: geometry:\n\tSizeRange: Add contains()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 16 Jun 2019 17:31:05 -0000"}}]