[{"id":16736,"web_url":"https://patchwork.libcamera.org/comment/16736/","msgid":"","date":"2021-05-03T10:51:58","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","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 2021-05-03 11:27:01 +0200, Jacopo Mondi wrote:\n> The IF rectangle height is iteratively computed by first subtracting\n> the scaling factor to the estimated height, then in a successive loop\n> by adding the same scaling factor until the maximum IF size is not\n> reached.\n> \n> As the computed IF height is not cached in any variable, the second\n> loop over-writes the result of the first one, even if the BDS alignment\n> condition is not satisfied.\n> \n> Fix this by caching the result of the two iterations, and use the one\n> that produced any result, with a preference for the one produced by the\n> second loop, as implemented in the reference python script.\n> \n> Tested-by: Jean-Michel Hautbois \n> Reviewed-by: Jean-Michel Hautbois \n> Reviewed-by: Laurent Pinchart \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++----\n> 1 file changed, 9 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\n> index 36fee31c1962..ea654e57b0e7 100644\n> --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \tfloat bdsHeight;\n> \n> \tif (!isSameRatio(pipe->input, gdc)) {\n> +\t\tunsigned int foundIfHeights[2] = { 0, 0 };\n> \t\tfloat estIFHeight = (iif.width * gdc.height) /\n> \t\t\t\t static_cast(gdc.width);\n> \t\testIFHeight = std::clamp(estIFHeight, minIFHeight, iif.height);\n> -\t\tbool found = false;\n> \n> \t\tifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> \t\twhile (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {\n> @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\t\t\tunsigned int bdsIntHeight = static_cast(bdsHeight);\n> \n> \t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> -\t\t\t\t\tfound = true;\n> +\t\t\t\t\tfoundIfHeights[0] = ifHeight;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t}\n> @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\t\t\tunsigned int bdsIntHeight = static_cast(bdsHeight);\n> \n> \t\t\t\tif (!(bdsIntHeight % BDS_ALIGN_H)) {\n> -\t\t\t\t\tfound = true;\n> +\t\t\t\t\tfoundIfHeights[1] = ifHeight;\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n> \t\t\t}\n> @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc\n> \t\t\tifHeight += IF_ALIGN_H;\n> \t\t}\n> \n> -\t\tif (found) {\n> +\t\tif (foundIfHeights[0])\n> +\t\t\tifHeight = foundIfHeights[0];\n> +\t\tif (foundIfHeights[1])\n> +\t\t\tifHeight = foundIfHeights[1];\n> +\n> +\t\tif (foundIfHeights[0] || foundIfHeights[1]) {\n\nnit: This could be simplified by turning 'found' into an unsigned int \ninitialized to 0 and storing ifHeight inside found instead of true. But \nI think this maybe a matter of taste so with or without this,\n\nReviewed-by: Niklas Söderlund \n\n> \t\t\tunsigned int bdsIntHeight = static_cast(bdsHeight);\n> \n> \t\t\tpipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },\n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 9E67EBDE77\n\tfor ;\n\tMon, 3 May 2021 10:52:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A5446890C;\n\tMon, 3 May 2021 12:52:02 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80D7160511\n\tfor ;\n\tMon, 3 May 2021 12:52:00 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id n138so7418973lfa.3\n\tfor ;\n\tMon, 03 May 2021 03:52:00 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t134sm1103991lff.165.2021.05.03.03.51.59\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 03 May 2021 03:51:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"A/uNmQPz\"; dkim-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=6eG5mQfJeadxUAfqdSs0evUuqeP/Bd9VZK+mwZe6pLg=;\n\tb=A/uNmQPzXZXUTAGNqJ2S70salm/Db/COJNirqo97CwFNSnwv+WYxZ4qXfWofhF+heZ\n\tvu+ffT/up/Ll4j/LdtUKD0JFkA7+Gt5xpiges2mLKoLnMsbSNX16Rax+qtmTi7PGk1oQ\n\tHvRP6uhwSFm1ZNLqrVGwNbrOwJpIkz/62CXvvjaGl8J+bsU2xI3EDDfs9BKqiiv1YYc7\n\tyzwi0aTmt1YKvXV1V0nqNizWQPo3nD7bQ3nbmfKs07EnWlRnxy3l43ZMZqUeeUKrGDvD\n\tkWXtj2N6tKgyDUhJrDguDcNlEjNufXzvCbIGyFtwaT1nu8EJuU1ubBfiXdbtr/0T9kdy\n\tpoVg==","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=6eG5mQfJeadxUAfqdSs0evUuqeP/Bd9VZK+mwZe6pLg=;\n\tb=tIqetcU+Fit9ULk19f3A7+A0EZWFSc2c1isRA5wqJegCUD8oJ1gcS/p2Ky/nAMrBUU\n\tvu83zXuxOtn4vcZfgHhr+qNfbpxif/EC+WgmUl655PKZZKW/nKaOEEBND9ROoJ+1B9Fx\n\tpdtlFlJluCFcYcHciDVjQLB3FrhLp4mThOzt4utWXQgeYkrs+pg4mV0O8mOj0+m7Pvdx\n\tI4uBccLzibOV79oT+yaIOE5qAIY/vOP9Vo8XWHVIGLI2Kp8PHaj7qdtV81FJkceFDt6u\n\t4ghJbV7hDav61T+ROBeWuB1jLPPT9yIyKoJQussuDXN0yNy4caA1Lxt2k1upayLISKlw\n\tZF1g==","X-Gm-Message-State":"AOAM53283fHNBvFiy4W2b0kDDkSYTHbh74/D/XvZZcYLFJdy+EvZke1l\n\tWkIWx53DH7ZAVv4uIyR+1pKAMA==","X-Google-Smtp-Source":"ABdhPJwTdeSRxSyIsXjl9X+mWQkTCftQBcoEoLXC4o2eNeNRGdwgeCJJTyoJVfDR14YwD/Qm1btfvw==","X-Received":"by 2002:ac2:5544:: with SMTP id l4mr1500963lfk.257.1620039119908;\n\tMon, 03 May 2021 03:51:59 -0700 (PDT)","Date":"Mon, 3 May 2021 12:51:58 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210503092705.15562-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16785,"web_url":"https://patchwork.libcamera.org/comment/16785/","msgid":"","date":"2021-05-06T05:51:03","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch,\n\nOn Mon, May 3, 2021 at 7:52 PM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-03 11:27:01 +0200, Jacopo Mondi wrote:\n> > The IF rectangle height is iteratively computed by first subtracting\n> > the scaling factor to the estimated height, then in a successive loop\n> > by adding the same scaling factor until the maximum IF size is not\n> > reached.\n> >\n> > As the computed IF height is not cached in any variable, the second\n> > loop over-writes the result of the first one, even if the BDS alignment\n> > condition is not satisfied.\n> >\n> > Fix this by caching the result of the two iterations, and use the one\n> > that produced any result, with a preference for the one produced by the\n> > second loop, as implemented in the reference python script.\n> >\n> > Tested-by: Jean-Michel Hautbois \n> > Reviewed-by: Jean-Michel Hautbois \n> > Reviewed-by: Laurent Pinchart \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++----\n> > 1 file changed, 9 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > index 36fee31c1962..ea654e57b0e7 100644\n> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> > float bdsHeight;\n> >\n> > if (!isSameRatio(pipe->input, gdc)) {\n> > + unsigned int foundIfHeights[2] = { 0, 0 };\n> > float estIFHeight = (iif.width * gdc.height) /\n> > static_cast(gdc.width);\n> > estIFHeight = std::clamp(estIFHeight, minIFHeight,\n> iif.height);\n> > - bool found = false;\n> >\n> > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > while (ifHeight >= minIFHeight && ifHeight / bdsSF >=\n> minBDSHeight) {\n> > @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> > unsigned int bdsIntHeight =\n> static_cast(bdsHeight);\n> >\n> > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > - found = true;\n> > + foundIfHeights[0] = ifHeight;\n> > break;\n> > }\n> > }\n> > @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> > unsigned int bdsIntHeight =\n> static_cast(bdsHeight);\n> >\n> > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > - found = true;\n> > + foundIfHeights[1] = ifHeight;\n> > break;\n> > }\n> > }\n> > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> const Size &iif, const Size &gdc\n> > ifHeight += IF_ALIGN_H;\n> > }\n> >\n> > - if (found) {\n> > + if (foundIfHeights[0])\n> > + ifHeight = foundIfHeights[0];\n> > + if (foundIfHeights[1])\n> > + ifHeight = foundIfHeights[1];\n> > +\n> > + if (foundIfHeights[0] || foundIfHeights[1]) {\n>\n> nit: This could be simplified by turning 'found' into an unsigned int\n> initialized to 0 and storing ifHeight inside found instead of true. But\n> I think this maybe a matter of taste so with or without this,\n>\n> Reviewed-by: Niklas Söderlund \n>\n>\nIs it necessary to do the second-loop when a height is found in the first\nloop?\n\n-Hiro\n\n> > unsigned int bdsIntHeight = static_cast int>(bdsHeight);\n> >\n> > pipeConfigs.push_back({ bdsSF, { iif.width,\n> ifHeight },\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 F1658BDE7D\n\tfor ;\n\tThu, 6 May 2021 05:51:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55CAB68911;\n\tThu, 6 May 2021 07:51:16 +0200 (CEST)","from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com\n\t[IPv6:2a00:1450:4864:20::62a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F884688E5\n\tfor ;\n\tThu, 6 May 2021 07:51:14 +0200 (CEST)","by mail-ej1-x62a.google.com with SMTP id f24so6428971ejc.6\n\tfor ;\n\tWed, 05 May 2021 22:51:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"NjVKoONX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=DlcUH5uoZyXvULo5UmwzuCfvYe/8xJ3Fu11Zt7bCpJ0=;\n\tb=NjVKoONXIBy8JtPQQXI8jhbWdq2JlHOdUF4ydJ8XesJJx8IENoCjdUxtj+cDZnHIYQ\n\t9WmhZwzIRrTTmPHeQYmwjpItZ7YAn3PWyyYIdUGW74shBAMvUikVqBIG7b+lLbWN1bSC\n\tYXjtczVdUpIhjtlIHNAFjfg4oVnAatihjkJm0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=DlcUH5uoZyXvULo5UmwzuCfvYe/8xJ3Fu11Zt7bCpJ0=;\n\tb=GmFlNxfEWiahUr80khiQCj3oFgWxU+SoIYe4pfSeoVpTMlIuz+uSLpaZKQiEYL6naq\n\tgnKlqAdlyzAmnzzxUpltZRVTlgNjA/qcp0JPWNMPSvqLFje8cbSvwOCaSTrCV0xrhaPg\n\tO7YaG1PYKoykG63dp0dHtAcRYXupuWS4OypUOInf28GzpMrZzsQWuX+Tp9JT8tDCuqsa\n\t6EJgEQPQ2aULbHhtYpvlUiAL9gBKiYE3vwqSR666dgro7WJAoFQHfH4I2gIkdZxgmksa\n\ttj5cSmXfdDcmYz/DOJwAx6yKztXnZMd2DypqK15RbQtcZy+wIcjR1P6reFLCJepsJeyP\n\twbRQ==","X-Gm-Message-State":"AOAM533UIxScZjQzltLIxD9QM+xRQZbI9SeZEYYhjOpbtNy1K8TshYR8\n\tXMtoh/QtzJrcAQ1clmPSXvGjXhPpOM/raHDJ31LTafZq3AY=","X-Google-Smtp-Source":"ABdhPJyGYJvZIlTNmEfOgXbzykGNOU22ngLa3fVeOMt2NNYtUoSeL3RwvRELLzisQmFp5oMAy9TCBdYdwiMFlKHAIyI=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr2603079ejc.292.1620280274016; \n\tWed, 05 May 2021 22:51:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-4-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 6 May 2021 14:51:03 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============0840977345093931811==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16902,"web_url":"https://patchwork.libcamera.org/comment/16902/","msgid":"<20210511143716.ssxjwrw4hng2e3dz@uno.localdomain>","date":"2021-05-11T14:37:16","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n time to time I dig this series out of the dead patches cemetery.\nI'm kind of the Lara Croft of libcamera\n\nOn Thu, May 06, 2021 at 02:51:03PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch,\n>\n> On Mon, May 3, 2021 at 7:52 PM Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2021-05-03 11:27:01 +0200, Jacopo Mondi wrote:\n> > > The IF rectangle height is iteratively computed by first subtracting\n> > > the scaling factor to the estimated height, then in a successive loop\n> > > by adding the same scaling factor until the maximum IF size is not\n> > > reached.\n> > >\n> > > As the computed IF height is not cached in any variable, the second\n> > > loop over-writes the result of the first one, even if the BDS alignment\n> > > condition is not satisfied.\n> > >\n> > > Fix this by caching the result of the two iterations, and use the one\n> > > that produced any result, with a preference for the one produced by the\n> > > second loop, as implemented in the reference python script.\n> > >\n> > > Tested-by: Jean-Michel Hautbois \n> > > Reviewed-by: Jean-Michel Hautbois \n> > > Reviewed-by: Laurent Pinchart \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++----\n> > > 1 file changed, 9 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > index 36fee31c1962..ea654e57b0e7 100644\n> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > const Size &iif, const Size &gdc\n> > > float bdsHeight;\n> > >\n> > > if (!isSameRatio(pipe->input, gdc)) {\n> > > + unsigned int foundIfHeights[2] = { 0, 0 };\n> > > float estIFHeight = (iif.width * gdc.height) /\n> > > static_cast(gdc.width);\n> > > estIFHeight = std::clamp(estIFHeight, minIFHeight,\n> > iif.height);\n> > > - bool found = false;\n> > >\n> > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > > while (ifHeight >= minIFHeight && ifHeight / bdsSF >=\n> > minBDSHeight) {\n> > > @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > const Size &iif, const Size &gdc\n> > > unsigned int bdsIntHeight =\n> > static_cast(bdsHeight);\n> > >\n> > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > - found = true;\n> > > + foundIfHeights[0] = ifHeight;\n> > > break;\n> > > }\n> > > }\n> > > @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > const Size &iif, const Size &gdc\n> > > unsigned int bdsIntHeight =\n> > static_cast(bdsHeight);\n> > >\n> > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > - found = true;\n> > > + foundIfHeights[1] = ifHeight;\n> > > break;\n> > > }\n> > > }\n> > > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > const Size &iif, const Size &gdc\n> > > ifHeight += IF_ALIGN_H;\n> > > }\n> > >\n> > > - if (found) {\n> > > + if (foundIfHeights[0])\n> > > + ifHeight = foundIfHeights[0];\n> > > + if (foundIfHeights[1])\n> > > + ifHeight = foundIfHeights[1];\n> > > +\n> > > + if (foundIfHeights[0] || foundIfHeights[1]) {\n> >\n> > nit: This could be simplified by turning 'found' into an unsigned int\n> > initialized to 0 and storing ifHeight inside found instead of true. But\n> > I think this maybe a matter of taste so with or without this,\n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> >\n> Is it necessary to do the second-loop when a height is found in the first\n> loop?\n\nYes, as\n\n if (foundIfHeights[0])\n ifHeight = foundIfHeights[0];\n if (foundIfHeights[1])\n ifHeight = foundIfHeights[1];\n\noverwrites the first found result with the second found one.\nActually as Niklas and Laurent pointed out, this can be handled with a\nsingle variable that gets overwritten by the second loop, if found.\n\nLet's deviate from the script, I'll take this change in.\n\nThanks\n j\n\n>\n> -Hiro\n>\n> > > unsigned int bdsIntHeight = static_cast > int>(bdsHeight);\n> > >\n> > > pipeConfigs.push_back({ bdsSF, { iif.width,\n> > ifHeight },\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 C159EBF839\n\tfor ;\n\tTue, 11 May 2021 14:36:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22F3568918;\n\tTue, 11 May 2021 16:36:34 +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 984A961538\n\tfor ;\n\tTue, 11 May 2021 16:36:33 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 6AB0260008;\n\tTue, 11 May 2021 14:36:32 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 16:37:16 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511143716.ssxjwrw4hng2e3dz@uno.localdomain>","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-4-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16907,"web_url":"https://patchwork.libcamera.org/comment/16907/","msgid":"","date":"2021-05-12T04:18:58","subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, May 11, 2021 at 11:36 PM Jacopo Mondi wrote:\n\n> Hello,\n> time to time I dig this series out of the dead patches cemetery.\n> I'm kind of the Lara Croft of libcamera\n>\n> On Thu, May 06, 2021 at 02:51:03PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch,\n> >\n> > On Mon, May 3, 2021 at 7:52 PM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2021-05-03 11:27:01 +0200, Jacopo Mondi wrote:\n> > > > The IF rectangle height is iteratively computed by first subtracting\n> > > > the scaling factor to the estimated height, then in a successive loop\n> > > > by adding the same scaling factor until the maximum IF size is not\n> > > > reached.\n> > > >\n> > > > As the computed IF height is not cached in any variable, the second\n> > > > loop over-writes the result of the first one, even if the BDS\n> alignment\n> > > > condition is not satisfied.\n> > > >\n> > > > Fix this by caching the result of the two iterations, and use the one\n> > > > that produced any result, with a preference for the one produced by\n> the\n> > > > second loop, as implemented in the reference python script.\n> > > >\n> > > > Tested-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > > > Reviewed-by: Jean-Michel Hautbois <\n> jeanmichel.hautbois@ideasonboard.com>\n> > > > Reviewed-by: Laurent Pinchart \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++----\n> > > > 1 file changed, 9 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > index 36fee31c1962..ea654e57b0e7 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n> > > > @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > > const Size &iif, const Size &gdc\n> > > > float bdsHeight;\n> > > >\n> > > > if (!isSameRatio(pipe->input, gdc)) {\n> > > > + unsigned int foundIfHeights[2] = { 0, 0 };\n> > > > float estIFHeight = (iif.width * gdc.height) /\n> > > > static_cast(gdc.width);\n> > > > estIFHeight = std::clamp(estIFHeight,\n> minIFHeight,\n> > > iif.height);\n> > > > - bool found = false;\n> > > >\n> > > > ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);\n> > > > while (ifHeight >= minIFHeight && ifHeight / bdsSF >=\n> > > minBDSHeight) {\n> > > > @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > > const Size &iif, const Size &gdc\n> > > > unsigned int bdsIntHeight =\n> > > static_cast(bdsHeight);\n> > > >\n> > > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > > - found = true;\n> > > > + foundIfHeights[0] = ifHeight;\n> > > > break;\n> > > > }\n> > > > }\n> > > > @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > > const Size &iif, const Size &gdc\n> > > > unsigned int bdsIntHeight =\n> > > static_cast(bdsHeight);\n> > > >\n> > > > if (!(bdsIntHeight % BDS_ALIGN_H)) {\n> > > > - found = true;\n> > > > + foundIfHeights[1] = ifHeight;\n> > > > break;\n> > > > }\n> > > > }\n> > > > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,\n> > > const Size &iif, const Size &gdc\n> > > > ifHeight += IF_ALIGN_H;\n> > > > }\n> > > >\n> > > > - if (found) {\n> > > > + if (foundIfHeights[0])\n> > > > + ifHeight = foundIfHeights[0];\n> > > > + if (foundIfHeights[1])\n> > > > + ifHeight = foundIfHeights[1];\n> > > > +\n> > > > + if (foundIfHeights[0] || foundIfHeights[1]) {\n> > >\n> > > nit: This could be simplified by turning 'found' into an unsigned int\n> > > initialized to 0 and storing ifHeight inside found instead of true. But\n> > > I think this maybe a matter of taste so with or without this,\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > >\n> > Is it necessary to do the second-loop when a height is found in the first\n> > loop?\n>\n> Yes, as\n>\n> if (foundIfHeights[0])\n> ifHeight = foundIfHeights[0];\n> if (foundIfHeights[1])\n> ifHeight = foundIfHeights[1];\n>\n> overwrites the first found result with the second found one.\n> Actually as Niklas and Laurent pointed out, this can be handled with a\n> single variable that gets overwritten by the second loop, if found.\n>\n> Let's deviate from the script, I'll take this change in.\n>\n>\nAck.\n\nReviewed-by: Hirokazu Honda \n\n\n> Thanks\n> j\n>\n> >\n> > -Hiro\n> >\n> > > > unsigned int bdsIntHeight =\n> static_cast > > int>(bdsHeight);\n> > > >\n> > > > pipeConfigs.push_back({ bdsSF, { iif.width,\n> > > ifHeight },\n> > > > --\n> > > > 2.31.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\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 A93ABBF829\n\tfor ;\n\tWed, 12 May 2021 04:19:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03A706891C;\n\tWed, 12 May 2021 06:19:12 +0200 (CEST)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A65EB602B6\n\tfor ;\n\tWed, 12 May 2021 06:19:09 +0200 (CEST)","by mail-ed1-x529.google.com with SMTP id b17so25430437ede.0\n\tfor ;\n\tTue, 11 May 2021 21:19:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"c1EI9Tnm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=65nv+UlM44CcBT0NSsgiKphL1f0meK3tJgGK191ckGY=;\n\tb=c1EI9Tnm82hLeAETckf6/7FawQLx+JD6vRlQHqIIY3prWMw2EEbFAlxOlQcbRBCj2T\n\tsUOO31We86GAKFk8ZKj/Nwr9fle3MPC05xeA078/TwQYN6g93uohIU9wcFRpgpB71Y+Y\n\t0CzDH6H9AbOFS8bCtQ80ZBEni8p7vyDpRaO/E=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=65nv+UlM44CcBT0NSsgiKphL1f0meK3tJgGK191ckGY=;\n\tb=OTwKFNzFbk6Hxw9Y0B94oBn94pFZBFS+iLOVLoQbj+ieYj+vNazIrvG/CG8RIZztS3\n\ts3OtNvCXqPGYsHePMhvdSIoXjgn8lActnis413MA+ppzHSbVlZtbg7UFZKf8o2MJ7JSW\n\t3GAeFRyfD6Vb7sfWgXERyBY0IES6iLP1pG3sFuZfe/AqN/AzeRoDRexRjr0MDUQEWnie\n\tu5qD++Di2R/I6AYncdK0lzNw4R4yPCBCt4SXbT0hp8IJQAeauV5Ah3ThLUT8Ha6KeS3+\n\tk3NGdIeCbDXrFx6AtaSdPFOZxR4yMenRrItc6pyiXEWK6/r8og+cFl1WE451oZGy3nQe\n\tq/hQ==","X-Gm-Message-State":"AOAM532K8DQriXccMFPhJ40MpG6+f+A6uIQ5DIn9XjTXv25jyyy0AQOz\n\th0/LlFkfg43s7hzzYSiNHPBnrEJO/yMUdWCr+DGDaRTGGCI=","X-Google-Smtp-Source":"ABdhPJymX26R+IIlPD4FJrCWNq/utm/1SALOVpxJvp+jNqUC5/MzzQo1ymDXLKTExd8AdFunb6xIKlNn7NPxDM367m8=","X-Received":"by 2002:a05:6402:50c6:: with SMTP id\n\th6mr41214378edb.327.1620793149269; \n\tTue, 11 May 2021 21:19:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20210503092705.15562-1-jacopo@jmondi.org>\n\t<20210503092705.15562-4-jacopo@jmondi.org>\n\t\n\t\n\t<20210511143716.ssxjwrw4hng2e3dz@uno.localdomain>","In-Reply-To":"<20210511143716.ssxjwrw4hng2e3dz@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 12 May 2021 13:18:58 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS\n\theight calculation","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============6635398218199223890==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]