[{"id":28024,"web_url":"https://patchwork.libcamera.org/comment/28024/","msgid":"<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>","date":"2023-10-23T08:54:52","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi\n\nOn Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> - Use division operator in rpi pipeline handler\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index da52d7fafcee..ee222d060e4a 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n>  \t}\n>\n>  \t/* Always send the user transform to the IPA. */\n> -\tparams.transform =\n> -\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n> +\tTransform transform = config->orientation / Orientation::Rotate0;\n> +\tparams.transform = static_cast<unsigned int>(transform);\n\nI wonder if this was correct in first place.\n\nconfig->orientation could be adjusted to report the sensor's mounting\norientation if the user requested orientation cannot be obtained, in\nwhich case the \"user transform\" will be set to Identity (see\nCameraSensor::computeTransform()).\n\nWouldn't it be better to send to the IPA 'combinedTransform_' ?\n\n>\n>  \t/* Ready the IPA - it must know about the sensor resolution. */\n>  \tret = ipa_->configure(sensorInfo_, params, result);\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0FF2AC0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 08:54:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C8ED60556;\n\tMon, 23 Oct 2023 10:54:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04AAD60556\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 10:54:55 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33957749;\n\tMon, 23 Oct 2023 10:54:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698051297;\n\tbh=e9q1y9AadUGGPlNmewfuTrBkofCmQ8DDnXYRs2SucxE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=y6mtS7ycUdLDfjJF54UVyBnTj+tZ8fYcJAEYIfVozHvJnNPnZPbWAdrKBRqK4jrY5\n\ts6ZLMsX7eZN05UwPtEyHPUsp8s7XgzuSdCcpBKQlQ+F2qkghEsZggAUdt02Pgvh0Fm\n\tA1316RU8a3Kdl7bsdj0itlD4INVlBi0LKu/r+t8B24rWYuUxa7lK+eH+c0P7dd4Vb2\n\tE5ML9jXLl5RW0FBZ+UInACublXEBJ9ISqx5l5hUCeJHzN41SIV5uuGC1nkbApV6RZR\n\tcQY8Jy2Lezh/I5c8Df8utybq1WHc+upgupvd0pOpOztlKYR5ufwSDHvZQgosHsXdGL\n\tslTjdNjwuPpDg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698051285;\n\tbh=e9q1y9AadUGGPlNmewfuTrBkofCmQ8DDnXYRs2SucxE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uj0P7h98pa73XUCFVjl7q+LA8+Ulhde9xiyzUsKCxIhUzGXaeGJUUipeMheTYaa/d\n\t/Nmcei4Q/qgekqA0RuZy39wWjBqCXj1j69mxjT3KYQBPksMc9EdoBrgJ4Pg5RKEuqP\n\t1XPX/AMpmf+mxureqZVxYYEbbme9AK2CNHffyn/c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uj0P7h98\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 10:54:52 +0200","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28027,"web_url":"https://patchwork.libcamera.org/comment/28027/","msgid":"<20231023102001.GE3336@pendragon.ideasonboard.com>","date":"2023-10-23T10:20:01","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > - Use division operator in rpi pipeline handler\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> >  1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index da52d7fafcee..ee222d060e4a 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> >  \t}\n> >\n> >  \t/* Always send the user transform to the IPA. */\n> > -\tparams.transform =\n> > -\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > +\tTransform transform = config->orientation / Orientation::Rotate0;\n> > +\tparams.transform = static_cast<unsigned int>(transform);\n> \n> I wonder if this was correct in first place.\n\nDo you mean in \"libcamera: Use CameraConfiguration::orientation\", or\nbefore that ?\n\n> config->orientation could be adjusted to report the sensor's mounting\n> orientation if the user requested orientation cannot be obtained, in\n> which case the \"user transform\" will be set to Identity (see\n> CameraSensor::computeTransform()).\n\nLet's assume a sensor mounted with a 180° rotation, and the user asks\nfor Rotate270 rotation. This can't be achieved, so config->orientation\nis adjusted to Rotate180. params.transform will be Rot180 in that case,\nnot Identiy. Am I missing something ?\n\n> Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> \n> >\n> >  \t/* Ready the IPA - it must know about the sensor resolution. */\n> >  \tret = ipa_->configure(sensorInfo_, params, result);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 33897BDCBD\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 10:19:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81AEF62973;\n\tMon, 23 Oct 2023 12:19:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD72161DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 12:19:53 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09324D20;\n\tMon, 23 Oct 2023 12:19:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698056395;\n\tbh=HSkwoqLOucThK3cI8a3f7yySw02oXGS4Dam1P679l60=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=AU5TbGl0MI8pUumr0kKzFN+Xv5BvVn9o2xF3lKcHcuW8LIN6HNDSoOfUZ3gI18xDl\n\t/G8wnyu9K60BywKj5UP7TEoct1YmBOTRbUS6p6Oas9QiMOa3jjl7pp3erCtc+wbDBu\n\tfdG/HXGbFjBEEqrDGzwfCtHYlTvoJkVR5BcDLjV7DVAiAOxSxaym88Kvt+ol2t8bUR\n\tQbj5Mb+wJG/YKMiH2yC0djPfLBcSUwZLk4zOYmel9sOVMdU2NXoBrGFnlEenhPEz6Y\n\tzFQBSJSrQTw7p8bt4OpIW7L2A7fcFY+JusxufIo5BrZZaJBdG4dH4PNoGCyJx/Np5n\n\t1Y9oVQnk4tTNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698056383;\n\tbh=HSkwoqLOucThK3cI8a3f7yySw02oXGS4Dam1P679l60=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TsD85kDNaN28B/pAJAFYQ5T1qF6FcSPt9PybugQoX4QKw+O6xszUzbG6yY6lGmBdy\n\t8sj5N2obYPWnaw1dWuAqFAontTe1k84Got4WTOI1YgceLfqc661TaoncA6sz/J4Aex\n\tWmHNwHZ7/NiF/tlqcxGpkKvla4Kr18I9EOQ4aLbI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TsD85kDN\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 13:20:01 +0300","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20231023102001.GE3336@pendragon.ideasonboard.com>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28029,"web_url":"https://patchwork.libcamera.org/comment/28029/","msgid":"<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>","date":"2023-10-23T10:35:53","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:\n> On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > - Use division operator in rpi pipeline handler\n> > >\n> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > ---\n> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > >  1 file changed, 2 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > index da52d7fafcee..ee222d060e4a 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > >  \t}\n> > >\n> > >  \t/* Always send the user transform to the IPA. */\n> > > -\tparams.transform =\n> > > -\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > > +\tTransform transform = config->orientation / Orientation::Rotate0;\n> > > +\tparams.transform = static_cast<unsigned int>(transform);\n> >\n> > I wonder if this was correct in first place.\n>\n> Do you mean in \"libcamera: Use CameraConfiguration::orientation\", or\n> before that ?\n>\n\nEven before that, when we had\n\n       params.transform = static_cast<unsigned int>(config->transform);\n> > config->orientation could be adjusted to report the sensor's mounting\n> > orientation if the user requested orientation cannot be obtained, in\n> > which case the \"user transform\" will be set to Identity (see\n> > CameraSensor::computeTransform()).\n>\n> Let's assume a sensor mounted with a 180° rotation, and the user asks\n> for Rotate270 rotation. This can't be achieved, so config->orientation\n> is adjusted to Rotate180. params.transform will be Rot180 in that case,\n> not Identiy. Am I missing something ?\n>\n\nI'm probably missing what's the use of transform is in the IPA\n\nLooking at the alsc.cpp algorithms from RPi (which seems to me to be\nthe only user of the transform field)\n\n            if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {\n                    xLo[i] = X - 1 - xLo[i];\n                    xHi[i] = X - 1 - xHi[i];\n            }\n\n            ...\n\n            if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {\n                    yLo = Y - 1 - yLo;\n                    yHi = Y - 1 - yHi;\n            }\n\nI'm not sure I fully got what the code does, but it seems to inspect\nthe actual transform as applied by the PH/CameraSensor, which\ncorresponds to the combinedTransform_ variable, which in your example\nis Identity, and not the image orientation in the buffers, which in\nyour example is Rot180 ?\n\nAnyway, as said this was here already, so it's not strictly related to\nthis patch and was here already\n\n\n> > Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> >\n> > >\n> > >  \t/* Ready the IPA - it must know about the sensor resolution. */\n> > >  \tret = ipa_->configure(sensorInfo_, params, result);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 9FAF7C0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 10:35:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 25EEC6297F;\n\tMon, 23 Oct 2023 12:35:58 +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 58CC561DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 12:35:57 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E096D20;\n\tMon, 23 Oct 2023 12:35:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698057358;\n\tbh=JFhj9ztU3OCZc9MJuL5+AmeA/jPb51Q00NcK6e8eQEk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=F81RXEF+Pe8IxIcIJVNfUv8qO8EEpw8R+/T7Mue794Ryjx8JjMpr1AawyCTqGXDOk\n\t3S0EMoNG/o1vUxn0fHHW5NJYkwIrSagvdN95JP+x3omQKX+Zz/Gxix5jCRF0RwajMX\n\tpCuEonXZW+xMAov+xz0EZpMNScCjOc2tVpLVq7TvPl/EnLwDDaOXVj8KEJV8Ooy7M6\n\teuQfPMF9NkrJFYhGi5VCVHKys5WONCImbXA/y2m6suXKlx4X8eLQ2XeJS8Rg/9f7mY\n\t8MylTCYNjMmF4BkoZq29jleAGIsxs3f8qHlKqnYEoA62hJuvXU4TE1KcmPEnkjK9NP\n\t07m9CPiJGU/tw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698057346;\n\tbh=JFhj9ztU3OCZc9MJuL5+AmeA/jPb51Q00NcK6e8eQEk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EG5n2yJ9ZnDtuHPcE62TK6/wvbXe1fL44EdFgMAN7FIV6rtqkFlEINah0g9Ny+8gk\n\tQmqYWZARQnNPW7C/QMWgJF9puSA+ec+SwJL4n2oZ++AC1D56ywKHATXMjDYGl0ccVf\n\tJRcKPJqdvJ5b0T3RvL0V4/NiAp1G8qZqzcO6/1Zw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EG5n2yJ9\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 12:35:53 +0200","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>\n\t<20231023102001.GE3336@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20231023102001.GE3336@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28030,"web_url":"https://patchwork.libcamera.org/comment/28030/","msgid":"<20231023111153.GG3336@pendragon.ideasonboard.com>","date":"2023-10-23T11:11:53","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 23, 2023 at 12:35:53PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:\n> > On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> > > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > - Use division operator in rpi pipeline handler\n> > > >\n> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > ---\n> > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > > >  1 file changed, 2 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > index da52d7fafcee..ee222d060e4a 100644\n> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > > >  \t}\n> > > >\n> > > >  \t/* Always send the user transform to the IPA. */\n> > > > -\tparams.transform =\n> > > > -\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > > > +\tTransform transform = config->orientation / Orientation::Rotate0;\n> > > > +\tparams.transform = static_cast<unsigned int>(transform);\n> > >\n> > > I wonder if this was correct in first place.\n> >\n> > Do you mean in \"libcamera: Use CameraConfiguration::orientation\", or\n> > before that ?\n> \n> Even before that, when we had\n> \n>        params.transform = static_cast<unsigned int>(config->transform);\n>\n> > > config->orientation could be adjusted to report the sensor's mounting\n> > > orientation if the user requested orientation cannot be obtained, in\n> > > which case the \"user transform\" will be set to Identity (see\n> > > CameraSensor::computeTransform()).\n> >\n> > Let's assume a sensor mounted with a 180° rotation, and the user asks\n> > for Rotate270 rotation. This can't be achieved, so config->orientation\n> > is adjusted to Rotate180. params.transform will be Rot180 in that case,\n> > not Identiy. Am I missing something ?\n> \n> I'm probably missing what's the use of transform is in the IPA\n> \n> Looking at the alsc.cpp algorithms from RPi (which seems to me to be\n> the only user of the transform field)\n> \n>             if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {\n>                     xLo[i] = X - 1 - xLo[i];\n>                     xHi[i] = X - 1 - xHi[i];\n>             }\n> \n>             ...\n> \n>             if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {\n>                     yLo = Y - 1 - yLo;\n>                     yHi = Y - 1 - yHi;\n>             }\n> \n> I'm not sure I fully got what the code does, but it seems to inspect\n> the actual transform as applied by the PH/CameraSensor, which\n> corresponds to the combinedTransform_ variable, which in your example\n> is Identity, and not the image orientation in the buffers, which in\n> your example is Rot180 ?\n\nAs far as I understand, it's only the sensor-side transform that needs\nto be taken into account by that code, not the combined transform from\nthe camera sensor and the ISP.\n\n> Anyway, as said this was here already, so it's not strictly related to\n> this patch and was here already\n> \n> > > Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> > >\n> > > >\n> > > >  \t/* Ready the IPA - it must know about the sensor resolution. */\n> > > >  \tret = ipa_->configure(sensorInfo_, params, result);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C8CBDBDCBD\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 11:11:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2501A62973;\n\tMon, 23 Oct 2023 13:11:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF65461DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 13:11:45 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F01532B6;\n\tMon, 23 Oct 2023 13:11:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698059507;\n\tbh=0pdZFlobLbUga6Ug4lcwL7ESwkWwBk9+RWoSU1FHuDk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cD+yI9QBIRqzTFCNdqehpX/wxTDlnpZvOEoDLxrANMr/sUakTCP7Ty+d1oubfiKrA\n\tHKKNBpTmvzmoRFJYyxSZx51dXibV8P9hT1mo6FW/i3Yd/a/0vFILHzMneG+kLNQ0Hx\n\tywmoyznlxZQ1KOv/S6SMkJ9brlLcUVWWKUYqsYY9q7XxBXmTvI5G/36+G6GacLg+16\n\tj35FRDHvGliPxMMV5ooEGqeMv3MZdHgIKogtroQtP3Dhi5B9L3U//CaubWSSuPJ51X\n\tj+ynNz6IRgwioj9FsGqGcDAm83csXb8hTow3WoqQU5sly8vIsYSqCtKVids0jehalW\n\twVaxxH6luvbhg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698059495;\n\tbh=0pdZFlobLbUga6Ug4lcwL7ESwkWwBk9+RWoSU1FHuDk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wSuUsbeoHddpatfhGOCFj+SgA+QTbZXb7QQSiEpvnKF2TfMWiUNB8uWAp1hkhNDHG\n\tz/GlT+i51rWbN2qaBv0bb5Bhh4+6lukZH4S8LNUX652rd6wTO+WvqX1cshe4jk2ZSe\n\tGLBjmUeltYl0aKCdDYqVQzIzEWQotAhoDVDTy3/s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wSuUsbeo\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 14:11:53 +0300","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20231023111153.GG3336@pendragon.ideasonboard.com>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>\n\t<20231023102001.GE3336@pendragon.ideasonboard.com>\n\t<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28031,"web_url":"https://patchwork.libcamera.org/comment/28031/","msgid":"<20231023112115.GH3336@pendragon.ideasonboard.com>","date":"2023-10-23T11:21:15","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"And CC'ing David and Naush, as the issue may affect them.\n\nOn Mon, Oct 23, 2023 at 02:11:53PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Mon, Oct 23, 2023 at 12:35:53PM +0200, Jacopo Mondi wrote:\n> > On Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:\n> > > On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> > > > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > > - Use division operator in rpi pipeline handler\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > > ---\n> > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > index da52d7fafcee..ee222d060e4a 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > > > >  \t}\n> > > > >\n> > > > >  \t/* Always send the user transform to the IPA. */\n> > > > > -\tparams.transform =\n> > > > > -\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > > > > +\tTransform transform = config->orientation / Orientation::Rotate0;\n> > > > > +\tparams.transform = static_cast<unsigned int>(transform);\n> > > >\n> > > > I wonder if this was correct in first place.\n> > >\n> > > Do you mean in \"libcamera: Use CameraConfiguration::orientation\", or\n> > > before that ?\n> > \n> > Even before that, when we had\n> > \n> >        params.transform = static_cast<unsigned int>(config->transform);\n> >\n> > > > config->orientation could be adjusted to report the sensor's mounting\n> > > > orientation if the user requested orientation cannot be obtained, in\n> > > > which case the \"user transform\" will be set to Identity (see\n> > > > CameraSensor::computeTransform()).\n> > >\n> > > Let's assume a sensor mounted with a 180° rotation, and the user asks\n> > > for Rotate270 rotation. This can't be achieved, so config->orientation\n> > > is adjusted to Rotate180. params.transform will be Rot180 in that case,\n> > > not Identiy. Am I missing something ?\n> > \n> > I'm probably missing what's the use of transform is in the IPA\n> > \n> > Looking at the alsc.cpp algorithms from RPi (which seems to me to be\n> > the only user of the transform field)\n> > \n> >             if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {\n> >                     xLo[i] = X - 1 - xLo[i];\n> >                     xHi[i] = X - 1 - xHi[i];\n> >             }\n> > \n> >             ...\n> > \n> >             if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {\n> >                     yLo = Y - 1 - yLo;\n> >                     yHi = Y - 1 - yHi;\n> >             }\n> > \n> > I'm not sure I fully got what the code does, but it seems to inspect\n> > the actual transform as applied by the PH/CameraSensor, which\n> > corresponds to the combinedTransform_ variable, which in your example\n> > is Identity, and not the image orientation in the buffers, which in\n> > your example is Rot180 ?\n> \n> As far as I understand, it's only the sensor-side transform that needs\n> to be taken into account by that code, not the combined transform from\n> the camera sensor and the ISP.\n> \n> > Anyway, as said this was here already, so it's not strictly related to\n> > this patch and was here already\n> > \n> > > > Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> > > >\n> > > > >\n> > > > >  \t/* Ready the IPA - it must know about the sensor resolution. */\n> > > > >  \tret = ipa_->configure(sensorInfo_, params, result);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 40CB5C0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 11:21:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 620806297F;\n\tMon, 23 Oct 2023 13:21:09 +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 38DA761DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 13:21:08 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55BF72B6;\n\tMon, 23 Oct 2023 13:20:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698060069;\n\tbh=7+taj55nItyPxJF7eYuclZG4kBXTmVV5tXYNuxwpqSs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=KlwcsX1bkTlJ2dvxPg59btEJynT18J0m3Q11yh96h5X+TR9L+w4yWfdSIqQU3Vzeg\n\tkpsbh46I9GSdWYTIZ7qahECECUxSk4KT1G2aAeLgUgp5lZCoJFkFq72C+6Mnn1/g6C\n\t38ayXEu7PiY5gNmzHuTU/4KqPL8f9b/15J35o89F04rQoZ8hrsFWXB0Ta0nlf2mehc\n\tG1HIlVxUmPdw/2tvQ+8Pmy50lgNPX3ZjAychdrJ7wlrb8sGOJcIwax6I6dOTAJW8fe\n\tZpjhnKPYmZbvLMXk5xenwWROjRQfnKUjQS5R6X5hAdetqWaoNuNBIUrXlWU2NWwyoc\n\toP6drcWPKPb4w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698060057;\n\tbh=7+taj55nItyPxJF7eYuclZG4kBXTmVV5tXYNuxwpqSs=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=jNF05LvJicv3BL7tzbcwArvcWbrXGRWP7UD+ES3xGrFOrsSIUL3+egBo+YHM1hN3G\n\tKHpy0pqvIdZ0lIbY/YIC1dPIbDl3xD17AwhPGu1lFJMN4xV52W/GmvdVXADqlAcliX\n\tR0P3G8AEsjZIoJPtWMWzFPI5D7CWULQzrreadUCE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jNF05LvJ\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 14:21:15 +0300","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org,\n\tDavid Plowman <david.plowman@raspberrypi.com>,\n\tNaushir Patuck <naush@raspberrypi.com>","Message-ID":"<20231023112115.GH3336@pendragon.ideasonboard.com>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>\n\t<20231023102001.GE3336@pendragon.ideasonboard.com>\n\t<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>\n\t<20231023111153.GG3336@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20231023111153.GG3336@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28032,"web_url":"https://patchwork.libcamera.org/comment/28032/","msgid":"<6masr5hykwoo6w4h6hums5v46rmvk2cngefmrwa75xgykz2h5n@hdd7t433iqad>","date":"2023-10-23T11:23:03","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Mon, Oct 23, 2023 at 02:11:53PM +0300, Laurent Pinchart wrote:\n> On Mon, Oct 23, 2023 at 12:35:53PM +0200, Jacopo Mondi wrote:\n> > On Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:\n> > > On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> > > > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > > - Use division operator in rpi pipeline handler\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > > ---\n> > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > index da52d7fafcee..ee222d060e4a 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > > > >  \t}\n> > > > >\n> > > > >  \t/* Always send the user transform to the IPA. */\n> > > > > -\tparams.transform =\n> > > > > -\t\tstatic_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > > > > +\tTransform transform = config->orientation / Orientation::Rotate0;\n> > > > > +\tparams.transform = static_cast<unsigned int>(transform);\n> > > >\n> > > > I wonder if this was correct in first place.\n> > >\n> > > Do you mean in \"libcamera: Use CameraConfiguration::orientation\", or\n> > > before that ?\n> >\n> > Even before that, when we had\n> >\n> >        params.transform = static_cast<unsigned int>(config->transform);\n> >\n> > > > config->orientation could be adjusted to report the sensor's mounting\n> > > > orientation if the user requested orientation cannot be obtained, in\n> > > > which case the \"user transform\" will be set to Identity (see\n> > > > CameraSensor::computeTransform()).\n> > >\n> > > Let's assume a sensor mounted with a 180° rotation, and the user asks\n> > > for Rotate270 rotation. This can't be achieved, so config->orientation\n> > > is adjusted to Rotate180. params.transform will be Rot180 in that case,\n> > > not Identiy. Am I missing something ?\n> >\n> > I'm probably missing what's the use of transform is in the IPA\n> >\n> > Looking at the alsc.cpp algorithms from RPi (which seems to me to be\n> > the only user of the transform field)\n> >\n> >             if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {\n> >                     xLo[i] = X - 1 - xLo[i];\n> >                     xHi[i] = X - 1 - xHi[i];\n> >             }\n> >\n> >             ...\n> >\n> >             if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {\n> >                     yLo = Y - 1 - yLo;\n> >                     yHi = Y - 1 - yHi;\n> >             }\n> >\n> > I'm not sure I fully got what the code does, but it seems to inspect\n> > the actual transform as applied by the PH/CameraSensor, which\n> > corresponds to the combinedTransform_ variable, which in your example\n> > is Identity, and not the image orientation in the buffers, which in\n> > your example is Rot180 ?\n>\n> As far as I understand, it's only the sensor-side transform that needs\n> to be taken into account by that code, not the combined transform from\n> the camera sensor and the ISP.\n>\n\nI agree, but in this case you would be passing in Rotate180 (which is\nthe mounting rotation) instead of the actual transform applied on the\nsensor (which, in case of rpi and all the other platforms we have is\nthe transform applied on the sensor, as the ISP does not do any transform)\n\nDavid, Naush, should this be changed on top ?\n\n> > Anyway, as said this was here already, so it's not strictly related to\n> > this patch and was here already\n> >\n> > > > Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> > > >\n> > > > >\n> > > > >  \t/* Ready the IPA - it must know about the sensor resolution. */\n> > > > >  \tret = ipa_->configure(sensorInfo_, params, result);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DFB49C0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 11:23:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63F9462980;\n\tMon, 23 Oct 2023 13:23:07 +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 2CFB261DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 13:23:06 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C2A62B6;\n\tMon, 23 Oct 2023 13:22:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698060187;\n\tbh=t7aiyulVs4opamY8sSUBraAoXmFcg8esGnSksO5PXSU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2MacKODsRK+jBOTO0d4ZxKQP6TrKtPFuZO9EQwUkkeAGcGfmoaCjR7GfvKueOYh+t\n\tmX0ADl3xYn9OhTaeBHfoAbxSwtSR31MeMd2OZWnnC8601vxcds/cy+RtiUZGIWGkzg\n\tPD+L4qbaGa0Yb/06P6TuC0UOoXQRwhcZqwmHWsatQVvcsLMLu6u6owebhj0HTXjBHR\n\tcHQqDNY2JDEGjOCElH2BaZORPIcDLUaN5L0UaUSJS6jXJODR3jcBP8mK4DbJelwhp0\n\tHi4i0+Z+/Tubc0Eaf2LA7UpM7AwYT3Mivnkkm8g5F1PepzwAuM97RwUmtDWloBaTex\n\tngpdIUXHHFNiQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698060175;\n\tbh=t7aiyulVs4opamY8sSUBraAoXmFcg8esGnSksO5PXSU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MbKUCNAg5Z7u9RKJt0GqYGIvjU4aYjJF2rRC/6pG00cDeYIKHNlNdaTFhieLO3gKP\n\tLK76+L9QAcMcSOzOv8VrY/xxs6t1FgCuxCGTC2BKKkHHFtQeyo05+NTqKMhAoKLW3/\n\tDgluHqvLOVHdmQGCW1jxq+ye5EmY8aY576fwBp0E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MbKUCNAg\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 13:23:03 +0200","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<6masr5hykwoo6w4h6hums5v46rmvk2cngefmrwa75xgykz2h5n@hdd7t433iqad>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>\n\t<20231023102001.GE3336@pendragon.ideasonboard.com>\n\t<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>\n\t<20231023111153.GG3336@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20231023111153.GG3336@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28036,"web_url":"https://patchwork.libcamera.org/comment/28036/","msgid":"<CAHW6GYKFT7EcvE_RiS11pSWLEJAf0ttCJjB0PxWGKtKOSQLWBQ@mail.gmail.com>","date":"2023-10-23T12:39:57","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Ah, *sigh*. Transforms and orientations.\n\nI agree it's probably only ALSC that cares about the orientation. It\ndoes seem slightly strange to pass the config->orientation (or\nconfig->transform previously) to the IPA rather than the\ncombinedTransform. But I guess maybe it depends on the orientation of\nthe LSC tables in the tuning files.\n\nThe quoted code looks to me as though it stands a chance of being\ncorrect if the tables are stored in the \"normal way up\" orientation,\ni.e. with the sensor applying both h and v flips for our modules,\ncompensating for the mounting orientation. If the tables were in the\nno h/vflips at all orientation, then I would guess it's wrong? I'm not\nentirely sure what it actually does but perhaps we can give it the\nbenefit of the doubt for a moment.\n\nThough even there I'm a bit doubtful about the behaviour. If another\nvendor made a board with a different mounting orientation, then I\nsuspect that would go wrong? Possibly we ought to pass the\ncombinedTransform over, and either flip our tables over, or store a\n\"reference transform/orientation\" in the tuning file which we could\ncompare against.\n\nIf that sounds plausible to everyone, then I'm hoping that there's\nnothing mega-urgent here, though this last point probably wants fixing\nin due course.\n\nDavid\n\nOn Mon, 23 Oct 2023 at 12:23, Jacopo Mondi via libcamera-devel\n<libcamera-devel@lists.libcamera.org> wrote:\n>\n> Hi Laurent\n>\n> On Mon, Oct 23, 2023 at 02:11:53PM +0300, Laurent Pinchart wrote:\n> > On Mon, Oct 23, 2023 at 12:35:53PM +0200, Jacopo Mondi wrote:\n> > > On Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:\n> > > > On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> > > > > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > > > - Use division operator in rpi pipeline handler\n> > > > > >\n> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > > > ---\n> > > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > index da52d7fafcee..ee222d060e4a 100644\n> > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > > > > >       }\n> > > > > >\n> > > > > >       /* Always send the user transform to the IPA. */\n> > > > > > -     params.transform =\n> > > > > > -             static_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > > > > > +     Transform transform = config->orientation / Orientation::Rotate0;\n> > > > > > +     params.transform = static_cast<unsigned int>(transform);\n> > > > >\n> > > > > I wonder if this was correct in first place.\n> > > >\n> > > > Do you mean in \"libcamera: Use CameraConfiguration::orientation\", or\n> > > > before that ?\n> > >\n> > > Even before that, when we had\n> > >\n> > >        params.transform = static_cast<unsigned int>(config->transform);\n> > >\n> > > > > config->orientation could be adjusted to report the sensor's mounting\n> > > > > orientation if the user requested orientation cannot be obtained, in\n> > > > > which case the \"user transform\" will be set to Identity (see\n> > > > > CameraSensor::computeTransform()).\n> > > >\n> > > > Let's assume a sensor mounted with a 180° rotation, and the user asks\n> > > > for Rotate270 rotation. This can't be achieved, so config->orientation\n> > > > is adjusted to Rotate180. params.transform will be Rot180 in that case,\n> > > > not Identiy. Am I missing something ?\n> > >\n> > > I'm probably missing what's the use of transform is in the IPA\n> > >\n> > > Looking at the alsc.cpp algorithms from RPi (which seems to me to be\n> > > the only user of the transform field)\n> > >\n> > >             if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {\n> > >                     xLo[i] = X - 1 - xLo[i];\n> > >                     xHi[i] = X - 1 - xHi[i];\n> > >             }\n> > >\n> > >             ...\n> > >\n> > >             if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {\n> > >                     yLo = Y - 1 - yLo;\n> > >                     yHi = Y - 1 - yHi;\n> > >             }\n> > >\n> > > I'm not sure I fully got what the code does, but it seems to inspect\n> > > the actual transform as applied by the PH/CameraSensor, which\n> > > corresponds to the combinedTransform_ variable, which in your example\n> > > is Identity, and not the image orientation in the buffers, which in\n> > > your example is Rot180 ?\n> >\n> > As far as I understand, it's only the sensor-side transform that needs\n> > to be taken into account by that code, not the combined transform from\n> > the camera sensor and the ISP.\n> >\n>\n> I agree, but in this case you would be passing in Rotate180 (which is\n> the mounting rotation) instead of the actual transform applied on the\n> sensor (which, in case of rpi and all the other platforms we have is\n> the transform applied on the sensor, as the ISP does not do any transform)\n>\n> David, Naush, should this be changed on top ?\n>\n> > > Anyway, as said this was here already, so it's not strictly related to\n> > > this patch and was here already\n> > >\n> > > > > Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> > > > >\n> > > > > >\n> > > > > >       /* Ready the IPA - it must know about the sensor resolution. */\n> > > > > >       ret = ipa_->configure(sensorInfo_, params, result);\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 89781C0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 12:40:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED33F6297F;\n\tMon, 23 Oct 2023 14:40:11 +0200 (CEST)","from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com\n\t[IPv6:2607:f8b0:4864:20::72c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60DA561DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 14:40:10 +0200 (CEST)","by mail-qk1-x72c.google.com with SMTP id\n\taf79cd13be357-778711ee748so258321185a.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 05:40:10 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698064812;\n\tbh=KuFDdMBDNSf3AuCSDLj2HsPUfvwREapHzFcQNhhdYts=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mVrO4UEzLWufSsKIBJwbERZGgux/hMKaqGv3CsKHDuwXy8IhG34io+4fV0o6S4I+c\n\tfp2vhmuWNxvccNbFTHCLCdNnojEtA8RxPWyISr8hsGwfG3Le43u6/LKn3VBu0HOt7G\n\t3UtgkItIuseC4QptT2JT5jav4K/Y8CXXnmeScdVfmr4JWYYyY2Mfy05N7hhGP/Plqn\n\t4YPLFKoW+4EDkP2bStipUAQKRoVmqr4h6gvmdT91jvReGRAZEvvR4bKrbYb+44jqUP\n\tiinPg5gtpsTVjKB2lsMZb+jLpuKCHMt3yUzW5dt9QXmpINKVi2iyU+uOdSNM2AnSa9\n\tCU2NXekzZImLg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1698064809; x=1698669609;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=s6xWe+qYP51xaVp4AllLsBuT9SW/0cVYzOQvphd3fGY=;\n\tb=GZAxYnv2mpaPz1Lu+48pdNU92m2DsusKo61rrS65KTyCJKk9GOlNAD2LNfEsaAFcaD\n\tJy5Ig2+pSyT6V9lUCN0kUBAA2g3rhrCGKzbpS5IOnSkwznYyVs3eM855UZ0NmQL8W9gS\n\tt1lDY3ucUUnRsRo1YlOOo3mxCf2UKkLmV9lQ6UPmXR5LMCu9V0kOFQsel36NfTWJaVHV\n\tYEUaa6b81sG1SeD9FVfYgphJEZ1WTyUr50tbtEQnryLAHO3T4ey7Oj4I78Gqt7f4Biil\n\t5heAv1FYzsbIKvU9RJaCAaaqIJHxSpeb93gND6hWIac/fesTi2cwkhC4I3Hm5hsFHcLM\n\texLg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"GZAxYnv2\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1698064809; x=1698669609;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=s6xWe+qYP51xaVp4AllLsBuT9SW/0cVYzOQvphd3fGY=;\n\tb=HqSbT+QXF6YaGWXfqGkfwTrMN1T8ySXcm9R+CUIWKHAq5GCUx/B2WThi6/fg+gUKdt\n\tM0M35Ivh7sX2BrgyxEPp0fZQnAgB9Z6X17WgQF/jTkw7iP+q0gvtlRsEVIfiYWWhYI9+\n\tUXxI1rp3fWGH60vlwThyLHHJYybRBzqvIdv0OqjrP2N/XcTTHwaYGM0F59Rc2SLL1Tg2\n\tIaV97NyfMb/BLEfosmsplDdOYopQ/uinr22hOLc6YZCP0/ELIe+gFVrKw7bs71RMWWEo\n\tginW0touCjhKUC/WBw00Ptmddf+fSuBt3loukZeTSF0/P3/iAfrPvW9slxuwhzCHofas\n\tShBw==","X-Gm-Message-State":"AOJu0Yxpnc82XEk+ep2JvbQcUVkyMJzZgMRzWhuodetE61AdDqR6McjZ\n\tQmqzXwv+kdcGgR/IIlUWmc+aaAY+juPFaFDbpIw/XVmN0XEvHa8L","X-Google-Smtp-Source":"AGHT+IEDy1uZ7jvuhu4JAokfPR768bQppEIsgkVkL5jQsF9a6Rf51PhPORCi6wT2q2juLCw1vnewar/4so10gjliDLo=","X-Received":"by 2002:ad4:5946:0:b0:65b:a66:3109 with SMTP id\n\teo6-20020ad45946000000b0065b0a663109mr10069618qvb.8.1698064809093;\n\tMon, 23 Oct 2023 05:40:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>\n\t<20231023102001.GE3336@pendragon.ideasonboard.com>\n\t<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>\n\t<20231023111153.GG3336@pendragon.ideasonboard.com>\n\t<6masr5hykwoo6w4h6hums5v46rmvk2cngefmrwa75xgykz2h5n@hdd7t433iqad>","In-Reply-To":"<6masr5hykwoo6w4h6hums5v46rmvk2cngefmrwa75xgykz2h5n@hdd7t433iqad>","Date":"Mon, 23 Oct 2023 13:39:57 +0100","Message-ID":"<CAHW6GYKFT7EcvE_RiS11pSWLEJAf0ttCJjB0PxWGKtKOSQLWBQ@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28037,"web_url":"https://patchwork.libcamera.org/comment/28037/","msgid":"<20231023130513.GN3336@pendragon.ideasonboard.com>","date":"2023-10-23T13:05:13","subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 23, 2023 at 01:39:57PM +0100, David Plowman wrote:\n> Ah, *sigh*. Transforms and orientations.\n> \n> I agree it's probably only ALSC that cares about the orientation. It\n> does seem slightly strange to pass the config->orientation (or\n> config->transform previously) to the IPA rather than the\n> combinedTransform. But I guess maybe it depends on the orientation of\n> the LSC tables in the tuning files.\n> \n> The quoted code looks to me as though it stands a chance of being\n> correct if the tables are stored in the \"normal way up\" orientation,\n> i.e. with the sensor applying both h and v flips for our modules,\n> compensating for the mounting orientation.\n\nIs that what you have in your tuning files ?\n\n> If the tables were in the\n> no h/vflips at all orientation, then I would guess it's wrong? I'm not\n> entirely sure what it actually does but perhaps we can give it the\n> benefit of the doubt for a moment.\n> \n> Though even there I'm a bit doubtful about the behaviour. If another\n> vendor made a board with a different mounting orientation, then I\n> suspect that would go wrong? Possibly we ought to pass the\n> combinedTransform over, and either flip our tables over, or store a\n> \"reference transform/orientation\" in the tuning file which we could\n> compare against.\n\nKnowing in which orientation the tables have been computed is certainly\nuseful information. This could be conveyed in the tuning file, or we\ncould require a particular orientation to be used unconditionally. The\nlatter seems simpler, but maybe it would cause other issues ?\n\n> If that sounds plausible to everyone, then I'm hoping that there's\n> nothing mega-urgent here, though this last point probably wants fixing\n> in due course.\n\nOK, I'll then merge the series :-)\n\n> On Mon, 23 Oct 2023 at 12:23, Jacopo Mondi via libcamera-devel wrote:\n> > On Mon, Oct 23, 2023 at 02:11:53PM +0300, Laurent Pinchart wrote:\n> > > On Mon, Oct 23, 2023 at 12:35:53PM +0200, Jacopo Mondi wrote:\n> > > > On Mon, Oct 23, 2023 at 01:20:01PM +0300, Laurent Pinchart wrote:\n> > > > > On Mon, Oct 23, 2023 at 10:54:52AM +0200, Jacopo Mondi wrote:\n> > > > > > On Mon, Oct 23, 2023 at 01:41:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > > > > - Use division operator in rpi pipeline handler\n> > > > > > >\n> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > > > > ---\n> > > > > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > > index da52d7fafcee..ee222d060e4a 100644\n> > > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > > > > > @@ -1234,8 +1234,8 @@ int CameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::Config\n> > > > > > >       }\n> > > > > > >\n> > > > > > >       /* Always send the user transform to the IPA. */\n> > > > > > > -     params.transform =\n> > > > > > > -             static_cast<unsigned int>(transformFromOrientation(config->orientation));\n> > > > > > > +     Transform transform = config->orientation / Orientation::Rotate0;\n> > > > > > > +     params.transform = static_cast<unsigned int>(transform);\n> > > > > >\n> > > > > > I wonder if this was correct in first place.\n> > > > >\n> > > > > Do you mean in \"libcamera: Use CameraConfiguration::orientation\", or\n> > > > > before that ?\n> > > >\n> > > > Even before that, when we had\n> > > >\n> > > >        params.transform = static_cast<unsigned int>(config->transform);\n> > > >\n> > > > > > config->orientation could be adjusted to report the sensor's mounting\n> > > > > > orientation if the user requested orientation cannot be obtained, in\n> > > > > > which case the \"user transform\" will be set to Identity (see\n> > > > > > CameraSensor::computeTransform()).\n> > > > >\n> > > > > Let's assume a sensor mounted with a 180° rotation, and the user asks\n> > > > > for Rotate270 rotation. This can't be achieved, so config->orientation\n> > > > > is adjusted to Rotate180. params.transform will be Rot180 in that case,\n> > > > > not Identiy. Am I missing something ?\n> > > >\n> > > > I'm probably missing what's the use of transform is in the IPA\n> > > >\n> > > > Looking at the alsc.cpp algorithms from RPi (which seems to me to be\n> > > > the only user of the transform field)\n> > > >\n> > > >             if (!!(cameraMode.transform & libcamera::Transform::HFlip)) {\n> > > >                     xLo[i] = X - 1 - xLo[i];\n> > > >                     xHi[i] = X - 1 - xHi[i];\n> > > >             }\n> > > >\n> > > >             ...\n> > > >\n> > > >             if (!!(cameraMode.transform & libcamera::Transform::VFlip)) {\n> > > >                     yLo = Y - 1 - yLo;\n> > > >                     yHi = Y - 1 - yHi;\n> > > >             }\n> > > >\n> > > > I'm not sure I fully got what the code does, but it seems to inspect\n> > > > the actual transform as applied by the PH/CameraSensor, which\n> > > > corresponds to the combinedTransform_ variable, which in your example\n> > > > is Identity, and not the image orientation in the buffers, which in\n> > > > your example is Rot180 ?\n> > >\n> > > As far as I understand, it's only the sensor-side transform that needs\n> > > to be taken into account by that code, not the combined transform from\n> > > the camera sensor and the ISP.\n> > >\n> >\n> > I agree, but in this case you would be passing in Rotate180 (which is\n> > the mounting rotation) instead of the actual transform applied on the\n> > sensor (which, in case of rpi and all the other platforms we have is\n> > the transform applied on the sensor, as the ISP does not do any transform)\n> >\n> > David, Naush, should this be changed on top ?\n> >\n> > > > Anyway, as said this was here already, so it's not strictly related to\n> > > > this patch and was here already\n> > > >\n> > > > > > Wouldn't it be better to send to the IPA 'combinedTransform_' ?\n> > > > > >\n> > > > > > >\n> > > > > > >       /* Ready the IPA - it must know about the sensor resolution. */\n> > > > > > >       ret = ipa_->configure(sensorInfo_, params, result);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C7443BDCBD\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 23 Oct 2023 13:05:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3808D62973;\n\tMon, 23 Oct 2023 15:05:08 +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 2F41061DCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Oct 2023 15:05:06 +0200 (CEST)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 336A7D20;\n\tMon, 23 Oct 2023 15:04:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1698066308;\n\tbh=/okP4xPW6xXZFHIXG/kWVKOBtJEykhavv1kg0MvH1u4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XZbTIKvyC8T3C7wucRTAkSK3TltEIgBlo8MevzIiMp9IHqp+7yBfyw7/2+6T5C9R4\n\tTYva8s+//2w/sIVAYPAiR+GOYjHsugOo/nhlJM5ywqS6NZT049PkTtp5NlF8pI/H6q\n\tYPARz7JFXRuR2NRu79A6wDXSRy2I/GNhBgduVaC4J1H5hyjXpT4biTuMzu/FBzTkQI\n\tR4e5jvknv3DC3sVTe5zqLOmN8hMfPuWFGz0j5epxjy2X7wAw5a9bCm2CbqRIFKxwHa\n\tSVaL4q2CIdEs4oHCJoegS30IAqOcevtpf4bIkCjnQvkqIrSXCIyoyoBS8jcGgr6IIm\n\tQ2j4tX1idh2hA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1698066295;\n\tbh=/okP4xPW6xXZFHIXG/kWVKOBtJEykhavv1kg0MvH1u4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F+SdMgmm3FLMWR4FRc4k6WhL4aHlMMTpNddRTYZQEeNrZbGGgQpoCayjy6/tJy5E1\n\t/IyTjKBqdoLe0uXocyy8xhR5Nxd3+qiPk3SW5xNVD6ZXen8TgHS7/3a1kg3O+8kH1H\n\trwOmDsdgWIYgPSvh86imq5Gwym7WTFJ/GWJvZtB0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"F+SdMgmm\"; dkim-atps=neutral","Date":"Mon, 23 Oct 2023 16:05:13 +0300","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20231023130513.GN3336@pendragon.ideasonboard.com>","References":"<20231019140133.32090-1-jacopo.mondi@ideasonboard.com>\n\t<20231022224159.30298-6-laurent.pinchart@ideasonboard.com>\n\t<boogsrhnydfav7si2ke3ynn5tdma2fv3rcftoasijrr64l62o6@nozrynazbsr2>\n\t<20231023102001.GE3336@pendragon.ideasonboard.com>\n\t<eimlm7fymcios7bod66mnrz7zf7fija6gfj3rovxa7cakbfhai@tq5xrzjypeit>\n\t<20231023111153.GG3336@pendragon.ideasonboard.com>\n\t<6masr5hykwoo6w4h6hums5v46rmvk2cngefmrwa75xgykz2h5n@hdd7t433iqad>\n\t<CAHW6GYKFT7EcvE_RiS11pSWLEJAf0ttCJjB0PxWGKtKOSQLWBQ@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<CAHW6GYKFT7EcvE_RiS11pSWLEJAf0ttCJjB0PxWGKtKOSQLWBQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v6 18/12] fixup! libcamera: Use\n\tCameraConfiguration::orientation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]