[{"id":27891,"web_url":"https://patchwork.libcamera.org/comment/27891/","msgid":"<20230926215951.GH5854@pendragon.ideasonboard.com>","date":"2023-09-26T21:59:51","subject":"Re: [libcamera-devel] [PATCH v5 03/13] libcamera: camera_sensor:\n\tSupport SensorConfiguration","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, Sep 21, 2023 at 06:55:40PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Add a class function to the CameraSensor class to apply a full\n> configuration to the sensor.\n> \n> The configuration shall be fully populated and shall apply without\n> modifications to the sensor.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Naushir Patuck \n> Reviewed-by: Kieran Bingham \n> ---\n> include/libcamera/internal/camera_sensor.h | 5 ++\n> src/libcamera/camera_sensor.cpp | 86 ++++++++++++++++++++++\n> 2 files changed, 91 insertions(+)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 02c77ab037da..06791c3c6425 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -29,6 +29,7 @@ namespace libcamera {\n> class BayerFormat;\n> class CameraLens;\n> class MediaEntity;\n> +class SensorConfiguration;\n> \n> struct CameraSensorProperties;\n> \n> @@ -58,6 +59,10 @@ public:\n> \t\t Transform transform = Transform::Identity);\n> \tint tryFormat(V4L2SubdeviceFormat *format) const;\n> \n> +\tint applyConfiguration(const SensorConfiguration &config,\n> +\t\t\t Transform transform = Transform::Identity,\n> +\t\t\t V4L2SubdeviceFormat *sensorFormat = nullptr);\n> +\n> \tconst ControlInfoMap &controls() const;\n> \tControlList getControls(const std::vector &ids);\n> \tint setControls(ControlList *ctrls);\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f3a5aa37149f..323081d25ce7 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -822,6 +823,91 @@ int CameraSensor::tryFormat(V4L2SubdeviceFormat *format) const\n> \t\t\t\t V4L2Subdevice::Whence::TryFormat);\n> }\n> \n> +/**\n> + * \\brief Apply a sensor configuration to the camera sensor\n> + * \\param[in] config The sensor configuration\n> + * \\param[in] transform The transform to be applied on the sensor.\n> + * Defaults to Identity\n> + * \\param[out] sensorFormat Optional output parameter where the format\n> + * actually applied to the sensor is returned to the caller\n\n * \\param[out] sensorFormat Format applied to the sensor (optional)\n\nSimple is more readable :-)\n\n> + *\n> + * Apply to the camera sensor the configuration \\a config.\n> + *\n> + * \\todo The configuration shall be fully populated and if any of the fields\n> + * specified cannot be applied exactly, an error code is returned.\n\nI will address this right away on top.\n\n> + *\n> + * \\return 0 if \\a config unpopulated or is applied correctly to the camera\n\nThe unpopulated case isn't handled by this function anymore, so\ns/unpopulated or//\n\n> + * sensor, a negative error code otherwise\n> + */\n> +int CameraSensor::applyConfiguration(const SensorConfiguration &config,\n> +\t\t\t\t Transform transform,\n> +\t\t\t\t V4L2SubdeviceFormat *sensorFormat)\n\nThe API seems a bit of a hack, and it's underdocumented as there's no\nexplanation about how the caller should pick applyConfiguration() or\nsetFormat(), or if it should call setFormat() in any case. I will likely\nrework this on top.\n\nWith the above changes applied,\n\nReviewed-by: Laurent Pinchart \n\n> +{\n> +\tif (!config.valid()) {\n> +\t\tLOG(CameraSensor, Error) << \"Invalid sensor configuration\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::vector filteredCodes;\n> +\tstd::copy_if(mbusCodes_.begin(), mbusCodes_.end(),\n> +\t\t std::back_inserter(filteredCodes),\n> +\t\t [&config](unsigned int mbusCode) {\n> +\t\t\t BayerFormat bayer = BayerFormat::fromMbusCode(mbusCode);\n> +\t\t\t if (bayer.bitDepth == config.bitDepth)\n> +\t\t\t\t return true;\n> +\t\t\t return false;\n> +\t\t });\n> +\tif (filteredCodes.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Cannot find any format with bit depth \"\n> +\t\t\t<< config.bitDepth;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/*\n> +\t * Compute the sensor's data frame size by applying the cropping\n> +\t * rectangle, subsampling and output crop to the sensor's pixel array\n> +\t * size.\n> +\t *\n> +\t * \\todo The actual size computation is for now ignored and only the\n> +\t * output size is considered. This implies that resolutions obtained\n> +\t * with two different cropping/subsampling will look identical and\n> +\t * only the first found one will be considered.\n> +\t */\n> +\tV4L2SubdeviceFormat subdevFormat = {};\n> +\tfor (unsigned int code : filteredCodes) {\n> +\t\tfor (const Size &size : sizes(code)) {\n> +\t\t\tif (size.width != config.outputSize.width ||\n> +\t\t\t size.height != config.outputSize.height)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tsubdevFormat.mbus_code = code;\n> +\t\t\tsubdevFormat.size = size;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tif (!subdevFormat.mbus_code) {\n> +\t\tLOG(CameraSensor, Error) << \"Invalid output size in sensor configuration\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tint ret = setFormat(&subdevFormat, transform);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Return to the caller the format actually applied to the sensor.\n> +\t * This is relevant if transform has changed the bayer pattern order.\n> +\t */\n> +\tif (sensorFormat)\n> +\t\t*sensorFormat = subdevFormat;\n> +\n> +\t/* \\todo Handle AnalogCrop. Most sensors do not support set_selection */\n> +\t/* \\todo Handle scaling in the digital domain. */\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Retrieve the supported V4L2 controls and their information\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 9A265BD808\n\tfor ;\n\tTue, 26 Sep 2023 21:59:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01A4A62944;\n\tTue, 26 Sep 2023 23:59:43 +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 0AC2E61E0E\n\tfor ;\n\tTue, 26 Sep 2023 23:59:42 +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 B19118C1;\n\tTue, 26 Sep 2023 23:58:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695765583;\n\tbh=z6iLldKl/qUvqkAabxdJaCsdoAbbdwcf+9cSmyapRM0=;\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=C0W9xd2zhHacBQawiQ1XF3HKOHOR7japdr1vNnbSb1KuCdZm7XHImS7F/v0eGY1vI\n\tV7/sZdHyN18Zz+CAoAUToidrxtBH9zGBf7j60wdxPiaBKIlOmlNSrCommAKIudYayp\n\tdGXWvWU+NkJbqN0yZ/VMXL3mi1/DD054TUOfr0k/FVRY0y3mVBXZa80TX+706Y5d7a\n\tJn3RG7bA2d5Re5aJeeh9couMOH/vjjsRhtprdyeK90qZAhE+I0QSbIyWR27etP/Yd1\n\tFro38fEBBcrVXa9ML9dU2evJ/IZmbcVkqyF7RWOC4zemOAmxtFW6cVfsphuuwlz4ch\n\tjB/kosw/3xeUw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695765480;\n\tbh=z6iLldKl/qUvqkAabxdJaCsdoAbbdwcf+9cSmyapRM0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uGNyM130D5x2nOywSecnlrSFw851A4QLgCrAIU/WUe+Z7odqycHx0+dfEHfM69g4G\n\tBFUsuzjbI2a6/Khl/4G55mckUX2KIMN1IKRjS7BTNkYoziYr9rASFTR79H+DbB+a+v\n\tSCu0udpBSThhiEuZZcJADC1iGZaOSdtAJMdxRVmA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uGNyM130\"; dkim-atps=neutral","Date":"Wed, 27 Sep 2023 00:59:51 +0300","To":"Jacopo Mondi ","Message-ID":"<20230926215951.GH5854@pendragon.ideasonboard.com>","References":"<20230921165550.50956-1-jacopo.mondi@ideasonboard.com>\n\t<20230921165550.50956-4-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230921165550.50956-4-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 03/13] libcamera: camera_sensor:\n\tSupport SensorConfiguration","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]