[{"id":26417,"web_url":"https://patchwork.libcamera.org/comment/26417/","msgid":"","date":"2023-02-07T09:57:38","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: Remove transform from\n\tV4L2SubdeviceFormat","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Feb 07, 2023 at 08:52:09AM +0100, Jacopo Mondi wrote:\n> Commit 6f6e1bf704fe (\"libcamera: camera_sensor: Apply flips at\n> setFormat()\") extended the CameraSensor::setFormat() function\n> to apply vertical/horizontal flips on the sensor based on the\n> supplied Transform. To pass the Transform to the function the\n> V4L2SubdeviceFormat structure has been augmented with a Transform\n> member.\n> \n> However as the newly added Transform is not used at all in the\n> V4L2Subdevice class, it should not be part of V4L2SubdeviceFormat.\n> \n> Fix that by removing the transform field from V4L2SubdeviceFormat\n> and pass it as an explicit parameter to CameraSensor::setFormat().\n> \n> Fixes: 6f6e1bf704fe (\"libcamera: camera_sensor: Apply flips at setFormat())\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Laurent Pinchart \n> ---\n> \n> Sending a v2 instead of applying it directly as there was a leftover\n> forward declaration of\n> \n> \tenum class Transform;\n> \n> in camera_sensor.h which I have now removed since I'm now including the full\n> header flie.\n> \n> ---\n> include/libcamera/internal/camera_sensor.h | 6 +++---\n> include/libcamera/internal/v4l2_subdevice.h | 2 --\n> src/libcamera/camera_sensor.cpp | 11 ++++-------\n> src/libcamera/pipeline/ipu3/cio2.cpp | 3 +--\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 ++++--\n> src/libcamera/v4l2_subdevice.cpp | 7 -------\n> 7 files changed, 18 insertions(+), 28 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index bea52badaff7..a8d980e3b439 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -29,8 +30,6 @@ class BayerFormat;\n> class CameraLens;\n> class MediaEntity;\n> \n> -enum class Transform;\n> -\n> struct CameraSensorProperties;\n> \n> class CameraSensor : protected Loggable\n> @@ -55,7 +54,8 @@ public:\n> \n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> -\tint setFormat(V4L2SubdeviceFormat *format);\n> +\tint setFormat(V4L2SubdeviceFormat *format,\n> +\t\t const Transform transform = Transform::Identity);\n\nYou could skip const.\n\n> \n> \tconst ControlInfoMap &controls() const;\n> \tControlList getControls(const std::vector &ids);\n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 576faf971a05..69862de0585a 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -20,7 +20,6 @@\n> \n> #include \n> #include \n> -#include \n> \n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/media_object.h\"\n> @@ -45,7 +44,6 @@ struct V4L2SubdeviceFormat {\n> \tuint32_t mbus_code;\n> \tSize size;\n> \tstd::optional colorSpace;\n> -\tTransform transform = Transform::Identity;\n> \n> \tconst std::string toString() const;\n> \tuint8_t bitsPerPixel() const;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 274ed419ddfd..19f0ce6a47bb 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -16,7 +16,6 @@\n> #include \n> \n> #include \n> -#include \n> \n> #include \n> \n> @@ -751,7 +750,6 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> \t\t.mbus_code = bestCode,\n> \t\t.size = *bestSize,\n> \t\t.colorSpace = ColorSpace::Raw,\n> -\t\t.transform = Transform::Identity,\n> \t};\n> \n> \treturn format;\n> @@ -760,6 +758,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> /**\n> * \\brief Set the sensor output format\n> * \\param[in] format The desired sensor output format\n> + * \\param[in] transform The transform to be applied on the sensor. Defaults to Identity.\n\nLine wrap ?\n\n> *\n> * If flips are writable they are configured according to the desired Transform.\n> * Transform::Identity always corresponds to H/V flip being disabled if the\n> @@ -770,18 +769,16 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> +int CameraSensor::setFormat(V4L2SubdeviceFormat *format, const Transform transform)\n> {\n> \t/* Configure flips if the sensor supports that. */\n> \tif (supportFlips_) {\n> \t\tControlList flipCtrls(subdev_->controls());\n> \n> \t\tflipCtrls.set(V4L2_CID_HFLIP,\n> -\t\t\t static_cast(!!(format->transform &\n> -\t\t\t\t\t\t Transform::HFlip)));\n> +\t\t\t static_cast(!!(transform & Transform::HFlip)));\n> \t\tflipCtrls.set(V4L2_CID_VFLIP,\n> -\t\t\t static_cast(!!(format->transform &\n> -\t\t\t\t\t\t Transform::VFlip)));\n> +\t\t\t static_cast(!!(transform & Transform::VFlip)));\n> \n> \t\tint ret = subdev_->setControls(&flipCtrls);\n> \t\tif (ret)\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index a819884f762d..7400cb0b644c 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -194,8 +194,7 @@ int CIO2Device::configure(const Size &size, const Transform &transform,\n> \t */\n> \tstd::vector mbusCodes = utils::map_keys(mbusCodesToPixelFormat);\n> \tsensorFormat = getSensorFormat(mbusCodes, size);\n> -\tsensorFormat.transform = transform;\n> -\tret = sensor_->setFormat(&sensorFormat);\n> +\tret = sensor_->setFormat(&sensorFormat, transform);\n> \tif (ret)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 77e860ab0e72..c0dd95513583 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -832,13 +832,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\t}\n> \t}\n> \n> -\t/* First calculate the best sensor mode we can use based on the user request. */\n> +\t/*\n> +\t * Calculate the best sensor mode we can use based on the user's\n> +\t * request, and apply it to the sensor with the cached transform, if\n> +\t * any.\n> +\t */\n> \tV4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize, bitDepth);\n> -\t/* Apply any cached transform. */\n> \tconst RPiCameraConfiguration *rpiConfig = static_cast(config);\n> -\tsensorFormat.transform = rpiConfig->combinedTransform_;\n> -\t/* Finally apply the format on the sensor. */\n> -\tret = data->sensor_->setFormat(&sensorFormat);\n> +\tret = data->sensor_->setFormat(&sensorFormat, rpiConfig->combinedTransform_);\n> \tif (ret)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 5f22a29d02c6..8a30fe061d04 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -125,6 +125,7 @@ public:\n> \tStatus validate() override;\n> \n> \tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> +\tconst Transform &combinedTransform() { return combinedTransform_; }\n> \n> private:\n> \tbool fitsAllPaths(const StreamConfiguration &cfg);\n> @@ -138,6 +139,7 @@ private:\n> \tconst RkISP1CameraData *data_;\n> \n> \tV4L2SubdeviceFormat sensorFormat_;\n> +\tTransform combinedTransform_;\n> };\n> \n> class PipelineHandlerRkISP1 : public PipelineHandler\n> @@ -591,7 +593,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n> \n> -\tsensorFormat_.transform = combined;\n> +\tcombinedTransform_ = combined;\n> \n> \treturn status;\n> }\n> @@ -720,7 +722,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tV4L2SubdeviceFormat format = config->sensorFormat();\n> \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n> \n> -\tret = sensor->setFormat(&format);\n> +\tret = sensor->setFormat(&format, config->combinedTransform());\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 38ff8b0c605b..15e8206a915c 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -216,13 +216,6 @@ const std::map formatInfoMap = {\n> * resulting color space is acceptable.\n> */\n> \n> -/**\n> - * \\var V4L2SubdeviceFormat::transform\n> - * \\brief The transform (vertical/horizontal flips) to be applied on the subdev\n> - *\n> - * Default initialized to Identity (no transform).\n> - */\n> -\n> /**\n> * \\brief Assemble and return a string describing the format\n> * \\return A string describing the V4L2SubdeviceFormat","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 9789EBEFBE\n\tfor ;\n\tTue, 7 Feb 2023 09:57:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5D23625F3;\n\tTue, 7 Feb 2023 10:57:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7600C603BD\n\tfor ;\n\tTue, 7 Feb 2023 10:57:40 +0100 (CET)","from pendragon.ideasonboard.com (unknown [109.136.43.56])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EEB6F4AF;\n\tTue, 7 Feb 2023 10:57:39 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675763861;\n\tbh=9i3SnKo12Eg0NT4dHJEenmIbIg1B90KMqz3CG0UAOtU=;\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=V4tg2uaPusizumpUvRbqiyKsCIRQ8lMGADz9nv5BOWE+sdmSoORbPszgkr5mBESrh\n\tPQShskcfPfl6+EFl28zK5hPLF8CiYJJIsqTR0Xqiik/8uo6DC6pMHeMZaoTvb3dyjo\n\tL7DVKULOSE/QiIPvqNduFR/BuwzMbusq1uFI605fDZ8Ypce6GtFGceqPrUIjYHOVVQ\n\tmpgbbcNWuHHMFFsJrWT27ClFurMD2Y9BCeTZl6MtFsuFMehxRx1Ci8j/ky+CRpktkA\n\t+Y0TEKPghWxha60y7KkZUPujepCRPhDUppqhQDrIcn2d/vPwmQzA4BTkgWydd2qU9h\n\tF8LZRMDkiHFvg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675763860;\n\tbh=9i3SnKo12Eg0NT4dHJEenmIbIg1B90KMqz3CG0UAOtU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X1i3JZ5NUZcE1GHv7aOu2aA1rlpcND3fvFymFR8LhxGScAiQKjPVn/w6yuAsfWh0p\n\tDv20lW3iJcxPJ0QYJwGvG7dMueprzoAZC6elximyPqnbpJ2tT/Cj0PfpQWldhZvyIA\n\tbdG6HZ/nR85C+WLk6MrgsK7c8L89zccvMFS3Qlak="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X1i3JZ5N\"; dkim-atps=neutral","Date":"Tue, 7 Feb 2023 11:57:38 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20230207075210.3789-1-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230207075210.3789-1-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: Remove transform from\n\tV4L2SubdeviceFormat","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]