[{"id":23967,"web_url":"https://patchwork.libcamera.org/comment/23967/","msgid":"<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>","date":"2022-07-19T15:22:59","subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush\n\nOn Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Now that controls::get() returns a std::optional handle invalid controls,\n> the error log message in ControlList::find() is unnecessary and likely invalid\n> in the case when calling controls::get() on a missing control. Remove this\n> error message.\n\nIsn't anyway up to the application to validate the returned\nstd::optional<> ? What if an application doesn't do so an tries to\naccess an std::nullopt ? At least the messages here would provide a\ntrace that something went wrong ?\n\n>\n> Fixes: 1c4d48018505 (\"libcamera: controls: Use std::optional to handle invalid control values\")\n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/controls.cpp | 12 ++----------\n> 1 file changed, 2 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 03ac6345247c..701a872185e3 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> const ControlValue *ControlList::find(unsigned int id) const\n> {\n> \tconst auto iter = controls_.find(id);\n> -\tif (iter == controls_.end()) {\n> -\t\tLOG(Controls, Error)\n> -\t\t\t<< \"Control \" << utils::hex(id) << \" not found\";\n> -\n> +\tif (iter == controls_.end())\n> \t\treturn nullptr;\n> -\t}\n>\n> \treturn &iter->second;\n> }\n>\n> ControlValue *ControlList::find(unsigned int id)\n> {\n> -\tif (validator_ && !validator_->validate(id)) {\n> -\t\tLOG(Controls, Error)\n> -\t\t\t<< \"Control \" << utils::hex(id)\n> -\t\t\t<< \" is not valid for \" << validator_->name();\n> +\tif (validator_ && !validator_->validate(id))\n> \t\treturn nullptr;\n> -\t}\n>\n> \treturn &controls_[id];\n> }\n> --\n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 95D54BD1F1\n\tfor ;\n\tTue, 19 Jul 2022 15:23:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1529A6048A;\n\tTue, 19 Jul 2022 17:23:04 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8D1F603F4\n\tfor ;\n\tTue, 19 Jul 2022 17:23:01 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 5FF0D240002;\n\tTue, 19 Jul 2022 15:23:01 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658244184;\n\tbh=TwPc5JZ6lsrkdbGHzshQWNd1OZJPbtzAF81G+dMp6UQ=;\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=h29Lva75KOMW+aVxh3yaOixSbYjwQNG/5wF2oYbsW9hK4zP4MxMheicEgw1TmN2rh\n\thgdIrRJDiC18fslgqydC8AvmMTa3R89BgwfgfoB89ruAvpM6uux7BZn35gJmlrvpGa\n\txF0Gt+FIikwv/S+dd+r7E5S08+N4ODucHmuYXgrt3R77WgdZjH41YYMJzQ3LawJT07\n\tYO6tZK76YBps+DTAOYMuzDk49nhP3vFTcahHDC4KFwGFAuzQHz45ypMekUDQAMj8UC\n\t7f0b+lX2q4gbXoPPzlTzCuBU1XazbWjm+TO9efKCyVuoAudbozvpAv2ed/6vzDUoa2\n\tKbe0Inc54s+9w==","Date":"Tue, 19 Jul 2022 17:22:59 +0200","To":"Naushir Patuck ","Message-ID":"<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>","References":"<20220719144517.15898-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220719144517.15898-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","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":23971,"web_url":"https://patchwork.libcamera.org/comment/23971/","msgid":"","date":"2022-07-19T16:13:15","subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Tue, 19 Jul 2022 at 16:23, Jacopo Mondi wrote:\n\n> Hi Naush\n>\n> On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > Now that controls::get() returns a std::optional handle invalid\n> controls,\n> > the error log message in ControlList::find() is unnecessary and likely\n> invalid\n> > in the case when calling controls::get() on a missing control. Remove\n> this\n> > error message.\n>\n> Isn't anyway up to the application to validate the returned\n> std::optional<> ? What if an application doesn't do so an tries to\n> access an std::nullopt ? At least the messages here would provide a\n> trace that something went wrong ?\n>\n\nI was seeing this error message continually raised in one of our\napplications.\nThe RPi pipeline handler checks if we have a ScalerCrop control set in the\nRequest. This is done by a ControlList::get with the new update [1]. If\nthis is not\nset, the error message pops up. I would want to defer the error handling\nin this\ncase to the pipeline handler as not having ScalerCrop is perfectly valid.\nIn the previous code, we called Controllist::contains(), which I can re-use\nbut\nthat sort of defeats the purpose of the std::optional change.\n\nI also may have misunderstood the new mechanism completely... :-)\n...but this change does fix my spew of error messages.\n\nRegards,\nNaush\n\n[1]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055\n\n\n>\n> >\n> > Fixes: 1c4d48018505 (\"libcamera: controls: Use std::optional to handle\n> invalid control values\")\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/controls.cpp | 12 ++----------\n> > 1 file changed, 2 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 03ac6345247c..701a872185e3 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const\n> ControlValue &value)\n> > const ControlValue *ControlList::find(unsigned int id) const\n> > {\n> > const auto iter = controls_.find(id);\n> > - if (iter == controls_.end()) {\n> > - LOG(Controls, Error)\n> > - << \"Control \" << utils::hex(id) << \" not found\";\n> > -\n> > + if (iter == controls_.end())\n> > return nullptr;\n> > - }\n> >\n> > return &iter->second;\n> > }\n> >\n> > ControlValue *ControlList::find(unsigned int id)\n> > {\n> > - if (validator_ && !validator_->validate(id)) {\n> > - LOG(Controls, Error)\n> > - << \"Control \" << utils::hex(id)\n> > - << \" is not valid for \" << validator_->name();\n> > + if (validator_ && !validator_->validate(id))\n> > return nullptr;\n> > - }\n> >\n> > return &controls_[id];\n> > }\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 18FC7BD1F1\n\tfor ;\n\tTue, 19 Jul 2022 16:13:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 958E8603F4;\n\tTue, 19 Jul 2022 18:13:33 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB21D603F4\n\tfor ;\n\tTue, 19 Jul 2022 18:13:31 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id t1so25589296lft.8\n\tfor ;\n\tTue, 19 Jul 2022 09:13:31 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658247213;\n\tbh=8yuCSdoZUN4fOXwTedwFH2rjlGD4e3ZE/LgKny8aoH8=;\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=jkQH7FV5nPhRT5nQtpRXeRB22vO8pDZWafvbtH5kvPyq38OEykPqUOd3O1Z7C/ke3\n\tBne1qHZ+bFUzNawbVkxvFksSvN87uTZsG4mnIEE6FD+Gx8hinO5dcmnnQFMw3bG3pN\n\tnrJzUfJ3maOJidNDjPQSbnu0eCOqFPfNBWE+19mo7ipo0xc3BqDK0nB1sMmnsqhvbQ\n\tG8ZyfGk54foiHXQ7BagypDYOCtpnCLEpABNY63KmxSP9HtRlXdN1+tbf+u0wkVsrlE\n\tVE4yh/Fi6hoXBLfNA+ss9XweBJzXSWAPgAevpwKJdiQdE1REUZh/hZuwdJkXN463IK\n\t478N5P7n6Pv1g==","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=IldwGcS8RdKYJ+Qo8JbuQiobTSCN7H88G01ncexUxpk=;\n\tb=ck+eCOmKzWz6h6VfekRQwCGxE4cFMWvG8vmjmoeuq+tx3k3ACjFDckZzzxlBsUOk+A\n\tJRq5XZUPcfEUuyRw2L9ryGGjve+0gL/otmPJBTebxRQC/ngwPcT3pVqtBv6/MN7Mvxe+\n\tAV1tppcxrIpqGjOtnvNRazp81PTs5Bg7OpChA3w+GJt4QoEfq2ZvB7/BO/HjzwKSAyoz\n\tc/mKhW+4H+KUe/fdG9EGnq3Nl4VajIEsMSJgJRqhrnpMMnmzK1UpWiUklnyYvupfbYbt\n\toOSMsUvCbaA/P2tirV2lnmymmaL5lN7Gz9DSE06g7BSu3DZPBZwDaXYQHWI2o11pC1ZM\n\tlKDQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ck+eCOmK\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=IldwGcS8RdKYJ+Qo8JbuQiobTSCN7H88G01ncexUxpk=;\n\tb=wJZ1ZPnCqCknl17vdYCcUmlUalIoFPOM4wXgqIgNsSmmuC2FB0haa7u76Em+/hBlfd\n\t2FtDpoOhlicZ96+M4xuAFTooJLfaImN1RG/N9qTSIQhB3621OsZ2LsyZqR7QB25HcUKB\n\tv4WSdFcauRPyxzaBiDdW6TTE2utKk849AfSZ2puoA4E8/7EMuAD5J7iNF0HADd5oW+KX\n\t16dmxECX+ynu7rjNQCEuc90w3FHsdaffHQB5Y9+bN7twm5aUHGLS6cks/FrVNsW5O2pP\n\tWViWCJfB4pcd235F0M/rAa8fAbmdvRrTX7Jd1nNSGr/EIMVw9KcxzAHYhwb7z6THo8iB\n\tyMhw==","X-Gm-Message-State":"AJIora8Llh+cJxXj/ilD+eC2Dq+Xyzljl49br5UpWme58B+BJ+3pc+xK\n\t7qh5nVzP95gjY2+TQz+vEdBcl39Uotp9eUAkOoecNO5XWknqLWjN","X-Google-Smtp-Source":"AGRyM1vn0GGqqCEoSGXzggS6OQsCLpF6IolhKVW0L6ukHUeciYnmAKNA/TUsAs2WdyzNCnDhZVJ86fl9tfd536v7als=","X-Received":"by 2002:a05:6512:3c9f:b0:48a:2c32:e22c with SMTP id\n\th31-20020a0565123c9f00b0048a2c32e22cmr10534190lfv.356.1658247210861;\n\tTue, 19 Jul 2022 09:13:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20220719144517.15898-1-naush@raspberrypi.com>\n\t<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>","In-Reply-To":"<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>","Date":"Tue, 19 Jul 2022 17:13:15 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000f41eb705e42ac352\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23972,"web_url":"https://patchwork.libcamera.org/comment/23972/","msgid":"<20220719164644.3dgslgdtzgqx4hp6@uno.localdomain>","date":"2022-07-19T16:46:44","subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi wrote:\n>\n> > Hi Naush\n> >\n> > On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > Now that controls::get() returns a std::optional handle invalid\n> > controls,\n> > > the error log message in ControlList::find() is unnecessary and likely\n> > invalid\n> > > in the case when calling controls::get() on a missing control. Remove\n> > this\n> > > error message.\n> >\n> > Isn't anyway up to the application to validate the returned\n> > std::optional<> ? What if an application doesn't do so an tries to\n> > access an std::nullopt ? At least the messages here would provide a\n> > trace that something went wrong ?\n> >\n>\n> I was seeing this error message continually raised in one of our\n> applications.\n> The RPi pipeline handler checks if we have a ScalerCrop control set in the\n> Request. This is done by a ControlList::get with the new update [1]. If\n> this is not\n> set, the error message pops up. I would want to defer the error handling\n> in this\n> case to the pipeline handler as not having ScalerCrop is perfectly valid.\n> In the previous code, we called Controllist::contains(), which I can re-use\n> but\n> that sort of defeats the purpose of the std::optional change.\n\nNo, you're right, as contains() has been dropped, get() is meant to\nbe called unconditionally and the control presence tested on the\nreturned optional pointer.\n\nThis is common for pipeline handlers, but I do expect applications to\ninspect metadata using the same pattern, and an error message is\nindeed misleading.\n\nLet's see what others think. I'm a bit skeptic about removing the\nmessage completely as it could be an indication of an un-checked error\ncondition, it could be demoted to Warn or Debug but again it seems\nto be a sub-optimal solution..\n\n>\n> I also may have misunderstood the new mechanism completely... :-)\n> ...but this change does fix my spew of error messages.\n>\n> Regards,\n> Naush\n>\n> [1]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055\n>\n>\n> >\n> > >\n> > > Fixes: 1c4d48018505 (\"libcamera: controls: Use std::optional to handle\n> > invalid control values\")\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/controls.cpp | 12 ++----------\n> > > 1 file changed, 2 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 03ac6345247c..701a872185e3 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const\n> > ControlValue &value)\n> > > const ControlValue *ControlList::find(unsigned int id) const\n> > > {\n> > > const auto iter = controls_.find(id);\n> > > - if (iter == controls_.end()) {\n> > > - LOG(Controls, Error)\n> > > - << \"Control \" << utils::hex(id) << \" not found\";\n> > > -\n> > > + if (iter == controls_.end())\n> > > return nullptr;\n> > > - }\n> > >\n> > > return &iter->second;\n> > > }\n> > >\n> > > ControlValue *ControlList::find(unsigned int id)\n> > > {\n> > > - if (validator_ && !validator_->validate(id)) {\n> > > - LOG(Controls, Error)\n> > > - << \"Control \" << utils::hex(id)\n> > > - << \" is not valid for \" << validator_->name();\n> > > + if (validator_ && !validator_->validate(id))\n> > > return nullptr;\n> > > - }\n> > >\n> > > return &controls_[id];\n> > > }\n> > > --\n> > > 2.25.1\n> > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1908DBD1F1\n\tfor ;\n\tTue, 19 Jul 2022 16:46:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56A376048A;\n\tTue, 19 Jul 2022 18:46:48 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0783A603F4\n\tfor ;\n\tTue, 19 Jul 2022 18:46:47 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7F1CC1C000A;\n\tTue, 19 Jul 2022 16:46:46 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658249208;\n\tbh=rb5heJa57bUSARKAVjAzZx5InM1jpXcnxfJAbb3WvIs=;\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=r8VE7Lh/OPueTOjriOZMWe+nJDuoKmgzSUBn7mAiWSPPfLEFyxC4pTrMgs6K+QaZ6\n\tSWiERKvmwJh8pYWmShvKIKSfyg4VURqq6vIB4Gpa7zgDVThNUfUUZebtksJitpqoy8\n\twb5a+NRQ6CzUH8KYMMNP7+ONtROvsEL+CcCrv+Otqu92u5+YDxk5Nx6JseMAfCcJw8\n\t7dvtrjo4idBa8DScp6wloCFbaYAS39MJi4yGZQ96QBpUzfCXYeKC6yrNfsgOsfxz9D\n\tdbVi2U6+rjAy8vQ2A0/byBuRz0szs/6AEJSe9ig3IDX7cEpt5CJ7I8zdmhGhtgOrZB\n\tNp9pPSlvdA/zw==","Date":"Tue, 19 Jul 2022 18:46:44 +0200","To":"Naushir Patuck ","Message-ID":"<20220719164644.3dgslgdtzgqx4hp6@uno.localdomain>","References":"<20220719144517.15898-1-naush@raspberrypi.com>\n\t<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23973,"web_url":"https://patchwork.libcamera.org/comment/23973/","msgid":"<3f74e8b3-ece6-ef72-3195-ce4148569361@ideasonboard.com>","date":"2022-07-19T17:06:52","subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 7/19/22 22:16, Jacopo Mondi via libcamera-devel wrote:\n> Hi Naush,\n>\n> On Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote:\n>> Hi Jacopo,\n>>\n>> On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi wrote:\n>>\n>>> Hi Naush\n>>>\n>>> On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via\n>>> libcamera-devel wrote:\n>>>> Now that controls::get() returns a std::optional handle invalid\n>>> controls,\n>>>> the error log message in ControlList::find() is unnecessary and likely\n>>> invalid\n>>>> in the case when calling controls::get() on a missing control. Remove\n>>> this\n>>>> error message.\n>>> Isn't anyway up to the application to validate the returned\n>>> std::optional<> ? What if an application doesn't do so an tries to\n>>> access an std::nullopt ? At least the messages here would provide a\n>>> trace that something went wrong ?\n>>>\n>> I was seeing this error message continually raised in one of our\n>> applications.\n>> The RPi pipeline handler checks if we have a ScalerCrop control set in the\n>> Request. This is done by a ControlList::get with the new update [1]. If\n>> this is not\n>> set, the error message pops up. I would want to defer the error handling\n>> in this\n>> case to the pipeline handler as not having ScalerCrop is perfectly valid.\n>> In the previous code, we called Controllist::contains(), which I can re-use\n>> but\n>> that sort of defeats the purpose of the std::optional change.\n> No, you're right, as contains() has been dropped, get() is meant to\n\n\ncontains() hasn't been dropped yet, it's still present.\n\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n934\n\nHowever, I have proposed dropping of it in the review although I missed \nto see ControlList::find() behaviour.\n\n> be called unconditionally and the control presence tested on the\n> returned optional pointer.\n>\n> This is common for pipeline handlers, but I do expect applications to\n> inspect metadata using the same pattern, and an error message is\n> indeed misleading.\n>\n> Let's see what others think. I'm a bit skeptic about removing the\n> message completely as it could be an indication of an un-checked error\n> condition, it could be demoted to Warn or Debug but again it seems\n> to be a sub-optimal solution..\n\n\nThe distinction between invalid control and a missing control is an \ninteresting one for ControlList::get()\n\n From the current documentation on master for get()\n\n  * The behaviour is undefined if the control \\a ctrl is not present in the\n  * list. Use ControlList::contains() to test for the presence of a \ncontrol in\n  * the list before retrieving its value.\n\nIs this still valid with the ::get() change that got introduced? If yes, \nthen I think this patch would make sense. But we can preserve the log \nand demote it to DEBUG/WARN as suggested.\n\n>\n>> I also may have misunderstood the new mechanism completely... :-)\n>> ...but this change does fix my spew of error messages.\n\n\nNow that I'm reading too, it makes me a bit fuzzy as well. With ::get(), \ncontains() and find() and the roles of each.\n\n/me puts on thinking cap!\n\n>>\n>> Regards,\n>> Naush\n>>\n>> [1]:\n>> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055\n>>\n>>\n>>>> Fixes: 1c4d48018505 (\"libcamera: controls: Use std::optional to handle\n>>> invalid control values\")\n>>>> Signed-off-by: Naushir Patuck \n>>>> ---\n>>>> src/libcamera/controls.cpp | 12 ++----------\n>>>> 1 file changed, 2 insertions(+), 10 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>> index 03ac6345247c..701a872185e3 100644\n>>>> --- a/src/libcamera/controls.cpp\n>>>> +++ b/src/libcamera/controls.cpp\n>>>> @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const\n>>> ControlValue &value)\n>>>> const ControlValue *ControlList::find(unsigned int id) const\n>>>> {\n>>>> const auto iter = controls_.find(id);\n>>>> - if (iter == controls_.end()) {\n>>>> - LOG(Controls, Error)\n>>>> - << \"Control \" << utils::hex(id) << \" not found\";\n>>>> -\n>>>> + if (iter == controls_.end())\n>>>> return nullptr;\n>>>> - }\n>>>>\n>>>> return &iter->second;\n>>>> }\n>>>>\n>>>> ControlValue *ControlList::find(unsigned int id)\n>>>> {\n>>>> - if (validator_ && !validator_->validate(id)) {\n>>>> - LOG(Controls, Error)\n>>>> - << \"Control \" << utils::hex(id)\n>>>> - << \" is not valid for \" << validator_->name();\n>>>> + if (validator_ && !validator_->validate(id))\n>>>> return nullptr;\n>>>> - }\n>>>>\n>>>> return &controls_[id];\n>>>> }\n>>>> --\n>>>> 2.25.1\n>>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A2CD7BD1F1\n\tfor ;\n\tTue, 19 Jul 2022 17:07:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CF4963312;\n\tTue, 19 Jul 2022 19:07:01 +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 22AFC603F4\n\tfor ;\n\tTue, 19 Jul 2022 19:07:00 +0200 (CEST)","from [IPV6:2401:4900:1f3f:73ce:9a9d:bd79:3fd0:39f1] (unknown\n\t[IPv6:2401:4900:1f3f:73ce:9a9d:bd79:3fd0:39f1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B54EA6EE;\n\tTue, 19 Jul 2022 19:06:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658250421;\n\tbh=enOIXliQYi6Ja41kHq41be7+H2xztNmUz79Fk+WCBlY=;\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=AZkTjWnlh3XWKtiKO+/WaG+NvIyrBNIL07G0w4j2g1yGayf2ifR89VoYnLLAbNgnA\n\tFWvEvT75dYv90z+7dKT8+OepBVUhifYedXfPXdYNSe3wDhH4ZTlVkGYB6CAsqyH8Wq\n\to/tgfbpndC3cuTREYVuOJ15jRJWc5G0ItThkoCzGqCebt6W5/cB3OX70v7vfcH8UCd\n\tQ89FiO6mRcSC+bBf8bHjoNcH0K+WgzpOtuL9CjxENPaNJoUkLUfAVEtwXdbbjzNHZR\n\t4CwtuIQ9R2EyiDdVEjSv9c3yqGRgzVp9EQ0gUwkeLy202vX2HXQS+D9bRd4vMkgyve\n\tucx2qjzMBA/Pw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658250419;\n\tbh=enOIXliQYi6Ja41kHq41be7+H2xztNmUz79Fk+WCBlY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rbDoVKeCXt5P7p1txFU/p+aEVLMd4JXCY7CAeaa1BilQkM09/lVelMp7GhgFvpzxv\n\tzLxn/Skw/rlQTF6ng5JAA8MDDn1r9yYE2FyPEvSZSOsD2UOqQ0E9Np2TmUgnYJlHGV\n\tfw6ZxnE8zvdjUmT35yIUPnpzYa8PPgIGhJrUyRP4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rbDoVKeC\"; dkim-atps=neutral","Message-ID":"<3f74e8b3-ece6-ef72-3195-ce4148569361@ideasonboard.com>","Date":"Tue, 19 Jul 2022 22:36:52 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi , Naushir Patuck ","References":"<20220719144517.15898-1-naush@raspberrypi.com>\n\t<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>\n\t\n\t<20220719164644.3dgslgdtzgqx4hp6@uno.localdomain>","In-Reply-To":"<20220719164644.3dgslgdtzgqx4hp6@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23974,"web_url":"https://patchwork.libcamera.org/comment/23974/","msgid":"","date":"2022-07-19T17:16:03","subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and Umang,\n\nOn Tue, 19 Jul 2022 at 18:07, Umang Jain \nwrote:\n\n> Hi,\n>\n> On 7/19/22 22:16, Jacopo Mondi via libcamera-devel wrote:\n> > Hi Naush,\n> >\n> > On Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote:\n> >> Hi Jacopo,\n> >>\n> >> On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi wrote:\n> >>\n> >>> Hi Naush\n> >>>\n> >>> On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via\n> >>> libcamera-devel wrote:\n> >>>> Now that controls::get() returns a std::optional handle invalid\n> >>> controls,\n> >>>> the error log message in ControlList::find() is unnecessary and likely\n> >>> invalid\n> >>>> in the case when calling controls::get() on a missing control. Remove\n> >>> this\n> >>>> error message.\n> >>> Isn't anyway up to the application to validate the returned\n> >>> std::optional<> ? What if an application doesn't do so an tries to\n> >>> access an std::nullopt ? At least the messages here would provide a\n> >>> trace that something went wrong ?\n> >>>\n> >> I was seeing this error message continually raised in one of our\n> >> applications.\n> >> The RPi pipeline handler checks if we have a ScalerCrop control set in\n> the\n> >> Request. This is done by a ControlList::get with the new update [1]. If\n> >> this is not\n> >> set, the error message pops up. I would want to defer the error\n> handling\n> >> in this\n> >> case to the pipeline handler as not having ScalerCrop is perfectly\n> valid.\n> >> In the previous code, we called Controllist::contains(), which I can\n> re-use\n> >> but\n> >> that sort of defeats the purpose of the std::optional change.\n> > No, you're right, as contains() has been dropped, get() is meant to\n>\n>\n> contains() hasn't been dropped yet, it's still present.\n>\n>\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n934\n>\n> However, I have proposed dropping of it in the review although I missed\n> to see ControlList::find() behaviour.\n>\n> > be called unconditionally and the control presence tested on the\n> > returned optional pointer.\n> >\n> > This is common for pipeline handlers, but I do expect applications to\n> > inspect metadata using the same pattern, and an error message is\n> > indeed misleading.\n> >\n> > Let's see what others think. I'm a bit skeptic about removing the\n> > message completely as it could be an indication of an un-checked error\n> > condition, it could be demoted to Warn or Debug but again it seems\n> > to be a sub-optimal solution..\n>\n>\n> The distinction between invalid control and a missing control is an\n> interesting one for ControlList::get()\n>\n> From the current documentation on master for get()\n>\n> * The behaviour is undefined if the control \\a ctrl is not present in the\n> * list. Use ControlList::contains() to test for the presence of a\n> control in\n> * the list before retrieving its value.\n>\n> Is this still valid with the ::get() change that got introduced? If yes,\n> then I think this patch would make sense. But we can preserve the log\n> and demote it to DEBUG/WARN as suggested.\n>\n\nIMHO ::get() should supersede ::contains() as using std::optional gives us\neverything we need to validate if a control is present or not.\nAdditionally,\nusing ::contains() implies a possible double lookup which Laurent carefully\nremoved from the codebase in his last patch :-)\n\nIf we do keep a log message here (again I think this we shouldn't as the\nabsence is a valid condition) then I would prefer it at DEBUG level.\n\nRegards,\nNaush\n\n\n\n> >\n> >> I also may have misunderstood the new mechanism completely... :-)\n> >> ...but this change does fix my spew of error messages.\n>\n>\n> Now that I'm reading too, it makes me a bit fuzzy as well. With ::get(),\n> contains() and find() and the roles of each.\n>\n> /me puts on thinking cap!\n>\n> >>\n> >> Regards,\n> >> Naush\n> >>\n> >> [1]:\n> >>\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055\n> >>\n> >>\n> >>>> Fixes: 1c4d48018505 (\"libcamera: controls: Use std::optional to handle\n> >>> invalid control values\")\n> >>>> Signed-off-by: Naushir Patuck \n> >>>> ---\n> >>>> src/libcamera/controls.cpp | 12 ++----------\n> >>>> 1 file changed, 2 insertions(+), 10 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>>> index 03ac6345247c..701a872185e3 100644\n> >>>> --- a/src/libcamera/controls.cpp\n> >>>> +++ b/src/libcamera/controls.cpp\n> >>>> @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const\n> >>> ControlValue &value)\n> >>>> const ControlValue *ControlList::find(unsigned int id) const\n> >>>> {\n> >>>> const auto iter = controls_.find(id);\n> >>>> - if (iter == controls_.end()) {\n> >>>> - LOG(Controls, Error)\n> >>>> - << \"Control \" << utils::hex(id) << \" not found\";\n> >>>> -\n> >>>> + if (iter == controls_.end())\n> >>>> return nullptr;\n> >>>> - }\n> >>>>\n> >>>> return &iter->second;\n> >>>> }\n> >>>>\n> >>>> ControlValue *ControlList::find(unsigned int id)\n> >>>> {\n> >>>> - if (validator_ && !validator_->validate(id)) {\n> >>>> - LOG(Controls, Error)\n> >>>> - << \"Control \" << utils::hex(id)\n> >>>> - << \" is not valid for \" << validator_->name();\n> >>>> + if (validator_ && !validator_->validate(id))\n> >>>> return nullptr;\n> >>>> - }\n> >>>>\n> >>>> return &controls_[id];\n> >>>> }\n> >>>> --\n> >>>> 2.25.1\n> >>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C1FD0BE173\n\tfor ;\n\tTue, 19 Jul 2022 17:16:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3400A63312;\n\tTue, 19 Jul 2022 19:16:22 +0200 (CEST)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24EA1603F4\n\tfor ;\n\tTue, 19 Jul 2022 19:16:20 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id q7so18126706lji.12\n\tfor ;\n\tTue, 19 Jul 2022 10:16:20 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658250982;\n\tbh=clf3jB5vCFaKxsAmMVNdN9Qry6tFKI8KsAjWsurIEmc=;\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=GBxu3qD74DGdeiSLwzgZn2A6AY0sjP/6iDOXIdJLNw0W4oXTFBkW3bM0dXny2E6Pr\n\ty09L3qe88OY1xYjQjuViYtIuQmpmdfSn8fTViZVgYh4wWnXNcPkK8J3+E6MtJDal1D\n\tMliqAJ+fCgjbzlijZIFWy+RDulFgtBDWdwaNts6ZS7hbAeOWFJpz0MQqW7bw1/2hjE\n\tkctsQbAFISUN7RhBzSELjwOrS9TxHDG2xPq1bjRg88+m0HCeZATfi5d3T9hqI2EzXO\n\tLKuctC97TuxyifAY+5Na/zG0HWhFhwqgnEoBwzu4rEPD70Vnar81dQBvT0WfXYenYE\n\tflt1fGZMTaY0A==","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=B6OV4rNCNRbtA7DJouMp90GtpRAwGUquHWH6Lui3Zx0=;\n\tb=iOCmajaRQdtLBeHeSJVEo/i5HM45hrBaLulPwOkRzaeFSEtZtJjWX3XkbKa28BbCne\n\tSB/w0XTH8aHyKc2i/E8/n2Q/tFB9C7sjItmTgJO27wmwq/MhnVrlmnLBScJlZDCHDyke\n\tBgpVJBXodABFh9lY+x/Mr+XSid5wUrbhTm3XXhG7CZLUsyNVcNqHFFW9TDWIAAQQGOEg\n\tffhFq+hMKpgQUGN2vZXlAIaDdUK7PaD2UwdvISkMW1+ORPvYN1aJojWAyk88L1A3NY0N\n\tnEa5eujGHT3Bek9Ig6LF0Ph4GLVVQnqVMYcqBTFPdI0tfLE7SBslg8lMLq7R3AHqfKJH\n\tIkMw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"iOCmajaR\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=B6OV4rNCNRbtA7DJouMp90GtpRAwGUquHWH6Lui3Zx0=;\n\tb=aXPBeCpxM/gISQJa5/24qldoKvgkzWZbp9nbvS1hmYNYciOd8gh4Zpgmfoefju/xme\n\tdT92jVgGZjrrJvMihOax3xZWxtP3h2TupxQ7/BG2twENG/ZknXbPs8G+LbU2CU0W9PdR\n\tMlJyvQ/SK7PMMYMes9lHxpFQRboJF55ZlJKPQ4EjyLfMT41tzg2Yla4tkrv35yc1kd1K\n\trze4eoFcPQ/GBsXVfX6GHS4PY7+evfXcjZ598o4BKkjbNA1X+qGTw9pOgqW4/w6GvV4+\n\to7hMniHCGfCutr8z+Yi/7o3csLG2TvjS8dx831WhiWm8Nu6a5O/o5rVnP/g0jOaf9nq4\n\tk8Dw==","X-Gm-Message-State":"AJIora+18RnZHury9+bvN3wdxGaz3+rgKoYTEi6hAEnnsRWqCNAyqbpa\n\tPuQqKYfWSYIZ6O7vYJ+xpgyKqdmWsMqiLpN/Ct5sctT9aByM0Q==","X-Google-Smtp-Source":"AGRyM1upUFDKPAkKDlklNpoVEDVDsg+Zv6QpggXrsGOzq8y6PZosNPPQUdT+0UP1lTIFCheGUfM6PSI0Xw92/BClF9M=","X-Received":"by 2002:a05:651c:a0c:b0:25d:7468:f9b6 with SMTP id\n\tk12-20020a05651c0a0c00b0025d7468f9b6mr14755231ljq.306.1658250979417;\n\tTue, 19 Jul 2022 10:16:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20220719144517.15898-1-naush@raspberrypi.com>\n\t<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>\n\t\n\t<20220719164644.3dgslgdtzgqx4hp6@uno.localdomain>\n\t<3f74e8b3-ece6-ef72-3195-ce4148569361@ideasonboard.com>","In-Reply-To":"<3f74e8b3-ece6-ef72-3195-ce4148569361@ideasonboard.com>","Date":"Tue, 19 Jul 2022 18:16:03 +0100","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"00000000000093b7b705e42ba4e2\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23975,"web_url":"https://patchwork.libcamera.org/comment/23975/","msgid":"","date":"2022-07-19T20:19:20","subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Jul 19, 2022 at 06:16:03PM +0100, Naushir Patuck via libcamera-devel wrote:\n> On Tue, 19 Jul 2022 at 18:07, Umang Jain wrote:\n> > On 7/19/22 22:16, Jacopo Mondi via libcamera-devel wrote:\n> > > On Tue, Jul 19, 2022 at 05:13:15PM +0100, Naushir Patuck wrote:\n> > >> On Tue, 19 Jul 2022 at 16:23, Jacopo Mondi wrote:\n> > >>> On Tue, Jul 19, 2022 at 03:45:17PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > >>>> Now that controls::get() returns a std::optional handle invalid controls,\n> > >>>> the error log message in ControlList::find() is unnecessary and likely invalid\n> > >>>> in the case when calling controls::get() on a missing control. Remove this\n> > >>>> error message.\n> > >>> \n> > >>> Isn't anyway up to the application to validate the returned\n> > >>> std::optional<> ? What if an application doesn't do so an tries to\n> > >>> access an std::nullopt ? At least the messages here would provide a\n> > >>> trace that something went wrong ?\n> > >>>\n> > >> I was seeing this error message continually raised in one of our applications.\n> > >> The RPi pipeline handler checks if we have a ScalerCrop control set in the\n> > >> Request. This is done by a ControlList::get with the new update [1]. If this is not\n> > >> set, the error message pops up. I would want to defer the error handling in this\n> > >> case to the pipeline handler as not having ScalerCrop is perfectly valid.\n> > >> In the previous code, we called Controllist::contains(), which I can re-use but\n> > >> that sort of defeats the purpose of the std::optional change.\n> > > \n> > > No, you're right, as contains() has been dropped, get() is meant to\n> >\n> > contains() hasn't been dropped yet, it's still present.\n> >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/controls.cpp#n934\n> >\n> > However, I have proposed dropping of it in the review although I missed\n> > to see ControlList::find() behaviour.\n> >\n> > > be called unconditionally and the control presence tested on the\n> > > returned optional pointer.\n> > >\n> > > This is common for pipeline handlers, but I do expect applications to\n> > > inspect metadata using the same pattern, and an error message is\n> > > indeed misleading.\n> > >\n> > > Let's see what others think. I'm a bit skeptic about removing the\n> > > message completely as it could be an indication of an un-checked error\n> > > condition, it could be demoted to Warn or Debug but again it seems\n> > > to be a sub-optimal solution..\n> >\n> > The distinction between invalid control and a missing control is an\n> > interesting one for ControlList::get()\n> >\n> > From the current documentation on master for get()\n> >\n> > * The behaviour is undefined if the control \\a ctrl is not present in the\n> > * list. Use ControlList::contains() to test for the presence of a control in\n> > * the list before retrieving its value.\n> >\n> > Is this still valid with the ::get() change that got introduced? If yes,\n> > then I think this patch would make sense. But we can preserve the log\n> > and demote it to DEBUG/WARN as suggested.\n\nThat's not valid, it looks like the documentation hasn't been updated\nwhen switching to std::optional. I'll send a fix.\n\n> IMHO ::get() should supersede ::contains() as using std::optional gives us\n> everything we need to validate if a control is present or not. Additionally,\n> using ::contains() implies a possible double lookup which Laurent carefully\n> removed from the codebase in his last patch :-)\n\nWe have two variants of get():\n\ntemplate\nstd::optional get(const Control &ctrl) const;\n\nand\n\nconst ControlValue &get(unsigned int id) const;\n\nThe latter is used when looking up controls by numerical ID, which is\nused internally but not meant to be used by applications. That still\nneeds a contains() check first. For the first variant, I agree\ncontains() shouldn't be used.\n\nI'd like to drop the second variant, but that's all we have today to\nhandle V4L2 controls. Jacopo is working on moving the IPA module\ninterfaces to non-V4L2 controls, so there's hope there, but we'll need\nmore time.\n\n> If we do keep a log message here (again I think this we shouldn't as the\n> absence is a valid condition) then I would prefer it at DEBUG level.\n\nI agree with the removal of the error message for the first variant of\nget(), but we should keep it for the second variant, as well as for\nset() which also calls find(). Note that we have const and non-const\nversions of find(), used in get() and set() respectively. The non-const\nversion should keep the error message as it's used for set() only.\n\nTo drop the message only from the std::optional version of get(), how\nabout the following ?\n\ndiff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex 192be78434dc..3e31b3633583 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -376,11 +376,11 @@ public:\n \ttemplate\n \tstd::optional get(const Control &ctrl) const\n \t{\n-\t\tconst ControlValue *val = find(ctrl.id());\n-\t\tif (!val)\n+\t\tconst auto entry = controls_.find(id);\n+\t\tif (entry == controls_.end())\n \t\t\treturn std::nullopt;\n\n-\t\treturn val->get();\n+\t\treturn entry->second.get();\n \t}\n\n \ttemplate\n\n> > >> I also may have misunderstood the new mechanism completely... :-)\n> > >> ...but this change does fix my spew of error messages.\n> >\n> > Now that I'm reading too, it makes me a bit fuzzy as well. With ::get(),\n> > contains() and find() and the roles of each.\n> >\n> > /me puts on thinking cap!\n> >\n> > >> [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2055\n> > >>\n> > >>>> Fixes: 1c4d48018505 (\"libcamera: controls: Use std::optional to handle invalid control values\")\n> > >>>> Signed-off-by: Naushir Patuck \n> > >>>> ---\n> > >>>> src/libcamera/controls.cpp | 12 ++----------\n> > >>>> 1 file changed, 2 insertions(+), 10 deletions(-)\n> > >>>>\n> > >>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > >>>> index 03ac6345247c..701a872185e3 100644\n> > >>>> --- a/src/libcamera/controls.cpp\n> > >>>> +++ b/src/libcamera/controls.cpp\n> > >>>> @@ -1051,24 +1051,16 @@ void ControlList::set(unsigned int id, const ControlValue &value)\n> > >>>> const ControlValue *ControlList::find(unsigned int id) const\n> > >>>> {\n> > >>>> const auto iter = controls_.find(id);\n> > >>>> - if (iter == controls_.end()) {\n> > >>>> - LOG(Controls, Error)\n> > >>>> - << \"Control \" << utils::hex(id) << \" not found\";\n> > >>>> -\n> > >>>> + if (iter == controls_.end())\n> > >>>> return nullptr;\n> > >>>> - }\n> > >>>>\n> > >>>> return &iter->second;\n> > >>>> }\n> > >>>>\n> > >>>> ControlValue *ControlList::find(unsigned int id)\n> > >>>> {\n> > >>>> - if (validator_ && !validator_->validate(id)) {\n> > >>>> - LOG(Controls, Error)\n> > >>>> - << \"Control \" << utils::hex(id)\n> > >>>> - << \" is not valid for \" << validator_->name();\n> > >>>> + if (validator_ && !validator_->validate(id))\n> > >>>> return nullptr;\n> > >>>> - }\n> > >>>>\n> > >>>> return &controls_[id];\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 C9493BE173\n\tfor ;\n\tTue, 19 Jul 2022 20:19:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DEA063312;\n\tTue, 19 Jul 2022 22:19:56 +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 87B7D603F4\n\tfor ;\n\tTue, 19 Jul 2022 22:19:54 +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 E0CED6EE;\n\tTue, 19 Jul 2022 22:19:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658261996;\n\tbh=XV18TiHIHgrUVvFfRvgXlcqN+9ezq6k6nsr4z2Q8YZs=;\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=HAGrJQlus9HVO+Yxl51EvdxOGPWabGc0zTnGYhwROuzpgmJTzaWbkdctvyz4cyuF0\n\tBRt5P2czktiLEPXc/aDkYYJcY4tb3HQ0c8iuuEXkHJrFgx+GeCSZO1vfminpuecLzn\n\tTIFD4AmAP56awVA7JPETLWjkOLOUlVsGS6DsRqYj8gRq4rsgxdX7MfIdoOzH5ZuA1m\n\twwJifPvKvIyDU5QFkp6CXZrT15aMKte6mRfxvcmxhWQ7IG+Q3KV9KufIbPwLckj5/M\n\teSrxl0ZRxfioCEM19Iq3J3YrRAHwpTIz259wC6TfX/18Z6JxPwuV/J3H6YgSLWs+t/\n\tg83O2UQpsRCZw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658261994;\n\tbh=XV18TiHIHgrUVvFfRvgXlcqN+9ezq6k6nsr4z2Q8YZs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C7xTEeWrLaQ/iCYaR3dRJdjudbZA2tsi0hu2DLfXnOIW0qE46pJwnck2kzXu0f5Fk\n\t1l+5Lp83ZU0NHhkm8dQu+Y8U5YOz0G7OOPx3G7lJzjnYg3LRz++vNwr8cntt/CbKoa\n\t7t1grj2LPd8T8LOC9uCyo7YVpAhZe9/6vhWfw6u4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"C7xTEeWr\"; dkim-atps=neutral","Date":"Tue, 19 Jul 2022 23:19:20 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220719144517.15898-1-naush@raspberrypi.com>\n\t<20220719152259.4ux7ufcvi4dggheg@uno.localdomain>\n\t\n\t<20220719164644.3dgslgdtzgqx4hp6@uno.localdomain>\n\t<3f74e8b3-ece6-ef72-3195-ce4148569361@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: controls: Suppress error\n\tmessage from ControlList::find()","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]