[{"id":15072,"web_url":"https://patchwork.libcamera.org/comment/15072/","msgid":"","date":"2021-02-09T01:22:15","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: ipu3: Register\n\tFrameDurations control","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 Tue, Jan 26, 2021 at 06:30:04PM +0100, Jacopo Mondi wrote:\n> Register the FrameDurations control in the IPU3 pipeline handler\n> computed using the vertical blanking limits and the current\n> configuration pixel rate as parameters.\n> \n> The FrameDurations control limits should be updated everytime a new\n> configuration is applied to the sensor.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++++++++++++++++-\n> 1 file changed, 34 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index db0d6b91be70..fe5694f9893a 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -754,6 +754,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t\treturn ret;\n> \n> \tControlInfoMap::Map controls = IPU3Controls;\n> +\tconst ControlInfoMap &sensorControls = sensor->controls();\n> \n> \t/*\n> \t * Compute exposure time limits.\n> @@ -766,7 +767,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \t */\n> \tdouble lineDuration = sensorInfo.lineLength\n> \t\t\t / (sensorInfo.pixelRate / 1e6);\n> -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> \tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> \tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> \tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> @@ -781,6 +781,39 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> \tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> \t\t\t\t\t\t\tdefExposure);\n> \n> +\t/*\n> +\t * Compute the frame duration limits.\n> +\t *\n> +\t * \\todo The frame duration limits depend on the sensor configuration.\n> +\t * Initialize the control using the frame sizes and pixel rate of the\n> +\t * current configuration.\n\nThis is the perfect way to make sure it will break in ways that are\nhorrible to debug, as the value we report will depend on the\nconfiguration applied to the subdev the previous time the camera was\nused. Could we use a fixed size instead, maybe sensor->resolution() ?\nWe'll still need to make the limits dynamic (and figure out how to\nhandle this on the Android side), so a todo comment will still be\nneeded, but at least the behaviour will be less random.\n\n> +\t *\n> +\t * The frame length is computed assuming a fixed line length combined\n> +\t * with the vertical frame sizes.\n> +\t */\n> +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> +\tuint32_t hblank = v4l2HBlank.def().get();\n> +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> +\n> +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> +\tstd::array frameHeights{\n> +\t\t(v4l2VBlank.min().get() + sensorInfo.outputSize.height),\n> +\t\t(v4l2VBlank.max().get() + sensorInfo.outputSize.height),\n> +\t\t(v4l2VBlank.def().get() + sensorInfo.outputSize.height),\n\nYou can drop the outer parentheses.\n\n> +\t};\n> +\n> +\tstd::vector frameDurations;\n> +\tframeDurations.reserve(3);\n> +\tstd::transform(frameHeights.begin(), frameHeights.end(),\n> +\t\t std::back_inserter(frameDurations),\n> +\t\t [&](int32_t frameHeight) {\n> +\t\t\t int64_t frameSize = lineLength * frameHeight;\n\nThis can be unsigned.\n\n> +\t\t\t return frameSize / (sensorInfo.pixelRate / 1000000U);\n> +\t\t });\n\nMaybe the following would be more readable ?\n\n\tstd::array frameDurations;\n\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n\t\tuint64_t frameSize = lineLength * frameHeights[i];\n\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U)\n\t}\n\n> +\tcontrols[&controls::FrameDurations] = ControlInfo(frameDurations[0],\n> +\t\t\t\t\t\t\t frameDurations[1],\n> +\t\t\t\t\t\t\t frameDurations[2]);\n> +\n> \t/*\n> \t * Compute the scaler crop limits.\n> \t *","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 1D392BD162\n\tfor ;\n\tTue, 9 Feb 2021 01:22:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 899BE6160B;\n\tTue, 9 Feb 2021 02:22:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA74E613F3\n\tfor ;\n\tTue, 9 Feb 2021 02:22:40 +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 0D965583;\n\tTue, 9 Feb 2021 02:22:39 +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=\"gcPGoEQ+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612833760;\n\tbh=rJfByvXLlURf1XLnd2PyqHeUadusIXbAuZm2STVWFsM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gcPGoEQ+oS7q6EklW9vKzVQAyqyCGlFu/fkmYWf70RlBorDGHP/4mGHZlMOZLlSae\n\tHFGalNaml23F96kAirU6wlcI5VQGBE/nprFTtOD8KCTdVVSDUYtoPKrebnFam46K9h\n\t3b+ZmBm75eMjsjRT0Oe1yRC8Llu5r8Ytt6LUYT+k=","Date":"Tue, 9 Feb 2021 03:22:15 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210126173008.446321-1-jacopo@jmondi.org>\n\t<20210126173008.446321-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126173008.446321-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: ipu3: Register\n\tFrameDurations control","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15228,"web_url":"https://patchwork.libcamera.org/comment/15228/","msgid":"<20210218170515.w5yck5nahekrl2wm@uno.localdomain>","date":"2021-02-18T17:05:15","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: ipu3: Register\n\tFrameDurations control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Feb 09, 2021 at 03:22:15AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 26, 2021 at 06:30:04PM +0100, Jacopo Mondi wrote:\n> > Register the FrameDurations control in the IPU3 pipeline handler\n> > computed using the vertical blanking limits and the current\n> > configuration pixel rate as parameters.\n> >\n> > The FrameDurations control limits should be updated everytime a new\n> > configuration is applied to the sensor.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++++++++++++++++-\n> > 1 file changed, 34 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index db0d6b91be70..fe5694f9893a 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -754,6 +754,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t\treturn ret;\n> >\n> > \tControlInfoMap::Map controls = IPU3Controls;\n> > +\tconst ControlInfoMap &sensorControls = sensor->controls();\n> >\n> > \t/*\n> > \t * Compute exposure time limits.\n> > @@ -766,7 +767,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \t */\n> > \tdouble lineDuration = sensorInfo.lineLength\n> > \t\t\t / (sensorInfo.pixelRate / 1e6);\n> > -\tconst ControlInfoMap &sensorControls = sensor->controls();\n> > \tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > \tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > \tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > @@ -781,6 +781,39 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > \tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > \t\t\t\t\t\t\tdefExposure);\n> >\n> > +\t/*\n> > +\t * Compute the frame duration limits.\n> > +\t *\n> > +\t * \\todo The frame duration limits depend on the sensor configuration.\n> > +\t * Initialize the control using the frame sizes and pixel rate of the\n> > +\t * current configuration.\n>\n> This is the perfect way to make sure it will break in ways that are\n> horrible to debug, as the value we report will depend on the\n> configuration applied to the subdev the previous time the camera was\n> used. Could we use a fixed size instead, maybe sensor->resolution() ?\n> We'll still need to make the limits dynamic (and figure out how to\n> handle this on the Android side), so a todo comment will still be\n> needed, but at least the behaviour will be less random.\n>\n\nPlease be aware that the exposure time is calculated in the same way.\nAs controls are initialized at library startup time it is not possible\nfor libcamera to change the sensor configuration before getting here.\n\nBut one could indeed mess with the sensor configuration manually then\nrun libcamera, and this can effectively cause confusion.\n\nI think I will have to change this in the whole function, and\nestablish a criteria that applies to all controls. I'll go for sake of\nsimplicity with the sensor resolution.\n\n> > +\t *\n> > +\t * The frame length is computed assuming a fixed line length combined\n> > +\t * with the vertical frame sizes.\n> > +\t */\n> > +\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> > +\tuint32_t hblank = v4l2HBlank.def().get();\n> > +\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> > +\n> > +\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> > +\tstd::array frameHeights{\n> > +\t\t(v4l2VBlank.min().get() + sensorInfo.outputSize.height),\n> > +\t\t(v4l2VBlank.max().get() + sensorInfo.outputSize.height),\n> > +\t\t(v4l2VBlank.def().get() + sensorInfo.outputSize.height),\n>\n> You can drop the outer parentheses.\n>\n> > +\t};\n> > +\n> > +\tstd::vector frameDurations;\n> > +\tframeDurations.reserve(3);\n> > +\tstd::transform(frameHeights.begin(), frameHeights.end(),\n> > +\t\t std::back_inserter(frameDurations),\n> > +\t\t [&](int32_t frameHeight) {\n> > +\t\t\t int64_t frameSize = lineLength * frameHeight;\n>\n> This can be unsigned.\n>\n> > +\t\t\t return frameSize / (sensorInfo.pixelRate / 1000000U);\n> > +\t\t });\n>\n> Maybe the following would be more readable ?\n>\n> \tstd::array frameDurations;\n> \tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> \t\tuint64_t frameSize = lineLength * frameHeights[i];\n> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U)\n\nPossibly, I'll change to the more C version you proposed\n\nThanks\n j\n\n> \t}\n>\n> > +\tcontrols[&controls::FrameDurations] = ControlInfo(frameDurations[0],\n> > +\t\t\t\t\t\t\t frameDurations[1],\n> > +\t\t\t\t\t\t\t frameDurations[2]);\n> > +\n> > \t/*\n> > \t * Compute the scaler crop limits.\n> > \t *\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 6FC7BBD1F6\n\tfor ;\n\tThu, 18 Feb 2021 17:04:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB377689B7;\n\tThu, 18 Feb 2021 18:04:51 +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 C8BF1637DA\n\tfor ;\n\tThu, 18 Feb 2021 18:04:50 +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 4DD771BF209;\n\tThu, 18 Feb 2021 17:04:50 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 18 Feb 2021 18:05:15 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210218170515.w5yck5nahekrl2wm@uno.localdomain>","References":"<20210126173008.446321-1-jacopo@jmondi.org>\n\t<20210126173008.446321-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: ipu3: Register\n\tFrameDurations control","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]