[{"id":24595,"web_url":"https://patchwork.libcamera.org/comment/24595/","msgid":"","date":"2022-08-16T03:23:35","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Kieran,\n\nThank you for the patch.\n\nOn Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> From: Kieran Bingham via libcamera-devel \n\nThis isn't right.\n\n> Report an error when failing to process controls, but still allow the\n> request to process and complete where possible.\n> \n> The Request ControlError flag is raised on the request.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=135\n> Signed-off-by: Kieran Bingham \n> Reviewed-by: Umang Jain \n> Reviewed-by: Jacopo Mondi \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index fbe02cdcd520..8ffa27b1337b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -26,6 +26,7 @@\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/request.h\"\n> #include \"libcamera/internal/sysfs.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> \t}\n> \n> \tint ret = processControls(data, request);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\tif (ret < 0) {\n> +\t\tLOG(UVC, Error) << \"Failed to process controls\";\n\nHave you ever noticed this ? I'm wondering if we would need to\nrate-limit this message.\n\n> +\t\trequest->_d()->setError(Request::ControlError);\n\nThere's a bit of a mismatch between this and the definition of\nControlError in patch 01/10, as it recommends checking the metadata to\nsee what happened, and the UVC pipeline handler doesn't generate any\nmetadata. Generally speaking, checking metadata seems cumbersome (at\nbest) for any camera that would have a non-zero max sync latency. This\nshould be considered to figure out how an application would handle\nerrors in that case.\n\n> +\t}\n> \n> \tret = data->video_->queueBuffer(buffer);\n> \tif (ret < 0)","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 F161CBE173\n\tfor ;\n\tTue, 16 Aug 2022 03:23:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CF4861FC0;\n\tTue, 16 Aug 2022 05:23:51 +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 7ECEC603E3\n\tfor ;\n\tTue, 16 Aug 2022 05:23:49 +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 E0E30496;\n\tTue, 16 Aug 2022 05:23:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660620231;\n\tbh=K8dfHdg8sHZoat9yaubJY5f5ZK8E57s9zIgXrtmsAAg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=arlyAkoXVsX9nvRu6S3uJr+ZRso9PtxTkopSxTNFHrpXvDecwZgK9aZYHeljpwsan\n\tko6Q7yP8SmclPp2gQl6VK9oSmHqqjpnHpc8LS/Pj+2URlTfMPIJGM/T5Kcx9Wy6NYX\n\tjVNVnlGcOq0zPujq/r5hPSzf9AeYq04XBx0jWWUwhKo0eGD3xbqyfgHTTLNgLnsLBR\n\tNtKLUkC+v8A8k8cpGDdQf6y8k/x0jHCVnyYPbfsUffJrOsYnOnDOB0zc4sgiceoli5\n\tdGZATzkKt1qUg3Be/cl5eBvbZsUnumQ4aamxzAmxGVkLuuoJ1LKsHWmeI1TXZRU1Nu\n\tPd38PEMw/Bidw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660620229;\n\tbh=K8dfHdg8sHZoat9yaubJY5f5ZK8E57s9zIgXrtmsAAg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WIxC0Xk+5E7KMtcJYvuPsqsZLna89HGGuGWE1rQ3IGVvCkK/pDBj62W5zeXVI8kAV\n\t+Ar6311LX7FekuwL59P7p+Vg1dF7aK3HZHvy54WLwW1E7lGax6f0Iz7jf/P85ZfaXi\n\tc4wDeyLXWt0TX3DeV8Jdh/El0Znp3lgH693FNjyo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WIxC0Xk+\"; dkim-atps=neutral","Date":"Tue, 16 Aug 2022 06:23:35 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220805135312.47497-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24619,"web_url":"https://patchwork.libcamera.org/comment/24619/","msgid":"<20220817091818.gkqtc2y5uwltyfdv@uno.localdomain>","date":"2022-08-17T09:18:18","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Aug 16, 2022 at 06:23:35AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo and Kieran,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > From: Kieran Bingham via libcamera-devel \n>\n> This isn't right.\n\nI -hate- this, it makes applying patches from email more painful than\nnecessary. How can we fix this ?\n\n>\n> > Report an error when failing to process controls, but still allow the\n> > request to process and complete where possible.\n> >\n> > The Request ControlError flag is raised on the request.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135\n> > Signed-off-by: Kieran Bingham \n> > Reviewed-by: Umang Jain \n> > Reviewed-by: Jacopo Mondi \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++--\n> > 1 file changed, 5 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index fbe02cdcd520..8ffa27b1337b 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -26,6 +26,7 @@\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > +#include \"libcamera/internal/request.h\"\n> > #include \"libcamera/internal/sysfs.h\"\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> >\n> > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> > \t}\n> >\n> > \tint ret = processControls(data, request);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > +\tif (ret < 0) {\n> > +\t\tLOG(UVC, Error) << \"Failed to process controls\";\n>\n> Have you ever noticed this ? I'm wondering if we would need to\n> rate-limit this message.\n>\n\nI didn't, but we have a bug for that, so I assume it happens..\n\n> > +\t\trequest->_d()->setError(Request::ControlError);\n>\n> There's a bit of a mismatch between this and the definition of\n> ControlError in patch 01/10, as it recommends checking the metadata to\n> see what happened, and the UVC pipeline handler doesn't generate any\n> metadata. Generally speaking, checking metadata seems cumbersome (at\n> best) for any camera that would have a non-zero max sync latency. This\n\nAnd comparing the metadata with the requested controls is indeed\nexpensive and error-prone\n\n> should be considered to figure out how an application would handle\n> errors in that case.\n>\n\nIf they have max-latency > 0 it means they don't support per-frame\ncontrol so indeed the returned metadata can possibily not match the\nrequested controls and finding out which ones will converge in the\nnext requests and which one have failed won't be possible.\n\nIs the Request's control list still populated in a completed Request ?\nIf that's the case we can add a flag to the Control<> instances\nthemselves ? (not excited by that tbh)\n\n> > +\t}\n> >\n> > \tret = data->video_->queueBuffer(buffer);\n> > \tif (ret < 0)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1CC6FBE173\n\tfor ;\n\tWed, 17 Aug 2022 09:18:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4106E61FC0;\n\tWed, 17 Aug 2022 11:18:22 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A93D861FA6\n\tfor ;\n\tWed, 17 Aug 2022 11:18:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 06A0F200008;\n\tWed, 17 Aug 2022 09:18:19 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660727902;\n\tbh=c4s4RTTU3Yn7JMSbKGki99LtTE1MtmClxsS974Gj12E=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=oA41wW4qk/ez/jzu+rDdD84+aismDcFDhmrE2XTsJ+kHUIiUWDe1DaynKSUNj1/XN\n\tgpJyNge8+1OII7WAL7ehs8m45EJ83ZLkmlur7lQiNr7VEXGUiexuCkzO/I2q41hZ5h\n\tFOrQSIujcr8avOTQ0ki/yF1jAC9+dcce12SyNoKFTDPZqaezPAJcImza4DSBfBB0G9\n\tc5SmXviGbX9naRiNP4CgwDg6/jMs/LeTs+Ar9zlEm4N25DjyRw9ZR72fs7aofEE5k/\n\t8pekc6SUyBgVhED3OUgGra7AZNweQXezr3depBRGCnKqSi3f+0riCmK/saJZSjVRZa\n\td2mQCLwaalHWQ==","Date":"Wed, 17 Aug 2022 11:18:18 +0200","To":"Laurent Pinchart ","Message-ID":"<20220817091818.gkqtc2y5uwltyfdv@uno.localdomain>","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24621,"web_url":"https://patchwork.libcamera.org/comment/24621/","msgid":"<166073750510.3403653.9070548075710487212@Monstersaurus>","date":"2022-08-17T11:58:25","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-08-17 10:18:18)\n> Hi Laurent\n> \n> On Tue, Aug 16, 2022 at 06:23:35AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo and Kieran,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > From: Kieran Bingham via libcamera-devel \n> >\n> > This isn't right.\n> \n> I -hate- this, it makes applying patches from email more painful than\n> necessary. How can we fix this ?\n> \n> >\n> > > Report an error when failing to process controls, but still allow the\n> > > request to process and complete where possible.\n> > >\n> > > The Request ControlError flag is raised on the request.\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135\n> > > Signed-off-by: Kieran Bingham \n> > > Reviewed-by: Umang Jain \n> > > Reviewed-by: Jacopo Mondi \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++--\n> > > 1 file changed, 5 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index fbe02cdcd520..8ffa27b1337b 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -26,6 +26,7 @@\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > +#include \"libcamera/internal/request.h\"\n> > > #include \"libcamera/internal/sysfs.h\"\n> > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > >\n> > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> > > }\n> > >\n> > > int ret = processControls(data, request);\n> > > - if (ret < 0)\n> > > - return ret;\n> > > + if (ret < 0) {\n> > > + LOG(UVC, Error) << \"Failed to process controls\";\n> >\n> > Have you ever noticed this ? I'm wondering if we would need to\n> > rate-limit this message.\n> >\n> \n> I didn't, but we have a bug for that, so I assume it happens..\n\nYes, it can quite easily be caused to happen (demonstrated by Utkarsh's\ncontrol settings window implementation).\n\nIt will occur once per request that contains a control that fails ...\nDoes it need to be rate limited more than that?\n\nThe catch is that if the request fails, in Utkarsh's implementation -\nthe control is still in the list - so it gets resubmitted.\n\nWe might have to make sure applications are checking that they don't\ncontinuously queue invalid controls... But perhaps that's up to the app.\n\n \n> > > + request->_d()->setError(Request::ControlError);\n> >\n> > There's a bit of a mismatch between this and the definition of\n> > ControlError in patch 01/10, as it recommends checking the metadata to\n> > see what happened, and the UVC pipeline handler doesn't generate any\n> > metadata. Generally speaking, checking metadata seems cumbersome (at\n\nThat's a separate issue in the UVC pipeline handler right ?\n\n> > best) for any camera that would have a non-zero max sync latency. This\n> \n> And comparing the metadata with the requested controls is indeed\n> expensive and error-prone\n\nAgreed, I did already start looking into getting the control lists to\nreturn 'which' control fails ... but that's a WIP, and changes the\nunderlying error handling of setting control lists.\n\n\n> > should be considered to figure out how an application would handle\n> > errors in that case.\n> >\n> \n> If they have max-latency > 0 it means they don't support per-frame\n> control so indeed the returned metadata can possibily not match the\n> requested controls and finding out which ones will converge in the\n> next requests and which one have failed won't be possible.\n> \n> Is the Request's control list still populated in a completed Request ?\n\nIt still exists there yes ...\n\n> If that's the case we can add a flag to the Control<> instances\n> themselves ? (not excited by that tbh)\n\nMe neither. Do we need to return a control list which contains a list of\nany controls that failed ? (separately to the metadata, and the incoming\ncontrols ?)\n\n--\nKieran\n\n\n> \n> > > + }\n> > >\n> > > ret = data->video_->queueBuffer(buffer);\n> > > if (ret < 0)\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B098EBE173\n\tfor ;\n\tWed, 17 Aug 2022 11:58:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB25861FC0;\n\tWed, 17 Aug 2022 13:58:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A21961FA8\n\tfor ;\n\tWed, 17 Aug 2022 13:58:27 +0200 (CEST)","from pendragon.ideasonboard.com\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 18F0B49C;\n\tWed, 17 Aug 2022 13:58:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660737508;\n\tbh=6slDELPDPYZOu5+F6PQPodT6paYp96qOtW3gLh5+rBs=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=cH0UPeVsu3BGId49Z1JvUVHTN39RijVjzKJvohZ1cNGgXbQGdFe5awFNpWjyvLGXj\n\tF5G1AtrQrCqdSi+k4Q8Sl7k1ZVDIjfFA2DtYGez+Kfj50LNTvYG/D/jKk+D5OX7wxZ\n\tgbz8zhptwthox7Dsmb+IrQ7uZlBna9y34nDa7BBBL06atdLFe1UePDWxERhenSHT0S\n\t5SkcUSkhJhO4wxLBQXC2iZ5ksKwcWHAB51tD3V2X9o8p0MhkCQMwhDEf5UJsMwm1P7\n\tmjbRfoY5Eq9zubVqd5RR889MvpT20RlqqpmtCOwWmpuxxZY1flAgcAe49nxKtlogn0\n\touZABpt30veBw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660737507;\n\tbh=6slDELPDPYZOu5+F6PQPodT6paYp96qOtW3gLh5+rBs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZdtIf/k3nOQp7CUdy0pEGI3Tt7DODnRmJ26FDRFcgJXTvXvfGybUurFvIQT5IcovM\n\tislUYxxFjJvhbpkIrBJvPjG4OjyHyypN/jInmSz4BXGuRLkVD9D4L+hpb/oOgrXRIn\n\t/df8re4DNoC95ag4cH53/TXObCa6CgWUsSAJfNzs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZdtIf/k3\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220817091818.gkqtc2y5uwltyfdv@uno.localdomain>","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-3-jacopo@jmondi.org>\n\t\n\t<20220817091818.gkqtc2y5uwltyfdv@uno.localdomain>","To":"Jacopo Mondi , Jacopo Mondi via libcamera-devel\n\t, \n\tLaurent Pinchart ","Date":"Wed, 17 Aug 2022 12:58:25 +0100","Message-ID":"<166073750510.3403653.9070548075710487212@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24622,"web_url":"https://patchwork.libcamera.org/comment/24622/","msgid":"","date":"2022-08-17T12:54:55","subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","submitter":{"id":114,"url":"https://patchwork.libcamera.org/api/people/114/","name":"Utkarsh Tiwari","email":"utkarsh02t@gmail.com"},"content":"On Wed, Aug 17, 2022 at 5:28 PM Kieran Bingham via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> Quoting Jacopo Mondi via libcamera-devel (2022-08-17 10:18:18)\n> > Hi Laurent\n> >\n> > On Tue, Aug 16, 2022 at 06:23:35AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo and Kieran,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Fri, Aug 05, 2022 at 03:53:04PM +0200, Jacopo Mondi via\n> libcamera-devel wrote:\n> > > > From: Kieran Bingham via libcamera-devel <\n> libcamera-devel@lists.libcamera.org>\n> > >\n> > > This isn't right.\n> >\n> > I -hate- this, it makes applying patches from email more painful than\n> > necessary. How can we fix this ?\n> >\n> > >\n> > > > Report an error when failing to process controls, but still allow the\n> > > > request to process and complete where possible.\n> > > >\n> > > > The Request ControlError flag is raised on the request.\n> > > >\n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135\n> > > > Signed-off-by: Kieran Bingham \n> > > > Reviewed-by: Umang Jain \n> > > > Reviewed-by: Jacopo Mondi \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++--\n> > > > 1 file changed, 5 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index fbe02cdcd520..8ffa27b1337b 100644\n> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > @@ -26,6 +26,7 @@\n> > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > #include \"libcamera/internal/media_device.h\"\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > +#include \"libcamera/internal/request.h\"\n> > > > #include \"libcamera/internal/sysfs.h\"\n> > > > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > > >\n> > > > @@ -373,8 +374,10 @@ int\n> PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> > > > }\n> > > >\n> > > > int ret = processControls(data, request);\n> > > > - if (ret < 0)\n> > > > - return ret;\n> > > > + if (ret < 0) {\n> > > > + LOG(UVC, Error) << \"Failed to process controls\";\n> > >\n> > > Have you ever noticed this ? I'm wondering if we would need to\n> > > rate-limit this message.\n> > >\n> >\n> > I didn't, but we have a bug for that, so I assume it happens..\n>\n> Yes, it can quite easily be caused to happen (demonstrated by Utkarsh's\n> control settings window implementation).\n>\n> It will occur once per request that contains a control that fails ...\n> Does it need to be rate limited more than that?\n>\nI don't think so, each request that fails generates this error message.\nHelps in easier debugging.\n\n>\n> The catch is that if the request fails, in Utkarsh's implementation -\n> the control is still in the list - so it gets resubmitted.\n>\n> We might have to make sure applications are checking that they don't\n> continuously queue invalid controls... But perhaps that's up to the app.\n>\n> Yeah libcamera should just inform that some controls have failed\n(possibly which ones) what happens next depends upon the application.\n\n>\n> > > > + request->_d()->setError(Request::ControlError);\n> > >\n> > > There's a bit of a mismatch between this and the definition of\n> > > ControlError in patch 01/10, as it recommends checking the metadata to\n> > > see what happened, and the UVC pipeline handler doesn't generate any\n> > > metadata. Generally speaking, checking metadata seems cumbersome (at\n>\n> That's a separate issue in the UVC pipeline handler right ?\n>\n> > > best) for any camera that would have a non-zero max sync latency. This\n> >\n> > And comparing the metadata with the requested controls is indeed\n> > expensive and error-prone\n>\n> Agreed, I did already start looking into getting the control lists to\n> return 'which' control fails ... but that's a WIP, and changes the\n> underlying error handling of setting control lists.\n>\n>\n> > > should be considered to figure out how an application would handle\n> > > errors in that case.\n> > >\n> >\n> > If they have max-latency > 0 it means they don't support per-frame\n> > control so indeed the returned metadata can possibily not match the\n> > requested controls and finding out which ones will converge in the\n> > next requests and which one have failed won't be possible.\n> >\n> > Is the Request's control list still populated in a completed Request ?\n>\n> It still exists there yes ...\n>\n> > If that's the case we can add a flag to the Control<> instances\n> > themselves ? (not excited by that tbh)\n>\n> Me neither. Do we need to return a control list which contains a list of\n> any controls that failed ? (separately to the metadata, and the incoming\n> controls ?)\n>\n> I like this idea, it would allow the app to exactly know which controls\nhave\nFailed.\n\n> --\n> Kieran\n>\n>\n> >\n> > > > + }\n> > > >\n> > > > ret = data->video_->queueBuffer(buffer);\n> > > > if (ret < 0)\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9877DC3272\n\tfor ;\n\tWed, 17 Aug 2022 12:55:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA08961FC0;\n\tWed, 17 Aug 2022 14:55:11 +0200 (CEST)","from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com\n\t[IPv6:2607:f8b0:4864:20::1036])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFB4A61FA8\n\tfor ;\n\tWed, 17 Aug 2022 14:55:09 +0200 (CEST)","by mail-pj1-x1036.google.com with SMTP id a8so12394359pjg.5\n\tfor ;\n\tWed, 17 Aug 2022 05:55:09 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660740911;\n\tbh=PM9Q6QY/gwc8zIM5O9gn5Wz+NDId4yjYhTWqS74gBco=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XPwtfzoB3NqvqVWKkboHhPUg4JhHcT+hZ7TOJn1SrdE07E71LWdsUKVMuTrA7TxWJ\n\tkYk9IC35t7NW41plw4eGM40TO5FZ8jZYoNCwUTwlwdtZq20dVYeoVBSWzD8n22LJnc\n\t+nF+DD0L0QO8tvEdryJPUckEZWtD0KXXZmHLVFhtL5v2M02x1EJ/Jfyb+YKBsWbBfq\n\tsqzgEzEQ0fsJQJUfDKKIVAXEaJhqKRKok9WKAoagaDEqTxYslP2xI462rfyRN4HD7G\n\txeAkmTej+NVYIaCfxp/hRltYFk/IjDmCT+eZlX/LH7o1i1lPAfkNsgci1qEA+P1pUV\n\trqjzZhAm3h79g==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc;\n\tbh=rTbvc5OpKJMHGJ6tOAUJ1FRQA1uWQLuOWuFC4N1j85M=;\n\tb=KUmL5apBkXcPqCjkbBfhE5MCt54VvKlsk/lQ+/VNqeyktefh89mmNiZHTJ7a0+AOpx\n\tD+QaP5dnkSNW9ciMfjW6Xo4KAGKNzqa2PIqAMU+3qDdQ+BjhyxqVQckSmMZGe+xuV6Fd\n\t6Fe3/UPwAWjQD6l6DotRnPZ5imme55x01fWot1YNesZRYxJqXyb3aN5OJEpPsotZrqHr\n\ty8yFQLB4akLUuEJz9YuTMnO2ueRojHkuPGcdRF40rsGcxfF0MnaVQliFlD76ZBlEVjDy\n\tC543RyGcnahh2PqdV/lrSMJRtgdhneHfRfiUjQfokCOsDyFRhdH8zLv0iz/xMIJbBdd+\n\t8c1Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"KUmL5apB\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc;\n\tbh=rTbvc5OpKJMHGJ6tOAUJ1FRQA1uWQLuOWuFC4N1j85M=;\n\tb=grSHEI3kpDpAMIEWBbmioN+sitLt3mab9JbHbAZc66fbuISlKAbvfpcB49hzFh9PkZ\n\t/kD8gAB+iACjSqTWQXrh/z5/3ABPpJylERmAx4an+40j8Drm9dsJ4Cz9rBX1vfnhv8l6\n\tCKCttOg52Q5fLBbkI20ZtO0DZnMHHW3SMzHnFxD6DZiJuFtj4UgTc7m4dB1/7dV/Yr78\n\tHgD14jlkwlCUTzwSMKGxAFb70LV6AFcUqVrcElFvVHx7Naeo4gbHwXXAndUXOfsfHiEz\n\t5avsJ8BhpvSvjnFnqO93YAiJzFiu6jT9UClASmTEKYLJv0UbsH3Bsh3FdXJST0gy5gLa\n\tXb8w==","X-Gm-Message-State":"ACgBeo2Mlg4zTtqSYu33izSrAExPG5ARqPUCnwRr5rCLiST8ZnFpedQ1\n\t/DaWBgPuQkJg65BuSNkO4sXG1YdVU9cRtYFAm8g=","X-Google-Smtp-Source":"AA6agR7r2vUJyN7kIrQPXmjb8+5/JaQKIvHUWdAkKrfV6xAVbo4i9lg4lkfboMVGbMs5eBxlrwjo3RZWfEpse/WyhjU=","X-Received":"by 2002:a17:90b:1241:b0:1f3:1d9f:a933 with SMTP id\n\tgx1-20020a17090b124100b001f31d9fa933mr3557533pjb.221.1660740908029;\n\tWed, 17 Aug 2022 05:55:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20220805135312.47497-1-jacopo@jmondi.org>\n\t<20220805135312.47497-3-jacopo@jmondi.org>\n\t\n\t<20220817091818.gkqtc2y5uwltyfdv@uno.localdomain>\n\t<166073750510.3403653.9070548075710487212@Monstersaurus>","In-Reply-To":"<166073750510.3403653.9070548075710487212@Monstersaurus>","Date":"Wed, 17 Aug 2022 18:24:55 +0530","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000e31bfd05e66f5f91\"","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] libcamera: pipeline:\n\tuvcvideo: Report control errors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Utkarsh Tiwari via libcamera-devel\n\t","Reply-To":"Utkarsh Tiwari ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]