[{"id":12094,"web_url":"https://patchwork.libcamera.org/comment/12094/","msgid":"<20200823015108.GD25161@pendragon.ideasonboard.com>","date":"2020-08-23T01:51:08","subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","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:39PM +0100, David Plowman wrote:\n> The Raspberry Pi pipeline handler allows all transforms except those\n> involving a transpose. The user transform is combined with any\n> inherent rotation of the camera, and the camera's H and V flip bits\n> are set accordingly.\n> \n> Signed-off-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---\n> 1 file changed, 30 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 236aa5c..a3f8438 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -324,6 +324,8 @@ public:\n> \tuint32_t expectedSequence_;\n> \tbool sensorMetadata_;\n> \n> +\tTransform transform_;\n> +\n> \t/*\n> \t * All the functions in this class are called from a single calling\n> \t * thread. So, we do not need to have any mutex to protect access to any\n> @@ -400,8 +402,27 @@ 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/*\n> +\t * What if the platform has a non-90 degree rotation? We can't even\n> +\t * \"adjust\" the configuration and carry on. Alternatively, raising an\n> +\t * error means the platform can never run. Let's just print a warning\n> +\t * and continue regardless; the rotation is effectively set to zero.\n> +\t */\n> +\tint32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> +\tbool success;\n> +\tTransform combined = transform * transformFromRotation(rotation, &success);\n> +\tif (!success)\n> +\t\tLOG(RPI, Warning) << \"Invalid rotation of \" << rotation\n> +\t\t\t\t << \" degrees - ignoring\";\n> +\n> +\t/*\n> +\t * We combine the platform and user transform, but must \"adjust away\"\n> +\t * any combined result that includes a transform, as we can't do those.\n> +\t * Flipping the transpose bit in either input transform causes the\n> +\t * corresponding bit in the combined result to flip.\n> +\t */\n> +\tif (!!(combined & Transform::Transpose)) {\n> +\t\ttransform ^= Transform::Transpose;\n> \t\tstatus = Adjusted;\n> \t}\n\nI wonder if this wouldn't be confusing for the application. Imagine the\nfollowing use case. We have a device with a screen, meant to operate in\nportrait mode. The camera sensor will typically be mounted with a 90°\n(or 270°) rotation, in order to match the aspect ratio of the scene and\nthe screen (otherwise the scene would be captured in landscape mode and\ncouldn't be displayed full-screen). Assuming the ISP can't transpose, as\nin the Raspberry Pi case the application will have to rotate the image\nby 90° before displaying it. Let's further assume the user doesn't need\nto apply any h/v flip on the camera side.\n\ntransformFromRotation() returns Rot90 or Rot270. As transform is set to\nIdentity by the application, combined is equal to Rot90 or Rot270, which\nhas the Transpose bit set. The code above will XOR the Transpose bit\nout, leaving transform set to HFlip or VFlip. This seems an unexpect\nside effect to me.\n\nWe could of course argue that the application should look at the\nRotation property and compensate for the 90° rotation by requesting\nRot90 or Rot270, but is that the best option, especially given that we\ndon't give a way to applications to enumerate what transformations are\nsupported. Maybe this is good enough for now as we don't really claim to\nsupport 90° or 270° rotations yet, but I feel this will need to be\nrevisited sooner than later.\n\n> \n> @@ -610,6 +631,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \tfor (auto const stream : data->streams_)\n> \t\tstream->reset();\n> \n> +\t/* We will want to know the transform requested by the application. */\n> +\tdata->transform_ = config->transform;\n> +\n> \tSize maxSize, sensorSize;\n> \tunsigned int maxIndex = 0;\n> \tbool rawStream = false;\n> @@ -1174,8 +1198,10 @@ int RPiCameraData::configureIPA()\n> \t\t/* Configure the H/V flip controls based on the sensor rotation. */\n> \t\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> \t\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> -\t\tctrls.set(V4L2_CID_HFLIP, static_cast(!!rotation));\n> -\t\tctrls.set(V4L2_CID_VFLIP, static_cast(!!rotation));\n> +\t\t/* The rotation was already checked in RPiCameraConfiguration::validate. */\n> +\t\tTransform combined = transform_ * transformFromRotation(rotation);\n> +\t\tctrls.set(V4L2_CID_HFLIP, static_cast(!!(combined & Transform::HFlip)));\n> +\t\tctrls.set(V4L2_CID_VFLIP, static_cast(!!(combined & Transform::VFlip)));\n> \t\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> \t}\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 9AAD6BD87C\n\tfor ;\n\tSun, 23 Aug 2020 01:51:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29F76626DB;\n\tSun, 23 Aug 2020 03:51:29 +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 3D66F60383\n\tfor ;\n\tSun, 23 Aug 2020 03:51:27 +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 B032C276;\n\tSun, 23 Aug 2020 03:51:26 +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=\"tLFH9RdJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598147486;\n\tbh=Iep0+SYUPuVab0nBUjoTeEtD4ZUkE7mwZhwCbKkhXhY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tLFH9RdJfCXS3PTArd/grTssau+7+/igfsqV98lOAqKUMqxlQbpaSnourNVVEGyBA\n\tfAQBYLCXri55AoBVO7AMeoA3sQ0jtIr2Qsyxg1coI8jOyrn49zj1gEoIONhM3hboAL\n\tLjsSLXPksNhZ2LIwMEVfQS6Dx1qOyxZd3OOouK5A=","Date":"Sun, 23 Aug 2020 04:51:08 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200823015108.GD25161@pendragon.ideasonboard.com>","References":"<20200821155641.11839-1-david.plowman@raspberrypi.com>\n\t<20200821155641.11839-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200821155641.11839-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12117,"web_url":"https://patchwork.libcamera.org/comment/12117/","msgid":"","date":"2020-08-24T10:01:13","subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nActually I did maybe get something wrong here, so thanks for making me\nthink again....\n\nOn Sun, 23 Aug 2020 at 02:51, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 21, 2020 at 04:56:39PM +0100, David Plowman wrote:\n> > The Raspberry Pi pipeline handler allows all transforms except those\n> > involving a transpose. The user transform is combined with any\n> > inherent rotation of the camera, and the camera's H and V flip bits\n> > are set accordingly.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---\n> > 1 file changed, 30 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 236aa5c..a3f8438 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -324,6 +324,8 @@ public:\n> > uint32_t expectedSequence_;\n> > bool sensorMetadata_;\n> >\n> > + Transform transform_;\n> > +\n> > /*\n> > * All the functions in this class are called from a single calling\n> > * thread. So, we do not need to have any mutex to protect access to any\n> > @@ -400,8 +402,27 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > if (config_.empty())\n> > return Invalid;\n> >\n> > - if (transform != Transform::Identity) {\n> > - transform = Transform::Identity;\n> > + /*\n> > + * What if the platform has a non-90 degree rotation? We can't even\n> > + * \"adjust\" the configuration and carry on. Alternatively, raising an\n> > + * error means the platform can never run. Let's just print a warning\n> > + * and continue regardless; the rotation is effectively set to zero.\n> > + */\n> > + int32_t rotation = data_->sensor_->properties().get(properties::Rotation);\n> > + bool success;\n> > + Transform combined = transform * transformFromRotation(rotation, &success);\n> > + if (!success)\n> > + LOG(RPI, Warning) << \"Invalid rotation of \" << rotation\n> > + << \" degrees - ignoring\";\n> > +\n> > + /*\n> > + * We combine the platform and user transform, but must \"adjust away\"\n> > + * any combined result that includes a transform, as we can't do those.\n> > + * Flipping the transpose bit in either input transform causes the\n> > + * corresponding bit in the combined result to flip.\n> > + */\n> > + if (!!(combined & Transform::Transpose)) {\n> > + transform ^= Transform::Transpose;\n> > status = Adjusted;\n> > }\n>\n> I wonder if this wouldn't be confusing for the application. Imagine the\n> following use case. We have a device with a screen, meant to operate in\n> portrait mode. The camera sensor will typically be mounted with a 90°\n> (or 270°) rotation, in order to match the aspect ratio of the scene and\n> the screen (otherwise the scene would be captured in landscape mode and\n> couldn't be displayed full-screen). Assuming the ISP can't transpose, as\n> in the Raspberry Pi case the application will have to rotate the image\n> by 90° before displaying it. Let's further assume the user doesn't need\n> to apply any h/v flip on the camera side.\n>\n> transformFromRotation() returns Rot90 or Rot270. As transform is set to\n> Identity by the application, combined is equal to Rot90 or Rot270, which\n> has the Transpose bit set. The code above will XOR the Transpose bit\n> out, leaving transform set to HFlip or VFlip. This seems an unexpect\n> side effect to me.\n>\n> We could of course argue that the application should look at the\n> Rotation property and compensate for the 90° rotation by requesting\n> Rot90 or Rot270, but is that the best option, especially given that we\n> don't give a way to applications to enumerate what transformations are\n> supported. Maybe this is good enough for now as we don't really claim to\n> support 90° or 270° rotations yet, but I feel this will need to be\n> revisited sooner than later.\n>\n\nSo, when the camera has a particular (non-zero) rotation with respect\nto the \"world view\", then I think the aim is, if the application uses\nthe default Identity transform, to correct the camera rotation so that\nthe image that comes out is in the \"world view\". Is that right? (This\nis the single most fundamental point, I think!)\n\nFor example, I think this means that if the camera rotation is, say,\n90 degrees, we should therefore automatically be applying 270 degrees\n(the inverse of 90 degrees) to correct this. Actually I'm not totally\nclear on this and this may be where my mistake was. Suppose the world\nlooks like this (i.e. this is what you actually see):\n\nAB\nCD\n\nIf the camera has no rotation, then the camera image will look\nidentical. But now let's suppose the camera has a rotation of 90\ndegrees (according to its rotation property). Will the uncorrected\ncamera image look like this (90 degree clockwise rotation):\n\nCA\nDB\n\nor (270 aka. -90 degree rotation)\n\nBD\nAC\n?\n\nSo where I'd previously composed the user transform with the camera\nrotation, I think I possibly needed to compose it with the *inverse*\nof the camera rotation (depending on the answer to the above!)\n\nOn the final point, flipping only the transpose bit seems helpful to\nme. It means that you always get the transform you asked for, up to\nthe transpose. So you either get what you wanted, or you still have to\ndo (just) a plain transpose, and you don't have to handle 90 degree\nrotations or the opposite-diagonal transpose (not to mention the\nhassle of figuring out which one you actually need!). Does that make\nsense?\n\nThanks!\nDavid\n\n> >\n> > @@ -610,6 +631,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > for (auto const stream : data->streams_)\n> > stream->reset();\n> >\n> > + /* We will want to know the transform requested by the application. */\n> > + data->transform_ = config->transform;\n> > +\n> > Size maxSize, sensorSize;\n> > unsigned int maxIndex = 0;\n> > bool rawStream = false;\n> > @@ -1174,8 +1198,10 @@ int RPiCameraData::configureIPA()\n> > /* Configure the H/V flip controls based on the sensor rotation. */\n> > ControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> > int32_t rotation = sensor_->properties().get(properties::Rotation);\n> > - ctrls.set(V4L2_CID_HFLIP, static_cast(!!rotation));\n> > - ctrls.set(V4L2_CID_VFLIP, static_cast(!!rotation));\n> > + /* The rotation was already checked in RPiCameraConfiguration::validate. */\n> > + Transform combined = transform_ * transformFromRotation(rotation);\n> > + ctrls.set(V4L2_CID_HFLIP, static_cast(!!(combined & Transform::HFlip)));\n> > + ctrls.set(V4L2_CID_VFLIP, static_cast(!!(combined & Transform::VFlip)));\n> > unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 B6CAEBD87C\n\tfor ;\n\tMon, 24 Aug 2020 10:01:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 84E5C62818;\n\tMon, 24 Aug 2020 12:01:29 +0200 (CEST)","from mail-oo1-xc42.google.com (mail-oo1-xc42.google.com\n\t[IPv6:2607:f8b0:4864:20::c42])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49C6061ED9\n\tfor ;\n\tMon, 24 Aug 2020 12:01:28 +0200 (CEST)","by mail-oo1-xc42.google.com with SMTP id a6so1772390oog.9\n\tfor ;\n\tMon, 24 Aug 2020 03:01:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"co4mGTqZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=9ShgNB+GuD6vYLoIq4QrpmRFp1YN2TYxySLv+qqMR4E=;\n\tb=co4mGTqZtnSfwH3ja7s1CSVr0lVhbAQ87XVoV31IKzyHf21RpC3yza/OmvN+SiMXNS\n\t9aCgdjmGAGLazC9I5j3CH/OK+Wz8fLyTTExOfP8FITcUhdOn6ZZLE30+aIWlKz2hj1T8\n\tkY0unpHhxRCgQMWk8m3t9tnhh7U7v+qqr9AXhQhovzxs6Q/3YWpzpJ0ZU/s52c/w9opX\n\tPNbZFexv56jkqcWDg7lQXDPa/UkjqZ3kqn6MrmLV5WxFrvaCpp2hStQIP70hoPa2jqUI\n\tK//GKLDCAHHVLbCci6toZvWbOipdCP0c67cNZ7RP9zc4y1j4NdAUtvgPM1xGDfUxDFLJ\n\tFIdQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=9ShgNB+GuD6vYLoIq4QrpmRFp1YN2TYxySLv+qqMR4E=;\n\tb=TUxe0a5YKaqHff3BjHoi7bka21v0WdXoIAG5aW9J2+eGT484qTOI5f5I+F1D3/TqM2\n\t62zkbdSKKQcZwGI41Q+ZesF7aSJnp0mVaVGlOnshNZZkmJp/hGkIVJ0VJBoQWHJWOWh+\n\tvaUdBVi1Fjhvay70h3BK5yM6yzw2yMO24eyJjO0K9GrWKAVLL889geHn7CGSIB7Awyey\n\ty2H+TxZ4VzBZJqk9owus6y129bMjJeuLRd6RGw4xAXm7Gito3NliqrGRAzE8tP57kP2Z\n\tZwkfLc5BG/+x+/PZ0602xtn41gfI/4m+D2of7/Q4H0mHqrrSQ8I7jWn/cZ8PbtB3Mrnj\n\tCbnQ==","X-Gm-Message-State":"AOAM5312AQYB1eDa8kZJ2y5hucoC0B/+6Nx096Pz/Xf9PbEZE+38e+Hb\n\txRK9AfBJV0paJ4IE1f5Ju/o8F8OaJk/zzK8orIA1GA==","X-Google-Smtp-Source":"ABdhPJy5Y7FQ1V5T8P1H1marOupCtEzK6fqbvaeUhy4s60Jyee8nG8jLWzjnNnoSMVm66ePR1/4vFOCUrC42hjcoVnk=","X-Received":"by 2002:a4a:ea29:: with SMTP id y9mr3146250ood.72.1598263285304; \n\tMon, 24 Aug 2020 03:01:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20200821155641.11839-1-david.plowman@raspberrypi.com>\n\t<20200821155641.11839-4-david.plowman@raspberrypi.com>\n\t<20200823015108.GD25161@pendragon.ideasonboard.com>","In-Reply-To":"<20200823015108.GD25161@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 24 Aug 2020 11:01:13 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: raspberrypi: Set\n\tcamera flips correctly from user transform","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]