[{"id":14956,"web_url":"https://patchwork.libcamera.org/comment/14956/","msgid":"","date":"2021-02-04T09:40:55","subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","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-02-03 17:25:58 +0100, Jacopo Mondi wrote:\n> Report the crop region for every completed request.\n> \n> The crop region is initialized as the sensor's analogue crop\n> rectangle and updated when a Request with a new region completes.\n> \n> Signed-off-by: Jacopo Mondi \n\nFor my understanding. Before this patch the controls::ScalerCrop is \ncurrently only carried as is from the Request controls to the Request \nmetadata. And after this change the rectangle is cached and reported in \nevery Request's metadata while being updated if set in a Requests \ncontrols?\n\nAnd before and after this patch this is just \"informative\" as the IPU3 \npipeline does not yet react and reflects the setting to the hardware?\n\nI think this patch is a good step in the right direction, the question \nabove is just to understand the bigger picture.\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----\n> 1 file changed, 9 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index db0d6b91be70..a9c8e180d1c4 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -65,6 +65,7 @@ public:\n> \tStream rawStream_;\n> \n> \tuint32_t exposureTime_;\n> +\tRectangle cropRegion;\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -472,6 +473,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tCameraSensorInfo sensorInfo;\n> +\tcio2->sensor()->sensorInfo(&sensorInfo);\n> +\tdata->cropRegion = sensorInfo.analogCrop;\n> +\n> \t/*\n> \t * If the ImgU gets configured, its driver seems to expect that\n> \t * buffers will be queued to its outputs, as otherwise the next\n> @@ -967,10 +972,10 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> \t/* \\todo Move the ExposureTime control to the IPA. */\n> \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> -\tif (request->controls().contains(controls::ScalerCrop)) {\n> -\t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n> -\t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> -\t}\n> +\tif (request->controls().contains(controls::ScalerCrop))\n> +\t\tcropRegion = request->controls().get(controls::ScalerCrop);\n> +\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> +\n> \tpipe_->completeRequest(request);\n> }\n> \n> -- \n> 2.30.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","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 8614DBD160\n\tfor ;\n\tThu, 4 Feb 2021 09:40:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B01661406;\n\tThu, 4 Feb 2021 10:40:59 +0100 (CET)","from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com\n\t[IPv6:2a00:1450:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A5B761402\n\tfor ;\n\tThu, 4 Feb 2021 10:40:57 +0100 (CET)","by mail-wr1-x42d.google.com with SMTP id g10so2722526wrx.1\n\tfor ;\n\tThu, 04 Feb 2021 01:40:57 -0800 (PST)","from localhost (p4fca2458.dip0.t-ipconnect.de. [79.202.36.88])\n\tby smtp.gmail.com with ESMTPSA id\n\ti7sm15102502wmq.2.2021.02.04.01.40.56\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 04 Feb 2021 01:40:56 -0800 (PST)"],"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=\"tPW69Rhu\"; 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=pnbsWhk8dPrCdXraXqzvXWJTpzbj9jEpI6vV0Uw9Ysg=;\n\tb=tPW69Rhu0YZcWYyJv80vSJsSnJx7KaiqzPWwfMwq0GoIzPw49/bLQLFjsBgIxW1jgq\n\tLB/G6iZItDKh+9TFEzthR/MHa9XM2UJLbjDlQbYCt9h9+8K9/xkf92OTXk/BO1YeUVxK\n\tuGPmgq48mQHFYAdpmKjMHCmay613/5tPX4cegVGUV8vp+OKGwxWtp0iGd+yzCdFFSL0z\n\ta/Cn5isWeP9MQtn+xBuS8uIR0pYcv6q8yS/HqDaRnGbd38KMQtGUFsS8eQ3P5G/gPtff\n\tIv+I2EJWW4+TL/kwO13vN8rOL4jpTXbaFE8shvCMB79sOXHgZT/E+3yKdH9s0LcibsLm\n\tsj6Q==","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=pnbsWhk8dPrCdXraXqzvXWJTpzbj9jEpI6vV0Uw9Ysg=;\n\tb=quxsdh/QmAT5ij+cY2DqbrRE0GvAE5cXcYA85tBVyTaMtiirqN+F93uwyAuA7B2X1f\n\t1+lb4HWFOXjqLpPpS4ofErp6X9r3n4j1kIl9deX9ichTNsx2v1u4y9tA0gN8xGSjlvdW\n\t/OX7bNpjcqAXKytm03e1rRJ865kHbtVQ6dxxdJ+pLCAa/1LXOgxje7vtZE2KKgS4V+79\n\tsIbZuUeLz4nVA710UkHb7Q48pCjL3c8ynyAWjUzRsR8IdQnSJJsd3FD7g2N0Cv1ajMhk\n\tiwBjnPgC+w6Coh2N0GJvZ+YeV0/ve4KJu+eo9vcPqqjQp1aq47yINwSHppTta7+Z5Rdt\n\tgsmA==","X-Gm-Message-State":"AOAM532YL7KOnIeGwVv1hENZkLM6acT5mSUqImRpAP5XK816wh5iI5h1\n\tZCgH99DD7YIcQTRcrRWrCeTOAung2PACAvP4","X-Google-Smtp-Source":"ABdhPJx0LWlEhmBMiCjVUjU8Qs0y/eGKz1SWTa5DCnKps8xhqN+lRxfHWP1/d2Kjjk5b3tlz1QmCbg==","X-Received":"by 2002:a05:6000:254:: with SMTP id\n\tm20mr8348988wrz.300.1612431657117; \n\tThu, 04 Feb 2021 01:40:57 -0800 (PST)","Date":"Thu, 4 Feb 2021 10:40:55 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210203162600.206297-1-jacopo@jmondi.org>\n\t<20210203162600.206297-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210203162600.206297-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","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":14959,"web_url":"https://patchwork.libcamera.org/comment/14959/","msgid":"<20210204094846.3srvqypjowcb3hur@uno.localdomain>","date":"2021-02-04T09:48:46","subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas\n\nOn Thu, Feb 04, 2021 at 10:40:55AM +0100, Niklas Söderlund wrote:\n> HI Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-02-03 17:25:58 +0100, Jacopo Mondi wrote:\n> > Report the crop region for every completed request.\n> >\n> > The crop region is initialized as the sensor's analogue crop\n> > rectangle and updated when a Request with a new region completes.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> For my understanding. Before this patch the controls::ScalerCrop is\n> currently only carried as is from the Request controls to the Request\n> metadata. And after this change the rectangle is cached and reported in\n> every Request's metadata while being updated if set in a Requests\n> controls?\n\nCorrect\n\n>\n> And before and after this patch this is just \"informative\" as the IPU3\n> pipeline does not yet react and reflects the setting to the hardware?\n\nAnd correct too\n\n>\n> I think this patch is a good step in the right direction, the question\n> above is just to understand the bigger picture.\n\nI mainly added this as android requires the scaler crop region\nreported for every completed request, but I think it the right think\nto do for libcamera as well, but... we need a policy in my opinion.\n\nThese days I think that having a clear distinction between controls\nand metadata in our controls definition would have helped in\ndocumenting things like \"this metadata has to be repotred for each\ncompleted requests\" and such\n\n>\n> Reviewed-by: Niklas Söderlund \n\nThanks\n j\n>\n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----\n> > 1 file changed, 9 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index db0d6b91be70..a9c8e180d1c4 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -65,6 +65,7 @@ public:\n> > \tStream rawStream_;\n> >\n> > \tuint32_t exposureTime_;\n> > +\tRectangle cropRegion;\n> > };\n> >\n> > class IPU3CameraConfiguration : public CameraConfiguration\n> > @@ -472,6 +473,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > +\tCameraSensorInfo sensorInfo;\n> > +\tcio2->sensor()->sensorInfo(&sensorInfo);\n> > +\tdata->cropRegion = sensorInfo.analogCrop;\n> > +\n> > \t/*\n> > \t * If the ImgU gets configured, its driver seems to expect that\n> > \t * buffers will be queued to its outputs, as otherwise the next\n> > @@ -967,10 +972,10 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > \t/* \\todo Move the ExposureTime control to the IPA. */\n> > \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> > \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> > -\tif (request->controls().contains(controls::ScalerCrop)) {\n> > -\t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n> > -\t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > -\t}\n> > +\tif (request->controls().contains(controls::ScalerCrop))\n> > +\t\tcropRegion = request->controls().get(controls::ScalerCrop);\n> > +\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > +\n> > \tpipe_->completeRequest(request);\n> > }\n> >\n> > --\n> > 2.30.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 7C0BDBD160\n\tfor ;\n\tThu, 4 Feb 2021 09:48:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDF9E6142A;\n\tThu, 4 Feb 2021 10:48:25 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF8EB61402\n\tfor ;\n\tThu, 4 Feb 2021 10:48:24 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 67A2C1BF208;\n\tThu, 4 Feb 2021 09:48:23 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 4 Feb 2021 10:48:46 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210204094846.3srvqypjowcb3hur@uno.localdomain>","References":"<20210203162600.206297-1-jacopo@jmondi.org>\n\t<20210203162600.206297-6-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15010,"web_url":"https://patchwork.libcamera.org/comment/15010/","msgid":"","date":"2021-02-04T22:19:46","subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Feb 04, 2021 at 10:48:46AM +0100, Jacopo Mondi wrote:\n> On Thu, Feb 04, 2021 at 10:40:55AM +0100, Niklas Söderlund wrote:\n> > On 2021-02-03 17:25:58 +0100, Jacopo Mondi wrote:\n> > > Report the crop region for every completed request.\n> > >\n> > > The crop region is initialized as the sensor's analogue crop\n> > > rectangle and updated when a Request with a new region completes.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> >\n> > For my understanding. Before this patch the controls::ScalerCrop is\n> > currently only carried as is from the Request controls to the Request\n> > metadata. And after this change the rectangle is cached and reported in\n> > every Request's metadata while being updated if set in a Requests\n> > controls?\n> \n> Correct\n> \n> > And before and after this patch this is just \"informative\" as the IPU3\n> > pipeline does not yet react and reflects the setting to the hardware?\n> \n> And correct too\n> \n> > I think this patch is a good step in the right direction, the question\n> > above is just to understand the bigger picture.\n> \n> I mainly added this as android requires the scaler crop region\n> reported for every completed request, but I think it the right think\n> to do for libcamera as well, but... we need a policy in my opinion.\n\nI agree with you, I think this is the right behaviour, and it definitely\nshould be documented. Maybe a quick patch to add a \\todo comment could\nbe added to this series ?\n\n> These days I think that having a clear distinction between controls\n> and metadata in our controls definition would have helped in\n> documenting things like \"this metadata has to be repotred for each\n> completed requests\" and such\n\nI don't like much how Android duplicates the definitions in the\ndocumentation they generate, but I agree with you we could do better.\nMaybe we could add request and metadata elements in the YAML file to\ndocument the behaviour specific to how a control is used in a request\nand in metadata ?\n\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----\n> > > 1 file changed, 9 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index db0d6b91be70..a9c8e180d1c4 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -65,6 +65,7 @@ public:\n> > > \tStream rawStream_;\n> > >\n> > > \tuint32_t exposureTime_;\n> > > +\tRectangle cropRegion;\n\ns/cropRegion/cropRegion_/\n\nReviewed-by: Laurent Pinchart \n\n> > > };\n> > >\n> > > class IPU3CameraConfiguration : public CameraConfiguration\n> > > @@ -472,6 +473,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > >\n> > > +\tCameraSensorInfo sensorInfo;\n> > > +\tcio2->sensor()->sensorInfo(&sensorInfo);\n> > > +\tdata->cropRegion = sensorInfo.analogCrop;\n> > > +\n> > > \t/*\n> > > \t * If the ImgU gets configured, its driver seems to expect that\n> > > \t * buffers will be queued to its outputs, as otherwise the next\n> > > @@ -967,10 +972,10 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > > \t/* \\todo Move the ExposureTime control to the IPA. */\n> > > \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> > > \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> > > -\tif (request->controls().contains(controls::ScalerCrop)) {\n> > > -\t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n> > > -\t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > > -\t}\n> > > +\tif (request->controls().contains(controls::ScalerCrop))\n> > > +\t\tcropRegion = request->controls().get(controls::ScalerCrop);\n> > > +\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > > +\n> > > \tpipe_->completeRequest(request);\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 DE4F7BD162\n\tfor ;\n\tThu, 4 Feb 2021 22:20:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A74AC6148A;\n\tThu, 4 Feb 2021 23:20:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D61161486\n\tfor ;\n\tThu, 4 Feb 2021 23:20:09 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1949D45D;\n\tThu, 4 Feb 2021 23:20:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZPKvFbpv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612477209;\n\tbh=RkWeizqeNO3JR8CYImrpnib5P8Q84/ElbmCt08xLwUA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZPKvFbpvMW9fmeTYOcJVqhDdO/R4IypLhd58l1Ugv22AMa52KPmUlOyD/B1QvHlzM\n\tsQMNoe5vxMxnrA+tj4QDyz54ScSmfEXz3Sdzl9Q35lsXwI5rNhPumjQOgWi0Kc+aIW\n\tZ8mVnZso+h/oLHyf+dbfHfKTQxAU0I/vDXXoMvac=","Date":"Fri, 5 Feb 2021 00:19:46 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210203162600.206297-1-jacopo@jmondi.org>\n\t<20210203162600.206297-6-jacopo@jmondi.org>\n\t\n\t<20210204094846.3srvqypjowcb3hur@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210204094846.3srvqypjowcb3hur@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15026,"web_url":"https://patchwork.libcamera.org/comment/15026/","msgid":"<20210205122153.5hyyxxl5ma3tlyfz@uno.localdomain>","date":"2021-02-05T12:21:53","subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n\nOn Fri, Feb 05, 2021 at 12:19:46AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Feb 04, 2021 at 10:48:46AM +0100, Jacopo Mondi wrote:\n> > On Thu, Feb 04, 2021 at 10:40:55AM +0100, Niklas Söderlund wrote:\n> > > On 2021-02-03 17:25:58 +0100, Jacopo Mondi wrote:\n> > > > Report the crop region for every completed request.\n> > > >\n> > > > The crop region is initialized as the sensor's analogue crop\n> > > > rectangle and updated when a Request with a new region completes.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > For my understanding. Before this patch the controls::ScalerCrop is\n> > > currently only carried as is from the Request controls to the Request\n> > > metadata. And after this change the rectangle is cached and reported in\n> > > every Request's metadata while being updated if set in a Requests\n> > > controls?\n> >\n> > Correct\n> >\n> > > And before and after this patch this is just \"informative\" as the IPU3\n> > > pipeline does not yet react and reflects the setting to the hardware?\n> >\n> > And correct too\n> >\n> > > I think this patch is a good step in the right direction, the question\n> > > above is just to understand the bigger picture.\n> >\n> > I mainly added this as android requires the scaler crop region\n> > reported for every completed request, but I think it the right think\n> > to do for libcamera as well, but... we need a policy in my opinion.\n>\n> I agree with you, I think this is the right behaviour, and it definitely\n> should be documented. Maybe a quick patch to add a \\todo comment could\n> be added to this series ?\n>\n> > These days I think that having a clear distinction between controls\n> > and metadata in our controls definition would have helped in\n> > documenting things like \"this metadata has to be repotred for each\n> > completed requests\" and such\n>\n> I don't like much how Android duplicates the definitions in the\n> documentation they generate, but I agree with you we could do better.\n> Maybe we could add request and metadata elements in the YAML file to\n> document the behaviour specific to how a control is used in a request\n> and in metadata ?\n\nI was thinking the same.. we could have a section in the controls\ndocumentation to specify specificities of the behaviour when the\ncontrol is used in a request or in metadata.\n\nI would not also mind a \"Pipeline implementation details\" section,\nhave you seen anywhere something similar ? :D\n\nIn the meantime, this series is fully reviewed, so I'll merge as it is\nif that's ok.\n\n>\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----\n> > > > 1 file changed, 9 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index db0d6b91be70..a9c8e180d1c4 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -65,6 +65,7 @@ public:\n> > > > \tStream rawStream_;\n> > > >\n> > > > \tuint32_t exposureTime_;\n> > > > +\tRectangle cropRegion;\n>\n> s/cropRegion/cropRegion_/\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > > > };\n> > > >\n> > > > class IPU3CameraConfiguration : public CameraConfiguration\n> > > > @@ -472,6 +473,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > >\n> > > > +\tCameraSensorInfo sensorInfo;\n> > > > +\tcio2->sensor()->sensorInfo(&sensorInfo);\n> > > > +\tdata->cropRegion = sensorInfo.analogCrop;\n> > > > +\n> > > > \t/*\n> > > > \t * If the ImgU gets configured, its driver seems to expect that\n> > > > \t * buffers will be queued to its outputs, as otherwise the next\n> > > > @@ -967,10 +972,10 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > > > \t/* \\todo Move the ExposureTime control to the IPA. */\n> > > > \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> > > > \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> > > > -\tif (request->controls().contains(controls::ScalerCrop)) {\n> > > > -\t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n> > > > -\t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > > > -\t}\n> > > > +\tif (request->controls().contains(controls::ScalerCrop))\n> > > > +\t\tcropRegion = request->controls().get(controls::ScalerCrop);\n> > > > +\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > > > +\n> > > > \tpipe_->completeRequest(request);\n> > > > }\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8CB5ABD162\n\tfor ;\n\tFri, 5 Feb 2021 12:21:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 077AF614B0;\n\tFri, 5 Feb 2021 13:21:34 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2684460306\n\tfor ;\n\tFri, 5 Feb 2021 13:21:32 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 4191010000C;\n\tFri, 5 Feb 2021 12:21:30 +0000 (UTC)"],"Date":"Fri, 5 Feb 2021 13:21:53 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210205122153.5hyyxxl5ma3tlyfz@uno.localdomain>","References":"<20210203162600.206297-1-jacopo@jmondi.org>\n\t<20210203162600.206297-6-jacopo@jmondi.org>\n\t\n\t<20210204094846.3srvqypjowcb3hur@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report\n\tcrop region","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]