[{"id":27892,"web_url":"https://patchwork.libcamera.org/comment/27892/","msgid":"<20230926222745.GJ5854@pendragon.ideasonboard.com>","date":"2023-09-26T22:27:45","subject":"Re: [libcamera-devel] [PATCH v5 04/13] libcamera: rpi: Handle\n\tSensorConfiguration","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:41PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Handle the SensorConfiguration provided by the application in the\n> pipeline validate() and configure() call chains.\n> \n> During validation, first make sure SensorConfiguration is valid, then\n> handle it to compute the sensor format.\n> \n> For the VC4 platform where the RAW stream follows the sensor's\n> configuration adjust the RAW stream configuration to match the sensor\n> configuration.\n> \n> Reviewed-by: Naushir Patuck \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Jacopo Mondi \n> Signed-off-by: Jacopo Mondi \n> ---\n> .../pipeline/rpi/common/pipeline_base.cpp | 66 ++++++++++++++++---\n> .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 28 +++++++-\n> 3 files changed, 84 insertions(+), 14 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 51fa1bbf9aa9..c02ceb65cae8 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -180,6 +180,15 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\t/*\n> +\t * Make sure that if a sensor configuration has been requested it\n> +\t * is valid.\n> +\t */\n> +\tif (sensorConfig && !sensorConfig->valid()) {\n> +\t\tLOG(RPI, Error) << \"Invalid sensor configuration request\";\n> +\t\treturn Invalid;\n> +\t}\n> +\n> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> \n> \t/*\n> @@ -207,19 +216,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \tstd::sort(outStreams.begin(), outStreams.end(),\n> \t\t [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> \n> -\t/* Compute the sensor configuration. */\n> -\tunsigned int bitDepth = defaultRawBitDepth;\n> -\tif (!rawStreams.empty()) {\n> +\t/* Compute the sensor's format then do any platform specific fixups. */\n> +\tunsigned int bitDepth;\n> +\tSize sensorSize;\n> +\n> +\tif (sensorConfig) {\n> +\t\t/* Use the application provided sensor configuration. */\n> +\t\tbitDepth = sensorConfig->bitDepth;\n> +\t\tsensorSize = sensorConfig->outputSize;\n> +\t} else if (!rawStreams.empty()) {\n> +\t\t/* Use the RAW stream format and size. */\n> \t\tBayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> \t\tbitDepth = bayerFormat.bitDepth;\n> +\t\tsensorSize = rawStreams[0].cfg->size;\n> +\t} else {\n> +\t\tbitDepth = defaultRawBitDepth;\n> +\t\tsensorSize = outStreams[0].cfg->size;\n> \t}\n> \n> -\tsensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> -\t\t\t\t\t\t\t\t : rawStreams[0].cfg->size,\n> -\t\t\t\t\t bitDepth);\n> +\tsensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);\n> +\n> +\t/*\n> +\t * If a sensor configuration has been requested, it should apply\n> +\t * without modifications.\n> +\t */\n> +\tif (sensorConfig) {\n> +\t\tBayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);\n> +\n> +\t\tif (bayer.bitDepth != sensorConfig->bitDepth ||\n> +\t\t sensorFormat_.size != sensorConfig->outputSize) {\n> +\t\t\tLOG(RPI, Error) << \"Invalid sensor configuration: \"\n> +\t\t\t\t\t<< \"bitDepth/size mismatch\";\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\t}\n> \n> \t/* Do any platform specific fixups. */\n> -\tstatus = data_->platformValidate(rawStreams, outStreams);\n> +\tstatus = data_->platformValidate(this, rawStreams, outStreams);\n> \tif (status == Invalid)\n> \t\treturn Invalid;\n> \n> @@ -467,12 +500,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> \tstd::sort(ispStreams.begin(), ispStreams.end(),\n> \t\t [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> \n> -\t/* Apply the format on the sensor with any cached transform. */\n> +\t/*\n> +\t * Apply the format on the sensor with any cached transform.\n> +\t *\n> +\t * If the application has provided a sensor configuration apply it\n> +\t * instead of just applying a format.\n> +\t */\n> \tconst RPiCameraConfiguration *rpiConfig =\n> \t\t\t\tstatic_cast(config);\n> -\tV4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;\n> +\tV4L2SubdeviceFormat sensorFormat;\n> \n> -\tret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> +\tif (rpiConfig->sensorConfig) {\n> +\t\tret = data->sensor_->applyConfiguration(*rpiConfig->sensorConfig,\n> +\t\t\t\t\t\t\trpiConfig->combinedTransform_,\n> +\t\t\t\t\t\t\t&sensorFormat);\n> +\t} else {\n> +\t\tsensorFormat = rpiConfig->sensorFormat_;\n> +\t\tret = data->sensor_->setFormat(&sensorFormat,\n> +\t\t\t\t\t rpiConfig->combinedTransform_);\n> +\t}\n> \tif (ret)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> index dbabc61ea48c..81b2b7d2f4d1 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> @@ -42,6 +42,7 @@ namespace RPi {\n> /* Map of mbus codes to supported sizes reported by the sensor. */\n> using SensorFormats = std::map>;\n> \n> +class RPiCameraConfiguration;\n> class CameraData : public Camera::Private\n> {\n> public:\n> @@ -72,7 +73,8 @@ public:\n> \t\tV4L2VideoDevice *dev;\n> \t};\n> \n> -\tvirtual CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> +\tvirtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,\n> +\t\t\t\t\t\t\t std::vector &rawStreams,\n> \t\t\t\t\t\t\t std::vector &outStreams) const = 0;\n> \tvirtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,\n> \t\t\t\t std::optional packing,\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index 018cf4881d0e..2670eb8c4bbc 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -65,7 +65,8 @@ public:\n> \t{\n> \t}\n> \n> -\tCameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> +\tCameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> +\t\t\t\t\t\t std::vector &rawStreams,\n> \t\t\t\t\t\t std::vector &outStreams) const override;\n> \n> \tint platformPipelineConfigure(const std::unique_ptr &root) override;\n> @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> \treturn 0;\n> }\n> \n> -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector &rawStreams,\n> +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> +\t\t\t\t\t\t\t std::vector &rawStreams,\n> \t\t\t\t\t\t\t std::vector &outStreams) const\n> {\n> \tCameraConfiguration::Status status = CameraConfiguration::Status::Valid;\n> @@ -405,9 +407,27 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector \t\treturn CameraConfiguration::Status::Invalid;\n> \t}\n> \n> -\tif (!rawStreams.empty())\n> +\tif (!rawStreams.empty()) {\n> \t\trawStreams[0].dev = unicam_[Unicam::Image].dev();\n> \n> +\t\t/* Adjust the RAW stream to match the computed sensor format. */\n> +\t\tStreamConfiguration *rawStream = rawStreams[0].cfg;\n> +\t\tBayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> +\n> +\t\t/* Handle flips to make sure to match the RAW stream format. */\n> +\t\tif (flipsAlterBayerOrder_)\n> +\t\t\trawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> +\t\tPixelFormat rawFormat = rawBayer.toPixelFormat();\n> +\n> +\t\tif (rawStream->pixelFormat != rawFormat ||\n> +\t\t rawStream->size != rpiConfig->sensorFormat_.size) {\n> +\t\t\trawStream->pixelFormat = rawFormat;\n> +\t\t\trawStream->size = rpiConfig->sensorFormat_.size;\n> +\n> +\t\t\tstatus = CameraConfiguration::Adjusted;\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * For the two ISP outputs, one stream must be equal or smaller than the\n> \t * other in all dimensions.\n> @@ -417,6 +437,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector \tfor (unsigned int i = 0; i < outStreams.size(); i++) {\n> \t\tSize size;\n> \n> +\t\t/* \\todo Warn if upscaling: reduces image quality. */\n> +\n\nWhy is this a todo item ?\n\n> \t\tsize.width = std::min(outStreams[i].cfg->size.width,\n> \t\t\t\t outStreams[0].cfg->size.width);\n> \t\tsize.height = std::min(outStreams[i].cfg->size.height,","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 8CD46C326B\n\tfor ;\n\tTue, 26 Sep 2023 22:27:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A67F562944;\n\tWed, 27 Sep 2023 00:27:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FB81628FA\n\tfor ;\n\tWed, 27 Sep 2023 00:27:36 +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 AA8EE13C5;\n\tWed, 27 Sep 2023 00:25:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695767257;\n\tbh=8NaBWPzCEOL80KByXrGuOpDEYhjEMVn8eBLlM2g/q7s=;\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=i77zhWP9RDCHvHnZSQ2ivlxb38enGRH7T2PpkUXsmnaXFzZPdlcvVY6bfKdRPLsxk\n\t4kIQqRGxqk1ZoIfvujhT3xfgqIieMvQzBhswa4D0v2OZHwgMXoOSvmVkICUs8ZHKA1\n\tswbNXn1cJXuEG+5DEpa6URtbB+/ql5rYaWEpMUY+XhxD0JSMcz+CaqQ07ZOvqdyOAq\n\tqf1Ouxgm3dZPNdBQDAc+j2vHr9nH/9w1lflHW2vFL1NTE/EB5HV7BP5v87ndL3GKXb\n\tycSrWisDf3KDeikHBijgDA7rrHVm1J+Fm61kOlFsKUL87i8k3HnCmHFm6zylauP/gW\n\tOnv1Yb1/oA6cg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695767154;\n\tbh=8NaBWPzCEOL80KByXrGuOpDEYhjEMVn8eBLlM2g/q7s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r+2lZ7mhjUsulC2TuX5F1JSAdU7cko+dk4jj9KaLwWAAo8ZKuqx2xEfuC8ABOARxl\n\tUjpPtj49n2mAKdcwZrvZ4KhdWClDIjXHBBYtqSEXod50xXQ2nEe62dHqQ0Z+rSP7cc\n\to+ZdyZSVy7IeRx4zXMiapWGtDpEJCp9isp9+Y/5g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"r+2lZ7mh\"; dkim-atps=neutral","Date":"Wed, 27 Sep 2023 01:27:45 +0300","To":"Jacopo Mondi ","Message-ID":"<20230926222745.GJ5854@pendragon.ideasonboard.com>","References":"<20230921165550.50956-1-jacopo.mondi@ideasonboard.com>\n\t<20230921165550.50956-5-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230921165550.50956-5-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 04/13] libcamera: rpi: Handle\n\tSensorConfiguration","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\" "}}]