[{"id":26414,"web_url":"https://patchwork.libcamera.org/comment/26414/","msgid":"","date":"2023-02-06T21:54:29","subject":"Re: [libcamera-devel] [PATCH] 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 Mon, Feb 06, 2023 at 08:58:09PM +0100, Jacopo Mondi via libcamera-devel 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\nThank you :-)\n\n> ---\n> Compiled tested only:\n> https://buildbot.libcamera.org/#/changes/4232\n> ---\n> include/libcamera/internal/camera_sensor.h | 4 +++-\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 | 10 +++++-----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 ++++--\n> src/libcamera/v4l2_subdevice.cpp | 7 -------\n> 7 files changed, 17 insertions(+), 26 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index bea52badaff7..77514de7a879 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> @@ -55,7 +56,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\nTransform is extremely lightweight, you can pass it by value:\n\n\tint setFormat(V4L2SubdeviceFormat *format,\n\t\t Transform transform = Transform::Identity);\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..903edbdcf233 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> *\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..da408c89bc3f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -832,13 +832,13 @@ 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 * First calculate the best sensor mode we can use based on the user\n\ns/user/user request/\n\n> +\t * and apply any cached transform.\n\nBut given that you drop the \"Finally\" comment below, there should be no\n\"First\" anymore.\n\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\nReviewed-by: Laurent Pinchart \n\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 3FDC5BEFBE\n\tfor ;\n\tMon, 6 Feb 2023 21:54:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AAE361EF4;\n\tMon, 6 Feb 2023 22:54:34 +0100 (CET)","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 16FA5603B8\n\tfor ;\n\tMon, 6 Feb 2023 22:54:32 +0100 (CET)","from pendragon.ideasonboard.com (unknown [109.136.43.56])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 77B9D891;\n\tMon, 6 Feb 2023 22:54:31 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675720474;\n\tbh=qJIuGiDXSwCVEBuxYA3XefT0O1FC75kyfQ424PoFPFA=;\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=zjRSnQnry1B5irEs6bHiSN6JHkpdIelXfRXRY3FyWLeTdOcoDi2Ujx+b6ohLsIzVD\n\tMm47QIXQgbVzYl8V3s6wtVuGgJH+Sv/jtsP4qt0e09dbOy0wcBsk7I4Bzt6O9NbmuQ\n\tGt9Yp2wSBU3XD/Ioimo8M6/FFuIAquAc8B3Imgha2BNxsATTMSmQxVWUqSDkTq02Rf\n\t8bRnbtWp+vYKbEVKMbwb0xnaiqph0Ah+4gvrKGI3MKt7Fnxpu5isRmpcFxTzX9s6//\n\trZX/SVmYoeWCPnpamgMhbrCUAZO+ixZwtYeIDn4JFTPYthZYWL+aetWBRpMwnxrbLq\n\tCT9pv8LormY2w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675720471;\n\tbh=qJIuGiDXSwCVEBuxYA3XefT0O1FC75kyfQ424PoFPFA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EJOysoCx3cPsy4U9xFqt70sgwOvKazPNLEaZaIONucWOHVoHAOWeTlHI6PwZQ8Ysu\n\tfEWG88QrOmJi57SRW7MCP996TIe2fqU+dUc/R4ki01D9ZR+GztcY3ARLtYV52P+aMd\n\tjZ8V/B1GtgspGPteofUkz4+MlOEEqSsPLJjvKl7k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EJOysoCx\"; dkim-atps=neutral","Date":"Mon, 6 Feb 2023 23:54:29 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20230206195809.62203-1-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230206195809.62203-1-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] 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\" "}}]