[{"id":11192,"web_url":"https://patchwork.libcamera.org/comment/11192/","msgid":"","date":"2020-07-06T08:31:31","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your patch.\n\nOn Sat, 4 Jul 2020 at 01:52, Laurent Pinchart\n wrote:\n>\n> Instead of receiving sensor orientation configuration from the IPA,\n> retrieve it from the CameraSensor Rotation property, and configure the\n> HFLIP and VFLIP controls accordingly.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> 1 file changed, 8 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 74bee6895dcd..fda6831e6a7e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> nullptr);\n>\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\nAre you missing a \">> 1\" or similar in the above code? Otherwise,\nboth H/V FLIP will be always set to the same value.\n\nRegards,\nNaush\n\n\n> + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> +\n> return 0;\n> }\n>\n> @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> { V4L2_CID_EXPOSURE, action.data[1] } });\n> sensorMetadata_ = action.data[2];\n> }\n> -\n> - /* Set the sensor orientation here as well. */\n> - ControlList controls = action.controls[0];\n> - unicam_[Unicam::Image].dev()->setControls(&controls);\n> return;\n> }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 D39F4BD792\n\tfor ;\n\tMon, 6 Jul 2020 08:31:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7077260C59;\n\tMon, 6 Jul 2020 10:31:49 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04009603B1\n\tfor ;\n\tMon, 6 Jul 2020 10:31:47 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id s9so44218200ljm.11\n\tfor ;\n\tMon, 06 Jul 2020 01:31:47 -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=\"UnoJKNCM\"; 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; bh=jYYCHNSll4gvHb7XOXoZKTZcg8gN9aJuCtXBOZArL1U=;\n\tb=UnoJKNCMyR6WoJDLNgLvm6r0MZIgva3gcCdjkfID2SrOZPEGiFbjgWljM+hsiUiVSk\n\tmS5GlLb6Ok/BJHE7G73rnjXAbnW+p8kVrrIkpDYMliyOXH5AdTap9px9yMFPEaqX8zRi\n\tYEFZ+nmi1kyt35ixt9q5fsR/O8snFp3QtwYKoHhvxjX7vVXTYRWU99MJESheOHWFK1J8\n\tNVibdqLzeL5AhS2c1UYKNlMjMVI8lB90gWxKx7gyW8Pcas9HKDrikKvUIKwcke+0Frrn\n\tlHGklq8PoFNdfoDyOezOSSzLKMft/giZFjdPLI50hR39MjQKYc8KhuR2X0GtYpPGBLIJ\n\to+2g==","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;\n\tbh=jYYCHNSll4gvHb7XOXoZKTZcg8gN9aJuCtXBOZArL1U=;\n\tb=WWF4pkNuExfPt+GoGfz40U6apRUCt9LrPROBIpDvKGjm2ihohqlDlAaPI8QuYPErow\n\tUpGEWQKdfRqdMET1BjSbJcYWVdHW1qhcUESL2pgTa1mscuJPTmmQI59LGoKE08mwZqyh\n\t7n06KuRDV5Pxic7fdaXvG/ElR6lH3GGOeAEyC4WMcdWROxKNYp+6c1xZVHODodWKjPRs\n\to82dGXXc/MA5RrZzw+Q0iIuv3crN5awqinzLhxLZ1KOM6PBbGgueCh6qKi8vOTmHP+Po\n\t80xbocmcgbSJSY3qG2I3r2oJY1340nVv8QkvzdWlOXdNzck+2kHtffG84G3khx63orj2\n\tSxrw==","X-Gm-Message-State":"AOAM530EEkR4u+sWvjFfVxD0E2c6drcGbxRbe8kc0KqLa8eNXOGICfi9\n\tnX4Qufv9cSSl30hT5CmMgZeRYq4VPwBW5OT4gvIb7Q==","X-Google-Smtp-Source":"ABdhPJzI5D39tHLPF5klgOhYgoruH6HQkJWYwSx4T+EJni2IWqWpfx5BNLOLvUJMXKC/OEmz+M58QSYuqNgocs24m3I=","X-Received":"by 2002:a2e:9dcc:: with SMTP id\n\tx12mr10184856ljj.415.1594024307290; \n\tMon, 06 Jul 2020 01:31:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 6 Jul 2020 09:31:31 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11197,"web_url":"https://patchwork.libcamera.org/comment/11197/","msgid":"<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>","date":"2020-07-06T09:16:11","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> Instead of receiving sensor orientation configuration from the IPA,\n> retrieve it from the CameraSensor Rotation property, and configure the\n> HFLIP and VFLIP controls accordingly.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> 1 file changed, 8 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 74bee6895dcd..fda6831e6a7e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -16,6 +16,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> \t\t\tnullptr);\n>\n> +\t/* Configure the H/V flip controls based on the sensor rotation. */\n> +\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> +\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> +\tctrls.set(V4L2_CID_HFLIP, static_cast(!!rotation));\n> +\tctrls.set(V4L2_CID_VFLIP, static_cast(!!rotation));\n\nmmm, I see in the IPA this, which is where I assume you got this from\n\n\t\tctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n\t\tctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n\nWhere the 'orientation' flag is retrieved from\n\t\tRPi::CamTransform orientation = helper_->GetOrientation();\n\nand expressed through members of this enumeration\n static constexpr CamTransform CamTransform_IDENTITY = 0;\n static constexpr CamTransform CamTransform_HFLIP = 1;\n static constexpr CamTransform CamTransform_VFLIP = 2;\n\nNow, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\ninterval, shouldn't we:\n if (rotation == 180)\n HFLIP\n if (rotation == 90 || roation == 270)\n VFLIP\n ?\n\n\n> +\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> +\n> \treturn 0;\n> }\n>\n> @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> \t\t\t\t\t { V4L2_CID_EXPOSURE, action.data[1] } });\n> \t\t\tsensorMetadata_ = action.data[2];\n> \t\t}\n> -\n> -\t\t/* Set the sensor orientation here as well. */\n> -\t\tControlList controls = action.controls[0];\n> -\t\tunicam_[Unicam::Image].dev()->setControls(&controls);\n\nDo you think we can now remove the custom rotation handling from the\nIPA helpers ?\n\n> \t\treturn;\n> \t}\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 52AAEBD790\n\tfor ;\n\tMon, 6 Jul 2020 09:12:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2605B60E01;\n\tMon, 6 Jul 2020 11:12:40 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7218603B1\n\tfor ;\n\tMon, 6 Jul 2020 11:12:38 +0200 (CEST)","from uno.localdomain (host-79-34-235-173.business.telecomitalia.it\n\t[79.34.235.173]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id CF28D200006;\n\tMon, 6 Jul 2020 09:12:37 +0000 (UTC)"],"Date":"Mon, 6 Jul 2020 11:16:11 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11201,"web_url":"https://patchwork.libcamera.org/comment/11201/","msgid":"","date":"2020-07-06T09:29:21","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo, Laurent, everyone!\n\nI have a couple of questions.\n\n1. Does this allow an application to set a transformation when the\ncamera is started? For example, it's not uncommon for a horizontal\nflip to be requested from the camera depending on the application.\nCertainly in the Raspberry Pi world you can't tell a priori what a\nsensor's orientation is.\n\n2nd question below...\n\nOn Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n>\n> Hi Laurent,\n>\n> On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > Instead of receiving sensor orientation configuration from the IPA,\n> > retrieve it from the CameraSensor Rotation property, and configure the\n> > HFLIP and VFLIP controls accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > 1 file changed, 8 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 74bee6895dcd..fda6831e6a7e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > nullptr);\n> >\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>\n> mmm, I see in the IPA this, which is where I assume you got this from\n>\n> ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n>\n> Where the 'orientation' flag is retrieved from\n> RPi::CamTransform orientation = helper_->GetOrientation();\n>\n> and expressed through members of this enumeration\n> static constexpr CamTransform CamTransform_IDENTITY = 0;\n> static constexpr CamTransform CamTransform_HFLIP = 1;\n> static constexpr CamTransform CamTransform_VFLIP = 2;\n>\n> Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> interval, shouldn't we:\n> if (rotation == 180)\n> HFLIP\n> if (rotation == 90 || roation == 270)\n> VFLIP\n> ?\n\nHow are we handling transformations that aren't rotations, e.g.\nhorizontal flip. As I said previously, it's a significant use case.\nAlso, how are we handling platforms that don't support things like\n90/270 degree rotations - it's certain that many platforms won't be\nable to do that.\n\nBest regards\nDavid\n\n>\n>\n> > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > sensorMetadata_ = action.data[2];\n> > }\n> > -\n> > - /* Set the sensor orientation here as well. */\n> > - ControlList controls = action.controls[0];\n> > - unicam_[Unicam::Image].dev()->setControls(&controls);\n>\n> Do you think we can now remove the custom rotation handling from the\n> IPA helpers ?\n>\n> > return;\n> > }\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 77A8FBD790\n\tfor ;\n\tMon, 6 Jul 2020 09:29:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A9EA60E01;\n\tMon, 6 Jul 2020 11:29:37 +0200 (CEST)","from mail-ot1-x342.google.com (mail-ot1-x342.google.com\n\t[IPv6:2607:f8b0:4864:20::342])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1DB1603B1\n\tfor ;\n\tMon, 6 Jul 2020 11:29:34 +0200 (CEST)","by mail-ot1-x342.google.com with SMTP id e90so628774ote.1\n\tfor ;\n\tMon, 06 Jul 2020 02:29:34 -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=\"UhGAOQPY\"; 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; bh=sEA9YJ2S86yeMiz9JfDX637WD0bPGL49uUA6Ka5NAqI=;\n\tb=UhGAOQPYzBW/vbvba3RY7iyO2TjOBeO10cCF+MsfNhJHac/dF2+P80GVdF/+ngf11R\n\tJQ77l03oHMNjootA6P7PCBAW9zVJrqoFq97WPRDV9snG+HbTH26XE8Ta3zpN/YMq680r\n\tvkyJeKisTQv7TjfInIOc5w5KqSc/UktnDCoR61CkDcw+vTv0Mfl2gI6ThBLgNDHeD4gN\n\tB0PSf5ZeBDmWCtcPTe9IMH0h4+ZA/OUoY2aQSdvN/dIEhjn37FHdk3MV8IORpmTcmIGq\n\tt0acl0EOGaYa1C5FsTotJ5jVcHK6Gv2elRtdJW10mYNyr7Bw+JeF2IEZKW0y/yRReTU7\n\tQi8g==","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;\n\tbh=sEA9YJ2S86yeMiz9JfDX637WD0bPGL49uUA6Ka5NAqI=;\n\tb=otU4TE9dA87SgXPnboIRaPLPCIoTLHqlP3uEZXcvwMdjbXe8tnvpFBOF21L8Yt716v\n\thHYuZ9T2QVTs1+lhfXM43mu55fF9ozSJAcdlObrvWqPou3XDT++uu3d3Qj394VvlkenT\n\tb+zIn94MtEnRqZkS/MzfnhagTUf3CRJggiutHoJgh8CLVHIIWyMPMAUPp9/c75LSS4bV\n\ttFDIFAEBRJXm7PR2wA0b9sy7Sa0R9M4rwWSWJAl8dQ3Eqvkfj1itZAlkHQRbVtkgOUc+\n\t9siLD1dbNve5XAzgEgdi4ZwI54XGqjqe/eQdgRU76pMrl/s/JFEf37FNIRNK+rgZLsjM\n\tjV3A==","X-Gm-Message-State":"AOAM532shCFo6OA6HiX/UjusTuS0/FQ8NF4ITNRJ1AqxtqTR6ov9weSM\n\t5mcY8D6A156Qu6Fh9W78bliGiRzZyNytcYamE70LxA==","X-Google-Smtp-Source":"ABdhPJzTWKHnKMrwwDUivGgTcRSL9FWHLrWiiSxPj8PKiYfA02/2uEB9lbyeKMmtYTCi+nE+1Q0F/HOYKUsShh3K0qI=","X-Received":"by 2002:a05:6830:14d4:: with SMTP id\n\tt20mr15782943otq.166.1594027772490; \n\tMon, 06 Jul 2020 02:29:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>","In-Reply-To":"<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>","From":"David Plowman ","Date":"Mon, 6 Jul 2020 10:29:21 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11204,"web_url":"https://patchwork.libcamera.org/comment/11204/","msgid":"<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>","date":"2020-07-06T10:04:37","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:\n> Hi Jacopo, Laurent, everyone!\n>\n> I have a couple of questions.\n>\n> 1. Does this allow an application to set a transformation when the\n> camera is started? For example, it's not uncommon for a horizontal\n> flip to be requested from the camera depending on the application.\n> Certainly in the Raspberry Pi world you can't tell a priori what a\n> sensor's orientation is.\n>\n\nIsn't this mostly related to allowing application provide a list of\ncontrols at camera start/configure time ? Those will have to be taken\ninto account and matched with the camera sensor reported orientation\nwhich is handled here I guess.\n\n> 2nd question below...\n>\n> On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n> >\n> > Hi Laurent,\n> >\n> > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > > Instead of receiving sensor orientation configuration from the IPA,\n> > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > HFLIP and VFLIP controls accordingly.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -16,6 +16,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > nullptr);\n> > >\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> >\n> > mmm, I see in the IPA this, which is where I assume you got this from\n> >\n> > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> >\n> > Where the 'orientation' flag is retrieved from\n> > RPi::CamTransform orientation = helper_->GetOrientation();\n> >\n> > and expressed through members of this enumeration\n> > static constexpr CamTransform CamTransform_IDENTITY = 0;\n> > static constexpr CamTransform CamTransform_HFLIP = 1;\n> > static constexpr CamTransform CamTransform_VFLIP = 2;\n> >\n> > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> > interval, shouldn't we:\n> > if (rotation == 180)\n> > HFLIP\n\nThis should be VFLIP, as the control reads \"mirror in the vertical\ndirection\", not \"along the horizontal axis\" as I interpreted it\n\n> > if (rotation == 90 || roation == 270)\n> > VFLIP\n\nAnd this is just wrong. Mirroring in H/V directions won't compensate\nfor such rotations.\n\n> > ?\n>\n> How are we handling transformations that aren't rotations, e.g.\n> horizontal flip. As I said previously, it's a significant use case.\n\nI think they merit a control to let application handle them\nI think this was meant to compensate the camera mounting rotation\nonly.\n\n> Also, how are we handling platforms that don't support things like\n> 90/270 degree rotations - it's certain that many platforms won't be\n> able to do that.\n\nThat is a more generic question: how do pipeline handlers advertise\nwhich control they support, or do you think H/V flips are different ?\n\nThanks\n j\n\n>\n> Best regards\n> David\n>\n> >\n> >\n> > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > sensorMetadata_ = action.data[2];\n> > > }\n> > > -\n> > > - /* Set the sensor orientation here as well. */\n> > > - ControlList controls = action.controls[0];\n> > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> >\n> > Do you think we can now remove the custom rotation handling from the\n> > IPA helpers ?\n> >\n> > > return;\n> > > }\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 DAE18BD792\n\tfor ;\n\tMon, 6 Jul 2020 10:01:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90E7060E01;\n\tMon, 6 Jul 2020 12:01:06 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83153603AA\n\tfor ;\n\tMon, 6 Jul 2020 12:01:05 +0200 (CEST)","from uno.localdomain (host-79-34-235-173.business.telecomitalia.it\n\t[79.34.235.173]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id DBCDF60014;\n\tMon, 6 Jul 2020 10:01:03 +0000 (UTC)"],"X-Originating-IP":"79.34.235.173","Date":"Mon, 6 Jul 2020 12:04:37 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11206,"web_url":"https://patchwork.libcamera.org/comment/11206/","msgid":"","date":"2020-07-06T10:41:13","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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 the quick reply! Let me just clarify my understanding of\nyour answers...\n\nOn Mon, 6 Jul 2020 at 11:01, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:\n> > Hi Jacopo, Laurent, everyone!\n> >\n> > I have a couple of questions.\n> >\n> > 1. Does this allow an application to set a transformation when the\n> > camera is started? For example, it's not uncommon for a horizontal\n> > flip to be requested from the camera depending on the application.\n> > Certainly in the Raspberry Pi world you can't tell a priori what a\n> > sensor's orientation is.\n> >\n>\n> Isn't this mostly related to allowing application provide a list of\n> controls at camera start/configure time ? Those will have to be taken\n> into account and matched with the camera sensor reported orientation\n> which is handled here I guess.\n\nSo far as I'm aware there is no control for transformations (h/vflip,\neven transpose) currently, is that correct? I'm also not sure the\nusual request->controls() mechanism is the right one as it sort-of\nimplies you can set it per frame, which I think is something we\nshouldn't allow.\n\nPerhaps it belongs in the StreamConfiguration? Some platforms might\nallow per-stream transformations, others (many?) will not. What do\npeople think?\n\n>\n> > 2nd question below...\n> >\n> > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n> > >\n> > > Hi Laurent,\n> > >\n> > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > > > Instead of receiving sensor orientation configuration from the IPA,\n> > > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > > HFLIP and VFLIP controls accordingly.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -16,6 +16,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > > nullptr);\n> > > >\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> > >\n> > > mmm, I see in the IPA this, which is where I assume you got this from\n> > >\n> > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> > >\n> > > Where the 'orientation' flag is retrieved from\n> > > RPi::CamTransform orientation = helper_->GetOrientation();\n> > >\n> > > and expressed through members of this enumeration\n> > > static constexpr CamTransform CamTransform_IDENTITY = 0;\n> > > static constexpr CamTransform CamTransform_HFLIP = 1;\n> > > static constexpr CamTransform CamTransform_VFLIP = 2;\n> > >\n> > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> > > interval, shouldn't we:\n> > > if (rotation == 180)\n> > > HFLIP\n>\n> This should be VFLIP, as the control reads \"mirror in the vertical\n> direction\", not \"along the horizontal axis\" as I interpreted it\n>\n> > > if (rotation == 90 || roation == 270)\n> > > VFLIP\n>\n> And this is just wrong. Mirroring in H/V directions won't compensate\n> for such rotations.\n>\n> > > ?\n> >\n> > How are we handling transformations that aren't rotations, e.g.\n> > horizontal flip. As I said previously, it's a significant use case.\n>\n> I think they merit a control to let application handle them\n> I think this was meant to compensate the camera mounting rotation\n> only.\n>\n> > Also, how are we handling platforms that don't support things like\n> > 90/270 degree rotations - it's certain that many platforms won't be\n> > able to do that.\n>\n> That is a more generic question: how do pipeline handlers advertise\n> which control they support, or do you think H/V flips are different ?\n\nAs I said above, I'm not sure the usual \"controls\" mechanism is the\nright one, and it's also not clear to me how a platform should\nadvertise what it supports. (Do we have any mechanism for signalling\nthat kind of thing?) Perhaps one could take the view that, for the\ntime being, we just overwrite anything we don't handle and report the\nconfiguration as \"adjusted\"?\n\n(Perhaps I am hijacking the original discussion... in which case\nplease say and I'll re-start it on its own.)\n\nBest regards\nDavid\n\n>\n> Thanks\n> j\n>\n> >\n> > Best regards\n> > David\n> >\n> > >\n> > >\n> > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > > sensorMetadata_ = action.data[2];\n> > > > }\n> > > > -\n> > > > - /* Set the sensor orientation here as well. */\n> > > > - ControlList controls = action.controls[0];\n> > > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > >\n> > > Do you think we can now remove the custom rotation handling from the\n> > > IPA helpers ?\n> > >\n> > > > return;\n> > > > }\n> > > >\n> > > > --\n> > > > Regards,\n> > > >\n> > > > Laurent Pinchart\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel","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 5A338BD792\n\tfor ;\n\tMon, 6 Jul 2020 10:41:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB162603AB;\n\tMon, 6 Jul 2020 12:41:32 +0200 (CEST)","from mail-ot1-x344.google.com (mail-ot1-x344.google.com\n\t[IPv6:2607:f8b0:4864:20::344])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8740E603AA\n\tfor ;\n\tMon, 6 Jul 2020 12:41:31 +0200 (CEST)","by mail-ot1-x344.google.com with SMTP id w17so23437302otl.4\n\tfor ;\n\tMon, 06 Jul 2020 03:41:31 -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=\"G5xhFUHu\"; 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; bh=Z5efgIMAmb0otxZzTo5py0SIHx0grCht1aY7ILgastk=;\n\tb=G5xhFUHu+TZNo+czSJmZScUmf4nnjsct/0GidckmQAO33uluyXcM3njK6gBOY1gLMn\n\tuaH484Ttt4heSIcU5zP5E5mEErYiVSsAJHuunDadRqltgnOK22706lgESrwLe1zMSw+l\n\t0jQ3X1YF6mlHmAEiOoRy5xXjU8dPrbA/6WTaoENM0CZsoXeOvlLjOB9q8V/bmpabzldr\n\t9Gvz0qGTkiC/iWHG8aa2hWUGDi2a21hIGattNR0UD+sqGW0+iZi5sllJ1/VxLgLLsMsC\n\tERrXy5bhSFh4TPHNfWqcVeHVfBPFWOc7MjW7L7kUpslEVbylCVhbYf7Py2ufik7ucW1D\n\tMRSQ==","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;\n\tbh=Z5efgIMAmb0otxZzTo5py0SIHx0grCht1aY7ILgastk=;\n\tb=EJX4qk3KL+m/BuDxDIZKlTNOkNBRqnmmb9bOB+xinWZiSSskzlNZhwkYzuIe67zFCa\n\tlePVTug9kJ8IjoL7TvbLEWwLtoutH/6TcQWuIWvr6GT91psISwzQ/AU/TEgFzAh6idQs\n\tnwCPNzMubqh0B4QmURvk79Ff/K6E6Fx4naIDfXsKzyJXhscGEGwHSxSMIIglpeEawUf2\n\tj1DrGl2ZphdbAKmt1jZSGjc8Sk6jEAFauWcEKQlUkhPQSj7ypmIIN1aEMG54zupwRRRP\n\tzbdTwOCAHJ/8wBpfvk666AzZ/2WIGKOxsTzSY6cYWP0U5PYeCLaKf+YBoFmVSvc0e+Ts\n\tjZoA==","X-Gm-Message-State":"AOAM532pN5I3QICdeaGZoknqf7u33gR4mAGZmAxm/dc+la1GiZFyZbyE\n\tDJNP6UT5JkPIyfbV562CDD37R797mrQAqTU3wQGkOg==","X-Google-Smtp-Source":"ABdhPJxLehVp5EHe3Hw5S4QkWQky7kTkAwAyxL747+8Q9C4Libm28sMPcLnEOxPkS5bWV2hTX9G66j1raPTVmSfNrTM=","X-Received":"by 2002:a05:6830:14d4:: with SMTP id\n\tt20mr15979955otq.166.1594032090039; \n\tMon, 06 Jul 2020 03:41:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>\n\t\n\t<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>","In-Reply-To":"<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>","From":"David Plowman ","Date":"Mon, 6 Jul 2020 11:41:13 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11209,"web_url":"https://patchwork.libcamera.org/comment/11209/","msgid":"<20200706112120.GC5912@pendragon.ideasonboard.com>","date":"2020-07-06T11:21:20","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jul 06, 2020 at 09:31:31AM +0100, Naushir Patuck wrote:\n> On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote:\n> >\n> > Instead of receiving sensor orientation configuration from the IPA,\n> > retrieve it from the CameraSensor Rotation property, and configure the\n> > HFLIP and VFLIP controls accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > 1 file changed, 8 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 74bee6895dcd..fda6831e6a7e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > nullptr);\n> >\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> \n> Are you missing a \">> 1\" or similar in the above code? Otherwise,\n> both H/V FLIP will be always set to the same value.\n\nThat's because the RPi IPA currently handles 0° and 180° rotations only,\nwithout separate control of h/v flip. We need to add h/v flip controls,\nand they will then be combined here in the pipeline handler.\n\n> > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > sensorMetadata_ = action.data[2];\n> > }\n> > -\n> > - /* Set the sensor orientation here as well. */\n> > - ControlList controls = action.controls[0];\n> > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > return;\n> > }\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 3581DBD790\n\tfor ;\n\tMon, 6 Jul 2020 11:21:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A184D60E01;\n\tMon, 6 Jul 2020 13:21:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86676603AA\n\tfor ;\n\tMon, 6 Jul 2020 13:21:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0ADA797E;\n\tMon, 6 Jul 2020 13:21:23 +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=\"YGT5f6du\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594034484;\n\tbh=Jcd4omjxTTaHC3GJtWaDdAnS67obnx7DHlCZL6hJrJk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YGT5f6dumVjpvvCb7SxkHg30nq3lhRPDyXK4rsF+w82JEDyl8s3+ZTeAupi8msx3H\n\tC9OmMcARFwMYqWg2Ya5xMlXyY/uCO/RTDXqz+Lj65gkTmIn0nCw3bouueovU5Y+/Az\n\tA7JsC8moX41/qQ1msT2zxuBJm15S6UPiZVGQtSWM=","Date":"Mon, 6 Jul 2020 14:21:20 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200706112120.GC5912@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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":11210,"web_url":"https://patchwork.libcamera.org/comment/11210/","msgid":"<20200706112349.GD5912@pendragon.ideasonboard.com>","date":"2020-07-06T11:23:49","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 06, 2020 at 11:16:11AM +0200, Jacopo Mondi wrote:\n> On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > Instead of receiving sensor orientation configuration from the IPA,\n> > retrieve it from the CameraSensor Rotation property, and configure the\n> > HFLIP and VFLIP controls accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > 1 file changed, 8 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 74bee6895dcd..fda6831e6a7e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > \t\t\tnullptr);\n> >\n> > +\t/* Configure the H/V flip controls based on the sensor rotation. */\n> > +\tControlList ctrls(unicam_[Unicam::Image].dev()->controls());\n> > +\tint32_t rotation = sensor_->properties().get(properties::Rotation);\n> > +\tctrls.set(V4L2_CID_HFLIP, static_cast(!!rotation));\n> > +\tctrls.set(V4L2_CID_VFLIP, static_cast(!!rotation));\n> \n> mmm, I see in the IPA this, which is where I assume you got this from\n> \n> \t\tctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> \t\tctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> \n> Where the 'orientation' flag is retrieved from\n> \t\tRPi::CamTransform orientation = helper_->GetOrientation();\n> \n> and expressed through members of this enumeration\n> static constexpr CamTransform CamTransform_IDENTITY = 0;\n> static constexpr CamTransform CamTransform_HFLIP = 1;\n> static constexpr CamTransform CamTransform_VFLIP = 2;\n> \n> Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> interval, shouldn't we:\n> if (rotation == 180)\n> HFLIP\n> if (rotation == 90 || roation == 270)\n> VFLIP\n> ?\n\n90° or 270° rotations can't be achieved through flipping, they require a\nreal rotation engine. Only 180° can be achieved with H+V flip. Note that\nthe RPi IPA currently sets either both or neither of the flip controls,\nbased on hard-coded per-sensor values.\n\n> > +\tunicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > \t\t\t\t\t { V4L2_CID_EXPOSURE, action.data[1] } });\n> > \t\t\tsensorMetadata_ = action.data[2];\n> > \t\t}\n> > -\n> > -\t\t/* Set the sensor orientation here as well. */\n> > -\t\tControlList controls = action.controls[0];\n> > -\t\tunicam_[Unicam::Image].dev()->setControls(&controls);\n> \n> Do you think we can now remove the custom rotation handling from the\n> IPA helpers ?\n\nSee other patches in this series :-)\n\n> > \t\treturn;\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 0806EBD790\n\tfor ;\n\tMon, 6 Jul 2020 11:23:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F97160E01;\n\tMon, 6 Jul 2020 13:23:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6C85603AA\n\tfor ;\n\tMon, 6 Jul 2020 13:23:53 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2EBA197E;\n\tMon, 6 Jul 2020 13:23:53 +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=\"sAORytmQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594034633;\n\tbh=CKK9zn0kxSOFNa5iLmj40QIbwrKU3TRDfsrj8HPbK3I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sAORytmQvdaEqb8SXfiEXOhndKTpzFB0s3bJEOgyX67rbqvAEPBM5I6KTEEjDNIck\n\t2rbxbqZW6AfFAITC3ag29rM85tSGJtif7v2AuJ3sTgf/YB7hCwanmkjt/5Zx8A4b0m\n\tnQh+u91EepISv7JYn9ilv1kxNK552qOEsEsjLU7A=","Date":"Mon, 6 Jul 2020 14:23:49 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200706112349.GD5912@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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":11211,"web_url":"https://patchwork.libcamera.org/comment/11211/","msgid":"<20200706112931.GE5912@pendragon.ideasonboard.com>","date":"2020-07-06T11:29:31","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David and Jacopo,\n\nOn Mon, Jul 06, 2020 at 11:41:13AM +0100, David Plowman wrote:\n> On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi wrote:\n> > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:\n> >> Hi Jacopo, Laurent, everyone!\n> >>\n> >> I have a couple of questions.\n> >>\n> >> 1. Does this allow an application to set a transformation when the\n> >> camera is started? For example, it's not uncommon for a horizontal\n> >> flip to be requested from the camera depending on the application.\n> >> Certainly in the Raspberry Pi world you can't tell a priori what a\n> >> sensor's orientation is.\n> >\n> > Isn't this mostly related to allowing application provide a list of\n> > controls at camera start/configure time ? Those will have to be taken\n> > into account and matched with the camera sensor reported orientation\n> > which is handled here I guess.\n> \n> So far as I'm aware there is no control for transformations (h/vflip,\n> even transpose) currently, is that correct?\n\nCorrect. I think they should be added though.\n\n> I'm also not sure the\n> usual request->controls() mechanism is the right one as it sort-of\n> implies you can set it per frame, which I think is something we\n> shouldn't allow.\n\nWhy not ? If platforms allow per-frame control of flipping, wouldn't it\nmake sense to allow it in the API ?\n\nWe've thought about adding a ControlList to CameraConfiguration to set\ncontrols that can't change per frame. Pipeline handlers could then\ndecide if they want to support the flip controls per frame, or only at\nconfigure time. We could extend the ControlInfo class to report whether\na control can be set per frame or only at configure time.\n\n> Perhaps it belongs in the StreamConfiguration? Some platforms might\n> allow per-stream transformations, others (many?) will not. What do\n> people think?\n> \n> >> 2nd question below...\n> >>\n> >> On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n> >>>\n> >>> Hi Laurent,\n> >>>\n> >>> On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> >>>> Instead of receiving sensor orientation configuration from the IPA,\n> >>>> retrieve it from the CameraSensor Rotation property, and configure the\n> >>>> HFLIP and VFLIP controls accordingly.\n> >>>>\n> >>>> Signed-off-by: Laurent Pinchart \n> >>>> ---\n> >>>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> >>>> 1 file changed, 8 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>> index 74bee6895dcd..fda6831e6a7e 100644\n> >>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>> @@ -16,6 +16,7 @@\n> >>>> #include \n> >>>> #include \n> >>>> #include \n> >>>> +#include \n> >>>> #include \n> >>>> #include \n> >>>>\n> >>>> @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> >>>> ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> >>>> nullptr);\n> >>>>\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> >>>\n> >>> mmm, I see in the IPA this, which is where I assume you got this from\n> >>>\n> >>> ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> >>> ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> >>>\n> >>> Where the 'orientation' flag is retrieved from\n> >>> RPi::CamTransform orientation = helper_->GetOrientation();\n> >>>\n> >>> and expressed through members of this enumeration\n> >>> static constexpr CamTransform CamTransform_IDENTITY = 0;\n> >>> static constexpr CamTransform CamTransform_HFLIP = 1;\n> >>> static constexpr CamTransform CamTransform_VFLIP = 2;\n> >>>\n> >>> Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> >>> interval, shouldn't we:\n> >>> if (rotation == 180)\n> >>> HFLIP\n> >\n> > This should be VFLIP, as the control reads \"mirror in the vertical\n> > direction\", not \"along the horizontal axis\" as I interpreted it\n> >\n> >>> if (rotation == 90 || roation == 270)\n> >>> VFLIP\n> >\n> > And this is just wrong. Mirroring in H/V directions won't compensate\n> > for such rotations.\n> >\n> >>> ?\n> >>\n> >> How are we handling transformations that aren't rotations, e.g.\n> >> horizontal flip. As I said previously, it's a significant use case.\n> >\n> > I think they merit a control to let application handle them\n> > I think this was meant to compensate the camera mounting rotation\n> > only.\n> >\n> >> Also, how are we handling platforms that don't support things like\n> >> 90/270 degree rotations - it's certain that many platforms won't be\n> >> able to do that.\n> >\n> > That is a more generic question: how do pipeline handlers advertise\n> > which control they support, or do you think H/V flips are different ?\n> \n> As I said above, I'm not sure the usual \"controls\" mechanism is the\n> right one, and it's also not clear to me how a platform should\n> advertise what it supports. (Do we have any mechanism for signalling\n> that kind of thing?) Perhaps one could take the view that, for the\n> time being, we just overwrite anything we don't handle and report the\n> configuration as \"adjusted\"?\n>\n> (Perhaps I am hijacking the original discussion... in which case\n> please say and I'll re-start it on its own.)\n\nHijacking discussions is how we usually operate, no worries about that\n:-) I don't think it should prevent this patch from being merged though.\n\n> >>>> + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> >>>> +\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> >>>> { V4L2_CID_EXPOSURE, action.data[1] } });\n> >>>> sensorMetadata_ = action.data[2];\n> >>>> }\n> >>>> -\n> >>>> - /* Set the sensor orientation here as well. */\n> >>>> - ControlList controls = action.controls[0];\n> >>>> - unicam_[Unicam::Image].dev()->setControls(&controls);\n> >>>\n> >>> Do you think we can now remove the custom rotation handling from the\n> >>> IPA helpers ?\n> >>>\n> >>>> return;\n> >>>> }\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 C8AE3BD792\n\tfor ;\n\tMon, 6 Jul 2020 11:29:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63EFF60E01;\n\tMon, 6 Jul 2020 13:29:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D84E5603AA\n\tfor ;\n\tMon, 6 Jul 2020 13:29:35 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 52E6197E;\n\tMon, 6 Jul 2020 13:29:35 +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=\"iIIwHOFO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594034975;\n\tbh=LuJxRE1GegUHfzVbj4q+GLb03UjijapeOvfoj6YMq6I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iIIwHOFOV4DKJGsG7aqtEtIXjEwZ/WIaMJfunBL8+ZNXd/Y3BBN5yC5byPsVTpgWQ\n\t1YZEJGvYnMiQIc5CQq2Dk6avHijGDB6VTHpnsY4MtQdUD6NMLVfEd4XMlRfH/Vj4Gm\n\teAPOZJYZXRNcmGYmJQsJpu4YwetcOeDy24qtCztA=","Date":"Mon, 6 Jul 2020 14:29:31 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20200706112931.GE5912@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>\n\t\n\t<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11222,"web_url":"https://patchwork.libcamera.org/comment/11222/","msgid":"","date":"2020-07-06T15:02:17","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David and Jacopo,\n\n\nOn Mon, 6 Jul 2020 at 11:41, David Plowman\n wrote:\n>\n> Hi Jacopo\n>\n> Thanks for the quick reply! Let me just clarify my understanding of\n> your answers...\n>\n> On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:\n> > > Hi Jacopo, Laurent, everyone!\n> > >\n> > > I have a couple of questions.\n> > >\n> > > 1. Does this allow an application to set a transformation when the\n> > > camera is started? For example, it's not uncommon for a horizontal\n> > > flip to be requested from the camera depending on the application.\n> > > Certainly in the Raspberry Pi world you can't tell a priori what a\n> > > sensor's orientation is.\n> > >\n> >\n> > Isn't this mostly related to allowing application provide a list of\n> > controls at camera start/configure time ? Those will have to be taken\n> > into account and matched with the camera sensor reported orientation\n> > which is handled here I guess.\n>\n> So far as I'm aware there is no control for transformations (h/vflip,\n> even transpose) currently, is that correct? I'm also not sure the\n> usual request->controls() mechanism is the right one as it sort-of\n> implies you can set it per frame, which I think is something we\n> shouldn't allow.\n>\n> Perhaps it belongs in the StreamConfiguration? Some platforms might\n> allow per-stream transformations, others (many?) will not. What do\n> people think?\n>\n> >\n> > > 2nd question below...\n> > >\n> > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n> > > >\n> > > > Hi Laurent,\n> > > >\n> > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > > > > Instead of receiving sensor orientation configuration from the IPA,\n> > > > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > > > HFLIP and VFLIP controls accordingly.\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -16,6 +16,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > > > nullptr);\n> > > > >\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> > > >\n> > > > mmm, I see in the IPA this, which is where I assume you got this from\n> > > >\n> > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> > > >\n> > > > Where the 'orientation' flag is retrieved from\n> > > > RPi::CamTransform orientation = helper_->GetOrientation();\n> > > >\n> > > > and expressed through members of this enumeration\n> > > > static constexpr CamTransform CamTransform_IDENTITY = 0;\n> > > > static constexpr CamTransform CamTransform_HFLIP = 1;\n> > > > static constexpr CamTransform CamTransform_VFLIP = 2;\n> > > >\n> > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> > > > interval, shouldn't we:\n> > > > if (rotation == 180)\n> > > > HFLIP\n> >\n> > This should be VFLIP, as the control reads \"mirror in the vertical\n> > direction\", not \"along the horizontal axis\" as I interpreted it\n> >\n> > > > if (rotation == 90 || roation == 270)\n> > > > VFLIP\n> >\n> > And this is just wrong. Mirroring in H/V directions won't compensate\n> > for such rotations.\n> >\n> > > > ?\n> > >\n> > > How are we handling transformations that aren't rotations, e.g.\n> > > horizontal flip. As I said previously, it's a significant use case.\n> >\n> > I think they merit a control to let application handle them\n> > I think this was meant to compensate the camera mounting rotation\n> > only.\n> >\n> > > Also, how are we handling platforms that don't support things like\n> > > 90/270 degree rotations - it's certain that many platforms won't be\n> > > able to do that.\n> >\n> > That is a more generic question: how do pipeline handlers advertise\n> > which control they support, or do you think H/V flips are different ?\n>\n> As I said above, I'm not sure the usual \"controls\" mechanism is the\n> right one, and it's also not clear to me how a platform should\n> advertise what it supports. (Do we have any mechanism for signalling\n> that kind of thing?) Perhaps one could take the view that, for the\n> time being, we just overwrite anything we don't handle and report the\n> configuration as \"adjusted\"?\n>\n> (Perhaps I am hijacking the original discussion... in which case\n> please say and I'll re-start it on its own.)\n\nIf I am not mistaken, the pipeline handlers need to maintain a list of\ncontrols supported, like in include/libcamera/ipa/raspberrypi.h\n(RPiControls). This then gets exported to the CameraData base class\nmember controlInfo_. ARequest will not allow setting a\nlibcamera::control that is not listed in CameraData::controlInfo_\n\nRegards,\nNaush\n\n\n>\n> Best regards\n> David\n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > Best regards\n> > > David\n> > >\n> > > >\n> > > >\n> > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > > > +\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > > > sensorMetadata_ = action.data[2];\n> > > > > }\n> > > > > -\n> > > > > - /* Set the sensor orientation here as well. */\n> > > > > - ControlList controls = action.controls[0];\n> > > > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > >\n> > > > Do you think we can now remove the custom rotation handling from the\n> > > > IPA helpers ?\n> > > >\n> > > > > return;\n> > > > > }\n> > > > >\n> > > > > --\n> > > > > Regards,\n> > > > >\n> > > > > Laurent Pinchart\n> > > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 DFF6ABD790\n\tfor ;\n\tMon, 6 Jul 2020 15:02:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A9C360E01;\n\tMon, 6 Jul 2020 17:02:36 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE06B603AA\n\tfor ;\n\tMon, 6 Jul 2020 17:02:34 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id q7so32528263ljm.1\n\tfor ;\n\tMon, 06 Jul 2020 08:02:34 -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=\"VvLLFPe5\"; 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; bh=vaNv2MRFI/LVGqeU+K0pijLjA/IPXGEQU8ictVmzGrs=;\n\tb=VvLLFPe5zUhnlGkvGZQLjMYz39yFvlXKRWLrAzLOPvaBxJkrEym9kQCp6nrPIVjPYI\n\tpY21hbqr+KlBk60zxo2Wkm06x317jdEz4vv07HIfLYcF4a6Y3aWeX7e7l48vHNncBgXm\n\tqW3ofcwkzRkQAUzMwXU1Z4SNmYdVKnPqawboHPY4r59/bDp35l+FTyBYGZhZKOsR09vL\n\tclfqzVnO7aDwSNuqK2hU+f3gMBaRIULNgZprr9TqTxiyZolpjLguI8TR0lU1lzTyFXWg\n\trxDW3KZXz+M8/+R61jr6D1DodCJp/hhflGApRu6taYLOPbaqfWObL4M39Zz3E+nhVw/A\n\tkluA==","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;\n\tbh=vaNv2MRFI/LVGqeU+K0pijLjA/IPXGEQU8ictVmzGrs=;\n\tb=VSrOgSltPZrQBWWc8pSibiUMdOnT1SON3BHy3M+EgXrmoYH5j3ymj/6NgpKiqqDB5t\n\teL3vtOVwv9+4gypTJ7MXOj/UAXaapOorFWWWEMAsfKOkercjGU/y060cq7EvidhbpG4r\n\ttwpsDqoCrnd0FZ/15IXguvw0+vgSsDXpsPeLkzupHm1QNTToYnxT9m7EIw60SAGZj1SB\n\tnLPM3f2bYuHfnuggJF84qFEhigT2If5WgzFoUz/URXmXNCIebb3X0bWakoNjBkqtL6gC\n\t4BErii93LpmSD7TBs++OiTgTmmMlEdKVyL42CZKasvQI+Op/E/WSWFcdGkMB308Lan5f\n\tioXQ==","X-Gm-Message-State":"AOAM5323l0ehmRa8g74kLhwiKtMtdI7kmxT6Fq/xDyQkFjI+m7ZOuNj9\n\t0IvTPCrAwxgCtYYnM7XnQ3uwUqT0sdqfusjiF8IYbg==","X-Google-Smtp-Source":"ABdhPJxqc9ov1y4W63fHAwlmiMqNmSYceZ6GlGKr/jWoe8DzUEj+DQGiaKvrdg4/BHKQRqGbmFLBc9u3LlkV3KPk900=","X-Received":"by 2002:a2e:9dcc:: with SMTP id\n\tx12mr11191666ljj.415.1594047753874; \n\tMon, 06 Jul 2020 08:02:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>\n\t\n\t<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 6 Jul 2020 16:02:17 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11258,"web_url":"https://patchwork.libcamera.org/comment/11258/","msgid":"<20200708204943.GS20298@pendragon.ideasonboard.com>","date":"2020-07-08T20:49:43","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jul 06, 2020 at 02:21:21PM +0300, Laurent Pinchart wrote:\n> On Mon, Jul 06, 2020 at 09:31:31AM +0100, Naushir Patuck wrote:\n> > On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote:\n> > >\n> > > Instead of receiving sensor orientation configuration from the IPA,\n> > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > HFLIP and VFLIP controls accordingly.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -16,6 +16,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > nullptr);\n> > >\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> > \n> > Are you missing a \">> 1\" or similar in the above code? Otherwise,\n> > both H/V FLIP will be always set to the same value.\n> \n> That's because the RPi IPA currently handles 0° and 180° rotations only,\n> without separate control of h/v flip. We need to add h/v flip controls,\n> and they will then be combined here in the pipeline handler.\n\nDo you think that's fine (in which case could you send me a R-b tag), or\nare changes needed ?\n\n> > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > sensorMetadata_ = action.data[2];\n> > > }\n> > > -\n> > > - /* Set the sensor orientation here as well. */\n> > > - ControlList controls = action.controls[0];\n> > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > return;\n> > > }\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 5F698BD792\n\tfor ;\n\tWed, 8 Jul 2020 20:49:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8A8E61163;\n\tWed, 8 Jul 2020 22:49:52 +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 1A18260E05\n\tfor ;\n\tWed, 8 Jul 2020 22:49:51 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1B167543;\n\tWed, 8 Jul 2020 22:49:49 +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=\"Pz/oCxtw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594241390;\n\tbh=J4Rlt5Fy/UI5g/xnWFsWeJSz5M6yn986Q8IQeY9vs8A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Pz/oCxtwmnyNUWt6rKcK1Mdw+uZyaPX2FTYxwOuDw0HrhVA5CNOGb8HCHzZ/dG8Hq\n\taT0PnnTqKprackCBtqccqcaJZv77CVXZ+7Wz5uZSjMvtS4C4moMCvTyjwpu64J6qP1\n\t+rUDNnV++XCpxv+VEsGrjff0MSx/2b1h9m6zSVs4=","Date":"Wed, 8 Jul 2020 23:49:43 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200708204943.GS20298@pendragon.ideasonboard.com>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t\n\t<20200706112120.GC5912@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200706112120.GC5912@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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":11259,"web_url":"https://patchwork.libcamera.org/comment/11259/","msgid":"","date":"2020-07-08T20:54:27","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 8 Jul 2020 at 21:49, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Mon, Jul 06, 2020 at 02:21:21PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jul 06, 2020 at 09:31:31AM +0100, Naushir Patuck wrote:\n> > > On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote:\n> > > >\n> > > > Instead of receiving sensor orientation configuration from the IPA,\n> > > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > > HFLIP and VFLIP controls accordingly.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -16,6 +16,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > > nullptr);\n> > > >\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> > >\n> > > Are you missing a \">> 1\" or similar in the above code? Otherwise,\n> > > both H/V FLIP will be always set to the same value.\n> >\n> > That's because the RPi IPA currently handles 0° and 180° rotations only,\n> > without separate control of h/v flip. We need to add h/v flip controls,\n> > and they will then be combined here in the pipeline handler.\n>\n> Do you think that's fine (in which case could you send me a R-b tag), or\n> are changes needed ?\n\nSorry, thought I had already replied to this one.\n\nReviewed-by: Naushir Patuck \n\n\n>\n> > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > > sensorMetadata_ = action.data[2];\n> > > > }\n> > > > -\n> > > > - /* Set the sensor orientation here as well. */\n> > > > - ControlList controls = action.controls[0];\n> > > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > > return;\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 25B8DBD790\n\tfor ;\n\tWed, 8 Jul 2020 20:54:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A257C61167;\n\tWed, 8 Jul 2020 22:54:45 +0200 (CEST)","from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A68E261159\n\tfor ;\n\tWed, 8 Jul 2020 22:54:44 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id g139so27677470lfd.10\n\tfor ;\n\tWed, 08 Jul 2020 13:54:44 -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=\"nHNiy9hh\"; 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=C3hxfhJK5wlJ+NIl/Fzi8joMjye43t4vbvZy0Eu8w0k=;\n\tb=nHNiy9hhFgbbtjUz81ShUN9dQM7Mdi43/7KoRIJXToEv5n05xBN1VS28h6fKshJJUq\n\tCunrTh48Jm5proDAvTkHLH4AdFmdaWkpnD9GaFVTXP5YR2Fj8uJlJrLaOGOR0r050ldW\n\tHOXfjPiKYlaYKpRJokBH9QYWQpKNSVddqyrIT0qG5TvvDdmb7zUKAIpqF9Eg15XQU65n\n\tS6++MXBNXBtCD+joAZfK5bhLbpDzddavQluYFBELvVW9cxz6vQWuO5Hajwbw5U/oGFyw\n\tTlwYZHI8+d9qc3XP9ElH1iRxfEpPNX5XvUF4jjT22nK2sB9g2JLduS8lhxWJ7S3hWLzR\n\tqFhA==","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=C3hxfhJK5wlJ+NIl/Fzi8joMjye43t4vbvZy0Eu8w0k=;\n\tb=NnCBjrTi5qnXCmdCsMruKhzZ2CxK7t/5J4XNyzsN+a13p3jjEOVwySmlRniQJrixiO\n\tRwRepub8xqM7FG+D6BIaLtsJzh8KkGHuPGRv0rxwp0NWZiXMogfIJxw/n9kP3HYvG6iJ\n\tTPARKuai901cOW4X6ifBviTM7G1wo+SRDJ6zafv+TQ7k0esulNLpkgz8lf4R558xnlIm\n\tqmTgv7MzP06jH9q6OxQBaNNB2YXCJcTy8Q77yotMO49yKDVIRwyesw09dX/u+rE0yBZR\n\tm5SxWwJ+IcELo6mMqVZhrCje2pNv5aYyM6bGsD3yagEAObHXblGlX/9y8sPvLP5Mbd+N\n\teKWQ==","X-Gm-Message-State":"AOAM533T1BoZWrFGeclDwqjGA5jhZUoqHJc7h3NHw/4yxwyJ+GoFEqQX\n\tZOfkriOQa0RgSGgb/Zcav/ZtT6kx3nhMAity2tSsRQ==","X-Google-Smtp-Source":"ABdhPJzhWwwhJoFKE+BXLV/8xnltkvht38SAo/AQmqvj/eRK0THCnE5lM+MHk2GySbf90CnhRkQIUcQQvv40C5DCqbw=","X-Received":"by 2002:a19:f20a:: with SMTP id\n\tq10mr38206869lfh.89.1594241683698; \n\tWed, 08 Jul 2020 13:54:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t\n\t<20200706112120.GC5912@pendragon.ideasonboard.com>\n\t<20200708204943.GS20298@pendragon.ideasonboard.com>","In-Reply-To":"<20200708204943.GS20298@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 8 Jul 2020 21:54:27 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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":11272,"web_url":"https://patchwork.libcamera.org/comment/11272/","msgid":"<20200708220213.lrfl2snrxdx63rbv@uno.localdomain>","date":"2020-07-08T22:02:13","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Mon, Jul 06, 2020 at 04:02:17PM +0100, Naushir Patuck wrote:\n> Hi David and Jacopo,\n>\n>\n> On Mon, 6 Jul 2020 at 11:41, David Plowman\n> wrote:\n> >\n> > Hi Jacopo\n> >\n> > Thanks for the quick reply! Let me just clarify my understanding of\n> > your answers...\n> >\n> > On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi wrote:\n> > >\n> > > Hi David,\n> > >\n> > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:\n> > > > Hi Jacopo, Laurent, everyone!\n> > > >\n> > > > I have a couple of questions.\n> > > >\n> > > > 1. Does this allow an application to set a transformation when the\n> > > > camera is started? For example, it's not uncommon for a horizontal\n> > > > flip to be requested from the camera depending on the application.\n> > > > Certainly in the Raspberry Pi world you can't tell a priori what a\n> > > > sensor's orientation is.\n> > > >\n> > >\n> > > Isn't this mostly related to allowing application provide a list of\n> > > controls at camera start/configure time ? Those will have to be taken\n> > > into account and matched with the camera sensor reported orientation\n> > > which is handled here I guess.\n> >\n> > So far as I'm aware there is no control for transformations (h/vflip,\n> > even transpose) currently, is that correct? I'm also not sure the\n> > usual request->controls() mechanism is the right one as it sort-of\n> > implies you can set it per frame, which I think is something we\n> > shouldn't allow.\n> >\n> > Perhaps it belongs in the StreamConfiguration? Some platforms might\n> > allow per-stream transformations, others (many?) will not. What do\n> > people think?\n> >\n> > >\n> > > > 2nd question below...\n> > > >\n> > > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi Laurent,\n> > > > >\n> > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > > > > > Instead of receiving sensor orientation configuration from the IPA,\n> > > > > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > > > > HFLIP and VFLIP controls accordingly.\n> > > > > >\n> > > > > > Signed-off-by: Laurent Pinchart \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > > > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -16,6 +16,7 @@\n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > >\n> > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > > > > nullptr);\n> > > > > >\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> > > > >\n> > > > > mmm, I see in the IPA this, which is where I assume you got this from\n> > > > >\n> > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> > > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> > > > >\n> > > > > Where the 'orientation' flag is retrieved from\n> > > > > RPi::CamTransform orientation = helper_->GetOrientation();\n> > > > >\n> > > > > and expressed through members of this enumeration\n> > > > > static constexpr CamTransform CamTransform_IDENTITY = 0;\n> > > > > static constexpr CamTransform CamTransform_HFLIP = 1;\n> > > > > static constexpr CamTransform CamTransform_VFLIP = 2;\n> > > > >\n> > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> > > > > interval, shouldn't we:\n> > > > > if (rotation == 180)\n> > > > > HFLIP\n> > >\n> > > This should be VFLIP, as the control reads \"mirror in the vertical\n> > > direction\", not \"along the horizontal axis\" as I interpreted it\n> > >\n> > > > > if (rotation == 90 || roation == 270)\n> > > > > VFLIP\n> > >\n> > > And this is just wrong. Mirroring in H/V directions won't compensate\n> > > for such rotations.\n> > >\n> > > > > ?\n> > > >\n> > > > How are we handling transformations that aren't rotations, e.g.\n> > > > horizontal flip. As I said previously, it's a significant use case.\n> > >\n> > > I think they merit a control to let application handle them\n> > > I think this was meant to compensate the camera mounting rotation\n> > > only.\n> > >\n> > > > Also, how are we handling platforms that don't support things like\n> > > > 90/270 degree rotations - it's certain that many platforms won't be\n> > > > able to do that.\n> > >\n> > > That is a more generic question: how do pipeline handlers advertise\n> > > which control they support, or do you think H/V flips are different ?\n> >\n> > As I said above, I'm not sure the usual \"controls\" mechanism is the\n> > right one, and it's also not clear to me how a platform should\n> > advertise what it supports. (Do we have any mechanism for signalling\n> > that kind of thing?) Perhaps one could take the view that, for the\n> > time being, we just overwrite anything we don't handle and report the\n> > configuration as \"adjusted\"?\n> >\n> > (Perhaps I am hijacking the original discussion... in which case\n> > please say and I'll re-start it on its own.)\n>\n> If I am not mistaken, the pipeline handlers need to maintain a list of\n> controls supported, like in include/libcamera/ipa/raspberrypi.h\n> (RPiControls). This then gets exported to the CameraData base class\n> member controlInfo_. ARequest will not allow setting a\n> libcamera::control that is not listed in CameraData::controlInfo_\n\nThat's correct, it seems to me from this discussion, a ControlInfo\ncould be augmented with information about a control being supported\nper-frame or just at at configuration time (or else...)\n\n>\n> Regards,\n> Naush\n>\n>\n> >\n> > Best regards\n> > David\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > Best regards\n> > > > David\n> > > >\n> > > > >\n> > > > >\n> > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > > > > +\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > > > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > > > > sensorMetadata_ = action.data[2];\n> > > > > > }\n> > > > > > -\n> > > > > > - /* Set the sensor orientation here as well. */\n> > > > > > - ControlList controls = action.controls[0];\n> > > > > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > > >\n> > > > > Do you think we can now remove the custom rotation handling from the\n> > > > > IPA helpers ?\n> > > > >\n> > > > > > return;\n> > > > > > }\n> > > > > >\n> > > > > > --\n> > > > > > Regards,\n> > > > > >\n> > > > > > Laurent Pinchart\n> > > > > >\n> > > > > > _______________________________________________\n> > > > > > libcamera-devel mailing list\n> > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 3B2D1BD790\n\tfor ;\n\tWed, 8 Jul 2020 21:58:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7FDB61179;\n\tWed, 8 Jul 2020 23:58:41 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D58161167\n\tfor ;\n\tWed, 8 Jul 2020 23:58:40 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 8B2441BF206;\n\tWed, 8 Jul 2020 21:58:39 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 9 Jul 2020 00:02:13 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20200708220213.lrfl2snrxdx63rbv@uno.localdomain>","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>\n\t\n\t<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}},{"id":11275,"web_url":"https://patchwork.libcamera.org/comment/11275/","msgid":"","date":"2020-07-09T08:09:57","subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Wed, 8 Jul 2020 at 22:58, Jacopo Mondi wrote:\n>\n> Hi Naush,\n>\n> On Mon, Jul 06, 2020 at 04:02:17PM +0100, Naushir Patuck wrote:\n> > Hi David and Jacopo,\n> >\n> >\n> > On Mon, 6 Jul 2020 at 11:41, David Plowman\n> > wrote:\n> > >\n> > > Hi Jacopo\n> > >\n> > > Thanks for the quick reply! Let me just clarify my understanding of\n> > > your answers...\n> > >\n> > > On Mon, 6 Jul 2020 at 11:01, Jacopo Mondi wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > On Mon, Jul 06, 2020 at 10:29:21AM +0100, David Plowman wrote:\n> > > > > Hi Jacopo, Laurent, everyone!\n> > > > >\n> > > > > I have a couple of questions.\n> > > > >\n> > > > > 1. Does this allow an application to set a transformation when the\n> > > > > camera is started? For example, it's not uncommon for a horizontal\n> > > > > flip to be requested from the camera depending on the application.\n> > > > > Certainly in the Raspberry Pi world you can't tell a priori what a\n> > > > > sensor's orientation is.\n> > > > >\n> > > >\n> > > > Isn't this mostly related to allowing application provide a list of\n> > > > controls at camera start/configure time ? Those will have to be taken\n> > > > into account and matched with the camera sensor reported orientation\n> > > > which is handled here I guess.\n> > >\n> > > So far as I'm aware there is no control for transformations (h/vflip,\n> > > even transpose) currently, is that correct? I'm also not sure the\n> > > usual request->controls() mechanism is the right one as it sort-of\n> > > implies you can set it per frame, which I think is something we\n> > > shouldn't allow.\n> > >\n> > > Perhaps it belongs in the StreamConfiguration? Some platforms might\n> > > allow per-stream transformations, others (many?) will not. What do\n> > > people think?\n> > >\n> > > >\n> > > > > 2nd question below...\n> > > > >\n> > > > > On Mon, 6 Jul 2020 at 10:12, Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Hi Laurent,\n> > > > > >\n> > > > > > On Sat, Jul 04, 2020 at 03:52:23AM +0300, Laurent Pinchart wrote:\n> > > > > > > Instead of receiving sensor orientation configuration from the IPA,\n> > > > > > > retrieve it from the CameraSensor Rotation property, and configure the\n> > > > > > > HFLIP and VFLIP controls accordingly.\n> > > > > > >\n> > > > > > > Signed-off-by: Laurent Pinchart \n> > > > > > > ---\n> > > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++----\n> > > > > > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > index 74bee6895dcd..fda6831e6a7e 100644\n> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > @@ -16,6 +16,7 @@\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > >\n> > > > > > > @@ -1174,6 +1175,13 @@ int RPiCameraData::configureIPA()\n> > > > > > > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > > > > > nullptr);\n> > > > > > >\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> > > > > >\n> > > > > > mmm, I see in the IPA this, which is where I assume you got this from\n> > > > > >\n> > > > > > ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));\n> > > > > > ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));\n> > > > > >\n> > > > > > Where the 'orientation' flag is retrieved from\n> > > > > > RPi::CamTransform orientation = helper_->GetOrientation();\n> > > > > >\n> > > > > > and expressed through members of this enumeration\n> > > > > > static constexpr CamTransform CamTransform_IDENTITY = 0;\n> > > > > > static constexpr CamTransform CamTransform_HFLIP = 1;\n> > > > > > static constexpr CamTransform CamTransform_VFLIP = 2;\n> > > > > >\n> > > > > > Now, 'rotation' is expressed in multiple of 90 degrees in the [0-360[\n> > > > > > interval, shouldn't we:\n> > > > > > if (rotation == 180)\n> > > > > > HFLIP\n> > > >\n> > > > This should be VFLIP, as the control reads \"mirror in the vertical\n> > > > direction\", not \"along the horizontal axis\" as I interpreted it\n> > > >\n> > > > > > if (rotation == 90 || roation == 270)\n> > > > > > VFLIP\n> > > >\n> > > > And this is just wrong. Mirroring in H/V directions won't compensate\n> > > > for such rotations.\n> > > >\n> > > > > > ?\n> > > > >\n> > > > > How are we handling transformations that aren't rotations, e.g.\n> > > > > horizontal flip. As I said previously, it's a significant use case.\n> > > >\n> > > > I think they merit a control to let application handle them\n> > > > I think this was meant to compensate the camera mounting rotation\n> > > > only.\n> > > >\n> > > > > Also, how are we handling platforms that don't support things like\n> > > > > 90/270 degree rotations - it's certain that many platforms won't be\n> > > > > able to do that.\n> > > >\n> > > > That is a more generic question: how do pipeline handlers advertise\n> > > > which control they support, or do you think H/V flips are different ?\n> > >\n> > > As I said above, I'm not sure the usual \"controls\" mechanism is the\n> > > right one, and it's also not clear to me how a platform should\n> > > advertise what it supports. (Do we have any mechanism for signalling\n> > > that kind of thing?) Perhaps one could take the view that, for the\n> > > time being, we just overwrite anything we don't handle and report the\n> > > configuration as \"adjusted\"?\n> > >\n> > > (Perhaps I am hijacking the original discussion... in which case\n> > > please say and I'll re-start it on its own.)\n> >\n> > If I am not mistaken, the pipeline handlers need to maintain a list of\n> > controls supported, like in include/libcamera/ipa/raspberrypi.h\n> > (RPiControls). This then gets exported to the CameraData base class\n> > member controlInfo_. ARequest will not allow setting a\n> > libcamera::control that is not listed in CameraData::controlInfo_\n>\n> That's correct, it seems to me from this discussion, a ControlInfo\n> could be augmented with information about a control being supported\n> per-frame or just at at configuration time (or else...)\n\nThat sounds like a good idea to me. Actually, I tried to start a new\nthread for this discussion (not very successfully, it seems!) so\nperhaps I can copy your comments onto that thread? But thanks for the\nfeedback!\n\nBest regards\nDavid\n\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> > >\n> > > Best regards\n> > > David\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > >\n> > > > > Best regards\n> > > > > David\n> > > > >\n> > > > > >\n> > > > > >\n> > > > > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls);\n> > > > > > > +\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -1202,10 +1210,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> > > > > > > { V4L2_CID_EXPOSURE, action.data[1] } });\n> > > > > > > sensorMetadata_ = action.data[2];\n> > > > > > > }\n> > > > > > > -\n> > > > > > > - /* Set the sensor orientation here as well. */\n> > > > > > > - ControlList controls = action.controls[0];\n> > > > > > > - unicam_[Unicam::Image].dev()->setControls(&controls);\n> > > > > >\n> > > > > > Do you think we can now remove the custom rotation handling from the\n> > > > > > IPA helpers ?\n> > > > > >\n> > > > > > > return;\n> > > > > > > }\n> > > > > > >\n> > > > > > > --\n> > > > > > > Regards,\n> > > > > > >\n> > > > > > > Laurent Pinchart\n> > > > > > >\n> > > > > > > _______________________________________________\n> > > > > > > libcamera-devel mailing list\n> > > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > > _______________________________________________\n> > > > > > libcamera-devel mailing list\n> > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel","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 9FB54BD792\n\tfor ;\n\tThu, 9 Jul 2020 08:10:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DBE261169;\n\tThu, 9 Jul 2020 10:10:11 +0200 (CEST)","from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C13161158\n\tfor ;\n\tThu, 9 Jul 2020 10:10:09 +0200 (CEST)","by mail-oi1-x242.google.com with SMTP id k6so1185957oij.11\n\tfor ;\n\tThu, 09 Jul 2020 01:10:09 -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=\"Ql/tOGI6\"; 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; bh=VSuBa5oyrHCWb55oS9zq5jihY0ROzlj22OAbVOtDLVE=;\n\tb=Ql/tOGI6XBY2meHJ1YI0bzb7CB4FAcgFO8/U1gF6nxSEsaSlKBWtLzqtIs5spdoRjO\n\txFik6DlnJs46ypxa3ykwFPEf7eCH5WGqFNFxkyHrG6OP64F34KA9iI7s22D3AuPFEFqV\n\tsLMRfClXA5MbNDwCxMzo3JDuJpRSJFOpywRipyvgyOiOQJow8K/Ut18LIz2LG8UWEJH1\n\t4bC92pp7BNwfonRLvEr9sw9hxa878QUqCHl2OxTELU3BqJcKyHpAiSencLSDltj1/ivq\n\tt64nuhn176K4DuE8vU/UNJoW4qohtUklFUFDZziPwVsI3HeKdkWUiryor9RIJvMVJR26\n\tiH5w==","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;\n\tbh=VSuBa5oyrHCWb55oS9zq5jihY0ROzlj22OAbVOtDLVE=;\n\tb=JsuBswY6i2PxdJ3z/a6nDOjQZnT3HlRlkJdkAXfD8XbxEJ1zQbnriZ+l08TsjFJGa7\n\tEO5K60x1YtskWTJIh6ZLLmADDVgAfjAwLhagcSWyPxv40tIhSRqVOAUBuj2GTC7yIWY1\n\tG+OtjJyy3rAxYuyw+nVPS2NQQp1dk+FkE4WMHuEc07FRgx9eb3owUlZlay5FbJoihhG6\n\tCGSW59pMi2xJvfbhPuIi8pRd8eoeSO7AOQgZNc6IakxHBUa1Ef30HQI3W28lvq0wUOu9\n\ttFCAlZ8UVz4jtyFIuKGPI6mqKSgcLwrXmEjQheNJiVRwY/7gxlprFkxnRTFXUwHBw0tK\n\tL7+w==","X-Gm-Message-State":"AOAM532ry57rx4yV+0ROAY5XtcV0h2+M7RWyoKv8xQX28YQXft4ZZvp8\n\t+8I1DtcxO8CNA7VjA/+HvXGdELC8OP2Lk+v2eydvTg==","X-Google-Smtp-Source":"ABdhPJwpkMCh3KON6GhMKLGqJJSiiSN87lP5gR+7srpPUXHO4UDozjmJfvteIhqc8UBN/HwCztSmu4rFeyg1ee4Pbjs=","X-Received":"by 2002:aca:ac01:: with SMTP id v1mr9777083oie.22.1594282207735; \n\tThu, 09 Jul 2020 01:10:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20200704005227.21782-1-laurent.pinchart@ideasonboard.com>\n\t<20200704005227.21782-9-laurent.pinchart@ideasonboard.com>\n\t<20200706091611.5hd52rgmgeftjvf3@uno.localdomain>\n\t\n\t<20200706100437.v5ciaeiwacmowuzw@uno.localdomain>\n\t\n\t\n\t<20200708220213.lrfl2snrxdx63rbv@uno.localdomain>","In-Reply-To":"<20200708220213.lrfl2snrxdx63rbv@uno.localdomain>","From":"David Plowman ","Date":"Thu, 9 Jul 2020 09:09:57 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 08/12] libcamera: pipeline:\n\traspberrypi: Set sensor flip based on rotation","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\" "}}]