[{"id":18695,"web_url":"https://patchwork.libcamera.org/comment/18695/","msgid":"<20210811123505.ezpcig5frw7brkhw@uno.localdomain>","date":"2021-08-11T12:35:05","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote:\n> Create a Camera specific CameraControlValidator for the Camera instance.\n> This will allow requests to use a single validor instance without having\n> to construct their own.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n>\n> Laurent:\n> - Is the use of the _o() reference valid here in the\n> initialiser list?\n\nCould the requirement to pass a Camera * to the CameraControlValidator\nconstructor be dropped ? The Request class has access to the internal\nCamera::Private representation and the need for the Validator to have\na Camera * is only to access the Camera's control info map.\n\nIOW: Can the CameraControlValidator be made to accept a\nCamera::Private ? There shouldn't be a need to expose it to\napplications, right ?\n\n>\n>\n> include/libcamera/internal/camera.h | 6 ++++++\n> src/libcamera/camera.cpp | 2 +-\n> 2 files changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index b60ed140356a..f14bafc75e05 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -16,6 +16,8 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/camera_controls.h\"\n> +\n> namespace libcamera {\n>\n> class PipelineHandler;\n> @@ -30,6 +32,8 @@ public:\n> \t\tconst std::set &streams);\n> \t~Private();\n>\n> +\tconst CameraControlValidator &validator() const { return validator_; }\n> +\n> private:\n> \tenum State {\n> \t\tCameraAvailable,\n> @@ -56,6 +60,8 @@ private:\n>\n> \tbool disconnected_;\n> \tstd::atomic state_;\n> +\n> +\tCameraControlValidator validator_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 6281e92057e4..b914bf188d57 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe,\n> \t\t\t const std::string &id,\n> \t\t\t const std::set &streams)\n> \t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> -\t disconnected_(false), state_(CameraAvailable)\n> +\t disconnected_(false), state_(CameraAvailable), validator_(_o())\n> {\n> }\n>\n> --\n> 2.30.2\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 C4EFCC3240\n\tfor ;\n\tWed, 11 Aug 2021 12:34:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 427066884D;\n\tWed, 11 Aug 2021 14:34:20 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C540168826\n\tfor ;\n\tWed, 11 Aug 2021 14:34:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 300CA240002;\n\tWed, 11 Aug 2021 12:34:17 +0000 (UTC)"],"Date":"Wed, 11 Aug 2021 14:35:05 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210811123505.ezpcig5frw7brkhw@uno.localdomain>","References":"<20210810161134.2243796-1-kieran.bingham@ideasonboard.com>\n\t<20210810161134.2243796-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210810161134.2243796-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18705,"web_url":"https://patchwork.libcamera.org/comment/18705/","msgid":"<2406b24d-c663-4a03-5900-d3aa90b4579c@ideasonboard.com>","date":"2021-08-11T15:03:50","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 11/08/2021 13:35, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote:\n>> Create a Camera specific CameraControlValidator for the Camera instance.\n>> This will allow requests to use a single validor instance without having\n>> to construct their own.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>>\n>> Laurent:\n>> - Is the use of the _o() reference valid here in the\n>> initialiser list?\n> \n> Could the requirement to pass a Camera * to the CameraControlValidator\n> constructor be dropped ? The Request class has access to the internal\n> Camera::Private representation and the need for the Validator to have\n> a Camera * is only to access the Camera's control info map.\n\n\nIt looks like Camera is used for both:\n camera_->name()\nand\n camera_->controls()\n\nindeed.\n\nthe name is a little trivial, but still there.\nI'm not sure what the fear is about passing the Camera *\n\n\nIndeed, if we can pass the Camera::Private we can dig the\npipe_->controls() ... but .. ahh.\n\nTo get the ControlInfoMap - it's indexed from the Pipe based on the\nCamera * ... which is because it's in the CameraData ...\n\nAha - Now I see where you're going with that - Laurent's series moves\nthat CameraData into Camera::Private. So - at that point, yes we could.\n\nBut I haven't got that series applied here yet ;-)\n\n\n> IOW: Can the CameraControlValidator be made to accept a\n> Camera::Private ? There shouldn't be a need to expose it to\n> applications, right ?\n\nPerhaps I need to rebase on top of Laurent's patches, or give this\nseries to him ;-)\n\n--\nKieran\n\n\n\n> \n>>\n>>\n>> include/libcamera/internal/camera.h | 6 ++++++\n>> src/libcamera/camera.cpp | 2 +-\n>> 2 files changed, 7 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n>> index b60ed140356a..f14bafc75e05 100644\n>> --- a/include/libcamera/internal/camera.h\n>> +++ b/include/libcamera/internal/camera.h\n>> @@ -16,6 +16,8 @@\n>>\n>> #include \n>>\n>> +#include \"libcamera/internal/camera_controls.h\"\n>> +\n>> namespace libcamera {\n>>\n>> class PipelineHandler;\n>> @@ -30,6 +32,8 @@ public:\n>> \t\tconst std::set &streams);\n>> \t~Private();\n>>\n>> +\tconst CameraControlValidator &validator() const { return validator_; }\n>> +\n>> private:\n>> \tenum State {\n>> \t\tCameraAvailable,\n>> @@ -56,6 +60,8 @@ private:\n>>\n>> \tbool disconnected_;\n>> \tstd::atomic state_;\n>> +\n>> +\tCameraControlValidator validator_;\n>> };\n>>\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>> index 6281e92057e4..b914bf188d57 100644\n>> --- a/src/libcamera/camera.cpp\n>> +++ b/src/libcamera/camera.cpp\n>> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe,\n>> \t\t\t const std::string &id,\n>> \t\t\t const std::set &streams)\n>> \t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n>> -\t disconnected_(false), state_(CameraAvailable)\n>> +\t disconnected_(false), state_(CameraAvailable), validator_(_o())\n>> {\n>> }\n>>\n>> --\n>> 2.30.2\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 244F5BD87D\n\tfor ;\n\tWed, 11 Aug 2021 15:03:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 904C86884F;\n\tWed, 11 Aug 2021 17:03:54 +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 D073668826\n\tfor ;\n\tWed, 11 Aug 2021 17:03:53 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DB01EE;\n\tWed, 11 Aug 2021 17:03:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bmHqAlQx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628694233;\n\tbh=XssbSjVwfD1FtpNi5abGr/d9fDryFet36tvYlm0ceBQ=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=bmHqAlQxBn8Smpix1nrx1zTqMzmfztI84gQRo5ka4csRp3pLyA97bVO3A3iwn96Tk\n\tfEF4GCWLNuvXViDMpMr6SL5QJzP3V6QwFFgc47SvbROIDX+jWJ+9n5UvNEY5hqmpFi\n\tmVfcxTWWXKil7RfU2qaNR2iDqRejKez1cXO12PGw=","From":"Kieran Bingham ","To":"Jacopo Mondi ","References":"<20210810161134.2243796-1-kieran.bingham@ideasonboard.com>\n\t<20210810161134.2243796-3-kieran.bingham@ideasonboard.com>\n\t<20210811123505.ezpcig5frw7brkhw@uno.localdomain>","Message-ID":"<2406b24d-c663-4a03-5900-d3aa90b4579c@ideasonboard.com>","Date":"Wed, 11 Aug 2021 16:03:50 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210811123505.ezpcig5frw7brkhw@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18706,"web_url":"https://patchwork.libcamera.org/comment/18706/","msgid":"<20210811151224.pc64ayh7dusvjmkk@uno.localdomain>","date":"2021-08-11T15:12:24","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 11/08/2021 13:35, Jacopo Mondi wrote:\n> > Hi Kieran,\n> >\n> > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote:\n> >> Create a Camera specific CameraControlValidator for the Camera instance.\n> >> This will allow requests to use a single validor instance without having\n> >> to construct their own.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >>\n> >> Laurent:\n> >> - Is the use of the _o() reference valid here in the\n> >> initialiser list?\n> >\n> > Could the requirement to pass a Camera * to the CameraControlValidator\n> > constructor be dropped ? The Request class has access to the internal\n> > Camera::Private representation and the need for the Validator to have\n> > a Camera * is only to access the Camera's control info map.\n>\n>\n> It looks like Camera is used for both:\n> camera_->name()\n> and\n> camera_->controls()\n>\n> indeed.\n>\n> the name is a little trivial, but still there.\n> I'm not sure what the fear is about passing the Camera *\n>\n>\n> Indeed, if we can pass the Camera::Private we can dig the\n> pipe_->controls() ... but .. ahh.\n>\n> To get the ControlInfoMap - it's indexed from the Pipe based on the\n> Camera * ... which is because it's in the CameraData ...\n>\n> Aha - Now I see where you're going with that - Laurent's series moves\n> that CameraData into Camera::Private. So - at that point, yes we could.\n>\n> But I haven't got that series applied here yet ;-)\n\nYes indeed, I was already projected to when the control info and\nproperties will be stored in Camera::Private...\n\n>\n>\n> > IOW: Can the CameraControlValidator be made to accept a\n> > Camera::Private ? There shouldn't be a need to expose it to\n> > applications, right ?\n>\n> Perhaps I need to rebase on top of Laurent's patches, or give this\n> series to him ;-)\n>\n> --\n> Kieran\n>\n>\n>\n> >\n> >>\n> >>\n> >> include/libcamera/internal/camera.h | 6 ++++++\n> >> src/libcamera/camera.cpp | 2 +-\n> >> 2 files changed, 7 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> >> index b60ed140356a..f14bafc75e05 100644\n> >> --- a/include/libcamera/internal/camera.h\n> >> +++ b/include/libcamera/internal/camera.h\n> >> @@ -16,6 +16,8 @@\n> >>\n> >> #include \n> >>\n> >> +#include \"libcamera/internal/camera_controls.h\"\n> >> +\n> >> namespace libcamera {\n> >>\n> >> class PipelineHandler;\n> >> @@ -30,6 +32,8 @@ public:\n> >> \t\tconst std::set &streams);\n> >> \t~Private();\n> >>\n> >> +\tconst CameraControlValidator &validator() const { return validator_; }\n> >> +\n> >> private:\n> >> \tenum State {\n> >> \t\tCameraAvailable,\n> >> @@ -56,6 +60,8 @@ private:\n> >>\n> >> \tbool disconnected_;\n> >> \tstd::atomic state_;\n> >> +\n> >> +\tCameraControlValidator validator_;\n> >> };\n> >>\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >> index 6281e92057e4..b914bf188d57 100644\n> >> --- a/src/libcamera/camera.cpp\n> >> +++ b/src/libcamera/camera.cpp\n> >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe,\n> >> \t\t\t const std::string &id,\n> >> \t\t\t const std::set &streams)\n> >> \t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> >> -\t disconnected_(false), state_(CameraAvailable)\n> >> +\t disconnected_(false), state_(CameraAvailable), validator_(_o())\n> >> {\n> >> }\n> >>\n> >> --\n> >> 2.30.2\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 4328EC3240\n\tfor ;\n\tWed, 11 Aug 2021 15:11:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADDFC6884F;\n\tWed, 11 Aug 2021 17:11:38 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFE6468826\n\tfor ;\n\tWed, 11 Aug 2021 17:11:36 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 4B368240002;\n\tWed, 11 Aug 2021 15:11:36 +0000 (UTC)"],"Date":"Wed, 11 Aug 2021 17:12:24 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20210811151224.pc64ayh7dusvjmkk@uno.localdomain>","References":"<20210810161134.2243796-1-kieran.bingham@ideasonboard.com>\n\t<20210810161134.2243796-3-kieran.bingham@ideasonboard.com>\n\t<20210811123505.ezpcig5frw7brkhw@uno.localdomain>\n\t<2406b24d-c663-4a03-5900-d3aa90b4579c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2406b24d-c663-4a03-5900-d3aa90b4579c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18715,"web_url":"https://patchwork.libcamera.org/comment/18715/","msgid":"","date":"2021-08-11T22:50:09","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Aug 11, 2021 at 05:12:24PM +0200, Jacopo Mondi wrote:\n> On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote:\n> > On 11/08/2021 13:35, Jacopo Mondi wrote:\n> > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote:\n> > >> Create a Camera specific CameraControlValidator for the Camera instance.\n> > >> This will allow requests to use a single validor instance without having\n> > >> to construct their own.\n> > >>\n> > >> Signed-off-by: Kieran Bingham \n> > >> ---\n> > >>\n> > >> Laurent:\n> > >> - Is the use of the _o() reference valid here in the\n> > >> initialiser list?\n\nThe _o() function is defined as\n\n\ttemplate\n\tT *_o()\n\t{\n\t\treturn static_cast(o_);\n\t}\n\nSince the recent change to decouple construction of Extensible and\nExtensible::Private, the Extensible::Private::o_ member variable is\ninitialized by the Extensible constructor. With the pending patch series\nthat makes the pipeline handlers subclass Camera::Private, construction\nof Camera::Private will happen before Camera is constructed. _o() is\nthus not valid here I'm afraid.\n\n> > > Could the requirement to pass a Camera * to the CameraControlValidator\n> > > constructor be dropped ? The Request class has access to the internal\n> > > Camera::Private representation and the need for the Validator to have\n> > > a Camera * is only to access the Camera's control info map.\n> >\n> > It looks like Camera is used for both:\n> > camera_->name()\n> > and\n> > camera_->controls()\n> >\n> > indeed.\n> >\n> > the name is a little trivial, but still there.\n> > I'm not sure what the fear is about passing the Camera *\n> >\n> >\n> > Indeed, if we can pass the Camera::Private we can dig the\n> > pipe_->controls() ... but .. ahh.\n> >\n> > To get the ControlInfoMap - it's indexed from the Pipe based on the\n> > Camera * ... which is because it's in the CameraData ...\n> >\n> > Aha - Now I see where you're going with that - Laurent's series moves\n> > that CameraData into Camera::Private. So - at that point, yes we could.\n> >\n> > But I haven't got that series applied here yet ;-)\n> \n> Yes indeed, I was already projected to when the control info and\n> properties will be stored in Camera::Private...\n> \n> > > IOW: Can the CameraControlValidator be made to accept a\n> > > Camera::Private ? There shouldn't be a need to expose it to\n> > > applications, right ?\n> >\n> > Perhaps I need to rebase on top of Laurent's patches, or give this\n> > series to him ;-)\n\nPlease feel free to rebase :-) Please also feel free to change the way\ncontrol validators are designed. The current implementation is more of a\nskeleton than anything else, and its design dates back to a time when\nlibcamera was very different. Don't feel constrained by how control\nvalidators work today.\n\n> > >> include/libcamera/internal/camera.h | 6 ++++++\n> > >> src/libcamera/camera.cpp | 2 +-\n> > >> 2 files changed, 7 insertions(+), 1 deletion(-)\n> > >>\n> > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > >> index b60ed140356a..f14bafc75e05 100644\n> > >> --- a/include/libcamera/internal/camera.h\n> > >> +++ b/include/libcamera/internal/camera.h\n> > >> @@ -16,6 +16,8 @@\n> > >>\n> > >> #include \n> > >>\n> > >> +#include \"libcamera/internal/camera_controls.h\"\n> > >> +\n> > >> namespace libcamera {\n> > >>\n> > >> class PipelineHandler;\n> > >> @@ -30,6 +32,8 @@ public:\n> > >> \t\tconst std::set &streams);\n> > >> \t~Private();\n> > >>\n> > >> +\tconst CameraControlValidator &validator() const { return validator_; }\n> > >> +\n> > >> private:\n> > >> \tenum State {\n> > >> \t\tCameraAvailable,\n> > >> @@ -56,6 +60,8 @@ private:\n> > >>\n> > >> \tbool disconnected_;\n> > >> \tstd::atomic state_;\n> > >> +\n> > >> +\tCameraControlValidator validator_;\n> > >> };\n> > >>\n> > >> } /* namespace libcamera */\n> > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > >> index 6281e92057e4..b914bf188d57 100644\n> > >> --- a/src/libcamera/camera.cpp\n> > >> +++ b/src/libcamera/camera.cpp\n> > >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe,\n> > >> \t\t\t const std::string &id,\n> > >> \t\t\t const std::set &streams)\n> > >> \t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> > >> -\t disconnected_(false), state_(CameraAvailable)\n> > >> +\t disconnected_(false), state_(CameraAvailable), validator_(_o())\n> > >> {\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 C6AD5BD87D\n\tfor ;\n\tWed, 11 Aug 2021 22:50:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3918F6884F;\n\tThu, 12 Aug 2021 00:50:16 +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 6935F687F0\n\tfor ;\n\tThu, 12 Aug 2021 00:50:13 +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 C7E01EE;\n\tThu, 12 Aug 2021 00:50:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DRGexcAA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628722213;\n\tbh=/o313ilEO2cfNb+mh0pYs53IIWIKbvWaE+sykvUPyuY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DRGexcAAZWbdU1rT91mCDKOR2vWIv2LydgVO/qlpVG1wEXkYRteWkNXqRx9pFZm4u\n\tFC7OVg2HHfaPZq1KisF1tkajlJYp1tfripub6Vms+prFunCXt8Y1P10XeoBR5OFlNo\n\tGAeP+g//85OXM0e9DnQPbpq43rQLJnnGaAZIVmOo=","Date":"Thu, 12 Aug 2021 01:50:09 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210810161134.2243796-1-kieran.bingham@ideasonboard.com>\n\t<20210810161134.2243796-3-kieran.bingham@ideasonboard.com>\n\t<20210811123505.ezpcig5frw7brkhw@uno.localdomain>\n\t<2406b24d-c663-4a03-5900-d3aa90b4579c@ideasonboard.com>\n\t<20210811151224.pc64ayh7dusvjmkk@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210811151224.pc64ayh7dusvjmkk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18717,"web_url":"https://patchwork.libcamera.org/comment/18717/","msgid":"","date":"2021-08-11T22:59:49","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: camera: Create a\n\tCameraControlValidator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 12, 2021 at 01:50:10AM +0300, Laurent Pinchart wrote:\n> On Wed, Aug 11, 2021 at 05:12:24PM +0200, Jacopo Mondi wrote:\n> > On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote:\n> > > On 11/08/2021 13:35, Jacopo Mondi wrote:\n> > > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote:\n> > > >> Create a Camera specific CameraControlValidator for the Camera instance.\n> > > >> This will allow requests to use a single validor instance without having\n> > > >> to construct their own.\n> > > >>\n> > > >> Signed-off-by: Kieran Bingham \n> > > >> ---\n> > > >>\n> > > >> Laurent:\n> > > >> - Is the use of the _o() reference valid here in the\n> > > >> initialiser list?\n> \n> The _o() function is defined as\n> \n> \ttemplate\n> \tT *_o()\n> \t{\n> \t\treturn static_cast(o_);\n> \t}\n> \n> Since the recent change to decouple construction of Extensible and\n> Extensible::Private, the Extensible::Private::o_ member variable is\n> initialized by the Extensible constructor. With the pending patch series\n> that makes the pipeline handlers subclass Camera::Private, construction\n> of Camera::Private will happen before Camera is constructed. _o() is\n> thus not valid here I'm afraid.\n\nNote that you can initialize members of the private class in the public\nclass constructor. For instance, the Camera constructor is implemented\nas\n\nCamera::Camera(std::unique_ptr d, const std::string &id,\n\t const std::set &streams)\n\t: Extensible(std::move(d))\n{\n\t_d()->id_ = id;\n\t_d()->streams_ = streams;\n}\n\nYou could store a\n\n\tstd::unique_ptr validator_;\n\nin Camera::Private, and construct and assign it in Camera::Camera() with\n\n\t_d()->validator_ = std::make_unique(this);\n\n(it may make sense to store the return value of _d() in a local\nvariable, and no, you can't use the d parameter, as it's moved before\nthe body of the Camera constructor is executed).\n\n> > > > Could the requirement to pass a Camera * to the CameraControlValidator\n> > > > constructor be dropped ? The Request class has access to the internal\n> > > > Camera::Private representation and the need for the Validator to have\n> > > > a Camera * is only to access the Camera's control info map.\n> > >\n> > > It looks like Camera is used for both:\n> > > camera_->name()\n> > > and\n> > > camera_->controls()\n> > >\n> > > indeed.\n> > >\n> > > the name is a little trivial, but still there.\n> > > I'm not sure what the fear is about passing the Camera *\n> > >\n> > >\n> > > Indeed, if we can pass the Camera::Private we can dig the\n> > > pipe_->controls() ... but .. ahh.\n> > >\n> > > To get the ControlInfoMap - it's indexed from the Pipe based on the\n> > > Camera * ... which is because it's in the CameraData ...\n> > >\n> > > Aha - Now I see where you're going with that - Laurent's series moves\n> > > that CameraData into Camera::Private. So - at that point, yes we could.\n> > >\n> > > But I haven't got that series applied here yet ;-)\n> > \n> > Yes indeed, I was already projected to when the control info and\n> > properties will be stored in Camera::Private...\n> > \n> > > > IOW: Can the CameraControlValidator be made to accept a\n> > > > Camera::Private ? There shouldn't be a need to expose it to\n> > > > applications, right ?\n> > >\n> > > Perhaps I need to rebase on top of Laurent's patches, or give this\n> > > series to him ;-)\n> \n> Please feel free to rebase :-) Please also feel free to change the way\n> control validators are designed. The current implementation is more of a\n> skeleton than anything else, and its design dates back to a time when\n> libcamera was very different. Don't feel constrained by how control\n> validators work today.\n> \n> > > >> include/libcamera/internal/camera.h | 6 ++++++\n> > > >> src/libcamera/camera.cpp | 2 +-\n> > > >> 2 files changed, 7 insertions(+), 1 deletion(-)\n> > > >>\n> > > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > >> index b60ed140356a..f14bafc75e05 100644\n> > > >> --- a/include/libcamera/internal/camera.h\n> > > >> +++ b/include/libcamera/internal/camera.h\n> > > >> @@ -16,6 +16,8 @@\n> > > >>\n> > > >> #include \n> > > >>\n> > > >> +#include \"libcamera/internal/camera_controls.h\"\n> > > >> +\n> > > >> namespace libcamera {\n> > > >>\n> > > >> class PipelineHandler;\n> > > >> @@ -30,6 +32,8 @@ public:\n> > > >> \t\tconst std::set &streams);\n> > > >> \t~Private();\n> > > >>\n> > > >> +\tconst CameraControlValidator &validator() const { return validator_; }\n> > > >> +\n> > > >> private:\n> > > >> \tenum State {\n> > > >> \t\tCameraAvailable,\n> > > >> @@ -56,6 +60,8 @@ private:\n> > > >>\n> > > >> \tbool disconnected_;\n> > > >> \tstd::atomic state_;\n> > > >> +\n> > > >> +\tCameraControlValidator validator_;\n> > > >> };\n> > > >>\n> > > >> } /* namespace libcamera */\n> > > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > >> index 6281e92057e4..b914bf188d57 100644\n> > > >> --- a/src/libcamera/camera.cpp\n> > > >> +++ b/src/libcamera/camera.cpp\n> > > >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe,\n> > > >> \t\t\t const std::string &id,\n> > > >> \t\t\t const std::set &streams)\n> > > >> \t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> > > >> -\t disconnected_(false), state_(CameraAvailable)\n> > > >> +\t disconnected_(false), state_(CameraAvailable), validator_(_o())\n> > > >> {\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 76876BD87D\n\tfor ;\n\tWed, 11 Aug 2021 22:59:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E98216884D;\n\tThu, 12 Aug 2021 00:59:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 782C0687F0\n\tfor ;\n\tThu, 12 Aug 2021 00:59:53 +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 DE2B6EE;\n\tThu, 12 Aug 2021 00:59:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sDvTrnxK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628722793;\n\tbh=1R+aaIA3HJVvJfyKdpVoByHK4qMhEn6btUujTZu8jUU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sDvTrnxKLabLHx+t0U5jzYmo6dWF1wyU119v7JSuebmJo0I58/0gOgaTKizA8ts1S\n\tZ8Qirqw7IF2U8oxFPa4qC1Efm8Scp6YozAheolxF6KXcxjlYHRm/kOUnTXOz4Dmz7p\n\tPOjzVfOGZB9YdQn+X3OL2KczIVa977YA3ZFXND6s=","Date":"Thu, 12 Aug 2021 01:59:49 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210810161134.2243796-1-kieran.bingham@ideasonboard.com>\n\t<20210810161134.2243796-3-kieran.bingham@ideasonboard.com>\n\t<20210811123505.ezpcig5frw7brkhw@uno.localdomain>\n\t<2406b24d-c663-4a03-5900-d3aa90b4579c@ideasonboard.com>\n\t<20210811151224.pc64ayh7dusvjmkk@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 2/3] libcamera: camera: Create a\n\tCameraControlValidator","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]