[{"id":16076,"web_url":"https://patchwork.libcamera.org/comment/16076/","msgid":"<20210331121259.dhnow54lrjx6e57o@uno.localdomain>","date":"2021-03-31T12:12:59","subject":"Re: [libcamera-devel] [PATCH 1/1] 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\nI admit I have not followed closely the recent discussions on this\ntopic, but this seems an acceptable solution to me, however I wonder\nif it would not be worth taking it a little step forward (I know...)\n\nThe problem I see here is fundamentally one: we're missing a method\nto update a (V4L2)ControlInfo. The method itself could be quite\ntrivial, a ControlInfo::update(ControlValue min, ...) and one\nV4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).\n\nThe lack of a mechanism to update a control info has this\nconsequences:\n1) V4L2 controls passed by the ph to the IPA could be stale after a\nsensor format update\n2) the Camera::controls_ might be stale after a Camera::configure()\n\nIf you introduce the above suggested update() (or whatever) methods in\n(V4L2)ControlInfo, then they could be used as you're doing here below\nto handle 1)\n\nIn order to handle 2) pipeline handlers that register controls would\nhave to get a freshened list of V4L2Controls from the CameraSensor (or\nuse a freshened CameraSensorInfo) to update the controls they register\nas Camera::controls_, performing the opportune re-calculations (in\nexample the IPU3::initControls() function would need to be split in\ncalculateExposure(), calculateDurations() etc which will then be\ncalled at configure() time to update Camera::controls_.\n\nDo you think your proposed solution could work well with\nControlInfo::update() ?\n\nA few minors on the patch\n\nOn Tue, Mar 30, 2021 at 05:57:43PM +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 refreshControls() method to perform\n> this function. Note that not all pipeline handlers invoke the\n> subdevice's setFormat method directly, so the new method must be made\n> publicly available.\n\nI would however call refreshControls() in CameraSensor::setFormat()\n\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_device.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 7 +++++++\n> src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++++++++\n> 3 files changed, 35 insertions(+)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index d006bf68..6efcb0db 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 refreshControls();\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..9f99ce3e 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> \tinfo->bitsPerPixel = format.bitsPerPixel();\n> \tinfo->outputSize = format.size;\n>\n> +\t/*\n> +\t * Some controls, specifically the vblanking, may have different min\n> +\t * or max values if the sensor format has been changed since it was\n> +\t * opened, so be sure to update them first.\n> +\t */\n> +\tsubdev_->refreshControls();\n> +\n\nI feel a bit like it's up to the ph to be aware if controls need to be\nrefreshed or not. For 'regular' ph this would be transpared, calling\nCameraSensor::setFormat() would refresh controls and it is guaranteed\nthat CameraSensorInfo is always up-to-date.\n\nFor ph like yours that use CameraSensor but do not set the format\ndirectly on it, I think it's up to the ph to refresh controls after\nyou have successfully set a format on the Unicam::Image node (this\nwould require adding a refreshControls() method to CameraSensor\nthough, I'm not 100% sure it is worth it :/)\n\nThis would allow unnecessary refresh of controls, as keeping the state\nin the V4L2Device class through a flag would require to tweak into the\nsubclasses' setFormat() methods, and it might get nasty, if even\ndoable in clean way.\n\nAll in all, I would be happy with this patch if it would provide a way\nto update ControlInfo first, and then use it to update the\nV4L2Controls during a refresh. Do you think it would be worth it ?\n\nThanks\n j\n\n> \t/*\n> \t * Retrieve the pixel rate, line length and minimum/maximum frame\n> \t * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..6d396f89 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> + */\n> +void V4L2Device::refreshControls()\n> +{\n\nAnd I would here add a flag to the class, something like needRefersh,\nto be set to true in\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 DBA8CC0DA3\n\tfor ;\n\tWed, 31 Mar 2021 12:12:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A1A26877F;\n\tWed, 31 Mar 2021 14:12:26 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 573486051B\n\tfor ;\n\tWed, 31 Mar 2021 14:12:25 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id BB4B0E000B;\n\tWed, 31 Mar 2021 12:12:24 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Wed, 31 Mar 2021 14:12:59 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210331121259.dhnow54lrjx6e57o@uno.localdomain>","References":"<20210330165743.4924-1-david.plowman@raspberrypi.com>\n\t<20210330165743.4924-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210330165743.4924-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] 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":16080,"web_url":"https://patchwork.libcamera.org/comment/16080/","msgid":"","date":"2021-04-01T09:52:27","subject":"Re: [libcamera-devel] [PATCH 1/1] 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\nThanks for joining this discussion!\n\nOn Wed, 31 Mar 2021 at 13:12, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> I admit I have not followed closely the recent discussions on this\n> topic, but this seems an acceptable solution to me, however I wonder\n> if it would not be worth taking it a little step forward (I know...)\n>\n> The problem I see here is fundamentally one: we're missing a method\n> to update a (V4L2)ControlInfo. The method itself could be quite\n> trivial, a ControlInfo::update(ControlValue min, ...) and one\n> V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).\n\nSo to help me understand, would\n\ncontrolInfo.update(min, max, default)\nand\nv4l2ControlInfo.update(ctrl)\n\nbe the same as\n\ncontrolInfo = ControlInfo(min, max, default)\nand\nv4l2ControlInfo = V4L2ControlInfo(ctrl)\n?\n\n>\n> The lack of a mechanism to update a control info has this\n> consequences:\n> 1) V4L2 controls passed by the ph to the IPA could be stale after a\n> sensor format update\n\nYes - though in my case the stale v4l2 controls are actually within\nthe CameraSensor class itself.\n\n> 2) the Camera::controls_ might be stale after a Camera::configure()\n\nTrue. Though in the Raspberry Pi pipeline handler I don't think we\nreally do anything about this. Should we? We do pass min and max\nframerates back out to the application in the metadata.\n\n>\n> If you introduce the above suggested update() (or whatever) methods in\n> (V4L2)ControlInfo, then they could be used as you're doing here below\n> to handle 1)\n>\n> In order to handle 2) pipeline handlers that register controls would\n> have to get a freshened list of V4L2Controls from the CameraSensor (or\n> use a freshened CameraSensorInfo) to update the controls they register\n> as Camera::controls_, performing the opportune re-calculations (in\n> example the IPU3::initControls() function would need to be split in\n> calculateExposure(), calculateDurations() etc which will then be\n> called at configure() time to update Camera::controls_.\n>\n> Do you think your proposed solution could work well with\n> ControlInfo::update() ?\n\nI think so, though - as per the above - I'm not really clear on the\ndifference between an update method and using assignment. Do we think\nan update method is a bit tidier?\n\n>\n> A few minors on the patch\n>\n> On Tue, Mar 30, 2021 at 05:57:43PM +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 refreshControls() method to perform\n> > this function. Note that not all pipeline handlers invoke the\n> > subdevice's setFormat method directly, so the new method must be made\n> > publicly available.\n>\n> I would however call refreshControls() in CameraSensor::setFormat()\n\nYes, that seems a reasonable thing to do.\n\n>\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 2 ++\n> > src/libcamera/camera_sensor.cpp | 7 +++++++\n> > src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++++++++\n> > 3 files changed, 35 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index d006bf68..6efcb0db 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 refreshControls();\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..9f99ce3e 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > info->bitsPerPixel = format.bitsPerPixel();\n> > info->outputSize = format.size;\n> >\n> > + /*\n> > + * Some controls, specifically the vblanking, may have different min\n> > + * or max values if the sensor format has been changed since it was\n> > + * opened, so be sure to update them first.\n> > + */\n> > + subdev_->refreshControls();\n> > +\n>\n> I feel a bit like it's up to the ph to be aware if controls need to be\n> refreshed or not. For 'regular' ph this would be transpared, calling\n> CameraSensor::setFormat() would refresh controls and it is guaranteed\n> that CameraSensorInfo is always up-to-date.\n\nAgree.\n\n>\n> For ph like yours that use CameraSensor but do not set the format\n> directly on it, I think it's up to the ph to refresh controls after\n> you have successfully set a format on the Unicam::Image node (this\n> would require adding a refreshControls() method to CameraSensor\n> though, I'm not 100% sure it is worth it :/)\n\nYes, we could make it our pipeline handler's business to cause the\nCameraSensor to refresh its V4L2 controls, and then not do it in the\nsensorInfo() method. I think CameraSensor would need some kind of\npublic method for us to call, I don't see that there's another way for\nus to get at the stuff within the CameraSensor class.\n\n>\n> This would allow unnecessary refresh of controls, as keeping the state\n> in the V4L2Device class through a flag would require to tweak into the\n> subclasses' setFormat() methods, and it might get nasty, if even\n> doable in clean way.\n\nAh, are we suggesting a public CameraSensor::refresh() method which\nmerely sets a flag? CameraSensor::setFormat would call it, and indeed\nso would our pipeline handler.\n\nThen there's a private method which checks the flag, does the actual\nwork if it's set, and clears it. CameraSensor::sensorInfo would call\nthis, as indeed could other parts of the CameraSensor class if they\nare using the ControlInfos and feeling a bit nervous. This approach\nsounds like it would work too if we prefer it.\n\nAnyway, please let me know what you think on these points and I can\nproduce a v2 to look at.\n\nThanks and best regards\n\nDavid\n\n>\n> All in all, I would be happy with this patch if it would provide a way\n> to update ControlInfo first, and then use it to update the\n> V4L2Controls during a refresh. Do you think it would be worth it ?\n>\n> Thanks\n> j\n>\n> > /*\n> > * Retrieve the pixel rate, line length and minimum/maximum frame\n> > * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index decd19ef..6d396f89 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> > + * \\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::refreshControls()\n> > +{\n>\n> And I would here add a flag to the class, something like needRefersh,\n> to be set to true in\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 17DBAC0DA3\n\tfor ;\n\tThu, 1 Apr 2021 09:52:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65A466877C;\n\tThu, 1 Apr 2021 11:52:52 +0200 (CEST)","from mail-yb1-xb2a.google.com (mail-yb1-xb2a.google.com\n\t[IPv6:2607:f8b0:4864:20::b2a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1F2A6084F\n\tfor ;\n\tThu, 1 Apr 2021 11:52:49 +0200 (CEST)","by mail-yb1-xb2a.google.com with SMTP id z1so1168926ybf.6\n\tfor ;\n\tThu, 01 Apr 2021 02:52:49 -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=\"CVa/Oul/\"; 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=9X+xUu7s8nJSskqAMBUINaU7oMbWnPjlH9sT2ziUGdU=;\n\tb=CVa/Oul/bYE/LdcXFdTe4pyQRK4F1bIYl50G7NbNwoMTqYRXCoRgbnHP2M37MTtFKT\n\ti5tGKRUcr34pGl1CEKaHZcgGJNmH0q6LSHm6MCXt6SVnQJMG44a2Q+UdMb5PHg7XHVGL\n\ti2baSEGdicMUT+sDKNFm1k1/EEDgqZhtXiTEmY/E7NIkMC9Q0WHsLTKxTrtB44L4Cut/\n\tJ+BvMqUqfd6k46DgOR8Z4aG+JFJNNM7ePuFP62uPde7oYF5ZZJz8Nr2XToi0o6PdbD/p\n\tUlyNhGm7zGcLF9Z+mXT6y2206UdpLNervoxxDap1zVOD3I23hsikRQaa8GipGNqu+aTJ\n\t5GLg==","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=9X+xUu7s8nJSskqAMBUINaU7oMbWnPjlH9sT2ziUGdU=;\n\tb=RRr31zjmwnCmjrZ8i0hfMgF0oTAovUN1PxZ2ovo0c2ockon/21HWjt5CNFaylfDhAo\n\thr0RGuBghbAxkIYs0bpnFFlAmCVe+u7V1R+/p3zjhg25kjjF3ErXQWB2xD8BTT6J7GcJ\n\tkCg78MEM+Anvz5isTdX+q/tXpqgqB+HgUwvXW7Ck9c2k+Ir+tbvhdIc07rXaI54Z8YKF\n\t8pXLiAHELTbxtx+V8u14SK07nWpnkawKEAeHpXp1JzU7v5CmhQF4Ib+/VWoTk9j+c7FK\n\tj3ha+JMA68hCqKsFBVls6nvxst8PjtcT5HsGjBBw5g84sh+PyQzimcggCOWV2Ssy6ZY0\n\tTa2w==","X-Gm-Message-State":"AOAM532IkiQORh59vCSr5kAw3oLOHfE8RxSuJGqsQ91Gdqgd7NO1atmI\n\tcw7t1FSAdvjub2mxPTbYOJ1sqTYK1zyNTNFycHu37QYpFmo=","X-Google-Smtp-Source":"ABdhPJyEs5+n9rGuTdMYK2Jtl9HKPi+ABYcx6KXq/ksBb3fSJvntXIcCBkcN9i0EHWXdTKwqkrcgCjZyr/lqYMTzw2M=","X-Received":"by 2002:a9d:6249:: with SMTP id i9mr6234273otk.166.1617270758241;\n\tThu, 01 Apr 2021 02:52:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330165743.4924-1-david.plowman@raspberrypi.com>\n\t<20210330165743.4924-2-david.plowman@raspberrypi.com>\n\t<20210331121259.dhnow54lrjx6e57o@uno.localdomain>","In-Reply-To":"<20210331121259.dhnow54lrjx6e57o@uno.localdomain>","From":"David Plowman ","Date":"Thu, 1 Apr 2021 10:52:27 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 1/1] 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":16084,"web_url":"https://patchwork.libcamera.org/comment/16084/","msgid":"<20210401154219.u5dvwe4tqytro3dz@uno.localdomain>","date":"2021-04-01T15:42:19","subject":"Re: [libcamera-devel] [PATCH 1/1] 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":"On Thu, Apr 01, 2021 at 10:52:27AM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for joining this discussion!\n>\n> On Wed, 31 Mar 2021 at 13:12, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > I admit I have not followed closely the recent discussions on this\n> > topic, but this seems an acceptable solution to me, however I wonder\n> > if it would not be worth taking it a little step forward (I know...)\n> >\n> > The problem I see here is fundamentally one: we're missing a method\n> > to update a (V4L2)ControlInfo. The method itself could be quite\n> > trivial, a ControlInfo::update(ControlValue min, ...) and one\n> > V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).\n>\n> So to help me understand, would\n>\n> controlInfo.update(min, max, default)\n> and\n> v4l2ControlInfo.update(ctrl)\n>\n> be the same as\n>\n> controlInfo = ControlInfo(min, max, default)\n> and\n> v4l2ControlInfo = V4L2ControlInfo(ctrl)\n> ?\n>\n\nIt's just a syntactic sugar maybe, but I was wondering if it would be\nsafest in case application takes references to the Camera::controls_..\n\n\n> >\n> > The lack of a mechanism to update a control info has this\n> > consequences:\n> > 1) V4L2 controls passed by the ph to the IPA could be stale after a\n> > sensor format update\n>\n> Yes - though in my case the stale v4l2 controls are actually within\n> the CameraSensor class itself.\n>\n\nYup, through the CameraSensorInfo I meant..\n\n> > 2) the Camera::controls_ might be stale after a Camera::configure()\n>\n> True. Though in the Raspberry Pi pipeline handler I don't think we\n> really do anything about this. Should we? We do pass min and max\n> framerates back out to the application in the metadata.\n>\n\nCurrently no pipeline does that afict, and I thought we had a todo for\n\"update this control when a mechanism to do so exists\" but it's gone\nthrough reviews of the frame durations series probably...\n\n> >\n> > If you introduce the above suggested update() (or whatever) methods in\n> > (V4L2)ControlInfo, then they could be used as you're doing here below\n> > to handle 1)\n> >\n> > In order to handle 2) pipeline handlers that register controls would\n> > have to get a freshened list of V4L2Controls from the CameraSensor (or\n> > use a freshened CameraSensorInfo) to update the controls they register\n> > as Camera::controls_, performing the opportune re-calculations (in\n> > example the IPU3::initControls() function would need to be split in\n> > calculateExposure(), calculateDurations() etc which will then be\n> > called at configure() time to update Camera::controls_.\n> >\n> > Do you think your proposed solution could work well with\n> > ControlInfo::update() ?\n>\n> I think so, though - as per the above - I'm not really clear on the\n> difference between an update method and using assignment. Do we think\n> an update method is a bit tidier?\n>\n\nthat and the concern about apps taking pointers to them. But it's a\nminor indeed.\n\n> >\n> > A few minors on the patch\n> >\n> > On Tue, Mar 30, 2021 at 05:57:43PM +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 refreshControls() method to perform\n> > > this function. Note that not all pipeline handlers invoke the\n> > > subdevice's setFormat method directly, so the new method must be made\n> > > publicly available.\n> >\n> > I would however call refreshControls() in CameraSensor::setFormat()\n>\n> Yes, that seems a reasonable thing to do.\n>\n> >\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/v4l2_device.h | 2 ++\n> > > src/libcamera/camera_sensor.cpp | 7 +++++++\n> > > src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++++++++\n> > > 3 files changed, 35 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index d006bf68..6efcb0db 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 refreshControls();\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..9f99ce3e 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > > info->bitsPerPixel = format.bitsPerPixel();\n> > > info->outputSize = format.size;\n> > >\n> > > + /*\n> > > + * Some controls, specifically the vblanking, may have different min\n> > > + * or max values if the sensor format has been changed since it was\n> > > + * opened, so be sure to update them first.\n> > > + */\n> > > + subdev_->refreshControls();\n> > > +\n> >\n> > I feel a bit like it's up to the ph to be aware if controls need to be\n> > refreshed or not. For 'regular' ph this would be transpared, calling\n> > CameraSensor::setFormat() would refresh controls and it is guaranteed\n> > that CameraSensorInfo is always up-to-date.\n>\n> Agree.\n>\n> >\n> > For ph like yours that use CameraSensor but do not set the format\n> > directly on it, I think it's up to the ph to refresh controls after\n> > you have successfully set a format on the Unicam::Image node (this\n> > would require adding a refreshControls() method to CameraSensor\n> > though, I'm not 100% sure it is worth it :/)\n>\n> Yes, we could make it our pipeline handler's business to cause the\n> CameraSensor to refresh its V4L2 controls, and then not do it in the\n> sensorInfo() method. I think CameraSensor would need some kind of\n> public method for us to call, I don't see that there's another way for\n> us to get at the stuff within the CameraSensor class.\n>\n\nI didn't notice the method was public already, and I was not a 100%\nsure making it public would be worth it. Although I'm not super-happy\neven about my proposal, missing a call to refreshControl() might cause nasty\nerrors during pipeline development, if that operation could be done\nautomatically that would be better. Your proposal does that for\nsensorInfo(), we could do the same in CameraSensor::controls(), but\nthat would cause unnecessary refreshes if the state is not kept\nsomewhere...\n\nAll in all I would prefer calling refresh() at setFormat() time as a\npublic method that ph that knows what they're doing have to call by\nthemselves.. Also, I would add a note in the CameraSensor::sensorInfo()\nand controls() documentation that states that ph that do not use\nsetFormat() have to refresh controls before accessing them. But I'm\nnot thrilled, so just take this a suggestion..\n\n> >\n> > This would allow unnecessary refresh of controls, as keeping the state\n> > in the V4L2Device class through a flag would require to tweak into the\n> > subclasses' setFormat() methods, and it might get nasty, if even\n> > doable in clean way.\n>\n> Ah, are we suggesting a public CameraSensor::refresh() method which\n> merely sets a flag? CameraSensor::setFormat would call it, and indeed\n> so would our pipeline handler.\n>\n\nNot really, I was just mumbling on how hard it would be to have the state\nflag kept at the video device level :) but your idea is good too!\n\n> Then there's a private method which checks the flag, does the actual\n> work if it's set, and clears it. CameraSensor::sensorInfo would call\n> this, as indeed could other parts of the CameraSensor class if they\n> are using the ControlInfos and feeling a bit nervous. This approach\n> sounds like it would work too if we prefer it.\n>\n> Anyway, please let me know what you think on these points and I can\n> produce a v2 to look at.\n>\n> Thanks and best regards\n>\n> David\n>\n> >\n> > All in all, I would be happy with this patch if it would provide a way\n> > to update ControlInfo first, and then use it to update the\n> > V4L2Controls during a refresh. Do you think it would be worth it ?\n> >\n> > Thanks\n> > j\n> >\n> > > /*\n> > > * Retrieve the pixel rate, line length and minimum/maximum frame\n> > > * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index decd19ef..6d396f89 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> > > + * \\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::refreshControls()\n> > > +{\n> >\n> > And I would here add a flag to the class, something like needRefersh,\n> > to be set to true in\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 A4EB0C0DA3\n\tfor ;\n\tThu, 1 Apr 2021 15:41:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29A9168784;\n\tThu, 1 Apr 2021 17:41:46 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AAD136877D\n\tfor ;\n\tThu, 1 Apr 2021 17:41:45 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 3C269E0005;\n\tThu, 1 Apr 2021 15:41:44 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 1 Apr 2021 17:42:19 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210401154219.u5dvwe4tqytro3dz@uno.localdomain>","References":"<20210330165743.4924-1-david.plowman@raspberrypi.com>\n\t<20210330165743.4924-2-david.plowman@raspberrypi.com>\n\t<20210331121259.dhnow54lrjx6e57o@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] 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\" "}}]