[{"id":27644,"web_url":"https://patchwork.libcamera.org/comment/27644/","msgid":"","date":"2023-08-01T09:02:15","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle\n\tSensorConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThanks for the update. Just one minor comment:\n\nOn Mon, 31 Jul 2023 at 12:31, Jacopo Mondi via libcamera-devel\n wrote:\n>\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> Signed-off-by: Jacopo Mondi \n> ---\n> .../pipeline/rpi/common/pipeline_base.cpp | 62 ++++++++++++++++---\n> .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> 3 files changed, 82 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 97acafbbb728..15c7a5c72bdd 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,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> if (config_.empty())\n> return Invalid;\n>\n> + if (!sensorConfig.valid()) {\n> + LOG(RPI, Error) << \"Invalid sensor configuration request\";\n> + return Invalid;\n> + }\n> +\n> status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n>\n> /*\n> @@ -207,19 +212,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> std::sort(outStreams.begin(), outStreams.end(),\n> [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n>\n> - /* Compute the sensor configuration. */\n> - unsigned int bitDepth = defaultRawBitDepth;\n> - if (!rawStreams.empty()) {\n> + /* Compute the sensor's format then do any platform specific fixups. */\n> + unsigned int bitDepth;\n> + Size sensorSize;\n> +\n> + if (sensorConfig) {\n> + /* Use the application provided sensor configuration. */\n> + bitDepth = sensorConfig.bitDepth;\n> + sensorSize = sensorConfig.outputSize;\n> + } else if (!rawStreams.empty()) {\n> + /* Use the RAW stream format and size. */\n> BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> bitDepth = bayerFormat.bitDepth;\n> + sensorSize = rawStreams[0].cfg->size;\n> + } else {\n> + bitDepth = defaultRawBitDepth;\n> + sensorSize = outStreams[0].cfg->size;\n> }\n>\n> - sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> - : rawStreams[0].cfg->size,\n> - bitDepth);\n> + sensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);\n> +\n> + /*\n> + * If a sensor configuration has been requested, it should apply\n> + * without modifications.\n> + */\n> + if (sensorConfig) {\n> + BayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);\n> +\n> + if (bayer.bitDepth != sensorConfig.bitDepth ||\n> + sensorFormat_.size != sensorConfig.outputSize) {\n> + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> + << \"bitDepth/size mismatch\";\n> + return Invalid;\n> + }\n> + }\n>\n> /* Do any platform specific fixups. */\n> - status = data_->platformValidate(rawStreams, outStreams);\n> + status = data_->platformValidate(this, rawStreams, outStreams);\n> if (status == Invalid)\n> return Invalid;\n>\n> @@ -467,12 +496,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> std::sort(ispStreams.begin(), ispStreams.end(),\n> [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n>\n> - /* Apply the format on the sensor with any cached transform. */\n> + /*\n> + * Apply the format on the sensor with any cached transform.\n> + *\n> + * If the application has provided a sensor configuration apply it\n> + * instead of just applying a format.\n> + */\n> const RPiCameraConfiguration *rpiConfig =\n> static_cast(config);\n> - V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;\n> + V4L2SubdeviceFormat sensorFormat;\n>\n> - ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> + if (rpiConfig->sensorConfig) {\n> + ret = data->sensor_->applyConfiguration(rpiConfig->sensorConfig,\n> + rpiConfig->combinedTransform_,\n> + &sensorFormat);\n> + } else {\n> + sensorFormat = rpiConfig->sensorFormat_;\n> + ret = data->sensor_->setFormat(&sensorFormat,\n> + rpiConfig->combinedTransform_);\n> + }\n> if (ret)\n> return 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 a139c98a5a2b..0a795f4d2689 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> V4L2VideoDevice *dev;\n> };\n>\n> - virtual CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> + virtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,\n\nCan the RPiCameraConfiguration *rpiConfig be made const in the declaration?\n\nOther than that:\nReviewed-by: Naushir Patuck \n\n\n> + std::vector &rawStreams,\n> std::vector &outStreams) const = 0;\n> virtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,\n> 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..bf864d4174b2 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> {\n> }\n>\n> - CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> + std::vector &rawStreams,\n> std::vector &outStreams) const override;\n>\n> int platformPipelineConfigure(const std::unique_ptr &root) override;\n> @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> return 0;\n> }\n>\n> -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector &rawStreams,\n> +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> + std::vector &rawStreams,\n> std::vector &outStreams) const\n> {\n> CameraConfiguration::Status status = CameraConfiguration::Status::Valid;\n> @@ -405,9 +407,29 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector return CameraConfiguration::Status::Invalid;\n> }\n>\n> - if (!rawStreams.empty())\n> + if (!rawStreams.empty()) {\n> rawStreams[0].dev = unicam_[Unicam::Image].dev();\n>\n> + /* Adjust the RAW stream to match the requested sensor config. */\n> + if (rpiConfig->sensorConfig) {\n> + StreamConfiguration *rawStream = rawStreams[0].cfg;\n> + BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> +\n> + /* Handle flips to make sure to match the RAW stream format. */\n> + if (flipsAlterBayerOrder_)\n> + rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> + PixelFormat rawFormat = rawBayer.toPixelFormat();\n> +\n> + if (rawStream->pixelFormat != rawFormat ||\n> + rawStream->size != rpiConfig->sensorConfig.outputSize) {\n> + rawStream->pixelFormat = rawFormat;\n> + rawStream->size = rpiConfig->sensorConfig.outputSize;\n> +\n> + status = CameraConfiguration::Adjusted;\n> + }\n> + }\n> + }\n> +\n> /*\n> * For the two ISP outputs, one stream must be equal or smaller than the\n> * other in all dimensions.\n> @@ -417,6 +439,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector for (unsigned int i = 0; i < outStreams.size(); i++) {\n> Size size;\n>\n> + /* \\todo Warn if upscaling: reduces image quality. */\n> +\n> size.width = std::min(outStreams[i].cfg->size.width,\n> outStreams[0].cfg->size.width);\n> size.height = std::min(outStreams[i].cfg->size.height,\n> --\n> 2.40.1\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 5891EBDCBF\n\tfor ;\n\tTue, 1 Aug 2023 09:02:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C66676037F;\n\tTue, 1 Aug 2023 11:02:33 +0200 (CEST)","from mail-yw1-x1135.google.com (mail-yw1-x1135.google.com\n\t[IPv6:2607:f8b0:4864:20::1135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1167C6037F\n\tfor ;\n\tTue, 1 Aug 2023 11:02:32 +0200 (CEST)","by mail-yw1-x1135.google.com with SMTP id\n\t00721157ae682-583b0637c04so76834577b3.1\n\tfor ;\n\tTue, 01 Aug 2023 02:02:31 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690880553;\n\tbh=ToeJpkMJ71Oto6s+wztnQ1xvA5jQeo4pLpAyDLtjiNU=;\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=XTt9aNrRDiW+SJr7ECzk2Ble9RruXm2mx61E89qzEEPs3Gkpj2jG+pM21V9wHZHqI\n\tk3KaaMfYqwdOQsG7vtZ0LfrKnwdhbrVfHFDnAYXlrI+oBHdZqNyW6OJ5MwaLzFfp2y\n\t5d1u4XlpTQP9QTWStgjDkiFzDwE+etaEJKWcAngcH9Eap0ozDU7KUfoPCuhrnVWnjK\n\t1Vd9eO3bK7u4lYook/Bi1rVN+a4jHhxsKJjmzSaxhkT+j3RFxxSWYcCszRGNJ6BW3c\n\tU0mUwzvNrFHYNbLZFn8ZDNaQNWuNDO1RPl4BdvOFeai+XECw3Jq6e4C8R+Hsm1LKSC\n\tbJjjy19wv8YoQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1690880551; x=1691485351;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Nm8wtLaiHHUluJmXdaW1VVKhEiuEqOfAJEsXrrp/X54=;\n\tb=sR5q/LLFVqasagqLm3BPEFpM4Cn2pOc37PlwqmfiUNn5SVhBAy+l3nTTqxq1jZ3Dgg\n\t+xhjXvFc+XA5btTwSyqPnMNbwIo8B5thxvizoWlrI0ej+ny5YCnelQQwlugeHW+JG8CA\n\t4JeCsfBgsyBR1s4Mgls2L05RzN8Q6eTx5zrURLe4qcuFzI2/ItgjNi2mSdfbpanMZMuy\n\t7UdWCuLUKcAQaLLXQ6nsoZ8QvVpgQOj0NrK1Ll5RYcVT8HtpySoMJx3vWuL67cT+RKeZ\n\tJE5BwJybxewzwUxG/ayOe7EhmCmMMJ2p/y0AxDWA1TOeTzt2QuRVs0AUYFjS9t0u0HsQ\n\tCngw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sR5q/LLF\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690880551; x=1691485351;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Nm8wtLaiHHUluJmXdaW1VVKhEiuEqOfAJEsXrrp/X54=;\n\tb=Nww/LrCklOwZY/SqrSkXLPOK87RHt7PATT1k5zcHTPr6OP8O14j8Cs1s2TGSy5FR/6\n\tvfk25KkBPcMJokwdKPhX4aU4BlT7ZDNCdqErVz4osGvRRDDS/0CCIhpt/iPxmYfTQE9n\n\tb1TEyyo+1AmEEtj140uw634OKkNqAbX+xTh7gWFGhx0WCAe+HOW4hXNRXQYTGhb7dXKP\n\tWQEgdTS1c1VFhVoN+TBqFNykZQH3tgeoIGA7ggK+9MBeY3Q53tw6VM2/OnajxXSXF5cm\n\tTU23vXPwjzs3ZIAdv2jE7FH/w8vbQ4AMts/wpXUtjpIkOc1wBzx3gebE2m2vjCQsWlA9\n\t3P/A==","X-Gm-Message-State":"ABy/qLYqh9bxU057lmu0NnGtGgRd3Hz2HNuUnUY6CYyITJFliTjNCn+D\n\tyggJXzLISWTokF+UndEimtwE/UHTLrE0VSIchnLNVw==","X-Google-Smtp-Source":"APBJJlHRsDxchUxhX5moDSph2OKipJk4S3mFAtqg5e9KE1qdgZzHzeIZElsMRuHXNKT+9wHXhJLY/6n+nq6PvjgynPs=","X-Received":"by 2002:a81:4886:0:b0:577:3f8c:fc60 with SMTP id\n\tv128-20020a814886000000b005773f8cfc60mr14094719ywa.1.1690880550764;\n\tTue, 01 Aug 2023 02:02:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-5-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230731113115.5915-5-jacopo.mondi@ideasonboard.com>","Date":"Tue, 1 Aug 2023 10:02:15 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27645,"web_url":"https://patchwork.libcamera.org/comment/27645/","msgid":"<7u5utkxieovkjcfw2avgv6mtl4c2sownko2w24hmmwlb5oez7m@wfh3sbj7zitc>","date":"2023-08-01T09:23:44","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle\n\tSensorConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Tue, Aug 01, 2023 at 10:02:15AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> Thanks for the update. Just one minor comment:\n>\n> On Mon, 31 Jul 2023 at 12:31, Jacopo Mondi via libcamera-devel\n> wrote:\n> >\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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../pipeline/rpi/common/pipeline_base.cpp | 62 ++++++++++++++++---\n> > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > 3 files changed, 82 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 97acafbbb728..15c7a5c72bdd 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,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > if (config_.empty())\n> > return Invalid;\n> >\n> > + if (!sensorConfig.valid()) {\n> > + LOG(RPI, Error) << \"Invalid sensor configuration request\";\n> > + return Invalid;\n> > + }\n> > +\n> > status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> >\n> > /*\n> > @@ -207,19 +212,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > std::sort(outStreams.begin(), outStreams.end(),\n> > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> >\n> > - /* Compute the sensor configuration. */\n> > - unsigned int bitDepth = defaultRawBitDepth;\n> > - if (!rawStreams.empty()) {\n> > + /* Compute the sensor's format then do any platform specific fixups. */\n> > + unsigned int bitDepth;\n> > + Size sensorSize;\n> > +\n> > + if (sensorConfig) {\n> > + /* Use the application provided sensor configuration. */\n> > + bitDepth = sensorConfig.bitDepth;\n> > + sensorSize = sensorConfig.outputSize;\n> > + } else if (!rawStreams.empty()) {\n> > + /* Use the RAW stream format and size. */\n> > BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> > bitDepth = bayerFormat.bitDepth;\n> > + sensorSize = rawStreams[0].cfg->size;\n> > + } else {\n> > + bitDepth = defaultRawBitDepth;\n> > + sensorSize = outStreams[0].cfg->size;\n> > }\n> >\n> > - sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> > - : rawStreams[0].cfg->size,\n> > - bitDepth);\n> > + sensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);\n> > +\n> > + /*\n> > + * If a sensor configuration has been requested, it should apply\n> > + * without modifications.\n> > + */\n> > + if (sensorConfig) {\n> > + BayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);\n> > +\n> > + if (bayer.bitDepth != sensorConfig.bitDepth ||\n> > + sensorFormat_.size != sensorConfig.outputSize) {\n> > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > + << \"bitDepth/size mismatch\";\n> > + return Invalid;\n> > + }\n> > + }\n> >\n> > /* Do any platform specific fixups. */\n> > - status = data_->platformValidate(rawStreams, outStreams);\n> > + status = data_->platformValidate(this, rawStreams, outStreams);\n> > if (status == Invalid)\n> > return Invalid;\n> >\n> > @@ -467,12 +496,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > std::sort(ispStreams.begin(), ispStreams.end(),\n> > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> >\n> > - /* Apply the format on the sensor with any cached transform. */\n> > + /*\n> > + * Apply the format on the sensor with any cached transform.\n> > + *\n> > + * If the application has provided a sensor configuration apply it\n> > + * instead of just applying a format.\n> > + */\n> > const RPiCameraConfiguration *rpiConfig =\n> > static_cast(config);\n> > - V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;\n> > + V4L2SubdeviceFormat sensorFormat;\n> >\n> > - ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> > + if (rpiConfig->sensorConfig) {\n> > + ret = data->sensor_->applyConfiguration(rpiConfig->sensorConfig,\n> > + rpiConfig->combinedTransform_,\n> > + &sensorFormat);\n> > + } else {\n> > + sensorFormat = rpiConfig->sensorFormat_;\n> > + ret = data->sensor_->setFormat(&sensorFormat,\n> > + rpiConfig->combinedTransform_);\n> > + }\n> > if (ret)\n> > return 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 a139c98a5a2b..0a795f4d2689 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> > V4L2VideoDevice *dev;\n> > };\n> >\n> > - virtual CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > + virtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,\n>\n> Can the RPiCameraConfiguration *rpiConfig be made const in the declaration?\n\nI'm surprised! platformValidate() could potentially change the\nStreamConfiguration part of the CameraConfiguration BUT as you wrap\nStreamConfiguration in a custom StreamParam function, we do not access\nthem through *config, so it can be made const even if conceptually it\nis not\n\n\tfor (const auto &[index, cfg] : utils::enumerate(config_)) {\n\t\tif (isRaw(cfg.pixelFormat))\n\t\t\trawStreams.emplace_back(index, &cfg);\n\t\telse\n\t\t\toutStreams.emplace_back(index, &cfg);\n\t}\n\n ...\n\n\n\t/* Do any platform specific fixups. */\n\tstatus = data_->platformValidate(this, rawStreams, outStreams);\n\nStreamParams has the potential to break some correctness guarantees\nthat 'const' gives you, as it stores StreamConfiguration by pointer,\nbypassing the 'const CameraConfiguration'. Can't say I like it very\nmuch, but not a comment for this patch...\n\n\n>\n> Other than that:\n> Reviewed-by: Naushir Patuck \n>\n>\n> > + std::vector &rawStreams,\n> > std::vector &outStreams) const = 0;\n> > virtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,\n> > 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..bf864d4174b2 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> > {\n> > }\n> >\n> > - CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > + std::vector &rawStreams,\n> > std::vector &outStreams) const override;\n> >\n> > int platformPipelineConfigure(const std::unique_ptr &root) override;\n> > @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > return 0;\n> > }\n> >\n> > -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector &rawStreams,\n> > +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > + std::vector &rawStreams,\n> > std::vector &outStreams) const\n> > {\n> > CameraConfiguration::Status status = CameraConfiguration::Status::Valid;\n> > @@ -405,9 +407,29 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > return CameraConfiguration::Status::Invalid;\n> > }\n> >\n> > - if (!rawStreams.empty())\n> > + if (!rawStreams.empty()) {\n> > rawStreams[0].dev = unicam_[Unicam::Image].dev();\n> >\n> > + /* Adjust the RAW stream to match the requested sensor config. */\n> > + if (rpiConfig->sensorConfig) {\n> > + StreamConfiguration *rawStream = rawStreams[0].cfg;\n> > + BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> > +\n> > + /* Handle flips to make sure to match the RAW stream format. */\n> > + if (flipsAlterBayerOrder_)\n> > + rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> > + PixelFormat rawFormat = rawBayer.toPixelFormat();\n> > +\n> > + if (rawStream->pixelFormat != rawFormat ||\n> > + rawStream->size != rpiConfig->sensorConfig.outputSize) {\n> > + rawStream->pixelFormat = rawFormat;\n> > + rawStream->size = rpiConfig->sensorConfig.outputSize;\n> > +\n> > + status = CameraConfiguration::Adjusted;\n> > + }\n> > + }\n> > + }\n> > +\n> > /*\n> > * For the two ISP outputs, one stream must be equal or smaller than the\n> > * other in all dimensions.\n> > @@ -417,6 +439,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > for (unsigned int i = 0; i < outStreams.size(); i++) {\n> > Size size;\n> >\n> > + /* \\todo Warn if upscaling: reduces image quality. */\n> > +\n> > size.width = std::min(outStreams[i].cfg->size.width,\n> > outStreams[0].cfg->size.width);\n> > size.height = std::min(outStreams[i].cfg->size.height,\n> > --\n> > 2.40.1\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 4D9BFBDB13\n\tfor ;\n\tTue, 1 Aug 2023 09:23:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1ABA627E6;\n\tTue, 1 Aug 2023 11:23:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB01C6037F\n\tfor ;\n\tTue, 1 Aug 2023 11:23:48 +0200 (CEST)","from ideasonboard.com (mob-5-90-55-80.net.vodafone.it [5.90.55.80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8D3223D4;\n\tTue, 1 Aug 2023 11:22:45 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690881829;\n\tbh=uA6HbuvyT5UrEekf5E4ujiBXnCs1E3euR981iZqUIso=;\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=AMNDNXi2olkj4tIp2tA9f+HUAwQ4oLpiMijHqc4WLTOYD7ZQ1FlOHnT4fLLWakr9I\n\twmmUxU7TKmrfaaVxAgI7ud3hxNO4tpZOVFm6ca+pCkCfULh5jhbHTags14WN6kXwll\n\twXwWOs2zcRS2KizhcD2jAoGGYeWkSywCnLqu07yRND3DtOISEsct1lufxtmUhRiSHC\n\tlzlL1yEVlq5sWJeQ5Lv/dpGkKrpFQILjOTS5LAF8QF2AL3QRXelaF3jEdsB08OCx6n\n\t8nbzXCYLzZ20szT8SXtjl1ebZSg9EiE/YkpTbRriHHwF6hHSEx06UjPRqGQFwcDLh5\n\tsQZTB4n6oEbGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1690881765;\n\tbh=uA6HbuvyT5UrEekf5E4ujiBXnCs1E3euR981iZqUIso=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iZJtlfahwoW9zRg0jrBbe2ru2GIBH07UDSOrIiZTMYfuNytHXIDcwNn4IdGHmMlaH\n\t6IDDD9L9Z4ABo/j9/yMstgLe2V5QxjBMwxHVipqnxwj7i3Z4+IKlWTlV36yRIhxcIW\n\tTTaL0glkN2Xii5+VFTGUm5gkI1AQnqLiTJ/V7/KY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iZJtlfah\"; dkim-atps=neutral","Date":"Tue, 1 Aug 2023 11:23:44 +0200","To":"Naushir Patuck ","Message-ID":"<7u5utkxieovkjcfw2avgv6mtl4c2sownko2w24hmmwlb5oez7m@wfh3sbj7zitc>","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-5-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27646,"web_url":"https://patchwork.libcamera.org/comment/27646/","msgid":"","date":"2023-08-01T09:25:32","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle\n\tSensorConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Tue, Aug 01, 2023 at 11:23:44AM +0200, Jacopo Mondi wrote:\n> Hi Naush\n>\n> On Tue, Aug 01, 2023 at 10:02:15AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for the update. Just one minor comment:\n> >\n> > On Mon, 31 Jul 2023 at 12:31, Jacopo Mondi via libcamera-devel\n> > wrote:\n> > >\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> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > .../pipeline/rpi/common/pipeline_base.cpp | 62 ++++++++++++++++---\n> > > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > > 3 files changed, 82 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 97acafbbb728..15c7a5c72bdd 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,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > if (config_.empty())\n> > > return Invalid;\n> > >\n> > > + if (!sensorConfig.valid()) {\n> > > + LOG(RPI, Error) << \"Invalid sensor configuration request\";\n> > > + return Invalid;\n> > > + }\n> > > +\n> > > status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> > >\n> > > /*\n> > > @@ -207,19 +212,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > std::sort(outStreams.begin(), outStreams.end(),\n> > > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> > >\n> > > - /* Compute the sensor configuration. */\n> > > - unsigned int bitDepth = defaultRawBitDepth;\n> > > - if (!rawStreams.empty()) {\n> > > + /* Compute the sensor's format then do any platform specific fixups. */\n> > > + unsigned int bitDepth;\n> > > + Size sensorSize;\n> > > +\n> > > + if (sensorConfig) {\n> > > + /* Use the application provided sensor configuration. */\n> > > + bitDepth = sensorConfig.bitDepth;\n> > > + sensorSize = sensorConfig.outputSize;\n> > > + } else if (!rawStreams.empty()) {\n> > > + /* Use the RAW stream format and size. */\n> > > BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> > > bitDepth = bayerFormat.bitDepth;\n> > > + sensorSize = rawStreams[0].cfg->size;\n> > > + } else {\n> > > + bitDepth = defaultRawBitDepth;\n> > > + sensorSize = outStreams[0].cfg->size;\n> > > }\n> > >\n> > > - sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> > > - : rawStreams[0].cfg->size,\n> > > - bitDepth);\n> > > + sensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);\n> > > +\n> > > + /*\n> > > + * If a sensor configuration has been requested, it should apply\n> > > + * without modifications.\n> > > + */\n> > > + if (sensorConfig) {\n> > > + BayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);\n> > > +\n> > > + if (bayer.bitDepth != sensorConfig.bitDepth ||\n> > > + sensorFormat_.size != sensorConfig.outputSize) {\n> > > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > > + << \"bitDepth/size mismatch\";\n> > > + return Invalid;\n> > > + }\n> > > + }\n> > >\n> > > /* Do any platform specific fixups. */\n> > > - status = data_->platformValidate(rawStreams, outStreams);\n> > > + status = data_->platformValidate(this, rawStreams, outStreams);\n> > > if (status == Invalid)\n> > > return Invalid;\n> > >\n> > > @@ -467,12 +496,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > > std::sort(ispStreams.begin(), ispStreams.end(),\n> > > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> > >\n> > > - /* Apply the format on the sensor with any cached transform. */\n> > > + /*\n> > > + * Apply the format on the sensor with any cached transform.\n> > > + *\n> > > + * If the application has provided a sensor configuration apply it\n> > > + * instead of just applying a format.\n> > > + */\n> > > const RPiCameraConfiguration *rpiConfig =\n> > > static_cast(config);\n> > > - V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;\n> > > + V4L2SubdeviceFormat sensorFormat;\n> > >\n> > > - ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> > > + if (rpiConfig->sensorConfig) {\n> > > + ret = data->sensor_->applyConfiguration(rpiConfig->sensorConfig,\n> > > + rpiConfig->combinedTransform_,\n> > > + &sensorFormat);\n> > > + } else {\n> > > + sensorFormat = rpiConfig->sensorFormat_;\n> > > + ret = data->sensor_->setFormat(&sensorFormat,\n> > > + rpiConfig->combinedTransform_);\n> > > + }\n> > > if (ret)\n> > > return 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 a139c98a5a2b..0a795f4d2689 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> > > V4L2VideoDevice *dev;\n> > > };\n> > >\n> > > - virtual CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > > + virtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,\n> >\n> > Can the RPiCameraConfiguration *rpiConfig be made const in the declaration?\n>\n> I'm surprised! platformValidate() could potentially change the\n> StreamConfiguration part of the CameraConfiguration BUT as you wrap\n> StreamConfiguration in a custom StreamParam function, we do not access\n\ns/function/structure/ ofc\n\n> them through *config, so it can be made const even if conceptually it\n> is not\n>\n> \tfor (const auto &[index, cfg] : utils::enumerate(config_)) {\n> \t\tif (isRaw(cfg.pixelFormat))\n> \t\t\trawStreams.emplace_back(index, &cfg);\n> \t\telse\n> \t\t\toutStreams.emplace_back(index, &cfg);\n> \t}\n>\n> ...\n>\n>\n> \t/* Do any platform specific fixups. */\n> \tstatus = data_->platformValidate(this, rawStreams, outStreams);\n>\n> StreamParams has the potential to break some correctness guarantees\n> that 'const' gives you, as it stores StreamConfiguration by pointer,\n> bypassing the 'const CameraConfiguration'. Can't say I like it very\n> much, but not a comment for this patch...\n>\n>\n> >\n> > Other than that:\n> > Reviewed-by: Naushir Patuck \n> >\n> >\n> > > + std::vector &rawStreams,\n> > > std::vector &outStreams) const = 0;\n> > > virtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,\n> > > 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..bf864d4174b2 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> > > {\n> > > }\n> > >\n> > > - CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > > + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > > + std::vector &rawStreams,\n> > > std::vector &outStreams) const override;\n> > >\n> > > int platformPipelineConfigure(const std::unique_ptr &root) override;\n> > > @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > > return 0;\n> > > }\n> > >\n> > > -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector &rawStreams,\n> > > +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > > + std::vector &rawStreams,\n> > > std::vector &outStreams) const\n> > > {\n> > > CameraConfiguration::Status status = CameraConfiguration::Status::Valid;\n> > > @@ -405,9 +407,29 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > > return CameraConfiguration::Status::Invalid;\n> > > }\n> > >\n> > > - if (!rawStreams.empty())\n> > > + if (!rawStreams.empty()) {\n> > > rawStreams[0].dev = unicam_[Unicam::Image].dev();\n> > >\n> > > + /* Adjust the RAW stream to match the requested sensor config. */\n> > > + if (rpiConfig->sensorConfig) {\n> > > + StreamConfiguration *rawStream = rawStreams[0].cfg;\n> > > + BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> > > +\n> > > + /* Handle flips to make sure to match the RAW stream format. */\n> > > + if (flipsAlterBayerOrder_)\n> > > + rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> > > + PixelFormat rawFormat = rawBayer.toPixelFormat();\n> > > +\n> > > + if (rawStream->pixelFormat != rawFormat ||\n> > > + rawStream->size != rpiConfig->sensorConfig.outputSize) {\n> > > + rawStream->pixelFormat = rawFormat;\n> > > + rawStream->size = rpiConfig->sensorConfig.outputSize;\n> > > +\n> > > + status = CameraConfiguration::Adjusted;\n> > > + }\n> > > + }\n> > > + }\n> > > +\n> > > /*\n> > > * For the two ISP outputs, one stream must be equal or smaller than the\n> > > * other in all dimensions.\n> > > @@ -417,6 +439,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > > for (unsigned int i = 0; i < outStreams.size(); i++) {\n> > > Size size;\n> > >\n> > > + /* \\todo Warn if upscaling: reduces image quality. */\n> > > +\n> > > size.width = std::min(outStreams[i].cfg->size.width,\n> > > outStreams[0].cfg->size.width);\n> > > size.height = std::min(outStreams[i].cfg->size.height,\n> > > --\n> > > 2.40.1\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 32279BDCBF\n\tfor ;\n\tTue, 1 Aug 2023 09:25:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83949627EA;\n\tTue, 1 Aug 2023 11:25:36 +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 F3C256037F\n\tfor ;\n\tTue, 1 Aug 2023 11:25:35 +0200 (CEST)","from ideasonboard.com (mob-5-90-55-80.net.vodafone.it [5.90.55.80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC2653D4;\n\tTue, 1 Aug 2023 11:24:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690881936;\n\tbh=xM1VOkJJzjJNgrwpzhozOuFx7OdV9sF2BLlSTSEPB/Q=;\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=hEsT4LlpqxDHNhdPEBe8yBUpWCFpHOSXmslLlGInvuqRQvd36/SJp/T7SJsJU1RVb\n\tnRVv21eszM4mUiTcw0dMp0mAKjy/Qs2y+LImc49W0q9WrZEOV+CkZKaJaK90a0L7Y4\n\tcsvnavDSsw8YU3fyRM106yfT19VK+Azn+dtkwjIO103CD9ernMA90TCVpMFuG3OgOG\n\t9eOSNN8eq7qm0qNf9I6ftccd994tE8qEFjQfC4esTosYhuFpp2n4g6WHezBPUwmmPN\n\t6r9epjAVa6FjYD/rOEBvBLfwFvzNzOR6gjjs/6OmCTTDKv/yhFx6rZ5YuC7tFOsPit\n\t+WMLv7LBH/u/A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1690881873;\n\tbh=xM1VOkJJzjJNgrwpzhozOuFx7OdV9sF2BLlSTSEPB/Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NyfBxSeDcf8WOh/eRVaaOZg7IE1+GIfNyxKDc7WZejWuJ52qpHlFcHV5rzGxOXpl7\n\teeMqST8DjJtKm1uoO/yeL/wAZjlq3iAijwkrgh6fzSwrIv7wVNu6kxZQZH4EKPxTlG\n\tyQ6x1ITsbutto2RNRU3X2cG0THTUahfQIvJ7g2u4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NyfBxSeD\"; dkim-atps=neutral","Date":"Tue, 1 Aug 2023 11:25:32 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-5-jacopo.mondi@ideasonboard.com>\n\t\n\t<7u5utkxieovkjcfw2avgv6mtl4c2sownko2w24hmmwlb5oez7m@wfh3sbj7zitc>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<7u5utkxieovkjcfw2avgv6mtl4c2sownko2w24hmmwlb5oez7m@wfh3sbj7zitc>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27647,"web_url":"https://patchwork.libcamera.org/comment/27647/","msgid":"","date":"2023-08-01T09:36:01","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle\n\tSensorConfiguration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Tue, 1 Aug 2023 at 10:23, Jacopo Mondi wrote:\n>\n> Hi Naush\n>\n> On Tue, Aug 01, 2023 at 10:02:15AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for the update. Just one minor comment:\n> >\n> > On Mon, 31 Jul 2023 at 12:31, Jacopo Mondi via libcamera-devel\n> > wrote:\n> > >\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> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > .../pipeline/rpi/common/pipeline_base.cpp | 62 ++++++++++++++++---\n> > > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > > 3 files changed, 82 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 97acafbbb728..15c7a5c72bdd 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,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > if (config_.empty())\n> > > return Invalid;\n> > >\n> > > + if (!sensorConfig.valid()) {\n> > > + LOG(RPI, Error) << \"Invalid sensor configuration request\";\n> > > + return Invalid;\n> > > + }\n> > > +\n> > > status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> > >\n> > > /*\n> > > @@ -207,19 +212,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > std::sort(outStreams.begin(), outStreams.end(),\n> > > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> > >\n> > > - /* Compute the sensor configuration. */\n> > > - unsigned int bitDepth = defaultRawBitDepth;\n> > > - if (!rawStreams.empty()) {\n> > > + /* Compute the sensor's format then do any platform specific fixups. */\n> > > + unsigned int bitDepth;\n> > > + Size sensorSize;\n> > > +\n> > > + if (sensorConfig) {\n> > > + /* Use the application provided sensor configuration. */\n> > > + bitDepth = sensorConfig.bitDepth;\n> > > + sensorSize = sensorConfig.outputSize;\n> > > + } else if (!rawStreams.empty()) {\n> > > + /* Use the RAW stream format and size. */\n> > > BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> > > bitDepth = bayerFormat.bitDepth;\n> > > + sensorSize = rawStreams[0].cfg->size;\n> > > + } else {\n> > > + bitDepth = defaultRawBitDepth;\n> > > + sensorSize = outStreams[0].cfg->size;\n> > > }\n> > >\n> > > - sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> > > - : rawStreams[0].cfg->size,\n> > > - bitDepth);\n> > > + sensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);\n> > > +\n> > > + /*\n> > > + * If a sensor configuration has been requested, it should apply\n> > > + * without modifications.\n> > > + */\n> > > + if (sensorConfig) {\n> > > + BayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);\n> > > +\n> > > + if (bayer.bitDepth != sensorConfig.bitDepth ||\n> > > + sensorFormat_.size != sensorConfig.outputSize) {\n> > > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > > + << \"bitDepth/size mismatch\";\n> > > + return Invalid;\n> > > + }\n> > > + }\n> > >\n> > > /* Do any platform specific fixups. */\n> > > - status = data_->platformValidate(rawStreams, outStreams);\n> > > + status = data_->platformValidate(this, rawStreams, outStreams);\n> > > if (status == Invalid)\n> > > return Invalid;\n> > >\n> > > @@ -467,12 +496,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > > std::sort(ispStreams.begin(), ispStreams.end(),\n> > > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> > >\n> > > - /* Apply the format on the sensor with any cached transform. */\n> > > + /*\n> > > + * Apply the format on the sensor with any cached transform.\n> > > + *\n> > > + * If the application has provided a sensor configuration apply it\n> > > + * instead of just applying a format.\n> > > + */\n> > > const RPiCameraConfiguration *rpiConfig =\n> > > static_cast(config);\n> > > - V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;\n> > > + V4L2SubdeviceFormat sensorFormat;\n> > >\n> > > - ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> > > + if (rpiConfig->sensorConfig) {\n> > > + ret = data->sensor_->applyConfiguration(rpiConfig->sensorConfig,\n> > > + rpiConfig->combinedTransform_,\n> > > + &sensorFormat);\n> > > + } else {\n> > > + sensorFormat = rpiConfig->sensorFormat_;\n> > > + ret = data->sensor_->setFormat(&sensorFormat,\n> > > + rpiConfig->combinedTransform_);\n> > > + }\n> > > if (ret)\n> > > return 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 a139c98a5a2b..0a795f4d2689 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> > > V4L2VideoDevice *dev;\n> > > };\n> > >\n> > > - virtual CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > > + virtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,\n> >\n> > Can the RPiCameraConfiguration *rpiConfig be made const in the declaration?\n>\n> I'm surprised! platformValidate() could potentially change the\n> StreamConfiguration part of the CameraConfiguration BUT as you wrap\n> StreamConfiguration in a custom StreamParam function, we do not access\n> them through *config, so it can be made const even if conceptually it\n> is not\n>\n> for (const auto &[index, cfg] : utils::enumerate(config_)) {\n> if (isRaw(cfg.pixelFormat))\n> rawStreams.emplace_back(index, &cfg);\n> else\n> outStreams.emplace_back(index, &cfg);\n> }\n>\n> ...\n>\n>\n> /* Do any platform specific fixups. */\n> status = data_->platformValidate(this, rawStreams, outStreams);\n>\n> StreamParams has the potential to break some correctness guarantees\n> that 'const' gives you, as it stores StreamConfiguration by pointer,\n> bypassing the 'const CameraConfiguration'. Can't say I like it very\n> much, but not a comment for this patch...\n\nI see. Agree that the StreamParams sort of duplicates the new rpiConfig param,\nbut with the added benefit of being sorted on size. This is something we can\nlook at changing on top of this change. In the meantime, I agree we probably\ndon't want to use const for rpiConfig for this reason.\n\nRegards,\nNaush\n\n>\n>\n> >\n> > Other than that:\n> > Reviewed-by: Naushir Patuck \n> >\n> >\n> > > + std::vector &rawStreams,\n> > > std::vector &outStreams) const = 0;\n> > > virtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,\n> > > 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..bf864d4174b2 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> > > {\n> > > }\n> > >\n> > > - CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > > + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > > + std::vector &rawStreams,\n> > > std::vector &outStreams) const override;\n> > >\n> > > int platformPipelineConfigure(const std::unique_ptr &root) override;\n> > > @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > > return 0;\n> > > }\n> > >\n> > > -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector &rawStreams,\n> > > +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > > + std::vector &rawStreams,\n> > > std::vector &outStreams) const\n> > > {\n> > > CameraConfiguration::Status status = CameraConfiguration::Status::Valid;\n> > > @@ -405,9 +407,29 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > > return CameraConfiguration::Status::Invalid;\n> > > }\n> > >\n> > > - if (!rawStreams.empty())\n> > > + if (!rawStreams.empty()) {\n> > > rawStreams[0].dev = unicam_[Unicam::Image].dev();\n> > >\n> > > + /* Adjust the RAW stream to match the requested sensor config. */\n> > > + if (rpiConfig->sensorConfig) {\n> > > + StreamConfiguration *rawStream = rawStreams[0].cfg;\n> > > + BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> > > +\n> > > + /* Handle flips to make sure to match the RAW stream format. */\n> > > + if (flipsAlterBayerOrder_)\n> > > + rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> > > + PixelFormat rawFormat = rawBayer.toPixelFormat();\n> > > +\n> > > + if (rawStream->pixelFormat != rawFormat ||\n> > > + rawStream->size != rpiConfig->sensorConfig.outputSize) {\n> > > + rawStream->pixelFormat = rawFormat;\n> > > + rawStream->size = rpiConfig->sensorConfig.outputSize;\n> > > +\n> > > + status = CameraConfiguration::Adjusted;\n> > > + }\n> > > + }\n> > > + }\n> > > +\n> > > /*\n> > > * For the two ISP outputs, one stream must be equal or smaller than the\n> > > * other in all dimensions.\n> > > @@ -417,6 +439,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > > for (unsigned int i = 0; i < outStreams.size(); i++) {\n> > > Size size;\n> > >\n> > > + /* \\todo Warn if upscaling: reduces image quality. */\n> > > +\n> > > size.width = std::min(outStreams[i].cfg->size.width,\n> > > outStreams[0].cfg->size.width);\n> > > size.height = std::min(outStreams[i].cfg->size.height,\n> > > --\n> > > 2.40.1\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 4BF33BDB13\n\tfor ;\n\tTue, 1 Aug 2023 09:36:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA3E8627E9;\n\tTue, 1 Aug 2023 11:36:19 +0200 (CEST)","from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com\n\t[IPv6:2607:f8b0:4864:20::112b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F49D6037F\n\tfor ;\n\tTue, 1 Aug 2023 11:36:18 +0200 (CEST)","by mail-yw1-x112b.google.com with SMTP id\n\t00721157ae682-583ae4818c8so62046417b3.3\n\tfor ;\n\tTue, 01 Aug 2023 02:36:18 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690882579;\n\tbh=+Mky1vfu5xNO7d0HB9tCTQEo5kheNH5k2OJgtiUM/CY=;\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=IOHzHdNKUgnddY8zR31Uk7Mxj8BCBspxDetAwk8HR7tBKj5lJSNIC1lqJb+zNM6qF\n\t8EDqmF0eyDx6xLxC3PbrnI0aP0cOaVtdeXhO9Ry8nQRMwf12IFPC5iPaYtnzXTgAQT\n\tbIAQ4P8OTFreG6Di4h0sUHfiPB70QNbS7kTlzZJ5TKFTSd0HJfrmAC3/pC+w50TMTE\n\tMXEgYFieeVVv6mrtUjhkfGw/FNUX+JYmDht4zWjs6xYCqj8Udvh8yav3V/b6gvNpcD\n\tZt2y6X5HrqLkmAskIZte7+6d6/1xE9iR73mBZeZevszHWJv0gwsHhz+oQozGwMGdNn\n\tNzU99HfRDI2qQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1690882577; x=1691487377;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Si/O9ZZnSmj7fV0RfS/ylz+gnt0EWdyLwN559lii7tA=;\n\tb=WglikwgxgfJhpP5UrhYncEGq8VEmKGKMEy5GkpmwweANG7cZAB/J/CqyoYWNLgzOU5\n\tsW3Hbr06XFFtHTgw46KxpO9Mn+SOr6N7IUPuPHoWQ9YqdVwHPVnCJRZit6WzC+BMPVB1\n\toBTiu0N4CuPtquneX5ELZq3PAz+QPqekEYnsVd2s9QDEn77Y+fMUn9lLBuFckESzSoLi\n\tGl/SUG5UZq5xgt+3Ng79eItdzBHzcgo5umeUzc1mavBSGJ1DGQAIQ/6OnCqASBapS7VR\n\tQEfB+Q/rxqpAxzocPAv10lkk7SSAaAD/tRBgIumcgXcknlEVrMxCtRtyyJaNEYpvMhzY\n\tREMA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Wglikwgx\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690882577; x=1691487377;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Si/O9ZZnSmj7fV0RfS/ylz+gnt0EWdyLwN559lii7tA=;\n\tb=eiBMKzQUTCBCDr+HKlyyldzI00nAokcy3BbSOzZAGzzEnl1bdCg/ej/Umd/hPZ8n9G\n\ti4kdyTLf6PFY1Zlgx39epRfdW2V98M6TZto9MnGhe9AfI+vDcpfjvGSjrYCZXagVbD2G\n\tgwcWn2vB3mjCbZWHsWMpB7vnwgukgMCVIuoM5LXNTLaeqaD/+380XrYBeiL18JsK9ItC\n\ty/7ycRYFBPEMvFAwAi9O2JNIKeHetD5cy1JW5iqZ2jBkJVdNn1lTSVj8sXb4Iwp80TxG\n\tR/snCmVo5tK280g0gWh+puz059Fsbkdi9V6Bqi7Bt3DzJiuJ1BGlSJCkuYPW1C5qn7iZ\n\t6jXA==","X-Gm-Message-State":"ABy/qLZCN43iEjoDscv1JP4jx0+qDizBD8dDT/wIcLFkE+RVXB5xjHQY\n\tVvZcVHadFsxcL5IDjbqJuOds/n+Wxl/fw9Sv3LZZ/z9nZAM/miFecQbhsA==","X-Google-Smtp-Source":"APBJJlHPR1fU1nwj4iINup4sgj0jFYX7HcjkJ3yYRTVpZwLkxoxGTM46Ukf9ltFSzX9L6TeMeYNF2mAl3hoW5DERYt0=","X-Received":"by 2002:a0d:ef07:0:b0:56d:a2d:d08c with SMTP id\n\ty7-20020a0def07000000b0056d0a2dd08cmr13670673ywe.51.1690882576987;\n\tTue, 01 Aug 2023 02:36:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-5-jacopo.mondi@ideasonboard.com>\n\t\n\t<7u5utkxieovkjcfw2avgv6mtl4c2sownko2w24hmmwlb5oez7m@wfh3sbj7zitc>","In-Reply-To":"<7u5utkxieovkjcfw2avgv6mtl4c2sownko2w24hmmwlb5oez7m@wfh3sbj7zitc>","Date":"Tue, 1 Aug 2023 10:36:01 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27778,"web_url":"https://patchwork.libcamera.org/comment/27778/","msgid":"<169473124437.2843013.1095915319272620603@ping.linuxembedded.co.uk>","date":"2023-09-14T22:40:44","subject":"Re: [libcamera-devel] [PATCH v2 4/4] libcamera: rpi: Handle\n\tSensorConfiguration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-08-01 10:02:15)\n> Hi Jacopo,\n> \n> Thanks for the update. Just one minor comment:\n> \n> On Mon, 31 Jul 2023 at 12:31, Jacopo Mondi via libcamera-devel\n> wrote:\n> >\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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > .../pipeline/rpi/common/pipeline_base.cpp | 62 ++++++++++++++++---\n> > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > 3 files changed, 82 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 97acafbbb728..15c7a5c72bdd 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,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > if (config_.empty())\n> > return Invalid;\n> >\n> > + if (!sensorConfig.valid()) {\n> > + LOG(RPI, Error) << \"Invalid sensor configuration request\";\n> > + return Invalid;\n> > + }\n> > +\n> > status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> >\n> > /*\n> > @@ -207,19 +212,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > std::sort(outStreams.begin(), outStreams.end(),\n> > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> >\n> > - /* Compute the sensor configuration. */\n> > - unsigned int bitDepth = defaultRawBitDepth;\n> > - if (!rawStreams.empty()) {\n> > + /* Compute the sensor's format then do any platform specific fixups. */\n> > + unsigned int bitDepth;\n> > + Size sensorSize;\n> > +\n> > + if (sensorConfig) {\n> > + /* Use the application provided sensor configuration. */\n> > + bitDepth = sensorConfig.bitDepth;\n> > + sensorSize = sensorConfig.outputSize;\n> > + } else if (!rawStreams.empty()) {\n> > + /* Use the RAW stream format and size. */\n> > BayerFormat bayerFormat = BayerFormat::fromPixelFormat(rawStreams[0].cfg->pixelFormat);\n> > bitDepth = bayerFormat.bitDepth;\n> > + sensorSize = rawStreams[0].cfg->size;\n> > + } else {\n> > + bitDepth = defaultRawBitDepth;\n> > + sensorSize = outStreams[0].cfg->size;\n> > }\n> >\n> > - sensorFormat_ = data_->findBestFormat(rawStreams.empty() ? outStreams[0].cfg->size\n> > - : rawStreams[0].cfg->size,\n> > - bitDepth);\n> > + sensorFormat_ = data_->findBestFormat(sensorSize, bitDepth);\n> > +\n> > + /*\n> > + * If a sensor configuration has been requested, it should apply\n> > + * without modifications.\n> > + */\n> > + if (sensorConfig) {\n> > + BayerFormat bayer = BayerFormat::fromMbusCode(sensorFormat_.mbus_code);\n> > +\n> > + if (bayer.bitDepth != sensorConfig.bitDepth ||\n> > + sensorFormat_.size != sensorConfig.outputSize) {\n> > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > + << \"bitDepth/size mismatch\";\n> > + return Invalid;\n> > + }\n> > + }\n> >\n> > /* Do any platform specific fixups. */\n> > - status = data_->platformValidate(rawStreams, outStreams);\n> > + status = data_->platformValidate(this, rawStreams, outStreams);\n> > if (status == Invalid)\n> > return Invalid;\n> >\n> > @@ -467,12 +496,25 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > std::sort(ispStreams.begin(), ispStreams.end(),\n> > [](auto &l, auto &r) { return l.cfg->size > r.cfg->size; });\n> >\n> > - /* Apply the format on the sensor with any cached transform. */\n> > + /*\n> > + * Apply the format on the sensor with any cached transform.\n> > + *\n> > + * If the application has provided a sensor configuration apply it\n> > + * instead of just applying a format.\n> > + */\n> > const RPiCameraConfiguration *rpiConfig =\n> > static_cast(config);\n> > - V4L2SubdeviceFormat sensorFormat = rpiConfig->sensorFormat_;\n> > + V4L2SubdeviceFormat sensorFormat;\n> >\n> > - ret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> > + if (rpiConfig->sensorConfig) {\n> > + ret = data->sensor_->applyConfiguration(rpiConfig->sensorConfig,\n> > + rpiConfig->combinedTransform_,\n> > + &sensorFormat);\n> > + } else {\n> > + sensorFormat = rpiConfig->sensorFormat_;\n> > + ret = data->sensor_->setFormat(&sensorFormat,\n> > + rpiConfig->combinedTransform_);\n> > + }\n> > if (ret)\n> > return 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 a139c98a5a2b..0a795f4d2689 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> > V4L2VideoDevice *dev;\n> > };\n> >\n> > - virtual CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > + virtual CameraConfiguration::Status platformValidate(RPiCameraConfiguration *rpiConfig,\n> \n> Can the RPiCameraConfiguration *rpiConfig be made const in the declaration?\n> \n> Other than that:\n> Reviewed-by: Naushir Patuck \n\nI've got nothing additional to add so:\n\n\nReviewed-by: Kieran Bingham \n\n> \n> \n> > + std::vector &rawStreams,\n> > std::vector &outStreams) const = 0;\n> > virtual int platformConfigure(const V4L2SubdeviceFormat &sensorFormat,\n> > 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..bf864d4174b2 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> > {\n> > }\n> >\n> > - CameraConfiguration::Status platformValidate(std::vector &rawStreams,\n> > + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > + std::vector &rawStreams,\n> > std::vector &outStreams) const override;\n> >\n> > int platformPipelineConfigure(const std::unique_ptr &root) override;\n> > @@ -394,7 +395,8 @@ int PipelineHandlerVc4::platformRegister(std::unique_ptr &camer\n> > return 0;\n> > }\n> >\n> > -CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector &rawStreams,\n> > +CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig,\n> > + std::vector &rawStreams,\n> > std::vector &outStreams) const\n> > {\n> > CameraConfiguration::Status status = CameraConfiguration::Status::Valid;\n> > @@ -405,9 +407,29 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > return CameraConfiguration::Status::Invalid;\n> > }\n> >\n> > - if (!rawStreams.empty())\n> > + if (!rawStreams.empty()) {\n> > rawStreams[0].dev = unicam_[Unicam::Image].dev();\n> >\n> > + /* Adjust the RAW stream to match the requested sensor config. */\n> > + if (rpiConfig->sensorConfig) {\n> > + StreamConfiguration *rawStream = rawStreams[0].cfg;\n> > + BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> > +\n> > + /* Handle flips to make sure to match the RAW stream format. */\n> > + if (flipsAlterBayerOrder_)\n> > + rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> > + PixelFormat rawFormat = rawBayer.toPixelFormat();\n> > +\n> > + if (rawStream->pixelFormat != rawFormat ||\n> > + rawStream->size != rpiConfig->sensorConfig.outputSize) {\n> > + rawStream->pixelFormat = rawFormat;\n> > + rawStream->size = rpiConfig->sensorConfig.outputSize;\n> > +\n> > + status = CameraConfiguration::Adjusted;\n> > + }\n> > + }\n> > + }\n> > +\n> > /*\n> > * For the two ISP outputs, one stream must be equal or smaller than the\n> > * other in all dimensions.\n> > @@ -417,6 +439,8 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(std::vector > for (unsigned int i = 0; i < outStreams.size(); i++) {\n> > Size size;\n> >\n> > + /* \\todo Warn if upscaling: reduces image quality. */\n> > +\n> > size.width = std::min(outStreams[i].cfg->size.width,\n> > outStreams[0].cfg->size.width);\n> > size.height = std::min(outStreams[i].cfg->size.height,\n> > --\n> > 2.40.1\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 62014BD160\n\tfor ;\n\tThu, 14 Sep 2023 22:40:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DBF962919;\n\tFri, 15 Sep 2023 00:40:48 +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 8E85E62907\n\tfor ;\n\tFri, 15 Sep 2023 00:40:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85D4118A1;\n\tFri, 15 Sep 2023 00:39:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694731248;\n\tbh=xA030iVRyl7WWCmJpI9+QPnQglGxdtYv6ydoMQswfQA=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=4N/n8r5RR+Ql9z00JfmZ2i2xSpA/WoKq9N4k6fw6AJgCtM847gcIiFcLV1sKyQcg0\n\tWc8DzCNm5C4jAxOKVvxs9aR38o2j01ai0K6Q8bKkghjcKaMZ7bBfC8DU4Ql0hLZclu\n\tvG6sXEA82mCYNOjoc4YOK/Pi5pWq1CvGdc7zmrGjWjHHi7v4xu73gigpmaVzwdp2bx\n\tUZmJvUOaQ3qGc53AaJUSLWhnmXJnU+bSSxDPnJJScpRZ17gfa2mO/WM1UGSYHsXU/I\n\tfBBFmZZxgYTcZcwNhHioknSl4ZTe0Q+PTKttXwnzCTeLnex9Gc7zt1A12hiKGLxJVk\n\tsst3vWcDv0o6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694731154;\n\tbh=xA030iVRyl7WWCmJpI9+QPnQglGxdtYv6ydoMQswfQA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QYpQjo/PeA+UQIFYxZbRC6TeM0VBEYZFI+gNA9UtaW/d9MpIDQTf72PqjOmoyvc1f\n\tMK1qiVjsZzDhWR7Ab3JHXLHxzvXkgBnFcjBbi7kNAzw+wUrqXLelb33sUbDrwIF4r1\n\tLuS1GxduPvtaVxJ+CPssSBba0xsU0K32ahPB4O4o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QYpQjo/P\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230731113115.5915-1-jacopo.mondi@ideasonboard.com>\n\t<20230731113115.5915-5-jacopo.mondi@ideasonboard.com>\n\t","To":"Jacopo Mondi ,\n\tNaushir Patuck ,\n\tNaushir Patuck via libcamera-devel ","Date":"Thu, 14 Sep 2023 23:40:44 +0100","Message-ID":"<169473124437.2843013.1095915319272620603@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]