[{"id":27554,"web_url":"https://patchwork.libcamera.org/comment/27554/","msgid":"","date":"2023-07-17T09:12:15","subject":"Re: [libcamera-devel] [PATCH v2 1/9] libcamera: camera_sensor:\n\tCache rotationTransform_","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for this patch set!\n\nOn Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n wrote:\n>\n> The rotationTransform_ depends on a V4L2 control whose value does not\n> change for the whole lifetime of the camera.\n>\n> Instead of re-calculating it everytime the camera is configured, cache\n> it at properties initialization time.\n>\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 54 +++++++++++++---------\n> 2 files changed, 32 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 02c77ab037da..02b4b4d25e6d 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -107,6 +107,7 @@ private:\n> Rectangle activeArea_;\n> const BayerFormat *bayerFormat_;\n> bool supportFlips_;\n> + Transform rotationTransform_;\n>\n> ControlList properties_;\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f3a5aa37149f..9a033459742f 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -461,18 +461,36 @@ int CameraSensor::initProperties()\n>\n> const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> if (rotationControl != controls.end()) {\n> + propertyValue = rotationControl->second.def().get();\n> +\n> /*\n> - * validateTransform() compensates for the mounting rotation.\n> - * However, as a camera sensor can only compensate rotations\n> - * by applying H/VFlips, only rotation of 180 degrees are\n> - * automatically compensated. The other valid rotations (Rot90\n> - * and Rot270) require transposition, which the camera sensor\n> - * cannot perform, so leave them untouched.\n> + * Cache the Transform associated with the camera mounting\n> + * rotation for later use in validateTransform().\n> + */\n> + bool success;\n> + rotationTransform_ = transformFromRotation(propertyValue, &success);\n> + if (!success) {\n> + LOG(CameraSensor, Warning)\n> + << \"Invalid rotation of \" << propertyValue\n> + << \" degrees - ignoring\";\n> + rotationTransform_ = Transform::Identity;\n> + }\n> +\n> + /*\n> + * Adjust property::Rotation as validateTransform() compensates\n> + * for the mounting rotation. However, as a camera sensor can\n> + * only compensate rotations by applying H/VFlips, only rotation\n> + * of 180 degrees are automatically compensated. The other valid\n> + * rotations (Rot90 and Rot270) require transposition, which the\n> + * camera sensor cannot perform, so leave them untouched.\n\nThese extra comments didn't quite make sense to me. I believe that\nthey correctly describe the behaviour here even though a later patch\nin this series changes it (as we agreed in Prague). So reading these\ncomments just left me a bit puzzled. But I don't think it matters very\nmuch, so:\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\n> */\n> - propertyValue = rotationControl->second.def().get();\n> if (propertyValue == 180 && supportFlips_)\n> propertyValue = 0;\n> properties_.set(properties::Rotation, propertyValue);\n> + } else {\n> + LOG(CameraSensor, Warning)\n> + << \"Rotation control not available, default to 0 degrees\";\n> + rotationTransform_ = Transform::Identity;\n> }\n>\n> properties_.set(properties::PixelArraySize, pixelArraySize_);\n> @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()\n> */\n> Transform CameraSensor::validateTransform(Transform *transform) const\n> {\n> - /* Adjust the requested transform to compensate the sensor mounting rotation. */\n> - const ControlInfoMap &controls = subdev_->controls();\n> - int rotation = 0;\n> -\n> - const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> - if (rotationControl != controls.end())\n> - rotation = rotationControl->second.def().get();\n> -\n> - bool success;\n> - Transform rotationTransform = transformFromRotation(rotation, &success);\n> - if (!success)\n> - LOG(CameraSensor, Warning) << \"Invalid rotation of \" << rotation\n> - << \" degrees - ignoring\";\n> -\n> - Transform combined = *transform * rotationTransform;\n> + /*\n> + * Combine the requested transform to compensate the sensor mounting\n> + * rotation.\n> + */\n> + Transform combined = *transform * rotationTransform_;\n>\n> /*\n> * We combine the platform and user transform, but must \"adjust away\"\n> @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> * rise to this is the inverse of the rotation. (Recall that\n> * combined = transform * rotationTransform.)\n> */\n> - *transform = -rotationTransform;\n> + *transform = -rotationTransform_;\n> combined = Transform::Identity;\n> }\n>\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 32ACDBEFBE\n\tfor ;\n\tMon, 17 Jul 2023 09:12:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5450E628C0;\n\tMon, 17 Jul 2023 11:12:29 +0200 (CEST)","from mail-qk1-x731.google.com (mail-qk1-x731.google.com\n\t[IPv6:2607:f8b0:4864:20::731])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFB5360381\n\tfor ;\n\tMon, 17 Jul 2023 11:12:27 +0200 (CEST)","by mail-qk1-x731.google.com with SMTP id\n\taf79cd13be357-7680e39103bso194601885a.3\n\tfor ;\n\tMon, 17 Jul 2023 02:12:27 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689585149;\n\tbh=yxqhEdp+Hzga7Cj6+Am0VyO8z0kznYhcBoQFdlwn8K8=;\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=0J2cn0AUhzNXwjndslCNGBLDMCiDt8yCeZw8zjSwxwR3NCHJUUDqM+BPvjaYJ58jp\n\tilUjT5FGIWMVC20SGAFsa3SleFt1WEaok5SuaMo6wkfxxkuFvRz2nDJiTCjsHpfD8L\n\tc1eUPqBCEA1dz8UTjlDh4sJ7s/I5zIOx7Fct3aB/GMPW2hycuF01Ki04uNyGXYi5J/\n\tsup0iJVDR3p7xTRMrpS0UXUJntlFbTNe4oxcjXg+7hBYH07c5TaT2l1GE9BWzlllBv\n\t1zK0ovXc5ucx29PWxV1BaKKrPfLE+aX6nSslG/woyB0KRB741tz0tcXZOs4BVZrVep\n\t+SvzmWb2xFtfA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1689585146; x=1692177146;\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=alW95ZNm7Y5spoPg4p9InsK2lmDimqjE3aa+LodhG3Y=;\n\tb=Em/qPgYGA9eEOHAXl225SveGFwANGa3Q/JeyYfOLMnTM0YxTVO1hyi6lm915HlZ3xi\n\tqlqfzhh2Wq36sDlHBAecxiYk5Ri84PCwjwUsQEpkBMD+s2mpW/3x91zxfp+CtAzeInTK\n\tRd6XbRUD3kO8tJz1ZwYvG3Gt3qaWUU5LX2Hp02VIEamz+GxQfjvA53cJNE9HXI756y38\n\tRdCsHwghxJYG5D0GXWYXA3lyWMSx3wNuu0qbiBQPF2wnbYZOCc6OCwyokxOZX7G+sDfn\n\tngzkLC+Ln5quotS7GnGgnwsaaJjgsbr0IG9A1qWG64a9j0Ausq7CxTDNyOQx6Nq3/3I3\n\t6Uwg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Em/qPgYG\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1689585146; x=1692177146;\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=alW95ZNm7Y5spoPg4p9InsK2lmDimqjE3aa+LodhG3Y=;\n\tb=ZEa9vLQ8MUnwsBe7yd4l99qqijEbw6zj3G6ijCSuegV7fU6q4DYm/kAUHWLuM+hXXK\n\tFy8A8Z7yAh8qEoCucUtuKSGGQ5hlw+7NnfaaEHneW2x8K657JPcD8TJvyuzTdr9shcYY\n\tQt3fWhCrYUU7WCOlQtwosuZdMwcuR6XcitNZS6Hx7oVYQmUWjx7F8sSJDQ4sqpuvnJOe\n\tZqLdzrODIZxf3nlYId6K1vSh/Nggg5j1UrJ+07JtWNoFLBCH8CJBYf8NA9H9CvamB3mA\n\tPXjMlyObqUsPRZI8ZuvzaGc6l+N8+IJSOj+8ZO1S4SzQj87vjJFWTbYHE+QuvCHlHrDb\n\tv6Gw==","X-Gm-Message-State":"ABy/qLanGgtxfLqN/xG8I6/VOkPwugYfQ1vactBtkTrVJfQ2TYnokNqv\n\ty6FUiaweukPK7T/bJlTiiiSPQKtvhlbXSBOyjhm4qw==","X-Google-Smtp-Source":"APBJJlE2KDEEotABu2OX1PCvVsr8994Pjh9bTXrFYoHYjFP8Gzfqq0eXibAkFbVSpTUlT8Sw4t0JwV3kj5GmQ/iVW30=","X-Received":"by 2002:a0c:b25b:0:b0:626:1be5:177b with SMTP id\n\tk27-20020a0cb25b000000b006261be5177bmr9197272qve.65.1689585146362;\n\tMon, 17 Jul 2023 02:12:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-2-jacopo.mondi@ideasonboard.com>","In-Reply-To":"<20230714141549.11085-2-jacopo.mondi@ideasonboard.com>","Date":"Mon, 17 Jul 2023 10:12:15 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/9] libcamera: camera_sensor:\n\tCache rotationTransform_","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27560,"web_url":"https://patchwork.libcamera.org/comment/27560/","msgid":"","date":"2023-07-17T12:35:30","subject":"Re: [libcamera-devel] [PATCH v2 1/9] libcamera: camera_sensor:\n\tCache rotationTransform_","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Mon, Jul 17, 2023 at 10:12:15AM +0100, David Plowman via libcamera-devel wrote:\n> Hi Jacopo\n>\n> Thanks for this patch set!\n>\n> On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi\n> wrote:\n> >\n> > The rotationTransform_ depends on a V4L2 control whose value does not\n> > change for the whole lifetime of the camera.\n> >\n> > Instead of re-calculating it everytime the camera is configured, cache\n> > it at properties initialization time.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Umang Jain \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 54 +++++++++++++---------\n> > 2 files changed, 32 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 02c77ab037da..02b4b4d25e6d 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -107,6 +107,7 @@ private:\n> > Rectangle activeArea_;\n> > const BayerFormat *bayerFormat_;\n> > bool supportFlips_;\n> > + Transform rotationTransform_;\n> >\n> > ControlList properties_;\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index f3a5aa37149f..9a033459742f 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -461,18 +461,36 @@ int CameraSensor::initProperties()\n> >\n> > const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > if (rotationControl != controls.end()) {\n> > + propertyValue = rotationControl->second.def().get();\n> > +\n> > /*\n> > - * validateTransform() compensates for the mounting rotation.\n> > - * However, as a camera sensor can only compensate rotations\n> > - * by applying H/VFlips, only rotation of 180 degrees are\n> > - * automatically compensated. The other valid rotations (Rot90\n> > - * and Rot270) require transposition, which the camera sensor\n> > - * cannot perform, so leave them untouched.\n> > + * Cache the Transform associated with the camera mounting\n> > + * rotation for later use in validateTransform().\n> > + */\n> > + bool success;\n> > + rotationTransform_ = transformFromRotation(propertyValue, &success);\n> > + if (!success) {\n> > + LOG(CameraSensor, Warning)\n> > + << \"Invalid rotation of \" << propertyValue\n> > + << \" degrees - ignoring\";\n> > + rotationTransform_ = Transform::Identity;\n> > + }\n> > +\n> > + /*\n> > + * Adjust property::Rotation as validateTransform() compensates\n> > + * for the mounting rotation. However, as a camera sensor can\n> > + * only compensate rotations by applying H/VFlips, only rotation\n> > + * of 180 degrees are automatically compensated. The other valid\n> > + * rotations (Rot90 and Rot270) require transposition, which the\n> > + * camera sensor cannot perform, so leave them untouched.\n>\n> These extra comments didn't quite make sense to me. I believe that\n> they correctly describe the behaviour here even though a later patch\n\nYes, they describe the current behaviour in this patch.\n\n> in this series changes it (as we agreed in Prague). So reading these\n> comments just left me a bit puzzled. But I don't think it matters very\n> much, so:\n\nThey describe the current behaviour that will later be changed.\nRemember this patch was already part of the previous series where the\nmounting rotation auto-compensation was not meant to be removed.\n\nAs the comment is anyway correct for the status at this patch, I\ndidn't think it was necessary removing it.\n\n>\n> Reviewed-by: David Plowman \n\nThanks\n j\n\n>\n> Thanks\n> David\n>\n> > */\n> > - propertyValue = rotationControl->second.def().get();\n> > if (propertyValue == 180 && supportFlips_)\n> > propertyValue = 0;\n> > properties_.set(properties::Rotation, propertyValue);\n> > + } else {\n> > + LOG(CameraSensor, Warning)\n> > + << \"Rotation control not available, default to 0 degrees\";\n> > + rotationTransform_ = Transform::Identity;\n> > }\n> >\n> > properties_.set(properties::PixelArraySize, pixelArraySize_);\n> > @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()\n> > */\n> > Transform CameraSensor::validateTransform(Transform *transform) const\n> > {\n> > - /* Adjust the requested transform to compensate the sensor mounting rotation. */\n> > - const ControlInfoMap &controls = subdev_->controls();\n> > - int rotation = 0;\n> > -\n> > - const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);\n> > - if (rotationControl != controls.end())\n> > - rotation = rotationControl->second.def().get();\n> > -\n> > - bool success;\n> > - Transform rotationTransform = transformFromRotation(rotation, &success);\n> > - if (!success)\n> > - LOG(CameraSensor, Warning) << \"Invalid rotation of \" << rotation\n> > - << \" degrees - ignoring\";\n> > -\n> > - Transform combined = *transform * rotationTransform;\n> > + /*\n> > + * Combine the requested transform to compensate the sensor mounting\n> > + * rotation.\n> > + */\n> > + Transform combined = *transform * rotationTransform_;\n> >\n> > /*\n> > * We combine the platform and user transform, but must \"adjust away\"\n> > @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const\n> > * rise to this is the inverse of the rotation. (Recall that\n> > * combined = transform * rotationTransform.)\n> > */\n> > - *transform = -rotationTransform;\n> > + *transform = -rotationTransform_;\n> > combined = Transform::Identity;\n> > }\n> >\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 81C57BEFBE\n\tfor ;\n\tMon, 17 Jul 2023 12:35:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C35B061E2C;\n\tMon, 17 Jul 2023 14:35:35 +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 3B31360383\n\tfor ;\n\tMon, 17 Jul 2023 14:35:34 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-150.net.vodafone.it\n\t[5.90.54.150])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 13BCD2F5E;\n\tMon, 17 Jul 2023 14:34:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689597335;\n\tbh=Po5qIvRFEhKV4jCCFn72qI3YEU8e3JbMQcZE/3Kaars=;\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=sIGbRaj817l0OoMLJXo2PPopgtzZRB49yejpJgAFEH1f50VH5F5/7Ji35Z5lGd6Qc\n\t+KFTayBKESLxubene+WOI/h4aNnuCtWgNJJ9TG6kJHlT0RC8++1UmAIpuTPZxseX5k\n\tfq+OAFhzAgFr5Nsi/r1vqx3gcC2mu/f4ZISEFhe6eGZIghcluAjks4MEXA8oQhKkiq\n\tNnWVUVU7H/yCfLKFhrzM4YfryEA0ZAPm4Rvrid5CDwgI4OjZaXPH3sgyqGdaZEgp6f\n\tp9N4n6ROw9wdOvx1hDpIS02fkC1FatPfgyc6zHjsjyLUF/vWk8nRCvfP1vkojeEGim\n\tIUcOsDda9aLFg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689597281;\n\tbh=Po5qIvRFEhKV4jCCFn72qI3YEU8e3JbMQcZE/3Kaars=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=usY0G+r6r0+VMbIx22P7xU9XfpRbCwCf+2ocasjgpzXFaFcVW/kMCl7ZJvaFsVLFx\n\tr8zasWXoIRNnXZowPeSsN3TgRSlP2kTkp162Lk5CV6bCKOSlT5rvgwK4sk02x21uT4\n\tsh7rHNtdpyj8ZY5yiCKAHLEL1+xu22Fty3zu/tic="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"usY0G+r6\"; dkim-atps=neutral","Date":"Mon, 17 Jul 2023 14:35:30 +0200","To":"David Plowman ","Message-ID":"","References":"<20230714141549.11085-1-jacopo.mondi@ideasonboard.com>\n\t<20230714141549.11085-2-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 v2 1/9] libcamera: camera_sensor:\n\tCache rotationTransform_","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\" "}}]