[{"id":16121,"web_url":"https://patchwork.libcamera.org/comment/16121/","msgid":"<20210406121009.574d5mqcdaddznom@uno.localdomain>","date":"2021-04-06T12:10:09","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:\n> The minimum and maximum vblanking can change when a new format is\n> applied to the sensor subdevice, so be sure to retrieve up-to-date\n> values.\n>\n> The V4L2Device acquires the new updateControlInfos() method to perform\n> this function, and which the CameraSensor calls automatically if its\n> setFormat method is used to update the sensor.\n>\n> However, not all pipeline handlers invoke the setFormat method\n> directly, so the new method must be made publicly available for\n> pipeline handlers to call if they need to.\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 ++\n> include/libcamera/internal/v4l2_device.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++-\n> src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++\n> 4 files changed, 60 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 3e98f71b..c94744f0 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -68,6 +68,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \tint sensorInfo(CameraSensorInfo *info) const;\n>\n> +\tvoid updateControlInfos();\n> +\n> protected:\n> \tstd::string logPrefix() const override;\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index d006bf68..274cbe65 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -41,6 +41,8 @@ public:\n> \tint setFrameStartEnabled(bool enable);\n> \tSignal frameStart;\n>\n> +\tvoid updateControlInfos();\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f7ed91d9..5dbe47a7 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> /**\n> * \\brief Set the sensor output format\n> * \\param[in] format The desired sensor output format\n> + *\n> + * The sensor output format is set. The ranges of any controls associated\n> + * with the sensor are also updated.\n> + *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> {\n> -\treturn subdev_->setFormat(pad_, format);\n> +\tint ret = subdev_->setFormat(pad_, format);\n> +\n> +\tif (ret == 0)\n> +\t\tupdateControlInfos();\n> +\n> +\treturn ret;\n> }\n>\n> /**\n> * \\brief Retrieve the supported V4L2 controls and their information\n> + *\n> + * Pipeline handlers that do not change the sensor format using the\n> + * CameraSensor::setFormat method may need to call\n> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> + * ranges are up to date.\n> + *\n> * \\return A map of the V4L2 controls supported by the sensor\n> */\n> const ControlInfoMap &CameraSensor::controls() const\n> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> * Sensor information is only available for raw sensors. When called for a YUV\n> * sensor, this function returns -EINVAL.\n> *\n> + * Pipeline handlers that do not change the sensor format using the\n> + * CameraSensor::setFormat method may need to call\n> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> + * ranges are up to date.\n> + *\n> * \\return 0 on success, a negative error code otherwise\n> */\n> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\fn void CameraSensor::updateControlInfos()\n> + * \\brief Update the sensor's ControlInfos in case they have changed\n> + */\n> +void CameraSensor::updateControlInfos()\n> +{\n> +\tsubdev_->updateControlInfos();\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + entity_->name() + \"'\";\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..9678962c 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -514,6 +514,32 @@ void V4L2Device::listControls()\n> \tcontrols_ = std::move(ctrls);\n> }\n>\n> +/*\n\nThis is now a public method and needs /**\nDoesn't doxygen give you a warning here ?\n\nThe patch looks good otherwise!\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> + * \\brief Update the control information that was previously stored by\n> + * listControls(). Some parts of this information, such as min and max\n> + * values of some controls, are liable to change when a new format is set.\n> + */\n> +void V4L2Device::updateControlInfos()\n> +{\n> +\tfor (auto &controlId : controlIds_) {\n> +\t\tunsigned int id = controlId->id();\n> +\n> +\t\tstruct v4l2_query_ext_ctrl ctrl = {};\n> +\t\tctrl.id = id;\n> +\n> +\t\tif (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> +\t\t\tLOG(V4L2, Debug)\n> +\t\t\t\t<< \"Could not refresh control \"\n> +\t\t\t\t<< utils::hex(ctrl.id);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* Assume these are present - listControls() made them. */\n> +\t\tcontrolInfo_[id] = ctrl;\n> +\t\tcontrols_.at(controlId.get()) = V4L2ControlInfo(ctrl);\n> +\t}\n> +}\n> +\n> /*\n> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> * values in \\a v4l2Ctrls\n> --\n> 2.20.1\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 D1814C0DA3\n\tfor ;\n\tTue, 6 Apr 2021 12:09:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E3DE6877C;\n\tTue, 6 Apr 2021 14:09:35 +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 9D80760518\n\tfor ;\n\tTue, 6 Apr 2021 14:09:33 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 17CA060002;\n\tTue, 6 Apr 2021 12:09:32 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 6 Apr 2021 14:10:09 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210406121009.574d5mqcdaddznom@uno.localdomain>","References":"<20210406104050.23814-1-david.plowman@raspberrypi.com>\n\t<20210406104050.23814-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210406104050.23814-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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":16142,"web_url":"https://patchwork.libcamera.org/comment/16142/","msgid":"","date":"2021-04-07T09:33:28","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Tue, 6 Apr 2021 at 13:09, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:\n> > The minimum and maximum vblanking can change when a new format is\n> > applied to the sensor subdevice, so be sure to retrieve up-to-date\n> > values.\n> >\n> > The V4L2Device acquires the new updateControlInfos() method to perform\n> > this function, and which the CameraSensor calls automatically if its\n> > setFormat method is used to update the sensor.\n> >\n> > However, not all pipeline handlers invoke the setFormat method\n> > directly, so the new method must be made publicly available for\n> > pipeline handlers to call if they need to.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 2 ++\n> > include/libcamera/internal/v4l2_device.h | 2 ++\n> > src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++-\n> > src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++\n> > 4 files changed, 60 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index 3e98f71b..c94744f0 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -68,6 +68,8 @@ public:\n> > const ControlList &properties() const { return properties_; }\n> > int sensorInfo(CameraSensorInfo *info) const;\n> >\n> > + void updateControlInfos();\n> > +\n> > protected:\n> > std::string logPrefix() const override;\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index d006bf68..274cbe65 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -41,6 +41,8 @@ public:\n> > int setFrameStartEnabled(bool enable);\n> > Signal frameStart;\n> >\n> > + void updateControlInfos();\n> > +\n> > protected:\n> > V4L2Device(const std::string &deviceNode);\n> > ~V4L2Device();\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index f7ed91d9..5dbe47a7 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> > /**\n> > * \\brief Set the sensor output format\n> > * \\param[in] format The desired sensor output format\n> > + *\n> > + * The sensor output format is set. The ranges of any controls associated\n> > + * with the sensor are also updated.\n> > + *\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > {\n> > - return subdev_->setFormat(pad_, format);\n> > + int ret = subdev_->setFormat(pad_, format);\n> > +\n> > + if (ret == 0)\n> > + updateControlInfos();\n> > +\n> > + return ret;\n> > }\n> >\n> > /**\n> > * \\brief Retrieve the supported V4L2 controls and their information\n> > + *\n> > + * Pipeline handlers that do not change the sensor format using the\n> > + * CameraSensor::setFormat method may need to call\n> > + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> > + * ranges are up to date.\n> > + *\n> > * \\return A map of the V4L2 controls supported by the sensor\n> > */\n> > const ControlInfoMap &CameraSensor::controls() const\n> > @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > * Sensor information is only available for raw sensors. When called for a YUV\n> > * sensor, this function returns -EINVAL.\n> > *\n> > + * Pipeline handlers that do not change the sensor format using the\n> > + * CameraSensor::setFormat method may need to call\n> > + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> > + * ranges are up to date.\n> > + *\n> > * \\return 0 on success, a negative error code otherwise\n> > */\n> > int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > return 0;\n> > }\n> >\n> > +/**\n> > + * \\fn void CameraSensor::updateControlInfos()\n> > + * \\brief Update the sensor's ControlInfos in case they have changed\n> > + */\n> > +void CameraSensor::updateControlInfos()\n> > +{\n> > + subdev_->updateControlInfos();\n> > +}\n> > +\n> > std::string CameraSensor::logPrefix() const\n> > {\n> > return \"'\" + entity_->name() + \"'\";\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index decd19ef..9678962c 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -514,6 +514,32 @@ void V4L2Device::listControls()\n> > controls_ = std::move(ctrls);\n> > }\n> >\n> > +/*\n>\n> This is now a public method and needs /**\n> Doesn't doxygen give you a warning here ?\n\nAh yes, the documentation seems to have stopped building some time\nago... there appear to be a couple more dependencies I have to install\nfor it to happen now. No problem!\n\nThanks\nDavid\n\n>\n> The patch looks good otherwise!\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > + * \\brief Update the control information that was previously stored by\n> > + * listControls(). Some parts of this information, such as min and max\n> > + * values of some controls, are liable to change when a new format is set.\n> > + */\n> > +void V4L2Device::updateControlInfos()\n> > +{\n> > + for (auto &controlId : controlIds_) {\n> > + unsigned int id = controlId->id();\n> > +\n> > + struct v4l2_query_ext_ctrl ctrl = {};\n> > + ctrl.id = id;\n> > +\n> > + if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> > + LOG(V4L2, Debug)\n> > + << \"Could not refresh control \"\n> > + << utils::hex(ctrl.id);\n> > + continue;\n> > + }\n> > +\n> > + /* Assume these are present - listControls() made them. */\n> > + controlInfo_[id] = ctrl;\n> > + controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);\n> > + }\n> > +}\n> > +\n> > /*\n> > * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> > * values in \\a v4l2Ctrls\n> > --\n> > 2.20.1\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 9EF38BD224\n\tfor ;\n\tWed, 7 Apr 2021 09:33:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C5D9687D7;\n\tWed, 7 Apr 2021 11:33:43 +0200 (CEST)","from mail-ot1-x335.google.com (mail-ot1-x335.google.com\n\t[IPv6:2607:f8b0:4864:20::335])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BD7468795\n\tfor ;\n\tWed, 7 Apr 2021 11:33:41 +0200 (CEST)","by mail-ot1-x335.google.com with SMTP id\n\tw21-20020a9d63950000b02901ce7b8c45b4so17425319otk.5\n\tfor ;\n\tWed, 07 Apr 2021 02:33:41 -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=\"qA6AA8Ga\"; 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=YnDDoUF0P2JbV2ddzencWVmuHE7bbAgRX9oPPeg6bm8=;\n\tb=qA6AA8GaH6Q5i7lmOxohH56fGT0FblWfLAQZ/W28kpj/RRdkMAKO+ZcbNlOV1KpRrK\n\t51CIeFjfkug9zTozOMjcbiLJZOp1RGPDKcZBUspREQieARdR2GxlAyuPjsDjrHV2cB5n\n\tgKTUIG9ymuG/nmcxzqH7YZPSjf4yHoP/yLmvKOn8SFV1tIQPEJKu67U+1zfjk2c+/pB5\n\txsdGGv/XMAqpAAp+LWxus8Bt68rRWsBzehVBnHykhJMkDK5KXelF1dVEUp4NpoDUsJoZ\n\tfJLhlAHPExT1XyqD+xi3mBpaIAEPSvOs2nETZZnU2aKeaPQNLQKly7zwj26osrTqR965\n\tyYBQ==","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=YnDDoUF0P2JbV2ddzencWVmuHE7bbAgRX9oPPeg6bm8=;\n\tb=oUo99np5jQw7OcVP8k12nAy9y38ORYCvHTSHnMukaovHy4IYVEw1bT1oykWWRyqvGo\n\t9PnwBsGex5PpbqNyXCuh1nm0Zfwe4WV+6fIr2quISC8ik7ItifEdoE/iPhgM2zu2HfUn\n\t6X9I8Go6pgj4S1/YO88duTAzA1GXOKaG8GTDgmrGQ3JCImxrA54N5WPmjRt61ZVhDldo\n\tMMKlQ1jL5yLpfFZ8D5vbjUFTnaXpkFYpCQVqDPzKzaSPh0p5zo8dMeXNsIq9AUrG3qqo\n\t4ipJ9cpdy91s9rkaysZrmoA4PF2UVCFeiPwQuT5mmlFjofki4WYVhtgTmPfIC6nUC/9n\n\t6g8A==","X-Gm-Message-State":"AOAM532rsKsHFsGb1XW6mkQUKjThRfcDK64whteqzKiZYF2Uy7uJt2Ij\n\tOqkguBGWNfKmONbENKUY86X6I7Rwr5W4xoYFuuzevA==","X-Google-Smtp-Source":"ABdhPJzJLRh9Y/ZLSdTsdBK++3Z9sGPBCGGmnocxICULQJzORPbYKhPT3LurNxmMDN8p0gmJLuQbFA+KBtH0oUlwKBI=","X-Received":"by 2002:a05:6830:158b:: with SMTP id\n\ti11mr2059083otr.317.1617788019690; \n\tWed, 07 Apr 2021 02:33:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20210406104050.23814-1-david.plowman@raspberrypi.com>\n\t<20210406104050.23814-2-david.plowman@raspberrypi.com>\n\t<20210406121009.574d5mqcdaddznom@uno.localdomain>","In-Reply-To":"<20210406121009.574d5mqcdaddznom@uno.localdomain>","From":"David Plowman ","Date":"Wed, 7 Apr 2021 10:33:28 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16248,"web_url":"https://patchwork.libcamera.org/comment/16248/","msgid":"<1f62c66a-cafb-f6f6-aa25-044b2b865045@ideasonboard.com>","date":"2021-04-13T21:34:26","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 07/04/2021 10:33, David Plowman wrote:\n> Hi Jacopo\n> \n> On Tue, 6 Apr 2021 at 13:09, Jacopo Mondi wrote:\n>>\n>> Hi David,\n>>\n>> On Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:\n>>> The minimum and maximum vblanking can change when a new format is\n>>> applied to the sensor subdevice, so be sure to retrieve up-to-date\n>>> values.\n>>>\n>>> The V4L2Device acquires the new updateControlInfos() method to perform\n>>> this function, and which the CameraSensor calls automatically if its\n>>> setFormat method is used to update the sensor.\n>>>\n>>> However, not all pipeline handlers invoke the setFormat method\n>>> directly, so the new method must be made publicly available for\n>>> pipeline handlers to call if they need to.\n>>>\n>>> Signed-off-by: David Plowman \n>>> ---\n>>> include/libcamera/internal/camera_sensor.h | 2 ++\n>>> include/libcamera/internal/v4l2_device.h | 2 ++\n>>> src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++-\n>>> src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++\n>>> 4 files changed, 60 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n>>> index 3e98f71b..c94744f0 100644\n>>> --- a/include/libcamera/internal/camera_sensor.h\n>>> +++ b/include/libcamera/internal/camera_sensor.h\n>>> @@ -68,6 +68,8 @@ public:\n>>> const ControlList &properties() const { return properties_; }\n>>> int sensorInfo(CameraSensorInfo *info) const;\n>>>\n>>> + void updateControlInfos();\n>>> +\n>>> protected:\n>>> std::string logPrefix() const override;\n>>>\n>>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n>>> index d006bf68..274cbe65 100644\n>>> --- a/include/libcamera/internal/v4l2_device.h\n>>> +++ b/include/libcamera/internal/v4l2_device.h\n>>> @@ -41,6 +41,8 @@ public:\n>>> int setFrameStartEnabled(bool enable);\n>>> Signal frameStart;\n>>>\n>>> + void updateControlInfos();\n>>> +\n>>> protected:\n>>> V4L2Device(const std::string &deviceNode);\n>>> ~V4L2Device();\n>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>>> index f7ed91d9..5dbe47a7 100644\n>>> --- a/src/libcamera/camera_sensor.cpp\n>>> +++ b/src/libcamera/camera_sensor.cpp\n>>> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n>>> /**\n>>> * \\brief Set the sensor output format\n>>> * \\param[in] format The desired sensor output format\n>>> + *\n>>> + * The sensor output format is set. The ranges of any controls associated\n>>> + * with the sensor are also updated.\n>>> + *\n>>> * \\return 0 on success or a negative error code otherwise\n>>> */\n>>> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n>>> {\n>>> - return subdev_->setFormat(pad_, format);\n>>> + int ret = subdev_->setFormat(pad_, format);\n>>> +\n>>> + if (ret == 0)\n>>> + updateControlInfos();\n>>> +\n>>> + return ret;\n>>> }\n>>>\n>>> /**\n>>> * \\brief Retrieve the supported V4L2 controls and their information\n>>> + *\n>>> + * Pipeline handlers that do not change the sensor format using the\n>>> + * CameraSensor::setFormat method may need to call\n>>> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n>>> + * ranges are up to date.\n>>> + *\n>>> * \\return A map of the V4L2 controls supported by the sensor\n>>> */\n>>> const ControlInfoMap &CameraSensor::controls() const\n>>> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n>>> * Sensor information is only available for raw sensors. When called for a YUV\n>>> * sensor, this function returns -EINVAL.\n>>> *\n>>> + * Pipeline handlers that do not change the sensor format using the\n>>> + * CameraSensor::setFormat method may need to call\n>>> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n>>> + * ranges are up to date.\n>>> + *\n>>> * \\return 0 on success, a negative error code otherwise\n>>> */\n>>> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n>>> @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n>>> return 0;\n>>> }\n>>>\n>>> +/**\n>>> + * \\fn void CameraSensor::updateControlInfos()\n>>> + * \\brief Update the sensor's ControlInfos in case they have changed\n>>> + */\n>>> +void CameraSensor::updateControlInfos()\n>>> +{\n>>> + subdev_->updateControlInfos();\n>>> +}\n>>> +\n>>> std::string CameraSensor::logPrefix() const\n>>> {\n>>> return \"'\" + entity_->name() + \"'\";\n>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>> index decd19ef..9678962c 100644\n>>> --- a/src/libcamera/v4l2_device.cpp\n>>> +++ b/src/libcamera/v4l2_device.cpp\n>>> @@ -514,6 +514,32 @@ void V4L2Device::listControls()\n>>> controls_ = std::move(ctrls);\n>>> }\n>>>\n>>> +/*\n>>\n>> This is now a public method and needs /**\n>> Doesn't doxygen give you a warning here ?\n> \n> Ah yes, the documentation seems to have stopped building some time\n> ago... there appear to be a couple more dependencies I have to install\n> for it to happen now. No problem!\n\nI've discovered today that it's best to build doxygen from source as you\nneed a very recent version to have a quiet build.\n\nFor me that was as straightforward as:\n\ngit clone https://github.com/doxygen/doxygen\ncd doxygen\nmkdir build\ncd build\ncmake -DCMAKE_INSTALL_PREFIX=/usr ../ && make -j 8 all install\n\n\n> Thanks\n> David\n> \n>>\n>> The patch looks good otherwise!\n>> Reviewed-by: Jacopo Mondi \n>>\n>> Thanks\n>> j\n>>\n>>> + * \\brief Update the control information that was previously stored by\n>>> + * listControls(). Some parts of this information, such as min and max\n>>> + * values of some controls, are liable to change when a new format is set.\n>>> + */\n>>> +void V4L2Device::updateControlInfos()\n>>> +{\n>>> + for (auto &controlId : controlIds_) {\n>>> + unsigned int id = controlId->id();\n>>> +\n>>> + struct v4l2_query_ext_ctrl ctrl = {};\n>>> + ctrl.id = id;\n>>> +\n>>> + if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n>>> + LOG(V4L2, Debug)\n>>> + << \"Could not refresh control \"\n>>> + << utils::hex(ctrl.id);\n>>> + continue;\n>>> + }\n\nI'm curious if 'all' controls need to be refreshed, but I think that's\nsimpler that trying to filter it with a list that would then bitrot.\n\nSo...\n\nReviewed-by: Kieran Bingham \n\n\n>>> +\n>>> + /* Assume these are present - listControls() made them. */\n>>> + controlInfo_[id] = ctrl;\n>>> + controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);\n>>> + }\n>>> +}\n>>> +\n>>> /*\n>>> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n>>> * values in \\a v4l2Ctrls\n>>> --\n>>> 2.20.1\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>","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 AF0BDBD1F6\n\tfor ;\n\tTue, 13 Apr 2021 21:34:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2730968800;\n\tTue, 13 Apr 2021 23:34:31 +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 578C3602D1\n\tfor ;\n\tTue, 13 Apr 2021 23:34:30 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90D719F0;\n\tTue, 13 Apr 2021 23:34:29 +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=\"Px1Lmiu+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618349669;\n\tbh=ytkBTxvSAkCAmhiKkcTm8V248mDSPUTGCfSv+miw0Ko=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Px1Lmiu+nkqtLs4lQA7/POYoAXw/0m3QBly6KmNQyitposqmBdSnbGGxiFFrGmoQv\n\tD1hTh0LvqZ8x5CfP7Lx9vV2Tk6uTYIJ3UmxB3gz31FsiwaiplMtxUabRsg8MsuC0/t\n\tmsl7H5gf9s5/f7vKGNJXVmXvcooU/RUcr3DcFMks=","To":"David Plowman ,\n\tJacopo Mondi ","References":"<20210406104050.23814-1-david.plowman@raspberrypi.com>\n\t<20210406104050.23814-2-david.plowman@raspberrypi.com>\n\t<20210406121009.574d5mqcdaddznom@uno.localdomain>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<1f62c66a-cafb-f6f6-aa25-044b2b865045@ideasonboard.com>","Date":"Tue, 13 Apr 2021 22:34:26 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16432,"web_url":"https://patchwork.libcamera.org/comment/16432/","msgid":"","date":"2021-04-21T08:56:10","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nJust a little nudge on this one... what are folks thinking, is there\nmore stuff to look at here?\n\nThanks\nDavid\n\nOn Tue, 13 Apr 2021 at 22:34, Kieran Bingham\n wrote:\n>\n> Hi David,\n>\n> On 07/04/2021 10:33, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > On Tue, 6 Apr 2021 at 13:09, Jacopo Mondi wrote:\n> >>\n> >> Hi David,\n> >>\n> >> On Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:\n> >>> The minimum and maximum vblanking can change when a new format is\n> >>> applied to the sensor subdevice, so be sure to retrieve up-to-date\n> >>> values.\n> >>>\n> >>> The V4L2Device acquires the new updateControlInfos() method to perform\n> >>> this function, and which the CameraSensor calls automatically if its\n> >>> setFormat method is used to update the sensor.\n> >>>\n> >>> However, not all pipeline handlers invoke the setFormat method\n> >>> directly, so the new method must be made publicly available for\n> >>> pipeline handlers to call if they need to.\n> >>>\n> >>> Signed-off-by: David Plowman \n> >>> ---\n> >>> include/libcamera/internal/camera_sensor.h | 2 ++\n> >>> include/libcamera/internal/v4l2_device.h | 2 ++\n> >>> src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++-\n> >>> src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++\n> >>> 4 files changed, 60 insertions(+), 1 deletion(-)\n> >>>\n> >>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> >>> index 3e98f71b..c94744f0 100644\n> >>> --- a/include/libcamera/internal/camera_sensor.h\n> >>> +++ b/include/libcamera/internal/camera_sensor.h\n> >>> @@ -68,6 +68,8 @@ public:\n> >>> const ControlList &properties() const { return properties_; }\n> >>> int sensorInfo(CameraSensorInfo *info) const;\n> >>>\n> >>> + void updateControlInfos();\n> >>> +\n> >>> protected:\n> >>> std::string logPrefix() const override;\n> >>>\n> >>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> >>> index d006bf68..274cbe65 100644\n> >>> --- a/include/libcamera/internal/v4l2_device.h\n> >>> +++ b/include/libcamera/internal/v4l2_device.h\n> >>> @@ -41,6 +41,8 @@ public:\n> >>> int setFrameStartEnabled(bool enable);\n> >>> Signal frameStart;\n> >>>\n> >>> + void updateControlInfos();\n> >>> +\n> >>> protected:\n> >>> V4L2Device(const std::string &deviceNode);\n> >>> ~V4L2Device();\n> >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> >>> index f7ed91d9..5dbe47a7 100644\n> >>> --- a/src/libcamera/camera_sensor.cpp\n> >>> +++ b/src/libcamera/camera_sensor.cpp\n> >>> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> >>> /**\n> >>> * \\brief Set the sensor output format\n> >>> * \\param[in] format The desired sensor output format\n> >>> + *\n> >>> + * The sensor output format is set. The ranges of any controls associated\n> >>> + * with the sensor are also updated.\n> >>> + *\n> >>> * \\return 0 on success or a negative error code otherwise\n> >>> */\n> >>> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> >>> {\n> >>> - return subdev_->setFormat(pad_, format);\n> >>> + int ret = subdev_->setFormat(pad_, format);\n> >>> +\n> >>> + if (ret == 0)\n> >>> + updateControlInfos();\n> >>> +\n> >>> + return ret;\n> >>> }\n> >>>\n> >>> /**\n> >>> * \\brief Retrieve the supported V4L2 controls and their information\n> >>> + *\n> >>> + * Pipeline handlers that do not change the sensor format using the\n> >>> + * CameraSensor::setFormat method may need to call\n> >>> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> >>> + * ranges are up to date.\n> >>> + *\n> >>> * \\return A map of the V4L2 controls supported by the sensor\n> >>> */\n> >>> const ControlInfoMap &CameraSensor::controls() const\n> >>> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> >>> * Sensor information is only available for raw sensors. When called for a YUV\n> >>> * sensor, this function returns -EINVAL.\n> >>> *\n> >>> + * Pipeline handlers that do not change the sensor format using the\n> >>> + * CameraSensor::setFormat method may need to call\n> >>> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> >>> + * ranges are up to date.\n> >>> + *\n> >>> * \\return 0 on success, a negative error code otherwise\n> >>> */\n> >>> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> >>> @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> >>> return 0;\n> >>> }\n> >>>\n> >>> +/**\n> >>> + * \\fn void CameraSensor::updateControlInfos()\n> >>> + * \\brief Update the sensor's ControlInfos in case they have changed\n> >>> + */\n> >>> +void CameraSensor::updateControlInfos()\n> >>> +{\n> >>> + subdev_->updateControlInfos();\n> >>> +}\n> >>> +\n> >>> std::string CameraSensor::logPrefix() const\n> >>> {\n> >>> return \"'\" + entity_->name() + \"'\";\n> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>> index decd19ef..9678962c 100644\n> >>> --- a/src/libcamera/v4l2_device.cpp\n> >>> +++ b/src/libcamera/v4l2_device.cpp\n> >>> @@ -514,6 +514,32 @@ void V4L2Device::listControls()\n> >>> controls_ = std::move(ctrls);\n> >>> }\n> >>>\n> >>> +/*\n> >>\n> >> This is now a public method and needs /**\n> >> Doesn't doxygen give you a warning here ?\n> >\n> > Ah yes, the documentation seems to have stopped building some time\n> > ago... there appear to be a couple more dependencies I have to install\n> > for it to happen now. No problem!\n>\n> I've discovered today that it's best to build doxygen from source as you\n> need a very recent version to have a quiet build.\n>\n> For me that was as straightforward as:\n>\n> git clone https://github.com/doxygen/doxygen\n> cd doxygen\n> mkdir build\n> cd build\n> cmake -DCMAKE_INSTALL_PREFIX=/usr ../ && make -j 8 all install\n>\n>\n> > Thanks\n> > David\n> >\n> >>\n> >> The patch looks good otherwise!\n> >> Reviewed-by: Jacopo Mondi \n> >>\n> >> Thanks\n> >> j\n> >>\n> >>> + * \\brief Update the control information that was previously stored by\n> >>> + * listControls(). Some parts of this information, such as min and max\n> >>> + * values of some controls, are liable to change when a new format is set.\n> >>> + */\n> >>> +void V4L2Device::updateControlInfos()\n> >>> +{\n> >>> + for (auto &controlId : controlIds_) {\n> >>> + unsigned int id = controlId->id();\n> >>> +\n> >>> + struct v4l2_query_ext_ctrl ctrl = {};\n> >>> + ctrl.id = id;\n> >>> +\n> >>> + if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> >>> + LOG(V4L2, Debug)\n> >>> + << \"Could not refresh control \"\n> >>> + << utils::hex(ctrl.id);\n> >>> + continue;\n> >>> + }\n>\n> I'm curious if 'all' controls need to be refreshed, but I think that's\n> simpler that trying to filter it with a list that would then bitrot.\n>\n> So...\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> >>> +\n> >>> + /* Assume these are present - listControls() made them. */\n> >>> + controlInfo_[id] = ctrl;\n> >>> + controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);\n> >>> + }\n> >>> +}\n> >>> +\n> >>> /*\n> >>> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> >>> * values in \\a v4l2Ctrls\n> >>> --\n> >>> 2.20.1\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>\n> --\n> Regards\n> --\n> Kieran","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 73068BDB16\n\tfor ;\n\tWed, 21 Apr 2021 08:56:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFE0268844;\n\tWed, 21 Apr 2021 10:56:24 +0200 (CEST)","from mail-ot1-x331.google.com (mail-ot1-x331.google.com\n\t[IPv6:2607:f8b0:4864:20::331])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D02A602C8\n\tfor ;\n\tWed, 21 Apr 2021 10:56:23 +0200 (CEST)","by mail-ot1-x331.google.com with SMTP id\n\t65-20020a9d03470000b02902808b4aec6dso33588074otv.6\n\tfor ;\n\tWed, 21 Apr 2021 01:56:22 -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=\"ruovxdLa\"; 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=TVPJbM1YZXj8PZaj7+lB5TmwRQroIR0j6JicUwV5olc=;\n\tb=ruovxdLaQtF+zMLT2no/u16yIAs22q+3tMAurXfxF24wkLeQfyY2so7xX5SrrZPPIt\n\tEtoloKZ7r9//BPUFsEthoHc6SvvUnBlod7b5c5kdasUszZGRAl8ezgEyyX2IlFOt6Zqi\n\tJp4ZbcnJCQs5djJv0BKAORYWh4MyoIeAf2mWaOwwH00mZaYMsjCGO18A/hNEl/2QeZO9\n\tlEqx3hxVwhwTi+A5gOXBsBazL/9C8HN4frTiWxmKM2+3Or/Fm/gQFRo1PVzlb32xYbG4\n\tzp7TbG38IwbxjPPaBlqVMjL6suCJgoWjWCy29oJ6iZ+ZEk4lh5RJq0qkDhJa3dlWQk+s\n\t6IOw==","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=TVPJbM1YZXj8PZaj7+lB5TmwRQroIR0j6JicUwV5olc=;\n\tb=FpAi/09I3nPdiKTqMvUJpJG/SpbQ9ocZP3zuAg3rLdqIdJQ5pXp2s6Dopb59pjzhRs\n\tzWf1EmP5pb79EeTXM9sO96z56LuY//jJfNfiV440TLQse6TVIg0hn2uRNOcj7BoRI8sv\n\thHjpM6s0unbzQkwKq+z1aDhCBnawR2fPfBNS0k5OpQS4HOlDmRR2lAy3uLsQURT0gmSn\n\t08/WgZYfkxJEi1FiTQp43727t1P2rYyX/BI1CcFuhmYImh4yHmf1lyBy8lQOpEGBK9Fb\n\tSbStbpvFP0QYNnFZ1d3K8Qv6plDOtsjLjCWA2GWZeiMSOqsjtj04L1fuoAvz7oFWVdWB\n\tLCvw==","X-Gm-Message-State":"AOAM533RP0fRNc2Q27Ph+PNKc1G0pKqQsNo3/RMj3LxN8NpDlAuFSrR0\n\tmcItB0k82BuHKAr/igRBz6Fw6jQC65k02jc3glksOs2i2wA=","X-Google-Smtp-Source":"ABdhPJyubxlKtyXYxNAVbYhm/NlQm5Z7Gi1WssZPAyrQRg3xkux4bRNuN1cCDgs/MX7pszTHDksEA5EQwHkiThc8Q8M=","X-Received":"by 2002:a9d:6f15:: with SMTP id\n\tn21mr13624758otq.166.1618995381642; \n\tWed, 21 Apr 2021 01:56:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210406104050.23814-1-david.plowman@raspberrypi.com>\n\t<20210406104050.23814-2-david.plowman@raspberrypi.com>\n\t<20210406121009.574d5mqcdaddznom@uno.localdomain>\n\t\n\t<1f62c66a-cafb-f6f6-aa25-044b2b865045@ideasonboard.com>","In-Reply-To":"<1f62c66a-cafb-f6f6-aa25-044b2b865045@ideasonboard.com>","From":"David Plowman ","Date":"Wed, 21 Apr 2021 09:56:10 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16753,"web_url":"https://patchwork.libcamera.org/comment/16753/","msgid":"","date":"2021-05-04T08:19:22","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again - can I give this one another little prod just so that it's\nnot forgotten?\n\nThanks!\nDavid\n\nOn Wed, 21 Apr 2021 at 09:56, David Plowman\n wrote:\n>\n> Hi everyone\n>\n> Just a little nudge on this one... what are folks thinking, is there\n> more stuff to look at here?\n>\n> Thanks\n> David\n>\n> On Tue, 13 Apr 2021 at 22:34, Kieran Bingham\n> wrote:\n> >\n> > Hi David,\n> >\n> > On 07/04/2021 10:33, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > On Tue, 6 Apr 2021 at 13:09, Jacopo Mondi wrote:\n> > >>\n> > >> Hi David,\n> > >>\n> > >> On Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:\n> > >>> The minimum and maximum vblanking can change when a new format is\n> > >>> applied to the sensor subdevice, so be sure to retrieve up-to-date\n> > >>> values.\n> > >>>\n> > >>> The V4L2Device acquires the new updateControlInfos() method to perform\n> > >>> this function, and which the CameraSensor calls automatically if its\n> > >>> setFormat method is used to update the sensor.\n> > >>>\n> > >>> However, not all pipeline handlers invoke the setFormat method\n> > >>> directly, so the new method must be made publicly available for\n> > >>> pipeline handlers to call if they need to.\n> > >>>\n> > >>> Signed-off-by: David Plowman \n> > >>> ---\n> > >>> include/libcamera/internal/camera_sensor.h | 2 ++\n> > >>> include/libcamera/internal/v4l2_device.h | 2 ++\n> > >>> src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++-\n> > >>> src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++\n> > >>> 4 files changed, 60 insertions(+), 1 deletion(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > >>> index 3e98f71b..c94744f0 100644\n> > >>> --- a/include/libcamera/internal/camera_sensor.h\n> > >>> +++ b/include/libcamera/internal/camera_sensor.h\n> > >>> @@ -68,6 +68,8 @@ public:\n> > >>> const ControlList &properties() const { return properties_; }\n> > >>> int sensorInfo(CameraSensorInfo *info) const;\n> > >>>\n> > >>> + void updateControlInfos();\n> > >>> +\n> > >>> protected:\n> > >>> std::string logPrefix() const override;\n> > >>>\n> > >>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > >>> index d006bf68..274cbe65 100644\n> > >>> --- a/include/libcamera/internal/v4l2_device.h\n> > >>> +++ b/include/libcamera/internal/v4l2_device.h\n> > >>> @@ -41,6 +41,8 @@ public:\n> > >>> int setFrameStartEnabled(bool enable);\n> > >>> Signal frameStart;\n> > >>>\n> > >>> + void updateControlInfos();\n> > >>> +\n> > >>> protected:\n> > >>> V4L2Device(const std::string &deviceNode);\n> > >>> ~V4L2Device();\n> > >>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > >>> index f7ed91d9..5dbe47a7 100644\n> > >>> --- a/src/libcamera/camera_sensor.cpp\n> > >>> +++ b/src/libcamera/camera_sensor.cpp\n> > >>> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> > >>> /**\n> > >>> * \\brief Set the sensor output format\n> > >>> * \\param[in] format The desired sensor output format\n> > >>> + *\n> > >>> + * The sensor output format is set. The ranges of any controls associated\n> > >>> + * with the sensor are also updated.\n> > >>> + *\n> > >>> * \\return 0 on success or a negative error code otherwise\n> > >>> */\n> > >>> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> > >>> {\n> > >>> - return subdev_->setFormat(pad_, format);\n> > >>> + int ret = subdev_->setFormat(pad_, format);\n> > >>> +\n> > >>> + if (ret == 0)\n> > >>> + updateControlInfos();\n> > >>> +\n> > >>> + return ret;\n> > >>> }\n> > >>>\n> > >>> /**\n> > >>> * \\brief Retrieve the supported V4L2 controls and their information\n> > >>> + *\n> > >>> + * Pipeline handlers that do not change the sensor format using the\n> > >>> + * CameraSensor::setFormat method may need to call\n> > >>> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> > >>> + * ranges are up to date.\n> > >>> + *\n> > >>> * \\return A map of the V4L2 controls supported by the sensor\n> > >>> */\n> > >>> const ControlInfoMap &CameraSensor::controls() const\n> > >>> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> > >>> * Sensor information is only available for raw sensors. When called for a YUV\n> > >>> * sensor, this function returns -EINVAL.\n> > >>> *\n> > >>> + * Pipeline handlers that do not change the sensor format using the\n> > >>> + * CameraSensor::setFormat method may need to call\n> > >>> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> > >>> + * ranges are up to date.\n> > >>> + *\n> > >>> * \\return 0 on success, a negative error code otherwise\n> > >>> */\n> > >>> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > >>> @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > >>> return 0;\n> > >>> }\n> > >>>\n> > >>> +/**\n> > >>> + * \\fn void CameraSensor::updateControlInfos()\n> > >>> + * \\brief Update the sensor's ControlInfos in case they have changed\n> > >>> + */\n> > >>> +void CameraSensor::updateControlInfos()\n> > >>> +{\n> > >>> + subdev_->updateControlInfos();\n> > >>> +}\n> > >>> +\n> > >>> std::string CameraSensor::logPrefix() const\n> > >>> {\n> > >>> return \"'\" + entity_->name() + \"'\";\n> > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > >>> index decd19ef..9678962c 100644\n> > >>> --- a/src/libcamera/v4l2_device.cpp\n> > >>> +++ b/src/libcamera/v4l2_device.cpp\n> > >>> @@ -514,6 +514,32 @@ void V4L2Device::listControls()\n> > >>> controls_ = std::move(ctrls);\n> > >>> }\n> > >>>\n> > >>> +/*\n> > >>\n> > >> This is now a public method and needs /**\n> > >> Doesn't doxygen give you a warning here ?\n> > >\n> > > Ah yes, the documentation seems to have stopped building some time\n> > > ago... there appear to be a couple more dependencies I have to install\n> > > for it to happen now. No problem!\n> >\n> > I've discovered today that it's best to build doxygen from source as you\n> > need a very recent version to have a quiet build.\n> >\n> > For me that was as straightforward as:\n> >\n> > git clone https://github.com/doxygen/doxygen\n> > cd doxygen\n> > mkdir build\n> > cd build\n> > cmake -DCMAKE_INSTALL_PREFIX=/usr ../ && make -j 8 all install\n> >\n> >\n> > > Thanks\n> > > David\n> > >\n> > >>\n> > >> The patch looks good otherwise!\n> > >> Reviewed-by: Jacopo Mondi \n> > >>\n> > >> Thanks\n> > >> j\n> > >>\n> > >>> + * \\brief Update the control information that was previously stored by\n> > >>> + * listControls(). Some parts of this information, such as min and max\n> > >>> + * values of some controls, are liable to change when a new format is set.\n> > >>> + */\n> > >>> +void V4L2Device::updateControlInfos()\n> > >>> +{\n> > >>> + for (auto &controlId : controlIds_) {\n> > >>> + unsigned int id = controlId->id();\n> > >>> +\n> > >>> + struct v4l2_query_ext_ctrl ctrl = {};\n> > >>> + ctrl.id = id;\n> > >>> +\n> > >>> + if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> > >>> + LOG(V4L2, Debug)\n> > >>> + << \"Could not refresh control \"\n> > >>> + << utils::hex(ctrl.id);\n> > >>> + continue;\n> > >>> + }\n> >\n> > I'm curious if 'all' controls need to be refreshed, but I think that's\n> > simpler that trying to filter it with a list that would then bitrot.\n> >\n> > So...\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > >>> +\n> > >>> + /* Assume these are present - listControls() made them. */\n> > >>> + controlInfo_[id] = ctrl;\n> > >>> + controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);\n> > >>> + }\n> > >>> +}\n> > >>> +\n> > >>> /*\n> > >>> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> > >>> * values in \\a v4l2Ctrls\n> > >>> --\n> > >>> 2.20.1\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> >\n> > --\n> > Regards\n> > --\n> > Kieran","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 721CBBDE78\n\tfor ;\n\tTue, 4 May 2021 08:19:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 005876891A;\n\tTue, 4 May 2021 10:19:35 +0200 (CEST)","from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com\n\t[IPv6:2a00:1450:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99F006890C\n\tfor ;\n\tTue, 4 May 2021 10:19:34 +0200 (CEST)","by mail-wr1-x42d.google.com with SMTP id l13so6723365wru.11\n\tfor ;\n\tTue, 04 May 2021 01:19: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=\"X0v0Jwgd\"; 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=U5V2994PUmZIGdrsQqmDcUlyO8vEPXM2sSauDNGrUkA=;\n\tb=X0v0JwgdTxCbUtvnBLuE51SRa67aB99rXaKC1K8B+Cvfd+esDoUCfSv2fWBzujWs1m\n\tVsAE1mFZoJFuLl3suGseddReSvYmbfP3vphE9dyFzpnbGkrqoa4HjFCN9d8yaPrnX6f8\n\tA1msdDvWK9Iw+x83jRnXoYD49k7BwAqRrCXdmcghPqJcKJH/vbZ7OvU+mWw7lPIgxNeH\n\t8okw1wt2/0yEPi9pEN2gHHc2i1oZPUslcGOeKxJyTDi81c/fcpQplXTia9x3WevrNA07\n\t+Cg6vJKj3NpRGPG+9tRCyoRY8OkjoQcQIY1TgNN7FuQjWtLv7tveRXSVviO4xw+ZBP5D\n\tv2vA==","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=U5V2994PUmZIGdrsQqmDcUlyO8vEPXM2sSauDNGrUkA=;\n\tb=dvJTtcKIH9YlLaYulB4PtcwyW1z7CF1xTIrGj/2nSN0arUf0C93fT//bq3kNdnvqT0\n\tTP63Sc5RFfh8Ve7gYX7efb+EoN/123RxO6Z8LXVHg/+fYS3YTnEW19AAjbSnFR9VCplt\n\tbc39r1PzrX5M9meKUuozoxcJvC1tzxAa+DV6cvPtqDDl2c4Zyh9/ubh5eOLFdhbEUAf9\n\tfBoIaxdYejS6ip0+2R3AAk0yKDIZ0HR5bonayuBok3Yp1NfSXLiRQo8qlpBchghdjWe7\n\tihO/ESIXwPgGgQOPHQ5Q9nAVpRHmTf4zuwsPRiF/KVsKKN6njpWDWpvXOi7OxA3UgKSS\n\tcJiA==","X-Gm-Message-State":"AOAM533ci74FIPxHzoIcOzS5aUEdqkvLsumQB26edh/aint/jF+yawTv\n\tZ+psPshxjYLJKECkOoP0fDBL/YSGHXuK8RXpkeFQig==","X-Google-Smtp-Source":"ABdhPJxJxSt34ikskSUKHGxw5euuriKYnB4w8VsaJcyVuCo3tbxj8A/9PQYy3MidWMcwKMyC6H/ukK7sJUU2VmXFbtM=","X-Received":"by 2002:a5d:408f:: with SMTP id\n\to15mr29184126wrp.89.1620116374167; \n\tTue, 04 May 2021 01:19:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210406104050.23814-1-david.plowman@raspberrypi.com>\n\t<20210406104050.23814-2-david.plowman@raspberrypi.com>\n\t<20210406121009.574d5mqcdaddznom@uno.localdomain>\n\t\n\t<1f62c66a-cafb-f6f6-aa25-044b2b865045@ideasonboard.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 4 May 2021 09:19:22 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16756,"web_url":"https://patchwork.libcamera.org/comment/16756/","msgid":"","date":"2021-05-04T12:15:18","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:\n\nOh my, that's such a long delay :-( Sorry about that.\n\n> The minimum and maximum vblanking can change when a new format is\n> applied to the sensor subdevice, so be sure to retrieve up-to-date\n> values.\n> \n> The V4L2Device acquires the new updateControlInfos() method to perform\n> this function, and which the CameraSensor calls automatically if its\n> setFormat method is used to update the sensor.\n> \n> However, not all pipeline handlers invoke the setFormat method\n> directly, so the new method must be made publicly available for\n> pipeline handlers to call if they need to.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 ++\n> include/libcamera/internal/v4l2_device.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++-\n> src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++\n\nI may have split the patch in two, but it's no big deal.\n\n> 4 files changed, 60 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 3e98f71b..c94744f0 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -68,6 +68,8 @@ public:\n> \tconst ControlList &properties() const { return properties_; }\n> \tint sensorInfo(CameraSensorInfo *info) const;\n> \n> +\tvoid updateControlInfos();\n\nLet's name this updateControlInfo() as information is uncountable. Same\nbelow.\n\n> +\n> protected:\n> \tstd::string logPrefix() const override;\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index d006bf68..274cbe65 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -41,6 +41,8 @@ public:\n> \tint setFrameStartEnabled(bool enable);\n> \tSignal frameStart;\n> \n> +\tvoid updateControlInfos();\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index f7ed91d9..5dbe47a7 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> /**\n> * \\brief Set the sensor output format\n> * \\param[in] format The desired sensor output format\n> + *\n> + * The sensor output format is set. The ranges of any controls associated\n> + * with the sensor are also updated.\n\nYou can drop the first sentence.\n\n> + *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> int CameraSensor::setFormat(V4L2SubdeviceFormat *format)\n> {\n> -\treturn subdev_->setFormat(pad_, format);\n> +\tint ret = subdev_->setFormat(pad_, format);\n> +\n> +\tif (ret == 0)\n> +\t\tupdateControlInfos();\n\nWe tend to return immediately on error.\n\n\tint ret = subdev_->setFormat(pad_, format);\n\tif (ret)\n\t\treturn ret;\n\n\tupdateControlInfos();\n\treturn 0;\n\n> +\n> +\treturn ret;\n> }\n> \n> /**\n> * \\brief Retrieve the supported V4L2 controls and their information\n> + *\n> + * Pipeline handlers that do not change the sensor format using the\n> + * CameraSensor::setFormat method may need to call\n\nJust \"setFormat()\" will be enough for doxygen to pick it up, as we're in\nthe same class. Same below.\n\n> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> + * ranges are up to date.\n\nI'd go the other way around, saying that the control information is\nupdated automatically when setFormat() is called.\n\n * Control information is updated automatically to reflect the current sensor\n * configuration when the setFormat() function is called, without invalidating\n * any iterator on the ControlInfoMap. A manual update can also be forced by\n * calling the updateControlInfo() function for pipeline handlers that change\n * the sensor configuration without using setFormat().\n\nNote that we really don't want to see more pipeline handlers doing this,\nand I'd like to deprecate this method in the future and switch to the\nregular CameraSensor::setFormat() for the Raspberry Pi pipeline handler\ntoo.\n\nPlease also note that we're considering removing the V4L2 dependency in\nthe API of the CameraSensor class. If that happens, we will really want\nto drop the manual updateControlInfo().\n\n> + *\n> * \\return A map of the V4L2 controls supported by the sensor\n> */\n> const ControlInfoMap &CameraSensor::controls() const\n> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)\n> * Sensor information is only available for raw sensors. When called for a YUV\n> * sensor, this function returns -EINVAL.\n> *\n> + * Pipeline handlers that do not change the sensor format using the\n> + * CameraSensor::setFormat method may need to call\n> + * CameraSensor::updateControlInfos beforehand, to ensure all the control\n> + * ranges are up to date.\n\nBased on the comment above, I'm tempted to drop this one.\n\n> + *\n> * \\return 0 on success, a negative error code otherwise\n> */\n> int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\fn void CameraSensor::updateControlInfos()\n> + * \\brief Update the sensor's ControlInfos in case they have changed\n\nIt's not ControlInfos but ControlInfoMap. You can add a \\sa\nV4L2Device::updateControlInfo()\n\n> + */\n> +void CameraSensor::updateControlInfos()\n> +{\n> +\tsubdev_->updateControlInfos();\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + entity_->name() + \"'\";\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..9678962c 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -514,6 +514,32 @@ void V4L2Device::listControls()\n> \tcontrols_ = std::move(ctrls);\n> }\n> \n> +/*\n> + * \\brief Update the control information that was previously stored by\n> + * listControls(). Some parts of this information, such as min and max\n> + * values of some controls, are liable to change when a new format is set.\n\nThat's not very brief :-) Let's split it. Also, listControls() being a\nprivate function, it's not great to document a public API based on it.\nAnd finally, we should specify the behaviour more precisely, as we're\nupdating information live.\n\n * \\brief Update the information for all device controls\n *\n * The V4L2Device class caches information about all controls supported by the\n * device and exposes it through the controls() and controlInfo() functions.\n * Control information may change at runtime, for instance when formats on a\n * subdev are modified. When this occurs, this function can be used to refresh\n * control information. The information is refreshed in-place, all pointers to\n * v4l2_query_ext_ctrl instances previously returned by controlInfo() and\n * iterators to the ControlInfoMap returned by controls() remain valid.\n *\n * Note that control information isn't refreshed automatically is it may be an\n * expensive operation. The V4L2Device users are responsible for calling this\n * function when required, based on their usage pattern of the class.\n\n> + */\n> +void V4L2Device::updateControlInfos()\n> +{\n> +\tfor (auto &controlId : controlIds_) {\n> +\t\tunsigned int id = controlId->id();\n> +\n> +\t\tstruct v4l2_query_ext_ctrl ctrl = {};\n> +\t\tctrl.id = id;\n> +\n> +\t\tif (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n> +\t\t\tLOG(V4L2, Debug)\n> +\t\t\t\t<< \"Could not refresh control \"\n> +\t\t\t\t<< utils::hex(ctrl.id);\n\ns/ctrl.id/id/\n\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* Assume these are present - listControls() made them. */\n> +\t\tcontrolInfo_[id] = ctrl;\n> +\t\tcontrols_.at(controlId.get()) = V4L2ControlInfo(ctrl);\n\nThat's two lookups for each entry, I think we can do better by iterating\nover controls_ instead of controlIds_. It could also be possibly to\navoid the copy by operating on the controlInfo_ entry directly.\n\n\tfor (auto &[controlId, info] : controls_) {\n\t\tunsigned int id = controlId->id();\n\n\t\t/*\n\t\t * Assume controlInfo_ has a corresponding entry, as it has been\n\t\t * generated by listControls().\n\t\t */\n\t\tstruct v4l2_query_ext_ctrl &ctrl = controlInfo_[id];\n\n\t\tif (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {\n\t\t\tLOG(V4L2, Debug)\n\t\t\t\t<< \"Could not refresh control \"\n\t\t\t\t<< utils::hex(id);\n\t\t\tcontinue;\n\t\t}\n\n\t\tinfo = V4L2ControlInfo(ctrl);\n\t}\n\n> +\t}\n> +}\n> +\n> /*\n> * \\brief Update the value of the first \\a count V4L2 controls in \\a ctrls using\n> * values in \\a v4l2Ctrls","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 590EABDE7A\n\tfor ;\n\tTue, 4 May 2021 12:15:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA6D76890E;\n\tTue, 4 May 2021 14:15:24 +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 9CF5D602BF\n\tfor ;\n\tTue, 4 May 2021 14:15:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE90E58E;\n\tTue, 4 May 2021 14:15:21 +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=\"Dk6InRMY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620130522;\n\tbh=WQBbUL6PCCh2lAFtgBpUyfc5yiaB064CHjGwnBWyZWI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Dk6InRMYbcnVST5Y5cf2wbRLqGiAVIsfnCI/RW0U1RhOgiNYjw3OmAbsIYnlsHsZC\n\to/ng161FOGy6LsRHIZ/eSXCujbqc76Zd1g8N3T0BxfV/koVqusMCqxHoruVpc1uZHC\n\tmh6aKS+DGB/sGbyhIRJrbSeiGLpnjXerRmsOrBFs=","Date":"Tue, 4 May 2021 15:15:18 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210406104050.23814-1-david.plowman@raspberrypi.com>\n\t<20210406104050.23814-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210406104050.23814-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix\n\tframe lengths calculated by sensorInfo()","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\" "}}]