[{"id":27615,"web_url":"https://patchwork.libcamera.org/comment/27615/","msgid":"","date":"2023-07-26T09:52:59","subject":"Re: [libcamera-devel] [PATCH 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\nThank you for your work.\n\nOn Mon, 24 Jul 2023 at 13:39, 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 | 64 +++++++++++++++----\n> .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> 3 files changed, 83 insertions(+), 15 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 e0fbeec37fe9..574e003402df 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\nWe don't necessarily need to do this in the case where sensorConfig is present\nright? But I suppose it's a good way to do a validation on the values in\nsensorConfig, so no harm in keeping it.\n\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>\n> - /* Do any platform specific fixups. */\n> - status = data_->platformValidate(rawStreams, outStreams);\n> + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> + << \"bitDepth/size mismatch\";\n> + return Invalid;\n> + }\n> + }\n> +\n> + status = data_->platformValidate(this, rawStreams, outStreams);\n\nMinor nit, we lost the comment above this statement.\n\n\n> if (status == Invalid)\n> return Invalid;\n>\n> @@ -466,12 +495,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> + 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\nI wonder if we can make some simplifications here? The block of code that\nhandles bayer orders and transforms already happens in\nRPiCameraConfiguration::validate()\n(done there to avoid derived pipeline handlers from repeating the same\nconstruct). Could it be simply removed from here without any loss of\nfunctionality?\n\nIf the answer is yes, can I suggest another change - instead of passing in\n*rpiConfig to this function, we can pass in sensorFormat_? This ought to be\nequivalent I think? In fact, a completely unrelated change that I'm working on\npasses in sensorFormat_ (or really just a V4L2SubdeviceFormat &), so this may\nbe ideal :)\n\nRegards,\nNaush\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 A52B7BDC71\n\tfor ;\n\tWed, 26 Jul 2023 09:53:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACAFF628BB;\n\tWed, 26 Jul 2023 11:53:16 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60B10628BA\n\tfor ;\n\tWed, 26 Jul 2023 11:53:15 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id\n\t3f1490d57ef6-d075a831636so5078172276.3\n\tfor ;\n\tWed, 26 Jul 2023 02:53:15 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690365196;\n\tbh=q4bXk+ftlvi+hdKmi2RmaGOXiIRMt77wnfxGoYtWvY8=;\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=LWCdGSP0xT5T4YFtQRRtLUX9CyMvrxLMNb21cmVIjk3IQ4wjMxNw6DTnQ6B+Id8uy\n\tSUHcdbhyQbK2lIyslTC2++KjRQYUP+eFoMzdU8hX+Yh+Djz8HNEGYFEyS+aB0LdLsS\n\t/ANUtWCaAyvkTZASKBsGjIEFUrracPsjGHzIg1KUnx7wZuwgnNdIWNLLQsc/9YprCx\n\tSPrxnd8PJQ9761muKVZHM7wglOWnaQPbTTp1V9b8VQ7Huqbvda8I6dK8H1NVCtyOk0\n\tycgJnz52ZfejKhgJSsz4naWZvD/QFLRf5j5E8iSV9Y+Mk7f+8ySqzOfG2+13k3mY6y\n\tYQjftcnuF1wSw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1690365194; x=1690969994;\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=8Ofg3eUjuO7AfOzYZsCeBd3taNbKk1NliPSXlMp/4bA=;\n\tb=niNNNqS/qdME63DscAiv/rNoO8CIacVL1zlCI4Q9Mtb5afKlj4/VjSzskhk/jiuYRh\n\tHSqJeF1+3To/311O657eVYoIm+N3RqnYEFhceAAixHFQZOEESpRcgc+/z5rebcdCyJY2\n\t+AcFHA/fGD/RYtecTn9IZAiV0rr2OOtHUWAefHkeaV7bf4u5twC8mwr+mgIs3E0lOQwW\n\tR/011XMxPf2sd/1QGYa6HKkPg9158cvznZ0R9rR+3qN2u+AmiL8fHfg7P2cIArYqDMxY\n\tC8JiPKA2tuSW0E/R7szp6+ClOJE8GPtcksDvQXY+9xzsuiu9b8fQ9Rgti28mCiR6Qgq1\n\t933A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"niNNNqS/\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690365194; x=1690969994;\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=8Ofg3eUjuO7AfOzYZsCeBd3taNbKk1NliPSXlMp/4bA=;\n\tb=MbJkv71F2aCwM7DhYHRkq9DoZQ96k3IkEGO2v8ebrImKMWVln5wqjCaouGs2eSHmsj\n\tkxWZDUNmeDTEEVTGNJSDcNmuYjQRbfNXUGJ0Dk4Vr+gIjm5F8KAAfZhrL2DA88noLQev\n\tk0F6izyB1E7OeMtkEqJLgfT+7PALBZNEv5KEXLJ7nmt6pUcNOewtAcxf7V5Gv6+rh8/t\n\toYbgbvD7kBAr0H3ftUJTteR9+cp9pCiYy4zofWbEshZyV0LOdQ64ARqGKpgDSS1mgmBJ\n\tpwafkbuYKlmbao8xCFZndt02bQcpkG22bu2/dC9IALe77dH/R4sCq8Q7+o8K88QvKGmc\n\tri/g==","X-Gm-Message-State":"ABy/qLYAILcCq+7Es/saJzxYsL66GvOi5sOZDTYDLb78FDsusi/3gxlF\n\trExqgEUtbvrqG0lXqycllHGfA94iZvMG7kdUwv1fEr0Wn7yT2SPXhtvBQA==","X-Google-Smtp-Source":"APBJJlG8Eaf+yve9NCBGZCz8HZuJLy9LnZNbSQlOJh2GpeWQt2s0PpQIeIpjQMtb9SN3bJDYsBzyEziNg6boHI5tc8E=","X-Received":"by 2002:a0d:ea0d:0:b0:56d:308f:1efa with SMTP id\n\tt13-20020a0dea0d000000b0056d308f1efamr1734535ywe.4.1690365194143;\n\tWed, 26 Jul 2023 02:53:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20230724123907.29086-1-jacopo.mondi@ideasonboard.com>\n\t<20230724123907.29086-5-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230724123907.29086-5-jacopo.mondi@ideasonboard.com>","Date":"Wed, 26 Jul 2023 10:52:59 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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":27617,"web_url":"https://patchwork.libcamera.org/comment/27617/","msgid":"","date":"2023-07-27T08:13:03","subject":"Re: [libcamera-devel] [PATCH 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 Wed, Jul 26, 2023 at 10:52:59AM +0100, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> Thank you for your work.\n>\n> On Mon, 24 Jul 2023 at 13:39, 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 | 64 +++++++++++++++----\n> > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > 3 files changed, 83 insertions(+), 15 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 e0fbeec37fe9..574e003402df 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> We don't necessarily need to do this in the case where sensorConfig is present\n> right? But I suppose it's a good way to do a validation on the values in\n> sensorConfig, so no harm in keeping it.\n>\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> >\n> > - /* Do any platform specific fixups. */\n> > - status = data_->platformValidate(rawStreams, outStreams);\n> > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > + << \"bitDepth/size mismatch\";\n> > + return Invalid;\n> > + }\n> > + }\n> > +\n> > + status = data_->platformValidate(this, rawStreams, outStreams);\n>\n> Minor nit, we lost the comment above this statement.\n>\n>\n> > if (status == Invalid)\n> > return Invalid;\n> >\n> > @@ -466,12 +495,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> > + 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> I wonder if we can make some simplifications here? The block of code that\n> handles bayer orders and transforms already happens in\n> RPiCameraConfiguration::validate()\n> (done there to avoid derived pipeline handlers from repeating the same\n> construct). Could it be simply removed from here without any loss of\n> functionality?\n\nAre you sure we're actually doing the same thing ?\n\nAs I understand it, the logic implemented here makes sure that the RAW\nStreamConfiguration matches the requested SensorConfiguration without\nactually testing it against the CSI-2 receiver video device.\n\nWhile in RPiCameraConfiguration::validate() we take the RAW\nStreamConfiguration, regardless of the SensorConfig, and create a\nV4L2DeviceFormat from it (using the media bus code from the sensor\nformat) and try to apply it to the CSI-2 receiver video device,\nadjusting the RAW StreamConfiguration according to the result of\napplying the format to the video device.\n\nWhen it comes to the bayer order handling are you referring to:\n\n /* Handle flips to make sure to match the RAW stream format. */\n if (flipsAlterBayerOrder_)\n rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n\nIf that's your actual concern ?\n\nIf we don't do it here, do we risk to set to Adjusted a correct\nconfiguration from user ?\n\nLet's assume your sensor is natively SRGGB10 and you have H/VFLIP\nenabled, so that the actual output format is BGGR10.\n\nYou receive a CameraConfiguration with a RAW stream with\n\n { PixelFormat::SBGGR10, 1080p }\n\nAnd a SensorConfiguration = { 10, 1080p }\n\nIn RPiCameraConfiguration::validate() we compute the 'best' sensor\nformat with CameraData::findBestFormat(). At this point (and here I'm\ngoing from memory so please confirm) flips are not activated, so you\nget back a SubdeviceFormat with the native SRGGB10 ordering.\n\nThen we enter platformValidate() with\n\n sensorFormat_ = { MEDIA_BUS_SRGGB10, 1080p }\n\nand we get to\n BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n\nlet's assume we don't do the above check for if\n(flipsAlterBayerOrder_) and we'll end up with\n\n if (rawStream->pixelFormat != rawFormat)\n\nand then we Adjust the RAW stream to PixelFormat::SRGGB10.\n\nThen we're back to RPiCameraConfiguration::validate(). We enter the\n\"Further fixup on RAW streams\" loop. We apply SRGGB10 to the CSI-2\nreceiver but here we notice that flipsAlterBayerOrder_. So we go and\nadjust back the RAW stream configuration to SBGGR10 and we mark it\nagain to Adjusted, while it is what the application has actually asked\nfor!\n\nIs my understanding correct here ?\n\n>\n> If the answer is yes, can I suggest another change - instead of passing in\n> *rpiConfig to this function, we can pass in sensorFormat_? This ought to be\n> equivalent I think? In fact, a completely unrelated change that I'm working on\n> passes in sensorFormat_ (or really just a V4L2SubdeviceFormat &), so this may\n> be ideal :)\n>\n> Regards,\n> Naush\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 1A069BDC71\n\tfor ;\n\tThu, 27 Jul 2023 08:13:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51704628C0;\n\tThu, 27 Jul 2023 10:13:10 +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 91B0861E24\n\tfor ;\n\tThu, 27 Jul 2023 10:13:08 +0200 (CEST)","from ideasonboard.com (mob-5-91-19-250.net.vodafone.it\n\t[5.91.19.250])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C52432E4;\n\tThu, 27 Jul 2023 10:12:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690445590;\n\tbh=/3+5G+2qLuvuijyJ8WqJLiE7sQflG8rio4gESigwdLY=;\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=VPVNLj+BGw9aE40YfUjafFt4CKNhVxsuWycRKG7ENLrEe42UC1vdxumlxEuHu+QQd\n\tujtvMDdsnUKnEOOyKpPW7xgB41M25jg+qT2EMacWlcbNVGrxzQ2bVX/5pEwBKdsThk\n\twKtL2hrAykLmDNLzGb5Khn5lijHNlDxqhgfvG/zrNZzfsDH7Lce89Eh/DxjD3pmie2\n\tHEWaL757Zud7/x47qcM9IPlVv6vvc8nTLdHIhMoR37ivJqx7xFB8x96N7R423rcsXP\n\tIhPpSI9xHHTYqOrtsheCg/V2CRGW28vqfJ7voqnbopnVd44sBLjWzK/n3cSrbcYmWb\n\tLGloCb4h9rPMQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1690445529;\n\tbh=/3+5G+2qLuvuijyJ8WqJLiE7sQflG8rio4gESigwdLY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EcqAohJ/rNOaTm57s0gV80zdCt+VMm2rO6e5IWZO1A/bANx6QDHyHNtGijpUBqLQl\n\tioE73GpLc1wsh66YlHQX6hqYEu6gVAY3Cp6Ekex6J7ROWo8kIuRalWfSxo+bFRdse3\n\t96w0c+4hHqox5YK8O9JkqJR/vwC9Vwr6xksuqRGI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EcqAohJ/\"; dkim-atps=neutral","Date":"Thu, 27 Jul 2023 10:13:03 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230724123907.29086-1-jacopo.mondi@ideasonboard.com>\n\t<20230724123907.29086-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 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":27624,"web_url":"https://patchwork.libcamera.org/comment/27624/","msgid":"","date":"2023-07-27T14:31:20","subject":"Re: [libcamera-devel] [PATCH 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 Thu, 27 Jul 2023 at 09:13, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Wed, Jul 26, 2023 at 10:52:59AM +0100, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for your work.\n> >\n> > On Mon, 24 Jul 2023 at 13:39, 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 | 64 +++++++++++++++----\n> > > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > > 3 files changed, 83 insertions(+), 15 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 e0fbeec37fe9..574e003402df 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> > We don't necessarily need to do this in the case where sensorConfig is present\n> > right? But I suppose it's a good way to do a validation on the values in\n> > sensorConfig, so no harm in keeping it.\n> >\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> > >\n> > > - /* Do any platform specific fixups. */\n> > > - status = data_->platformValidate(rawStreams, outStreams);\n> > > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > > + << \"bitDepth/size mismatch\";\n> > > + return Invalid;\n> > > + }\n> > > + }\n> > > +\n> > > + status = data_->platformValidate(this, rawStreams, outStreams);\n> >\n> > Minor nit, we lost the comment above this statement.\n> >\n> >\n> > > if (status == Invalid)\n> > > return Invalid;\n> > >\n> > > @@ -466,12 +495,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> > > + 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> > I wonder if we can make some simplifications here? The block of code that\n> > handles bayer orders and transforms already happens in\n> > RPiCameraConfiguration::validate()\n> > (done there to avoid derived pipeline handlers from repeating the same\n> > construct). Could it be simply removed from here without any loss of\n> > functionality?\n>\n> Are you sure we're actually doing the same thing ?\n>\n> As I understand it, the logic implemented here makes sure that the RAW\n> StreamConfiguration matches the requested SensorConfiguration without\n> actually testing it against the CSI-2 receiver video device.\n>\n> While in RPiCameraConfiguration::validate() we take the RAW\n> StreamConfiguration, regardless of the SensorConfig, and create a\n> V4L2DeviceFormat from it (using the media bus code from the sensor\n> format) and try to apply it to the CSI-2 receiver video device,\n> adjusting the RAW StreamConfiguration according to the result of\n> applying the format to the video device.\n\nSorry, I should have been clearer - I was only referring to the Bayer order\nhandling below, not the whole block of code.\n\n>\n> When it comes to the bayer order handling are you referring to:\n>\n> /* Handle flips to make sure to match the RAW stream format. */\n> if (flipsAlterBayerOrder_)\n> rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n>\n> If that's your actual concern ?\n\nYes, this is the bit that I'm wondering if we can remove.\n\n>\n> If we don't do it here, do we risk to set to Adjusted a correct\n> configuration from user ?\n>\n> Let's assume your sensor is natively SRGGB10 and you have H/VFLIP\n> enabled, so that the actual output format is BGGR10.\n>\n> You receive a CameraConfiguration with a RAW stream with\n>\n> { PixelFormat::SBGGR10, 1080p }\n>\n> And a SensorConfiguration = { 10, 1080p }\n>\n> In RPiCameraConfiguration::validate() we compute the 'best' sensor\n> format with CameraData::findBestFormat(). At this point (and here I'm\n> going from memory so please confirm) flips are not activated, so you\n> get back a SubdeviceFormat with the native SRGGB10 ordering.\n\nWith you so far.\n\n>\n> Then we enter platformValidate() with\n>\n> sensorFormat_ = { MEDIA_BUS_SRGGB10, 1080p }\n>\n> and we get to\n> BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n>\n> let's assume we don't do the above check for if\n> (flipsAlterBayerOrder_) and we'll end up with\n>\n> if (rawStream->pixelFormat != rawFormat)\n>\n> and then we Adjust the RAW stream to PixelFormat::SRGGB10.\n>\n> Then we're back to RPiCameraConfiguration::validate(). We enter the\n> \"Further fixup on RAW streams\" loop. We apply SRGGB10 to the CSI-2\n> receiver but here we notice that flipsAlterBayerOrder_. So we go and\n> adjust back the RAW stream configuration to SBGGR10 and we mark it\n> again to Adjusted, while it is what the application has actually asked\n> for!\n>\n> Is my understanding correct here ?\n\nYes, I agree with your sequencing and final outcome here.\n\nI guess my thinking here was that in Vc4CameraData::platformValidate we would\nnot ever touch cfg->pixelFormat. So no if (flipsAlterBayerOrder_) clause, and\nno if (rawStream->pixelFormat != rawFormat) test.\n\nFrom your example above, in \"Further fixup on RAW streams\" loop, rawFormat\nwould be BGGR10, then after the if (data_->flipsAlterBayerOrder_), we remain at\nBGGR10 and this would not signal Adjusted. I think that's what would happen,\nbut my head hurts too much thinking about it!\n\nMaybe the simplification I'm thinking of will not work for all circumstances,\nand your change is more complete and explicit. But I wonder then, should we\nremove the if (data_->flipsAlterBayerOrder_) test in the \"Further fixup on RAW\nstreams\" loop instead?\n\nRegards,\nNaush\n\n>\n> >\n> > If the answer is yes, can I suggest another change - instead of passing in\n> > *rpiConfig to this function, we can pass in sensorFormat_? This ought to be\n> > equivalent I think? In fact, a completely unrelated change that I'm working on\n> > passes in sensorFormat_ (or really just a V4L2SubdeviceFormat &), so this may\n> > be ideal :)\n> >\n> > Regards,\n> > Naush\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 12D92BDC71\n\tfor ;\n\tThu, 27 Jul 2023 14:31:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56433628C0;\n\tThu, 27 Jul 2023 16:31:37 +0200 (CEST)","from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com\n\t[IPv6:2607:f8b0:4864:20::1132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1783C600F7\n\tfor ;\n\tThu, 27 Jul 2023 16:31:36 +0200 (CEST)","by mail-yw1-x1132.google.com with SMTP id\n\t00721157ae682-579de633419so11065077b3.3\n\tfor ;\n\tThu, 27 Jul 2023 07:31:35 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690468297;\n\tbh=939cVCb6sQAGHt5zF1qQClwQmdJ7VoXzi5ffFPcAaSc=;\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=xr88G1KjBbKfjQyt0oMJeoHQrPo0bNPB2HAttetCv3kDFMOqhFVkAhoEIur5B/LkS\n\tvlP6mVbF7EyX9P4kFVPqbuwQ7o6Xc31PIqo5IRRs/4ge/9tfqqtdpVM7cRLwzYNks5\n\tlviFv4y//03R2uJIEWqH/C1BBiKYxXxsBedUmpZzOF0RIzCiPhqsmkfk6wfuq4bWBJ\n\tkRruv5KbFg51XgB8BukXoIRwpOFAS6djQtmzjFogpe2iW94DciKcxwtZ6ra5UKoXsb\n\teBcMyHRrako5OtIAy8vAR+aSvfPPch06BOHDNaCABQhfPU1pi5SV0yf8O0WjEc8ORQ\n\tFM2nbGAbERMOQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1690468295; x=1691073095;\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=bsEGLi4KtLtkdQE0TrFBBmsGNdTZh57RGzwLjM27ozc=;\n\tb=Na0wH1EcUcJIsm1bbu+wZk2sGjgU+HxYL80g0JKjd/Ht2ynBqIX+qkGoQTmKRQRYb0\n\tIdK5rI0HvUIQUtxaIgHgkqyTnr4Q42TZ0kFbN/N6WyYJJbQD0IxUAmbo3eLBD+9OXlRA\n\tQy6d80VJQVa8bCR21IVlOjSgzbsl6jMtJnK1v8IPrMIuhPiViqU8qcncoDR0Wh7QKjeg\n\t2LKfUJW60pOuHafN+U4MP4lOaDVbEHjNP+j7z6pgSjSmFhYRx0QSTpbindRZg6z0+3ZX\n\t7FHbYSL9Vtjap4SQy48/P78771F5WcxUsKHZ/Mn3+P78ZtRHEWIqxvdXwVc1+3qpoZBp\n\tzkVg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Na0wH1Ec\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690468295; x=1691073095;\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=bsEGLi4KtLtkdQE0TrFBBmsGNdTZh57RGzwLjM27ozc=;\n\tb=RBiEh2FZCx/7MMN4gRL/XUzXmLIEX0OIW56XD8rDk20qYcN31kuCClojLGN20hTww9\n\t7xGGRZ+baJj2Z6heFskDU8sprrwTIcOAEdpZO9Yl5sAuI18aE0PZMnDZhiAmny+Bqraw\n\tQN58d2/LPGHCycXLqyCwBptE0E/Xn45uNrzAogNwQIIA/Os5yzmeA0SFABMVOdViaVo/\n\tIErh5ZVvj8vbTNf0M/AEozDKDakzpyAo7wiflJmVmAfjBBQJmjGjm4gvZvL59T5QoS3z\n\tG7Ad9eiYixHbUIL8Ju1DLYIhB6Vu/tRQZbvvJJqp1sz6d+s+3gE7AOBqkDVUXYgvjhEd\n\tW1Hg==","X-Gm-Message-State":"ABy/qLZdFs0xLlQXAjnYXHjwDN1XryheI+zCIVMYtO+cpt0ztPg6GVuj\n\t+Cc3H54S5wBo+3m1jOWvpNbbTKImCJNCVqzos9ez/ejYQGrEUuymKcTX9Q==","X-Google-Smtp-Source":"APBJJlGhOKySnHZyKDYjbxjKcpaGk1LH5DtTAtdFaNVtWIa9SfOUGt653CSqz4tKSrkWfnr76NS7LCFSY/zZFT0wJ4Q=","X-Received":"by 2002:a81:6dd5:0:b0:570:899f:3a52 with SMTP id\n\ti204-20020a816dd5000000b00570899f3a52mr5521746ywc.35.1690468294673;\n\tThu, 27 Jul 2023 07:31:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20230724123907.29086-1-jacopo.mondi@ideasonboard.com>\n\t<20230724123907.29086-5-jacopo.mondi@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Date":"Thu, 27 Jul 2023 15:31:20 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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":27635,"web_url":"https://patchwork.libcamera.org/comment/27635/","msgid":"","date":"2023-07-31T08:36:59","subject":"Re: [libcamera-devel] [PATCH 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 Thu, Jul 27, 2023 at 03:31:20PM +0100, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Thu, 27 Jul 2023 at 09:13, Jacopo Mondi\n> wrote:\n> >\n> > Hi Naush\n> >\n> > On Wed, Jul 26, 2023 at 10:52:59AM +0100, Naushir Patuck wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for your work.\n> > >\n> > > On Mon, 24 Jul 2023 at 13:39, 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 | 64 +++++++++++++++----\n> > > > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > > > 3 files changed, 83 insertions(+), 15 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 e0fbeec37fe9..574e003402df 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> > > We don't necessarily need to do this in the case where sensorConfig is present\n> > > right? But I suppose it's a good way to do a validation on the values in\n> > > sensorConfig, so no harm in keeping it.\n> > >\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> > > >\n> > > > - /* Do any platform specific fixups. */\n> > > > - status = data_->platformValidate(rawStreams, outStreams);\n> > > > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > > > + << \"bitDepth/size mismatch\";\n> > > > + return Invalid;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + status = data_->platformValidate(this, rawStreams, outStreams);\n> > >\n> > > Minor nit, we lost the comment above this statement.\n> > >\n> > >\n> > > > if (status == Invalid)\n> > > > return Invalid;\n> > > >\n> > > > @@ -466,12 +495,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> > > > + 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> > > I wonder if we can make some simplifications here? The block of code that\n> > > handles bayer orders and transforms already happens in\n> > > RPiCameraConfiguration::validate()\n> > > (done there to avoid derived pipeline handlers from repeating the same\n> > > construct). Could it be simply removed from here without any loss of\n> > > functionality?\n> >\n> > Are you sure we're actually doing the same thing ?\n> >\n> > As I understand it, the logic implemented here makes sure that the RAW\n> > StreamConfiguration matches the requested SensorConfiguration without\n> > actually testing it against the CSI-2 receiver video device.\n> >\n> > While in RPiCameraConfiguration::validate() we take the RAW\n> > StreamConfiguration, regardless of the SensorConfig, and create a\n> > V4L2DeviceFormat from it (using the media bus code from the sensor\n> > format) and try to apply it to the CSI-2 receiver video device,\n> > adjusting the RAW StreamConfiguration according to the result of\n> > applying the format to the video device.\n>\n> Sorry, I should have been clearer - I was only referring to the Bayer order\n> handling below, not the whole block of code.\n>\n> >\n> > When it comes to the bayer order handling are you referring to:\n> >\n> > /* Handle flips to make sure to match the RAW stream format. */\n> > if (flipsAlterBayerOrder_)\n> > rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> >\n> > If that's your actual concern ?\n>\n> Yes, this is the bit that I'm wondering if we can remove.\n>\n> >\n> > If we don't do it here, do we risk to set to Adjusted a correct\n> > configuration from user ?\n> >\n> > Let's assume your sensor is natively SRGGB10 and you have H/VFLIP\n> > enabled, so that the actual output format is BGGR10.\n> >\n> > You receive a CameraConfiguration with a RAW stream with\n> >\n> > { PixelFormat::SBGGR10, 1080p }\n> >\n> > And a SensorConfiguration = { 10, 1080p }\n> >\n> > In RPiCameraConfiguration::validate() we compute the 'best' sensor\n> > format with CameraData::findBestFormat(). At this point (and here I'm\n> > going from memory so please confirm) flips are not activated, so you\n> > get back a SubdeviceFormat with the native SRGGB10 ordering.\n>\n> With you so far.\n>\n> >\n> > Then we enter platformValidate() with\n> >\n> > sensorFormat_ = { MEDIA_BUS_SRGGB10, 1080p }\n> >\n> > and we get to\n> > BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> >\n> > let's assume we don't do the above check for if\n> > (flipsAlterBayerOrder_) and we'll end up with\n> >\n> > if (rawStream->pixelFormat != rawFormat)\n> >\n> > and then we Adjust the RAW stream to PixelFormat::SRGGB10.\n> >\n> > Then we're back to RPiCameraConfiguration::validate(). We enter the\n> > \"Further fixup on RAW streams\" loop. We apply SRGGB10 to the CSI-2\n> > receiver but here we notice that flipsAlterBayerOrder_. So we go and\n> > adjust back the RAW stream configuration to SBGGR10 and we mark it\n> > again to Adjusted, while it is what the application has actually asked\n> > for!\n> >\n> > Is my understanding correct here ?\n>\n> Yes, I agree with your sequencing and final outcome here.\n>\n> I guess my thinking here was that in Vc4CameraData::platformValidate we would\n> not ever touch cfg->pixelFormat. So no if (flipsAlterBayerOrder_) clause, and\n> no if (rawStream->pixelFormat != rawFormat) test.\n>\n> From your example above, in \"Further fixup on RAW streams\" loop, rawFormat\n> would be BGGR10, then after the if (data_->flipsAlterBayerOrder_), we remain at\n> BGGR10 and this would not signal Adjusted. I think that's what would happen,\n> but my head hurts too much thinking about it!\n>\n> Maybe the simplification I'm thinking of will not work for all circumstances,\n> and your change is more complete and explicit. But I wonder then, should we\n> remove the if (data_->flipsAlterBayerOrder_) test in the \"Further fixup on RAW\n> streams\" loop instead?\n\nWith your pending patch \"[PATCH] pipeline: rpi: Don't call\ntoV4L2DeviceFormat() from validate()\" that removes\n\n rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, subdevFormat, packing);\n\nin favour of\n\n rawFormat.fourcc = raw.dev->toV4L2PixelFormat(cfg.pixelFormat);\n\nwe use the pixelformat for the raw stream as computed in\nplatformValidate(), you're right, so on can presume there is no need\nto re-check for the Bayer ordering in validate() as it has been corrected in\nplatformValidate().\n\nBut be aware that the StreamConfiguration PixelFormat only gets\nchanged when there is a SensorConfig, not in all cases, so I'm afraid\nwe need to keep the:\n\n if (data->flipsAlterBayerOrder_)\n\ntest in the \"Further fixup\" loop in validate().\n\nDo you agree ?\n\n>\n> Regards,\n> Naush\n>\n> >\n> > >\n> > > If the answer is yes, can I suggest another change - instead of passing in\n> > > *rpiConfig to this function, we can pass in sensorFormat_? This ought to be\n> > > equivalent I think? In fact, a completely unrelated change that I'm working on\n> > > passes in sensorFormat_ (or really just a V4L2SubdeviceFormat &), so this may\n> > > be ideal :)\n> > >\n> > > Regards,\n> > > Naush\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 3E9B5BDB13\n\tfor ;\n\tMon, 31 Jul 2023 08:37:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B8F1627EC;\n\tMon, 31 Jul 2023 10:37:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1B1461E1C\n\tfor ;\n\tMon, 31 Jul 2023 10:37:03 +0200 (CEST)","from ideasonboard.com (mob-5-90-53-43.net.vodafone.it [5.90.53.43])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 45CBE2E4;\n\tMon, 31 Jul 2023 10:36:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690792625;\n\tbh=JaRCsdxJCIzj6jaQdVPTWh+EXSEhDk0kbdV7GWaHzLs=;\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=1hlc77EDCwi8kpFwAUc/uI+beAb4VGKIGd5LImgyTPvrJQo86Fz0+WpwE5dS9Ak7O\n\tYLwOY/TQVAoidXgs4GVJir/zuGJAUYjNn7Q6k4wHY8PuA7C/k2oWalMQfFrPVl3f3i\n\txNW8Z/GfaWS1tzFWUOhDBM7l6Tb4nB0NWB3ni0GEhocUE5vdRe4UdnT1MOo+BoZVva\n\t0yV76/C9cl2hvLm6CVddWV2vQ4EKQpwh9p4Yf6eLhq9PaiQAN6jANGY2/iadXY7T/c\n\tW3n+GbMpN+2D+rnSPr2X37MY2Bo1wLOSSGz2jyIgQEZLWvrNMpydqV0KsXosF39i8m\n\t5Swx3ggCRwmfw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1690792561;\n\tbh=JaRCsdxJCIzj6jaQdVPTWh+EXSEhDk0kbdV7GWaHzLs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wj8PSVWE4LlJc//Hrm4PtAkXpzkjvPsNz5cjGTazWqLWicVjhCk+iC/ZJy5YHzrQ/\n\tekAnoWqx9tLu8NoClFJSTHcuHQJQGCdy+DiYzQneRpqwjyRvBW6o8eLk5GakBVnlTE\n\tzk4v1N8ZP80S5Qm8H4jt7aw22HeFvXyoGld8j+88="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wj8PSVWE\"; dkim-atps=neutral","Date":"Mon, 31 Jul 2023 10:36:59 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230724123907.29086-1-jacopo.mondi@ideasonboard.com>\n\t<20230724123907.29086-5-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 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":27636,"web_url":"https://patchwork.libcamera.org/comment/27636/","msgid":"","date":"2023-07-31T09:42:55","subject":"Re: [libcamera-devel] [PATCH 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\n\nOn Mon, 31 Jul 2023 at 09:37, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Thu, Jul 27, 2023 at 03:31:20PM +0100, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > On Thu, 27 Jul 2023 at 09:13, Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Naush\n> > >\n> > > On Wed, Jul 26, 2023 at 10:52:59AM +0100, Naushir Patuck wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for your work.\n> > > >\n> > > > On Mon, 24 Jul 2023 at 13:39, 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 | 64 +++++++++++++++----\n> > > > > .../pipeline/rpi/common/pipeline_base.h | 4 +-\n> > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 30 ++++++++-\n> > > > > 3 files changed, 83 insertions(+), 15 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 e0fbeec37fe9..574e003402df 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> > > > We don't necessarily need to do this in the case where sensorConfig is present\n> > > > right? But I suppose it's a good way to do a validation on the values in\n> > > > sensorConfig, so no harm in keeping it.\n> > > >\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> > > > >\n> > > > > - /* Do any platform specific fixups. */\n> > > > > - status = data_->platformValidate(rawStreams, outStreams);\n> > > > > + LOG(RPI, Error) << \"Invalid sensor configuration: \"\n> > > > > + << \"bitDepth/size mismatch\";\n> > > > > + return Invalid;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + status = data_->platformValidate(this, rawStreams, outStreams);\n> > > >\n> > > > Minor nit, we lost the comment above this statement.\n> > > >\n> > > >\n> > > > > if (status == Invalid)\n> > > > > return Invalid;\n> > > > >\n> > > > > @@ -466,12 +495,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> > > > > + 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> > > > I wonder if we can make some simplifications here? The block of code that\n> > > > handles bayer orders and transforms already happens in\n> > > > RPiCameraConfiguration::validate()\n> > > > (done there to avoid derived pipeline handlers from repeating the same\n> > > > construct). Could it be simply removed from here without any loss of\n> > > > functionality?\n> > >\n> > > Are you sure we're actually doing the same thing ?\n> > >\n> > > As I understand it, the logic implemented here makes sure that the RAW\n> > > StreamConfiguration matches the requested SensorConfiguration without\n> > > actually testing it against the CSI-2 receiver video device.\n> > >\n> > > While in RPiCameraConfiguration::validate() we take the RAW\n> > > StreamConfiguration, regardless of the SensorConfig, and create a\n> > > V4L2DeviceFormat from it (using the media bus code from the sensor\n> > > format) and try to apply it to the CSI-2 receiver video device,\n> > > adjusting the RAW StreamConfiguration according to the result of\n> > > applying the format to the video device.\n> >\n> > Sorry, I should have been clearer - I was only referring to the Bayer order\n> > handling below, not the whole block of code.\n> >\n> > >\n> > > When it comes to the bayer order handling are you referring to:\n> > >\n> > > /* Handle flips to make sure to match the RAW stream format. */\n> > > if (flipsAlterBayerOrder_)\n> > > rawBayer = rawBayer.transform(rpiConfig->combinedTransform_);\n> > >\n> > > If that's your actual concern ?\n> >\n> > Yes, this is the bit that I'm wondering if we can remove.\n> >\n> > >\n> > > If we don't do it here, do we risk to set to Adjusted a correct\n> > > configuration from user ?\n> > >\n> > > Let's assume your sensor is natively SRGGB10 and you have H/VFLIP\n> > > enabled, so that the actual output format is BGGR10.\n> > >\n> > > You receive a CameraConfiguration with a RAW stream with\n> > >\n> > > { PixelFormat::SBGGR10, 1080p }\n> > >\n> > > And a SensorConfiguration = { 10, 1080p }\n> > >\n> > > In RPiCameraConfiguration::validate() we compute the 'best' sensor\n> > > format with CameraData::findBestFormat(). At this point (and here I'm\n> > > going from memory so please confirm) flips are not activated, so you\n> > > get back a SubdeviceFormat with the native SRGGB10 ordering.\n> >\n> > With you so far.\n> >\n> > >\n> > > Then we enter platformValidate() with\n> > >\n> > > sensorFormat_ = { MEDIA_BUS_SRGGB10, 1080p }\n> > >\n> > > and we get to\n> > > BayerFormat rawBayer = BayerFormat::fromMbusCode(rpiConfig->sensorFormat_.mbus_code);\n> > >\n> > > let's assume we don't do the above check for if\n> > > (flipsAlterBayerOrder_) and we'll end up with\n> > >\n> > > if (rawStream->pixelFormat != rawFormat)\n> > >\n> > > and then we Adjust the RAW stream to PixelFormat::SRGGB10.\n> > >\n> > > Then we're back to RPiCameraConfiguration::validate(). We enter the\n> > > \"Further fixup on RAW streams\" loop. We apply SRGGB10 to the CSI-2\n> > > receiver but here we notice that flipsAlterBayerOrder_. So we go and\n> > > adjust back the RAW stream configuration to SBGGR10 and we mark it\n> > > again to Adjusted, while it is what the application has actually asked\n> > > for!\n> > >\n> > > Is my understanding correct here ?\n> >\n> > Yes, I agree with your sequencing and final outcome here.\n> >\n> > I guess my thinking here was that in Vc4CameraData::platformValidate we would\n> > not ever touch cfg->pixelFormat. So no if (flipsAlterBayerOrder_) clause, and\n> > no if (rawStream->pixelFormat != rawFormat) test.\n> >\n> > From your example above, in \"Further fixup on RAW streams\" loop, rawFormat\n> > would be BGGR10, then after the if (data_->flipsAlterBayerOrder_), we remain at\n> > BGGR10 and this would not signal Adjusted. I think that's what would happen,\n> > but my head hurts too much thinking about it!\n> >\n> > Maybe the simplification I'm thinking of will not work for all circumstances,\n> > and your change is more complete and explicit. But I wonder then, should we\n> > remove the if (data_->flipsAlterBayerOrder_) test in the \"Further fixup on RAW\n> > streams\" loop instead?\n>\n> With your pending patch \"[PATCH] pipeline: rpi: Don't call\n> toV4L2DeviceFormat() from validate()\" that removes\n>\n> rawFormat = PipelineHandlerBase::toV4L2DeviceFormat(raw.dev, subdevFormat, packing);\n>\n> in favour of\n>\n> rawFormat.fourcc = raw.dev->toV4L2PixelFormat(cfg.pixelFormat);\n>\n> we use the pixelformat for the raw stream as computed in\n> platformValidate(), you're right, so on can presume there is no need\n> to re-check for the Bayer ordering in validate() as it has been corrected in\n> platformValidate().\n>\n> But be aware that the StreamConfiguration PixelFormat only gets\n> changed when there is a SensorConfig, not in all cases, so I'm afraid\n> we need to keep the:\n>\n> if (data->flipsAlterBayerOrder_)\n>\n> test in the \"Further fixup\" loop in validate().\n>\n> Do you agree ?\n\nYes, I think I agree with this!\nThere's too many changes on the go touching the exact same lines of code :-)\nWe may have to wait until they are all merged before noticing any\nsubtle breakages.\n\nRegards,\nNaush\n\n\n>\n> >\n> > Regards,\n> > Naush\n> >\n> > >\n> > > >\n> > > > If the answer is yes, can I suggest another change - instead of passing in\n> > > > *rpiConfig to this function, we can pass in sensorFormat_? This ought to be\n> > > > equivalent I think? In fact, a completely unrelated change that I'm working on\n> > > > passes in sensorFormat_ (or really just a V4L2SubdeviceFormat &), so this may\n> > > > be ideal :)\n> > > >\n> > > > Regards,\n> > > > Naush\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 2BEEFBDCBF\n\tfor ;\n\tMon, 31 Jul 2023 09:42:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7770A627EB;\n\tMon, 31 Jul 2023 11:42:47 +0200 (CEST)","from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com\n\t[IPv6:2607:f8b0:4864:20::1132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CF396037D\n\tfor ;\n\tMon, 31 Jul 2023 11:42:45 +0200 (CEST)","by mail-yw1-x1132.google.com with SMTP id\n\t00721157ae682-583b019f1cbso46120447b3.3\n\tfor ;\n\tMon, 31 Jul 2023 02:42:45 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690796567;\n\tbh=PI0zp3+koxqgYKyAQ9rv4jal/qO5APvoLGu8IOp2PB0=;\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=dDwclVMVGDhr5wangGp2OzaCD0Nt/l/v4MaPhjfXxTocb02g86w5pr0Ht4nzC3H65\n\tunAAQWBGi7iH1OsoL+//UJWs/Z3osN+ElSoQxhsNIXj3d6wwVzmp5VbBufEX2MitUs\n\tKBr/Fu/7JTA3clAanulPbPBMS6nQX+kqYxL/oWZAjWN28n13bxaQ6IZag/2PsOROUM\n\tXp6aQ7PUWeuHRb14J75jdt9rvuVVTcR0cIHsBoq9sqj68tISaFHut2+ivKxdGvN4oI\n\tAJNBl42bG43YIicgZzPrhG50NSqvp95XyEB+yLITwXHLM5iAPKwWOO1z8gC1MZuVZe\n\tPSpFoBYulcmlw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1690796564; x=1691401364;\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=xIuXmu1dx4XkIIh8QF2AvsOJFfaAcsICio6vuMXnIbA=;\n\tb=ED3jPY1SJ9gR5w1C1z1Sik3cioi9QIr18ApPqwL1vN/JnEFh/NYvBEQfhtvWRJAZHT\n\tETC+J5yJSyppDnK9E2pYUL3fUQFPYo31TuW09gW83T+s+QSGzceYCl7v1ix3a7gCY3I5\n\tU7aLGuts+fN3+E8CAELjZErCEFTc7QlCMmzVZ38rrdCE6NJbK9P6f/Jm7FZ9Le7dIyvr\n\tb/3EzGvJKughculF5DozU8PJNW6SqcWoHiWL7peSKCzjv0O2Im2V9JZLKXRHsSpxzRiQ\n\t86WwNfp692UocNFNeLeMUHONVxi91S8IqaOOFYhEOzDi43QUm33ffRJZJVYr98gD3Aum\n\t5xyw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ED3jPY1S\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690796564; x=1691401364;\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=xIuXmu1dx4XkIIh8QF2AvsOJFfaAcsICio6vuMXnIbA=;\n\tb=Zdth+Uoo7kRa41L5+Idk7aycMYZbAv/nMhe9ReMwH5IQc42blKZEqfCfsMKacPthVY\n\ta+vsDsZ43sVQkRBwYAarq84Egb2k0lGEU5lS45ABTwX/HZgEyFnaJ8ZXeerDZn6pEak8\n\tdKTAyQcnC+F7WWspE/AWB6U+XE/xIAr6VfJJSS2UnUv0575eZ4wcVdpy4FtFbTz+CYuU\n\tMga/DYWopjJZ3CuPyA4LF150+PSoQ9FRWG53IS1HYFwMTsuAVvOABJNB5NS+AjDoeY4V\n\tWOv0aV1cVjM9/nEoE+KVr1Qg52YsmEO1LmuWLxESyUyovaw6tulVK5AHr2DCDSKbC0Od\n\tqtmA==","X-Gm-Message-State":"ABy/qLbPUD1Gmlr/neKA73n4cxP795ZjQosD8LtR0kheMM1g4S4elMOT\n\tZ0Mv8CcayyGVuW6Aasik5gdSRZnTQon0hlKib6sZeQ==","X-Google-Smtp-Source":"APBJJlE689RgDKkjP+jN1QutMVcg7T1newx++XMNWQk+4uVqFNmsr9ZSHuKrEdBhYjHT6xNbkTbmjBcPEWXIHUar4ns=","X-Received":"by 2002:a0d:ca92:0:b0:577:1e1e:c94e with SMTP id\n\tm140-20020a0dca92000000b005771e1ec94emr8269395ywd.39.1690796563938;\n\tMon, 31 Jul 2023 02:42:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20230724123907.29086-1-jacopo.mondi@ideasonboard.com>\n\t<20230724123907.29086-5-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Date":"Mon, 31 Jul 2023 10:42:55 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]