[{"id":12093,"web_url":"https://patchwork.libcamera.org/comment/12093/","msgid":"<20200823012807.GC25161@pendragon.ideasonboard.com>","date":"2020-08-23T01:28:07","subject":"Re: [libcamera-devel] [PATCH v3 2/5] libcamera: Add user Transform\n\tto CameraConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, Aug 21, 2020 at 04:56:38PM +0100, David Plowman wrote:\n> Add a field to the CameraConfiguration (including the necessary\n> documentation) to represent a 2D transform requested by the\n> application. All pipeline handlers are amended to coerce this to the\n> Identity, marking the configuration as \"adjusted\" if something\n> different had been requested.\n> \n> Pipeline handlers that support Transforms can be amended subsequently.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/camera.h | 3 +++\n> src/libcamera/camera.cpp | 16 +++++++++++++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++\n> src/libcamera/pipeline/simple/simple.cpp | 5 +++++\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +++++\n> src/libcamera/pipeline/vimc/vimc.cpp | 5 +++++\n> 8 files changed, 48 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 272c12c..a2ee4e7 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -61,6 +62,8 @@ public:\n> \tbool empty() const;\n> \tstd::size_t size() const;\n> \n> +\tTransform transform;\n> +\n> protected:\n> \tCameraConfiguration();\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 4a9c19c..e12c1a0 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -93,7 +93,7 @@ LOG_DECLARE_CATEGORY(Camera)\n> * \\brief Create an empty camera configuration\n> */\n> CameraConfiguration::CameraConfiguration()\n> -\t: config_({})\n> +\t: transform(Transform::Identity), config_({})\n> {\n> }\n> \n> @@ -250,6 +250,20 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n> \n> +/**\n> + * \\var CameraConfiguration::transform\n> + * \\brief User-specified transform to be applied to the image\n> + *\n> + * The transform is a user-specified 2D plane transform that will be applied\n> + * to the camera images by the processing pipeline before being handed to\n> + * the application. This is subsequent to any transform that is already\n> + * required to fix up any platform-defined rotation.\n\nI wonder if this is the right thing to do, or if we're trying to be too\nclever. Maybe the default configuration should set the transform field\nto the best value instead (that's mostly Rot180 if the sensor is mounted\nupside-down), and let the application overwrite it if desired, without\nhaving to think about how the user-specified transform will be composed\nwith the platform-defined rotation. I don't think we specify anywhere\nthat cameras will try to correct the image orientation behind the scene.\nThere's no need to change this now, but I think it will be reworked\nbefore we stabilize the API.\n\n> + *\n> + * The usual 2D plane transforms are allowed here (horizontal/vertical\n> + * flips, multiple of 90-degree rotations etc.), but pipeline handlers may\n> + * adjust this field at their discretion if the selection is not supported.\n\nFrom an application point of view, pipeline handlers should be\nirrelevant, they're an internal implementation \"detail\". I would say\n\"... but the validate() function may adjust this field at its discretion\n...\". With this addressed,\n\nReviewed-by: Laurent Pinchart \n\n> + */\n> +\n> /**\n> * \\var CameraConfiguration::config_\n> * \\brief The vector of stream configurations\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 019e50b..0f5ad73 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -138,6 +138,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tif (transform != Transform::Identity) {\n> +\t\ttransform = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > IPU3_MAX_STREAMS) {\n> \t\tconfig_.resize(IPU3_MAX_STREAMS);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index eeaf335..236aa5c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -400,6 +400,11 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tif (transform != Transform::Identity) {\n> +\t\ttransform = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> \tunsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;\n> \tstd::pair outSize[2];\n> \tSize maxSize;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 32fdaed..f846733 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -478,6 +478,11 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tif (transform != Transform::Identity) {\n> +\t\ttransform = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > 1) {\n> \t\tconfig_.resize(1);\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index eb72e3b..10223a9 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -438,6 +438,11 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tif (transform != Transform::Identity) {\n> +\t\ttransform = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > 1) {\n> \t\tconfig_.resize(1);\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index bc892ec..fd14248 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -108,6 +108,11 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tif (transform != Transform::Identity) {\n> +\t\ttransform = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > 1) {\n> \t\tconfig_.resize(1);\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index cf244f1..bb791d6 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -130,6 +130,11 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tif (transform != Transform::Identity) {\n> +\t\ttransform = Transform::Identity;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > 1) {\n> \t\tconfig_.resize(1);","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 821EDBE173\n\tfor ;\n\tSun, 23 Aug 2020 01:28:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E44960383;\n\tSun, 23 Aug 2020 03:28:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D7C260383\n\tfor ;\n\tSun, 23 Aug 2020 03:28:26 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A00E7276;\n\tSun, 23 Aug 2020 03:28:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sC7zQVLf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598146105;\n\tbh=RnP7U0UO0qYgqX+7+s/NitV3rC1eopCUWstuagkbTYo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sC7zQVLf/EozOg26xFA1e1KE30nPdQirvvmYzb17BRfJXzR9IlpYI6/u7Hhpadf2c\n\tKXwol8RpjD2+exBWNd/MchWQM2hWuy96HGIeIbJdqTRTU/2DFKQHKwrbFmzoBKh8OB\n\tiZX5fcYN8qkfqc2Wcch4JElnRqufRr1CA579/Ijk=","Date":"Sun, 23 Aug 2020 04:28:07 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200823012807.GC25161@pendragon.ideasonboard.com>","References":"<20200821155641.11839-1-david.plowman@raspberrypi.com>\n\t<20200821155641.11839-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200821155641.11839-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/5] libcamera: Add user Transform\n\tto CameraConfiguration","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]