[{"id":20166,"web_url":"https://patchwork.libcamera.org/comment/20166/","msgid":"<20211013064308.w4cmfwjr7e5zqysi@uno.localdomain>","date":"2021-10-13T06:43:08","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:\n> The Size class has new helpers that can simplify the code in the IPU3\n> pipeline handler. Use them.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------\n> 1 file changed, 6 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 92e869257e53..262b9a23703e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\t * \\todo Clarify the alignment constraints as explained\n> \t\t\t * in validate()\n> \t\t\t */\n> -\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> -\t\t\tsize.width = utils::alignDown(size.width - 1,\n> -\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> -\t\t\tsize.height = utils::alignDown(size.height - 1,\n> -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> +\t\t\t\t.shrunkBy({ 1, 1 })\n> +\t\t\t\t.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n\nCan you align the dots ?\n\n\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n\t\t\t\t .shrunkBy({ 1, 1 })\n\t\t\t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n\nSame below.\n\nOr was it intentional ?\n\n(I'm honest, I found the previous version more immediate compared than\ngoing through shrunk).\n\nAnyway, there's a repeating pattern of\n\n {size.width - 1, size.height - 1}.alignDownTo(margins)\n {size.width + 1, size.height + 1}.alignUpTo(margins)\n\nWhich makes me wonder if what we're looking for is actually a 'Align\nto the next strictly minor/major value' function.\n\nThanks\n j\n\n\n> \t\t\tpixelFormat = formats::NV12;\n> \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> \t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t */\n>\n> \t/* The strictly smaller size than the sensor resolution, aligned to margins. */\n> -\tSize minSize = Size(sensor->resolution().width - 1,\n> -\t\t\t sensor->resolution().height - 1)\n> +\tSize minSize = sensor->resolution().shrunkBy({ 1, 1 })\n> \t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> \t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n>\n> @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t * Either the smallest margin-aligned size larger than the viewfinder\n> \t * size or the adjusted sensor resolution.\n> \t */\n> -\tminSize = Size(IPU3ViewfinderSize.width + 1,\n> -\t\t IPU3ViewfinderSize.height + 1)\n> +\tminSize = IPU3ViewfinderSize.grownBy({ 1, 1 })\n> \t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> \t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> \t\t .boundedTo(minSize);\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF516C323E\n\tfor ;\n\tWed, 13 Oct 2021 06:42:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 401A468F4F;\n\tWed, 13 Oct 2021 08:42:23 +0200 (CEST)","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 854F2604FF\n\tfor ;\n\tWed, 13 Oct 2021 08:42:21 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 8EE8D60007;\n\tWed, 13 Oct 2021 06:42:20 +0000 (UTC)"],"Date":"Wed, 13 Oct 2021 08:43:08 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211013064308.w4cmfwjr7e5zqysi@uno.localdomain>","References":"<20211013012630.18877-1-laurent.pinchart@ideasonboard.com>\n\t<20211013012630.18877-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211013012630.18877-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20168,"web_url":"https://patchwork.libcamera.org/comment/20168/","msgid":"","date":"2021-10-13T08:30:45","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:\n> On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:\n> > The Size class has new helpers that can simplify the code in the IPU3\n> > pipeline handler. Use them.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------\n> > 1 file changed, 6 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 92e869257e53..262b9a23703e 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > \t\t\t * \\todo Clarify the alignment constraints as explained\n> > \t\t\t * in validate()\n> > \t\t\t */\n> > -\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> > -\t\t\tsize.width = utils::alignDown(size.width - 1,\n> > -\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> > -\t\t\tsize.height = utils::alignDown(size.height - 1,\n> > -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > +\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> > +\t\t\t\t.shrunkBy({ 1, 1 })\n> > +\t\t\t\t.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > +\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> \n> Can you align the dots ?\n> \n> \t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> \t\t\t\t .shrunkBy({ 1, 1 })\n> \t\t\t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> \t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> \n> Same below.\n> \n> Or was it intentional ?\n\nI've copied the style from further down in this file, I don't mind it\neither way.\n\n> (I'm honest, I found the previous version more immediate compared than\n> going through shrunk).\n\nWe can skip this patch if you think it doesn't bring any readability\nimprovement.\n\n> Anyway, there's a repeating pattern of\n> \n> {size.width - 1, size.height - 1}.alignDownTo(margins)\n> {size.width + 1, size.height + 1}.alignUpTo(margins)\n> \n> Which makes me wonder if what we're looking for is actually a 'Align\n> to the next strictly minor/major value' function.\n\nGood question. For IPU3 in particular, I'm still not sure what we'll\nneed once we complete the (mythical) rework of the ImgU configuration\n:-) I haven't checked the other pipeline handlers and IPAs yet, this\nseries was a by-product of the review of your frame durations series\nwhere I noticed we were missing these helpers.\n\n> > \t\t\tpixelFormat = formats::NV12;\n> > \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> > \t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t */\n> >\n> > \t/* The strictly smaller size than the sensor resolution, aligned to margins. */\n> > -\tSize minSize = Size(sensor->resolution().width - 1,\n> > -\t\t\t sensor->resolution().height - 1)\n> > +\tSize minSize = sensor->resolution().shrunkBy({ 1, 1 })\n> > \t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > \t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> >\n> > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t * Either the smallest margin-aligned size larger than the viewfinder\n> > \t * size or the adjusted sensor resolution.\n> > \t */\n> > -\tminSize = Size(IPU3ViewfinderSize.width + 1,\n> > -\t\t IPU3ViewfinderSize.height + 1)\n> > +\tminSize = IPU3ViewfinderSize.grownBy({ 1, 1 })\n> > \t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > \t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> > \t\t .boundedTo(minSize);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10ACAC323E\n\tfor ;\n\tWed, 13 Oct 2021 08:31:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99BCC68F4F;\n\tWed, 13 Oct 2021 10:31:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0CCF068546\n\tfor ;\n\tWed, 13 Oct 2021 10:31:00 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 77B27291;\n\tWed, 13 Oct 2021 10:30:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HBEm65mO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634113859;\n\tbh=IRad/TiC8aYG2DkwRXvW1LHsNX1Z1XyVGGrEKE13uEk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HBEm65mOXWS5NZLrShESxLKgEcyDEnwbfBAKDnGLIZjF6i1H86kOoQeBusd+2qvH/\n\tu2PfGjJgn3lpODpn2kgZqEUfCiMG87YNYH4UUG/CWEnaNN1DGRJPk0rvmFnLEcxg/0\n\tnQ9YdP/lhtRU8b1lyNq9GO5SbHon5dYe8LfKQ38k=","Date":"Wed, 13 Oct 2021 11:30:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211013012630.18877-1-laurent.pinchart@ideasonboard.com>\n\t<20211013012630.18877-3-laurent.pinchart@ideasonboard.com>\n\t<20211013064308.w4cmfwjr7e5zqysi@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211013064308.w4cmfwjr7e5zqysi@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20202,"web_url":"https://patchwork.libcamera.org/comment/20202/","msgid":"<20211014112317.vckff26fw6fgn7dk@uno.localdomain>","date":"2021-10-14T11:23:17","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Oct 13, 2021 at 11:30:45AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:\n> > On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:\n> > > The Size class has new helpers that can simplify the code in the IPU3\n> > > pipeline handler. Use them.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------\n> > > 1 file changed, 6 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 92e869257e53..262b9a23703e 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > \t\t\t * \\todo Clarify the alignment constraints as explained\n> > > \t\t\t * in validate()\n> > > \t\t\t */\n> > > -\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> > > -\t\t\tsize.width = utils::alignDown(size.width - 1,\n> > > -\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> > > -\t\t\tsize.height = utils::alignDown(size.height - 1,\n> > > -\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > > +\t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> > > +\t\t\t\t.shrunkBy({ 1, 1 })\n> > > +\t\t\t\t.alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > +\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> >\n> > Can you align the dots ?\n> >\n> > \t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> > \t\t\t\t .shrunkBy({ 1, 1 })\n> > \t\t\t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > \t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> >\n> > Same below.\n> >\n> > Or was it intentional ?\n>\n> I've copied the style from further down in this file, I don't mind it\n> either way.\n>\n> > (I'm honest, I found the previous version more immediate compared than\n> > going through shrunk).\n>\n> We can skip this patch if you think it doesn't bring any readability\n> improvement.\n>\n\nNah, we're talking details\n\n> > Anyway, there's a repeating pattern of\n> >\n> > {size.width - 1, size.height - 1}.alignDownTo(margins)\n> > {size.width + 1, size.height + 1}.alignUpTo(margins)\n> >\n> > Which makes me wonder if what we're looking for is actually a 'Align\n> > to the next strictly minor/major value' function.\n>\n> Good question. For IPU3 in particular, I'm still not sure what we'll\n> need once we complete the (mythical) rework of the ImgU configuration\n> :-) I haven't checked the other pipeline handlers and IPAs yet, this\n> series was a by-product of the review of your frame durations series\n> where I noticed we were missing these helpers.\n>\n\nSure, if we'll end up having to maintain the shrunks we'll reconsider\na new helper.\n\nReviewed-by: Jacopo Mondi \n\nI'll base my next frame duration series on top of these patches!\n\nThanks\n j\n\n> > > \t\t\tpixelFormat = formats::NV12;\n> > > \t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> > > \t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> > > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t */\n> > >\n> > > \t/* The strictly smaller size than the sensor resolution, aligned to margins. */\n> > > -\tSize minSize = Size(sensor->resolution().width - 1,\n> > > -\t\t\t sensor->resolution().height - 1)\n> > > +\tSize minSize = sensor->resolution().shrunkBy({ 1, 1 })\n> > > \t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > \t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > >\n> > > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > \t * Either the smallest margin-aligned size larger than the viewfinder\n> > > \t * size or the adjusted sensor resolution.\n> > > \t */\n> > > -\tminSize = Size(IPU3ViewfinderSize.width + 1,\n> > > -\t\t IPU3ViewfinderSize.height + 1)\n> > > +\tminSize = IPU3ViewfinderSize.grownBy({ 1, 1 })\n> > > \t\t .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > \t\t\t IMGU_OUTPUT_HEIGHT_MARGIN)\n> > > \t\t .boundedTo(minSize);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7CB1ABDC71\n\tfor ;\n\tThu, 14 Oct 2021 11:22:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF8E568F4F;\n\tThu, 14 Oct 2021 13:22:30 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC6ED68541\n\tfor ;\n\tThu, 14 Oct 2021 13:22:29 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 53E67C0007;\n\tThu, 14 Oct 2021 11:22:29 +0000 (UTC)"],"Date":"Thu, 14 Oct 2021 13:23:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211014112317.vckff26fw6fgn7dk@uno.localdomain>","References":"<20211013012630.18877-1-laurent.pinchart@ideasonboard.com>\n\t<20211013012630.18877-3-laurent.pinchart@ideasonboard.com>\n\t<20211013064308.w4cmfwjr7e5zqysi@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20249,"web_url":"https://patchwork.libcamera.org/comment/20249/","msgid":"<163433852692.456562.4403167272819129352@Monstersaurus>","date":"2021-10-15T22:55:26","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-10-14 12:23:17)\n> Hi Laurent\n> \n> On Wed, Oct 13, 2021 at 11:30:45AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Wed, Oct 13, 2021 at 08:43:08AM +0200, Jacopo Mondi wrote:\n> > > On Wed, Oct 13, 2021 at 04:26:30AM +0300, Laurent Pinchart wrote:\n> > > > The Size class has new helpers that can simplify the code in the IPU3\n> > > > pipeline handler. Use them.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 15 ++++++---------\n> > > > 1 file changed, 6 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 92e869257e53..262b9a23703e 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -438,11 +438,10 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > > * \\todo Clarify the alignment constraints as explained\n> > > > * in validate()\n> > > > */\n> > > > - size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n> > > > - size.width = utils::alignDown(size.width - 1,\n> > > > - IMGU_OUTPUT_WIDTH_MARGIN);\n> > > > - size.height = utils::alignDown(size.height - 1,\n> > > > - IMGU_OUTPUT_HEIGHT_MARGIN);\n> > > > + size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> > > > + .shrunkBy({ 1, 1 })\n> > > > + .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > > + IMGU_OUTPUT_HEIGHT_MARGIN);\n> > >\n> > > Can you align the dots ?\n> > >\n> > > size = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE)\n> > > .shrunkBy({ 1, 1 })\n> > > .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > IMGU_OUTPUT_HEIGHT_MARGIN);\n> > >\n> > > Same below.\n> > >\n> > > Or was it intentional ?\n> >\n> > I've copied the style from further down in this file, I don't mind it\n> > either way.\n\nFor chaining calls, I think aligning on the dots is better.\n\n.shrunkBy({1,1}) looks cleaner than the size.width -1 ...\n\n> >\n> > > (I'm honest, I found the previous version more immediate compared than\n> > > going through shrunk).\n> >\n> > We can skip this patch if you think it doesn't bring any readability\n> > improvement.\n> >\n> \n> Nah, we're talking details\n> \n> > > Anyway, there's a repeating pattern of\n> > >\n> > > {size.width - 1, size.height - 1}.alignDownTo(margins)\n> > > {size.width + 1, size.height + 1}.alignUpTo(margins)\n> > >\n> > > Which makes me wonder if what we're looking for is actually a 'Align\n> > > to the next strictly minor/major value' function.\n\nBut it does seem like this higher level helper might end up being used.\nStill, these improvements stand on their own until we figure out if we\ndo need something else.\n\n\nReviewed-by: Kieran Bingham \n\n> >\n> > Good question. For IPU3 in particular, I'm still not sure what we'll\n> > need once we complete the (mythical) rework of the ImgU configuration\n> > :-) I haven't checked the other pipeline handlers and IPAs yet, this\n> > series was a by-product of the review of your frame durations series\n> > where I noticed we were missing these helpers.\n> >\n> \n> Sure, if we'll end up having to maintain the shrunks we'll reconsider\n> a new helper.\n> \n> Reviewed-by: Jacopo Mondi \n> \n> I'll base my next frame duration series on top of these patches!\n> \n> Thanks\n> j\n> \n> > > > pixelFormat = formats::NV12;\n> > > > bufferCount = IPU3_BUFFER_COUNT;\n> > > > streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> > > > @@ -996,8 +995,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > > */\n> > > >\n> > > > /* The strictly smaller size than the sensor resolution, aligned to margins. */\n> > > > - Size minSize = Size(sensor->resolution().width - 1,\n> > > > - sensor->resolution().height - 1)\n> > > > + Size minSize = sensor->resolution().shrunkBy({ 1, 1 })\n> > > > .alignedDownTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > > IMGU_OUTPUT_HEIGHT_MARGIN);\n> > > >\n> > > > @@ -1005,8 +1003,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > > * Either the smallest margin-aligned size larger than the viewfinder\n> > > > * size or the adjusted sensor resolution.\n> > > > */\n> > > > - minSize = Size(IPU3ViewfinderSize.width + 1,\n> > > > - IPU3ViewfinderSize.height + 1)\n> > > > + minSize = IPU3ViewfinderSize.grownBy({ 1, 1 })\n> > > > .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> > > > IMGU_OUTPUT_HEIGHT_MARGIN)\n> > > > .boundedTo(minSize);\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B685EC323E\n\tfor ;\n\tFri, 15 Oct 2021 22:55:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A60068F4F;\n\tSat, 16 Oct 2021 00:55:31 +0200 (CEST)","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 D25C568541\n\tfor ;\n\tSat, 16 Oct 2021 00:55:29 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C71629B;\n\tSat, 16 Oct 2021 00:55:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j/JUmFte\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634338529;\n\tbh=2/Sx0krH1eUEIsPbdeztZP/yFSBljPz/yTNe9im2USQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=j/JUmFteL71Xn7fRt14hswPt/Kp2yd6GjADe/mVNWklDO9gbS/6eSYnFYoUF7aDHq\n\t61hhYl+b1vuQFYzRTEj0tYkq9ZDPv/NPg99AdvJ+reCXo9TaHJoY+YyNqQuIZsjuEq\n\tkKdPedl2vT2HK+Tro13Fm4suuQs9z0OnArtSBxHY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211014112317.vckff26fw6fgn7dk@uno.localdomain>","References":"<20211013012630.18877-1-laurent.pinchart@ideasonboard.com>\n\t<20211013012630.18877-3-laurent.pinchart@ideasonboard.com>\n\t<20211013064308.w4cmfwjr7e5zqysi@uno.localdomain>\n\t\n\t<20211014112317.vckff26fw6fgn7dk@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Date":"Fri, 15 Oct 2021 23:55:26 +0100","Message-ID":"<163433852692.456562.4403167272819129352@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipu3: Use\n\tnew Size grownBy() and shrunkBy() helpers","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]