[{"id":1788,"web_url":"https://patchwork.libcamera.org/comment/1788/","msgid":"<20190607163719.GA14958@pendragon.ideasonboard.com>","date":"2019-06-07T16:37:19","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n> Provide a set to contain all controls applicable to the request.\n> The set contains all controls whether they are write, read, or write-read controls.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/request.h | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 58de6f00a554..5fae0d5fc838 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -36,6 +37,8 @@ public:\n> \tint setBuffers(const std::map &streamMap);\n> \tBuffer *findBuffer(Stream *stream) const;\n> \n> +\tstd::set &controls() { return controls_; };\n\nI think we will need something a bit more complicated than a std::set.\nHere's what I was thinking about.\n\n- Let's split control information from control value. The former are\n static for the lifetime of the camera (or at least of the capture\n session), while the latter vary per-request.\n\n- The control values in the request could be stored in a\n std::map. This would make copies as\n cheap as possible as we won't have to duplicate the control\n information.\n\n- The return value of the controls() function should be a custom class\n wrapping around the std::map. This is needed to perform control\n validation for instance (min/max/step), and will likely come handy to\n implement other custom behaviours.\n\n- The control information should be exposed by the camera so that\n application can enumerate supported controls and query their\n information.\n\n- We need to think of the common use cases from an application point of\n view. In particular I think applications will need an API to get a\n default set of control values from libcamera, that they will then\n possibly modify and set for the first request. Subsequent requests\n will likely just modify a small number of controls, and will likely be\n constructed from scratch. For control that applications want to read,\n I expect most requests to contain the same controls, so there should\n be an easy and efficient way to handle that. Splitting read and write\n controls may be a good idea.\n\n> +\n> \tStatus status() const { return status_; }\n> \n> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> @@ -52,6 +55,7 @@ private:\n> \tCamera *camera_;\n> \tstd::map bufferMap_;\n> \tstd::unordered_set pending_;\n> +\tstd::set controls_;\n> \n> \tStatus status_;\n> };","headers":{"Return-Path":"","Received":["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 AE8726A2C0\n\tfor ;\n\tFri, 7 Jun 2019 18:37:34 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:44f0:8500:ca05:8177:199c:fed4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3B609334;\n\tFri, 7 Jun 2019 18:37:34 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559925454;\n\tbh=dAom26TMNylCH5n1+/xnxXgVtZYlXjcM8QQlVSNN71Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=InTT5IgTw/iN/d/zTuIgq2RZYosy8xTEl9NtlYa4JxzaysiFibeX825NpX049QkaF\n\ty4DNXNdHb4Oad+6yxOaFsXWVEPEou2sQa+3vu2aLAVwzt7BMBAvfgi/qz/+RBPO1mf\n\t+HPmZYbY8nS+bOzam0eXqhE9CQamkwrZlgXpH9Cc=","Date":"Fri, 7 Jun 2019 19:37:19 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190607163719.GA14958@pendragon.ideasonboard.com>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190606205654.9311-3-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 07 Jun 2019 16:37:34 -0000"}},{"id":1824,"web_url":"https://patchwork.libcamera.org/comment/1824/","msgid":"<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>","date":"2019-06-10T10:45:22","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n just some more thoughts on your patch and Laurent comments..\n\nOn Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote:\n\n\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n> > Provide a set to contain all controls applicable to the request.\n> > The set contains all controls whether they are write, read, or write-read controls.\n\nNot on this patch strictly, but, the 'nature' of the control (w, r,\nrw) is define by how the Control is constructed, right? If it is\nprovided a value is a write, otherwise is a read.\n\nI think controls should have a 'direction' type assigned as part of\ntheir definition which should be checked against what the user tries\nto do on it, and not only depend on how the user creates them.\n\nIn example, the static controls that Laurent mentioned here below,\nwhich are stored in the Camera, should be 'static'/'read-only'\ncontrols, which provides to application non-modifiable\ninformations on the camera (orientation, position etc). Will the\n'direction' be part of the controls definition?\n\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/request.h | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 58de6f00a554..5fae0d5fc838 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -36,6 +37,8 @@ public:\n> > \tint setBuffers(const std::map &streamMap);\n> > \tBuffer *findBuffer(Stream *stream) const;\n> >\n> > +\tstd::set &controls() { return controls_; };\n>\n> I think we will need something a bit more complicated than a std::set.\n> Here's what I was thinking about.\n>\n> - Let's split control information from control value. The former are\n> static for the lifetime of the camera (or at least of the capture\n> session), while the latter vary per-request.\n>\n> - The control values in the request could be stored in a\n> std::map. This would make copies as\n> cheap as possible as we won't have to duplicate the control\n> information.\n\nWhere do you envision them to live? I assume as they won't have to be\nduplicated we'll maintain a table somewhere of ControlInfo each\nrequest's control will reference to...\n\n>\n> - The return value of the controls() function should be a custom class\n> wrapping around the std::map. This is needed to perform control\n> validation for instance (min/max/step), and will likely come handy to\n> implement other custom behaviours.\n\nEach control should be validated against it's associated ControlInfo.\nWhy is wrapping the container relevant if not from the API usage\neasiness, maybe?\n\n>\n> - The control information should be exposed by the camera so that\n> application can enumerate supported controls and query their\n> information.\n>\n\nDo we expect the camera to have anything like \"readControl()\" ?\nMy understanding was that all interactions with control goes through\nrequests, so there's not actually anything like an asynchronous\nread().\n\nI wonder if\n1) getting from the camera a list of controls with associated default\nvalues\n2) modify the one the app is interested into ad submit a request\n3) at request complete time verify the control value has changed to\nthe requested value (or is converging to it) and modify it again if\nrequired.\n\nIs not a better interaction model compared to the asynchronous\n\"getControl/setControl\" one. We briefly discussed that, and one point\nwas that the delays in completing the request might delay the read of\na control. I wonder if that's relevant or if we can do anything about it\nie. the control value comes from a statistic value generated from\ninspecting the pixel data: this should always be associated with a\ncompleted request. This holds for other \"async\" controls as well: do we care\nwhat is the lens exposure \"now\" or what is value was at the time the picture\nhas been taken?.\n\nIf we have to fast-track some controls with an immediate\nCamera::readControl() I agree this might be needed for some case but\nnot pretty, as it offers an API easy to abuse.\n\n> - We need to think of the common use cases from an application point of\n> view. In particular I think applications will need an API to get a\n> default set of control values from libcamera, that they will then\n> possibly modify and set for the first request. Subsequent requests\n> will likely just modify a small number of controls, and will likely be\n> constructed from scratch. For control that applications want to read,\n> I expect most requests to contain the same controls, so there should\n> be an easy and efficient way to handle that. Splitting read and write\n> controls may be a good idea.\n>\n\nI'm not clear how do you think an interaction to read controls\nasynchronously from request would look like. I'm mostly concerned\nabout the fact that most (some?) parameter would depend on the\nstatistics generated processing the most recently captured image, and\nbeing able to provide the most recent values would require quite some\ncaching somewhere. Do we want to cache -all- control values to be able\nto promptly reply to a \"getControl()\" at any point in time?\n\nSee, Kieran did a great job defining the container types and their\nstorage, but what I would like to happen before I can really make up\nmy mind is a better definition of the designed interaction model and\nthe definition of some Libcamera controls which are not just plain\nV4L2Control which Kieran has rightfully used in this series.\n\n-------------------------------------------------------------------------\nLet me write down how I imagine interacting with a control would look\nlike from an application point of view. Ignore it if it's too long :)\n\n\nLet's use in example, the lens aperture (I don't see V4L2 control that\nmaps to it, I might be wrong, as it is quite a fundamental parameter).\n\nAs an application I would expect to know:\n- the type of the control value (ie float, in this case)\n- if I can control it or not\n- the available values I can chose from, as a list of values or a range with\n min, max and a step value.\n\nWhere should this informations be stored? In the control info? How\nwould a pipeline handler create them? For each control the camera supports\nshould it fill up a control info with all this informations?\n\nSo I'm now an application, I have a camera, and I would like to list\nthe control it supports. I should call something like controls() and\nget back a map of .\n\nThe control info should live somewhere, if we want to cache it to\nperform validations, so a * is fine. The defaultValue is a reference or\npointer to a value which could be cached as well, to perform something like\n\"reset to default\".\n\nNow I want to know\n1) Can I control aperture?\n2) which values are accepted?\n\nSo I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE\nreturns a valid pair from where get the associated controlInfo. This would\ntell me:\n1) The direction: can I set it, or just read it?\n2) The supported values as a list of floats, or max-min with steps\n3) its default value in the pair.second\n\nNow, should I create a ControlValue to associate it with a request? So\nI go, create a new instance, fill it with one value, and store it in\nthe request. The control value alone, or does it need the control\ninfo? I guess the frameworks as a cache of control info, indexed by\nID, so a control value with an ID, is enough for the framework to\nretrieve the ControlInfo and perform validations.\n\nThe request gets processed, and once completed, it returns with an\nupdated control value. I inspect it's content and I could either\ndelete it if I'm done, or re-use it in the next request and delete it\nlater once I'm done? It is responsability of the application to create\ncontrols and delete them opportunely. To me this is acceptable.\n\n-------------------------------------------------------------------------\n\nDoes this match your understanding? Can you name other controls which\nwould require a different interaction model?\n\nThanks\n j\n\n\n> > +\n> > \tStatus status() const { return status_; }\n> >\n> > \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> > @@ -52,6 +55,7 @@ private:\n> > \tCamera *camera_;\n> > \tstd::map bufferMap_;\n> > \tstd::unordered_set pending_;\n> > +\tstd::set controls_;\n> >\n> > \tStatus status_;\n> > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9192636F3\n\tfor ;\n\tMon, 10 Jun 2019 12:44:11 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 291F91BF20A;\n\tMon, 10 Jun 2019 10:44:09 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 10 Jun 2019 12:45:22 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>\n\t<20190607163719.GA14958@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"vhrmtaiw43tt76wm\"","Content-Disposition":"inline","In-Reply-To":"<20190607163719.GA14958@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 10 Jun 2019 10:44:12 -0000"}},{"id":1828,"web_url":"https://patchwork.libcamera.org/comment/1828/","msgid":"","date":"2019-06-10T12:11:02","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 10/06/2019 11:45, Jacopo Mondi wrote:\n> Hi Kieran,\n> just some more thoughts on your patch and Laurent comments..\n> \n> On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote:\n> \n> \n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n>>> Provide a set to contain all controls applicable to the request.\n>>> The set contains all controls whether they are write, read, or write-read controls.\n> \n> Not on this patch strictly, but, the 'nature' of the control (w, r,\n> rw) is define by how the Control is constructed, right? If it is\n> provided a value is a write, otherwise is a read.\n\nIn it's initial construct yes, a ControlValue with no value (but given a\ntype) must be a Read control ... and a ControlValue given a value (which\ninfers it's type) will be a Write.\n\nI envisaged perhaps it would necessitate an extra call (not too fond of\nthat, but still a WIP) to get a 'Write' control to also 'Read' afterwards.\n\n\n\n> I think controls should have a 'direction' type assigned as part of\n> their definition which should be checked against what the user tries\n> to do on it, and not only depend on how the user creates them.\n\nOk - so, you see that as a third parameter in construction?\n \n\n\n> In example, the static controls that Laurent mentioned here below,\n> which are stored in the Camera, should be 'static'/'read-only'\n> controls, which provides to application non-modifiable\n> informations on the camera (orientation, position etc). Will the\n> 'direction' be part of the controls definition?\n\nI imagine if some controls are read only - then yes we would store that\nstate information as part of the ControlInfo table/class?\n\n\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> include/libcamera/request.h | 4 ++++\n>>> 1 file changed, 4 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>>> index 58de6f00a554..5fae0d5fc838 100644\n>>> --- a/include/libcamera/request.h\n>>> +++ b/include/libcamera/request.h\n>>> @@ -10,6 +10,7 @@\n>>> #include \n>>> #include \n>>>\n>>> +#include \n>>> #include \n>>>\n>>> namespace libcamera {\n>>> @@ -36,6 +37,8 @@ public:\n>>> \tint setBuffers(const std::map &streamMap);\n>>> \tBuffer *findBuffer(Stream *stream) const;\n>>>\n>>> +\tstd::set &controls() { return controls_; };\n>>\n>> I think we will need something a bit more complicated than a std::set.\n>> Here's what I was thinking about.\n>>\n>> - Let's split control information from control value. The former are\n>> static for the lifetime of the camera (or at least of the capture\n>> session), while the latter vary per-request.\n\nOk - that was one of the directions I considered as well, and I think\nnow that more thought has gone into things it could be a reasonable\nroute. Or of course the class Control::id_ could be changed to a\nControl::info_ as a &ControlInfo type in an unordered_set.\n\n>> - The control values in the request could be stored in a\n>> std::map. This would make copies as\n>> cheap as possible as we won't have to duplicate the control\n>> information.\n\nI agree, the ControlInformation should not be duplicated, and the 'ID'\nrefernce should be as cheap as possible.\n\n\n> Where do you envision them to live? I assume as they won't have to be\n> duplicated we'll maintain a table somewhere of ControlInfo each\n> request's control will reference to...\n\nMore so than that, as some controls will be dynamic - and we must query\nthe underlying kernel drivers for the properties (min/max/defaults).\n\nSo would we do that at open? StreamOn? Some-other time?\n\n\nPerhaps it will be up to the PipelineHandler to create it's ControlInfo\nset and populate values appropriately. This would then let the\nPipelineHandler decide which controls are valid to support or not.\n\n\n\n\n>> - The return value of the controls() function should be a custom class\n>> wrapping around the std::map. This is needed to perform control\n>> validation for instance (min/max/step), and will likely come handy to\n>> implement other custom behaviours.\n> \n> Each control should be validated against it's associated ControlInfo.\n> Why is wrapping the container relevant if not from the API usage\n> easiness, maybe?\n\nHrm... this makes me wonder if that validation should be done by the\n'container of multiple controls' ... or a container of a single control.\n\nI.e. either we create a ControlList() which stores a map , and it does the validation and hides the map,\n\nOr we change the control class to store \nand let that be stored in any container as required.\n\n\n>>\n>> - The control information should be exposed by the camera so that\n>> application can enumerate supported controls and query their\n>> information.\n>>\n> \n> Do we expect the camera to have anything like \"readControl()\" ?\n> My understanding was that all interactions with control goes through\n> requests, so there's not actually anything like an asynchronous\n> read().\n\n\nNo I don't think we'll have readControl() - but populating any relevant\nControlInfo sturctures might certainly have to be done at run-time (but\nthat's more like queryControl() - not readControl().\n\n> I wonder if\n> 1) getting from the camera a list of controls with associated default\n> values\n> 2) modify the one the app is interested into ad submit a request\n> 3) at request complete time verify the control value has changed to\n> the requested value (or is converging to it) and modify it again if\n> required.\n> \n> Is not a better interaction model compared to the asynchronous\n> \"getControl/setControl\" one. We briefly discussed that, and one point\n> was that the delays in completing the request might delay the read of\n> a control. I wonder if that's relevant or if we can do anything about it\n\nNo, I think a delayed request is fine. It can only be delayed due to\nother actions within that request, and we guarantee (I believe) that\nrequests complete in sequential order. Thus, the control data/state in\nthe request will match the state of that request completion. (i.e. if\nany other buffers were included.).\n\nIn my initial work - I expected reading of controls to be the *last*\nthing that a request completion does. So the control state would be as\nrecent as possible, and no further delays should be expected.\n\n> ie. the control value comes from a statistic value generated from\n> inspecting the pixel data: this should always be associated with a\n> completed request. This holds for other \"async\" controls as well: do we care\n> what is the lens exposure \"now\" or what is value was at the time the picture\n> has been taken?.\n\nI would say it's important to associate the control information with the\nother request content. I.e. - \"At the time 'this' picture was taken, the\nlens exposure 'was' $E, and the brightness was $B\".\n\n\n> If we have to fast-track some controls with an immediate\n> Camera::readControl() I agree this might be needed for some case but\n> not pretty, as it offers an API easy to abuse.\n\nI don't yet know what 'fast tracking' we would have to do.\n\nIf there is a control you want to read/write - without picture data, it\nhas to go into a request of it's own (and the framework should handle\nthat correctly, i.e. not block on waiting for any buffers to complete).\n\n\n>> - We need to think of the common use cases from an application point of\n>> view. In particular I think applications will need an API to get a\n>> default set of control values from libcamera, that they will then\n>> possibly modify and set for the first request. Subsequent requests\n>> will likely just modify a small number of controls, and will likely be\n>> constructed from scratch. For control that applications want to read,\n>> I expect most requests to contain the same controls, so there should\n>> be an easy and efficient way to handle that. Splitting read and write\n>> controls may be a good idea.\n\nLaurent: I'm a bit confused as to whether you expect a request to\ncontain all controls and all state, or just ones that are modified...\n\n\n> I'm not clear how do you think an interaction to read controls\n> asynchronously from request would look like. I'm mostly concerned\n\n\nAsynchronously from request ..? You mean not using a request? (I don't\nthink we would ever read controls without it going through a request?)\nor do you mean handling the timeing of when a control is read during a\nrequest lifecycle.\n\n\n> about the fact that most (some?) parameter would depend on the\n> statistics generated processing the most recently captured image, and\n> being able to provide the most recent values would require quite some\n> caching somewhere. Do we want to cache -all- control values to be able\n> to promptly reply to a \"getControl()\" at any point in time?\n\n\nHrm... some control values might be 'cached' internally, becuase they\nmight not really be a value that is queried from hardware. (As you state\nbelow). ... but any control which is represented by hardware, I can't\nimagine caching it - as if you read it - you want to know - what that\nvalue really is?\n\nThe IPA's might likely keep track of the values they need to monitor\nover a time series perhaps? But that should be in the IPA - not at a\ncore level I don't think... \n\n\n> See, Kieran did a great job defining the container types and their\n> storage, but what I would like to happen before I can really make up\n> my mind is a better definition of the designed interaction model and\n> the definition of some Libcamera controls which are not just plain\n> V4L2Control which Kieran has rightfully used in this series.\n> \n> -------------------------------------------------------------------------\n> Let me write down how I imagine interacting with a control would look\n> like from an application point of view. Ignore it if it's too long :)\n> \n> \n> Let's use in example, the lens aperture (I don't see V4L2 control that\n> maps to it, I might be wrong, as it is quite a fundamental parameter).\n> \n> As an application I would expect to know:\n> - the type of the control value (ie float, in this case)\n> - if I can control it or not\n> - the available values I can chose from, as a list of values or a range with\n> min, max and a step value.\n> \n> Where should this informations be stored? In the control info? How\n\nYes, that all sounds like ControlInfo information to me ..., which is\n(at least runtime) constant.\n\n> would a pipeline handler create them? For each control the camera supports\n> should it fill up a control info with all this informations?\n\nYes, I think so.\n\n\n\n> So I'm now an application, I have a camera, and I would like to list\n> the control it supports. I should call something like controls() and\n> get back a map of .\n\nPerhaps, or the ControlValue &default might actually just be insside the\nControlInfo &\n\n\n> The control info should live somewhere, if we want to cache it to\n> perform validations, so a * is fine. The defaultValue is a reference or\n\nWhy a pointer and not a reference to the ControlInfo? It should be\nguaranteed to exist... (And we should guarantee it's pointer/address\nshould not change)\n\n> pointer to a value which could be cached as well, to perform something like\n> \"reset to default\".\n\nYes, somehow we want to be able to reset controls to defaults. Perhaps\nindividually, or in bulk, or as a complete (re)set.\n\n\n> Now I want to know\n> 1) Can I control aperture?\n> 2) which values are accepted?\n> \n> So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE\n> returns a valid pair from where get the associated controlInfo. This would\n> tell me:\n\n\nThis is why I don't like std::map ... If you try to obtain a\nControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be\ncreated!\n\nSo an unordered_set might be more appropriate there.\n\n(Or perhaps only [] creates, and maybe .find() will be ok)\n\n> 1) The direction: can I set it, or just read it?\n\nIndeed, and should this be a bitfield, or enum like my ControlAction enum...\n\n> 2) The supported values as a list of floats, or max-min with steps\n> 3) its default value in the pair.second\n\nAs the ControlInfo is a class, it can just as well contain a\nControlValue default; imho.\n\n\n> Now, should I create a ControlValue to associate it with a request? So\n> I go, create a new instance, fill it with one value, and store it in\n> the request. The control value alone, or does it need the control\n> info? I guess the frameworks as a cache of control info, indexed by\n> ID, so a control value with an ID, is enough for the framework to\n> retrieve the ControlInfo and perform validations.\n\nYes, you would create a ControlValue to put in the request somehow.\n\n\nThat ControlValue should somehow be associated with a ControlInfo &\nreference, and it should be easy to obtain the ControlValue for\nLIBCAMERA_LENS_APERTURE within the container (either the List, Set, or\nMap) stored in the request.\n\n\n> The request gets processed, and once completed, it returns with an\n> updated control value. I inspect it's content and I could either\n> delete it if I'm done, or re-use it in the next request and delete it\n> later once I'm done? It is responsability of the application to create\n> controls and delete them opportunely. To me this is acceptable.\n\nI 'think' when you get a new request, you would create a new set of\ncontrols.\n\nI don't think you'd necessarily use the same objects - but you might\nhave a predefined list of controls that you could add to a request\n(which would create a copy of that list) for some known determined state\nthat you desire.\n\n\n> \n> -------------------------------------------------------------------------\n> \n> Does this match your understanding? Can you name other controls which\n> would require a different interaction model?\n\n\nFurther discussion all inline, I think (/hope) we're quite aligned on\nmost things...\n\n> \n> Thanks\n> j\n> \n> \n>>> +\n>>> \tStatus status() const { return status_; }\n>>>\n>>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n>>> @@ -52,6 +55,7 @@ private:\n>>> \tCamera *camera_;\n>>> \tstd::map bufferMap_;\n>>> \tstd::unordered_set pending_;\n>>> +\tstd::set controls_;\n>>>\n>>> \tStatus status_;\n>>> };\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9737C6325A\n\tfor ;\n\tMon, 10 Jun 2019 14:11:06 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6B1B9CB;\n\tMon, 10 Jun 2019 14:11:05 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560168665;\n\tbh=30RpLtJeO5fssZWKgw4BWE9DzmqhF0DBsaXXGaJjdG8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=KeIuxTJHceTGenwoZITKsqn706XZrl8n5efws00SfSLu1Xr5wHZ1DhOAT6MqAU6ml\n\tQS9GA3sALBx1CYkO2Y6ie3XdTqPofjp0bscB/LLhF1gAl3o80nmoz1UpEfcmAQViF5\n\tG9mqrESppNP89fadwyUfgIZUdx/cxVWzFrSXoC/c=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>\n\t<20190607163719.GA14958@pendragon.ideasonboard.com>\n\t<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 10 Jun 2019 13:11:02 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 10 Jun 2019 12:11:06 -0000"}},{"id":1841,"web_url":"https://patchwork.libcamera.org/comment/1841/","msgid":"<20190611120716.GG5016@pendragon.ideasonboard.com>","date":"2019-06-11T12:07:16","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote:\n> On 10/06/2019 11:45, Jacopo Mondi wrote:\n> > On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote:\n> >> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n> >>> Provide a set to contain all controls applicable to the request.\n> >>> The set contains all controls whether they are write, read, or write-read controls.\n> > \n> > Not on this patch strictly, but, the 'nature' of the control (w, r,\n> > rw) is define by how the Control is constructed, right? If it is\n> > provided a value is a write, otherwise is a read.\n> \n> In it's initial construct yes, a ControlValue with no value (but given a\n> type) must be a Read control ... and a ControlValue given a value (which\n> infers it's type) will be a Write.\n> \n> I envisaged perhaps it would necessitate an extra call (not too fond of\n> that, but still a WIP) to get a 'Write' control to also 'Read' afterwards.\n\nThis still doesn't answer the question of whether application should\nprovide a list of controls they want to read, or if the pipeline handler\nwill just report all the information is has. I was envisioning the\nlatter, but if you think the former is better, please explain why :-)\n\nNote that the data reported by the pipeline handler is more in the form\nof metadata (data related to how the image was captured) than in the\nform of controls (tunable that are set to influence the image). It could\nmake sense to separate them at the API level with different names to\nmake this clear, although combining them through a single API could also\nmake sense if the implementations become too similar.\n\n> > I think controls should have a 'direction' type assigned as part of\n> > their definition which should be checked against what the user tries\n> > to do on it, and not only depend on how the user creates them.\n> \n> Ok - so, you see that as a third parameter in construction?\n> \n> \n> > In example, the static controls that Laurent mentioned here below,\n> > which are stored in the Camera, should be 'static'/'read-only'\n> > controls, which provides to application non-modifiable\n> > informations on the camera (orientation, position etc). Will the\n> > 'direction' be part of the controls definition?\n> \n> I imagine if some controls are read only - then yes we would store that\n> state information as part of the ControlInfo table/class?\n> \n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> include/libcamera/request.h | 4 ++++\n> >>> 1 file changed, 4 insertions(+)\n> >>>\n> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> >>> index 58de6f00a554..5fae0d5fc838 100644\n> >>> --- a/include/libcamera/request.h\n> >>> +++ b/include/libcamera/request.h\n> >>> @@ -10,6 +10,7 @@\n> >>> #include \n> >>> #include \n> >>>\n> >>> +#include \n> >>> #include \n> >>>\n> >>> namespace libcamera {\n> >>> @@ -36,6 +37,8 @@ public:\n> >>> \tint setBuffers(const std::map &streamMap);\n> >>> \tBuffer *findBuffer(Stream *stream) const;\n> >>>\n> >>> +\tstd::set &controls() { return controls_; };\n> >>\n> >> I think we will need something a bit more complicated than a std::set.\n> >> Here's what I was thinking about.\n> >>\n> >> - Let's split control information from control value. The former are\n> >> static for the lifetime of the camera (or at least of the capture\n> >> session), while the latter vary per-request.\n> \n> Ok - that was one of the directions I considered as well, and I think\n> now that more thought has gone into things it could be a reasonable\n> route. Or of course the class Control::id_ could be changed to a\n> Control::info_ as a &ControlInfo type in an unordered_set.\n> \n> >> - The control values in the request could be stored in a\n> >> std::map. This would make copies as\n> >> cheap as possible as we won't have to duplicate the control\n> >> information.\n> \n> I agree, the ControlInformation should not be duplicated, and the 'ID'\n> refernce should be as cheap as possible.\n> \n> > Where do you envision them to live? I assume as they won't have to be\n> > duplicated we'll maintain a table somewhere of ControlInfo each\n> > request's control will reference to...\n> \n> More so than that, as some controls will be dynamic - and we must query\n> the underlying kernel drivers for the properties (min/max/defaults).\n> \n> So would we do that at open? StreamOn? Some-other time?\n\nWill they be dynamic ? They certainly can't be hardcoded in the sourced\ncode, but I don't expect them to change for the lifetime of the camera\n(feel free to prove me wrong). They could thus be created when creating\nthe camera.\n\n> Perhaps it will be up to the PipelineHandler to create it's ControlInfo\n> set and populate values appropriately. This would then let the\n> PipelineHandler decide which controls are valid to support or not.\n\nI think that's what will happen, yes (with the help of the IPA).\n\n> >> - The return value of the controls() function should be a custom class\n> >> wrapping around the std::map. This is needed to perform control\n> >> validation for instance (min/max/step), and will likely come handy to\n> >> implement other custom behaviours.\n> > \n> > Each control should be validated against it's associated ControlInfo.\n> > Why is wrapping the container relevant if not from the API usage\n> > easiness, maybe?\n> \n> Hrm... this makes me wonder if that validation should be done by the\n> 'container of multiple controls' ... or a container of a single control.\n> \n> I.e. either we create a ControlList() which stores a map ControlValue>, and it does the validation and hides the map,\n> \n> Or we change the control class to store \n> and let that be stored in any container as required.\n\nWe have started by exposing STL containers directly (for format\nenumeration for instance), and then moved to custom classes. I think\ncustom classes give is more flexibility and allow creation of better\nAPIs for applications. Regardless of where the validation is performed,\nI would use a custom container class to store controls in requests.\n\n> >> - The control information should be exposed by the camera so that\n> >> application can enumerate supported controls and query their\n> >> information.\n> > \n> > Do we expect the camera to have anything like \"readControl()\" ?\n> > My understanding was that all interactions with control goes through\n> > requests, so there's not actually anything like an asynchronous\n> > read().\n> \n> No I don't think we'll have readControl() - but populating any relevant\n> ControlInfo sturctures might certainly have to be done at run-time (but\n> that's more like queryControl() - not readControl().\n\nYes, that's what I meant.\n\n> > I wonder if\n> > 1) getting from the camera a list of controls with associated default\n> > values\n\nI don't think we'll have global defaults, we will likely need an API to\nquery defaults for a given use case, similar to\nCamera::generateConfiguration().\n\n> > 2) modify the one the app is interested into ad submit a request\n> > 3) at request complete time verify the control value has changed to\n> > the requested value (or is converging to it) and modify it again if\n> > required.\n> > \n> > Is not a better interaction model compared to the asynchronous\n> > \"getControl/setControl\" one. We briefly discussed that, and one point\n> > was that the delays in completing the request might delay the read of\n> > a control. I wonder if that's relevant or if we can do anything about it\n> \n> No, I think a delayed request is fine. It can only be delayed due to\n> other actions within that request, and we guarantee (I believe) that\n> requests complete in sequential order. Thus, the control data/state in\n> the request will match the state of that request completion. (i.e. if\n> any other buffers were included.).\n> \n> In my initial work - I expected reading of controls to be the *last*\n> thing that a request completion does. So the control state would be as\n> recent as possible, and no further delays should be expected.\n\nAmong all the information reported to the application through the\nrequest, we will have data coming from V4L2 controls, data coming from\nbuffers (e.g. metadata sent by the sensor as embedded data), data\ncreated by the IPA (e.g. the locked state of the control loops), ... All\nthese will potentially be generated at different times.\n\n> > ie. the control value comes from a statistic value generated from\n> > inspecting the pixel data: this should always be associated with a\n> > completed request. This holds for other \"async\" controls as well: do we care\n> > what is the lens exposure \"now\" or what is value was at the time the picture\n> > has been taken?.\n> \n> I would say it's important to associate the control information with the\n> other request content. I.e. - \"At the time 'this' picture was taken, the\n> lens exposure 'was' $E, and the brightness was $B\".\n\nI agree, I don't think we need asynchronous reads.\n\n> > If we have to fast-track some controls with an immediate\n> > Camera::readControl() I agree this might be needed for some case but\n> > not pretty, as it offers an API easy to abuse.\n> \n> I don't yet know what 'fast tracking' we would have to do.\n> \n> If there is a control you want to read/write - without picture data, it\n> has to go into a request of it's own (and the framework should handle\n> that correctly, i.e. not block on waiting for any buffers to complete).\n\nI don't see a use-case for fast-tracking reads. Writes could be\ndifferent though, we will have to evaluate the use cases.\n\n> >> - We need to think of the common use cases from an application point of\n> >> view. In particular I think applications will need an API to get a\n> >> default set of control values from libcamera, that they will then\n> >> possibly modify and set for the first request. Subsequent requests\n> >> will likely just modify a small number of controls, and will likely be\n> >> constructed from scratch. For control that applications want to read,\n> >> I expect most requests to contain the same controls, so there should\n> >> be an easy and efficient way to handle that. Splitting read and write\n> >> controls may be a good idea.\n> \n> Laurent: I'm a bit confused as to whether you expect a request to\n> contain all controls and all state, or just ones that are modified...\n\nJust the ones that are modified, but I expect the first request to\ntypically contain all/most/many controls. The camera should provide\ndefault control values for a given use case, and I think that will be\nused to create the first request. Subsequent requests will likely\ncontain less controls.\n\n> > I'm not clear how do you think an interaction to read controls\n> > asynchronously from request would look like. I'm mostly concerned\n> \n> Asynchronously from request ..? You mean not using a request? (I don't\n> think we would ever read controls without it going through a request?)\n> or do you mean handling the timeing of when a control is read during a\n> request lifecycle.\n> \n> > about the fact that most (some?) parameter would depend on the\n> > statistics generated processing the most recently captured image, and\n> > being able to provide the most recent values would require quite some\n> > caching somewhere. Do we want to cache -all- control values to be able\n> > to promptly reply to a \"getControl()\" at any point in time?\n> \n> Hrm... some control values might be 'cached' internally, becuase they\n> might not really be a value that is queried from hardware. (As you state\n> below). ... but any control which is represented by hardware, I can't\n> imagine caching it - as if you read it - you want to know - what that\n> value really is?\n> \n> The IPA's might likely keep track of the values they need to monitor\n> over a time series perhaps? But that should be in the IPA - not at a\n> core level I don't think... \n> \n> > See, Kieran did a great job defining the container types and their\n> > storage, but what I would like to happen before I can really make up\n> > my mind is a better definition of the designed interaction model and\n> > the definition of some Libcamera controls which are not just plain\n> > V4L2Control which Kieran has rightfully used in this series.\n> > \n> > -------------------------------------------------------------------------\n> > Let me write down how I imagine interacting with a control would look\n> > like from an application point of view. Ignore it if it's too long :)\n> > \n> > \n> > Let's use in example, the lens aperture (I don't see V4L2 control that\n> > maps to it, I might be wrong, as it is quite a fundamental parameter).\n> > \n> > As an application I would expect to know:\n> > - the type of the control value (ie float, in this case)\n> > - if I can control it or not\n> > - the available values I can chose from, as a list of values or a range with\n> > min, max and a step value.\n> > \n> > Where should this informations be stored? In the control info? How\n> \n> Yes, that all sounds like ControlInfo information to me ..., which is\n> (at least runtime) constant.\n> \n> > would a pipeline handler create them? For each control the camera supports\n> > should it fill up a control info with all this informations?\n> \n> Yes, I think so.\n> \n> > So I'm now an application, I have a camera, and I would like to list\n> > the control it supports. I should call something like controls() and\n> > get back a map of .\n> \n> Perhaps, or the ControlValue &default might actually just be insside the\n> ControlInfo &\n\nI think the default values will be use-case-dependent, so I believe\nwe'll need a method to retrieve information about all supported\ncontrols, without a default. The default should be retrieved through a\nseparate method that will take a use case (possible a set of stream\nroles).\n\n> > The control info should live somewhere, if we want to cache it to\n> > perform validations, so a * is fine. The defaultValue is a reference or\n> \n> Why a pointer and not a reference to the ControlInfo? It should be\n> guaranteed to exist... (And we should guarantee it's pointer/address\n> should not change)\n> \n> > pointer to a value which could be cached as well, to perform something like\n> > \"reset to default\".\n> \n> Yes, somehow we want to be able to reset controls to defaults. Perhaps\n> individually, or in bulk, or as a complete (re)set.\n\nAs defaults are not global, we can't have a reset method, but\napplications can simply use the defaults they have retrieved for a given\nuse case and set them in a request.\n\n> > Now I want to know\n> > 1) Can I control aperture?\n> > 2) which values are accepted?\n> > \n> > So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE\n> > returns a valid pair from where get the associated controlInfo. This would\n> > tell me:\n> \n> This is why I don't like std::map ... If you try to obtain a\n> ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be\n> created!\n\nstd::map::find()\n\n> So an unordered_set might be more appropriate there.\n> \n> (Or perhaps only [] creates, and maybe .find() will be ok)\n> \n> > 1) The direction: can I set it, or just read it?\n> \n> Indeed, and should this be a bitfield, or enum like my ControlAction enum...\n> \n> > 2) The supported values as a list of floats, or max-min with steps\n> > 3) its default value in the pair.second\n> \n> As the ControlInfo is a class, it can just as well contain a\n> ControlValue default; imho.\n\nSee above.\n\n> > Now, should I create a ControlValue to associate it with a request? So\n> > I go, create a new instance, fill it with one value, and store it in\n> > the request. The control value alone, or does it need the control\n> > info? I guess the frameworks as a cache of control info, indexed by\n> > ID, so a control value with an ID, is enough for the framework to\n> > retrieve the ControlInfo and perform validations.\n> \n> Yes, you would create a ControlValue to put in the request somehow.\n> \n> \n> That ControlValue should somehow be associated with a ControlInfo &\n> reference, and it should be easy to obtain the ControlValue for\n> LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or\n> Map) stored in the request.\n> \n> > The request gets processed, and once completed, it returns with an\n> > updated control value. I inspect it's content and I could either\n> > delete it if I'm done, or re-use it in the next request and delete it\n> > later once I'm done? It is responsability of the application to create\n> > controls and delete them opportunely. To me this is acceptable.\n> \n> I 'think' when you get a new request, you would create a new set of\n> controls.\n> \n> I don't think you'd necessarily use the same objects - but you might\n> have a predefined list of controls that you could add to a request\n> (which would create a copy of that list) for some known determined state\n> that you desire.\n\nThat's how I envision it too. When a request is dequeued and reused all\nthe controls it contains should be removed. Applications can then set\ncontrols manually, or use a predefined list.\n\n> > -------------------------------------------------------------------------\n> > \n> > Does this match your understanding? Can you name other controls which\n> > would require a different interaction model?\n> \n> Further discussion all inline, I think (/hope) we're quite aligned on\n> most things...\n> \n> >>> +\n> >>> \tStatus status() const { return status_; }\n> >>>\n> >>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> >>> @@ -52,6 +55,7 @@ private:\n> >>> \tCamera *camera_;\n> >>> \tstd::map bufferMap_;\n> >>> \tstd::unordered_set pending_;\n> >>> +\tstd::set controls_;\n> >>>\n> >>> \tStatus status_;\n> >>> };","headers":{"Return-Path":"","Received":["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 069C261E1B\n\tfor ;\n\tTue, 11 Jun 2019 14:07:32 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 470C5FA0;\n\tTue, 11 Jun 2019 14:07:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560254851;\n\tbh=C3NPZMWbmj6wQg3Hh6IbgVsybqy3IbXjsguXeLoWTn0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gO1/eZxojoaMRbb8yLFrUghsS3f8BvcubSwGkEJif++ZpSByqyYFa2mCBm4YCfkLy\n\tz31Mv23ScmR57Dq8ajFEFCBHqU2QXp3yids5cpAGRQuSffdxlT28elPZUu7EE37BTt\n\tVZoFPtv+83EIytHLFrMEpPmUdmZbDxy/d3V6JikE=","Date":"Tue, 11 Jun 2019 15:07:16 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tLibCamera Devel ","Message-ID":"<20190611120716.GG5016@pendragon.ideasonboard.com>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>\n\t<20190607163719.GA14958@pendragon.ideasonboard.com>\n\t<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 12:07:32 -0000"}},{"id":1847,"web_url":"https://patchwork.libcamera.org/comment/1847/","msgid":"<20190611131606.2v5hbzp4nf4cb5l5@uno.localdomain>","date":"2019-06-11T13:16:06","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Kieran,\n\nOn Tue, Jun 11, 2019 at 03:07:16PM +0300, Laurent Pinchart wrote:\n\nThank you both for your replies. Now it's all much clear to me (let's\nfor how long).\n\nThanks\n j\n\n\n> On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote:\n> > On 10/06/2019 11:45, Jacopo Mondi wrote:\n> > > On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote:\n> > >> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n> > >>> Provide a set to contain all controls applicable to the request.\n> > >>> The set contains all controls whether they are write, read, or write-read controls.\n> > >\n> > > Not on this patch strictly, but, the 'nature' of the control (w, r,\n> > > rw) is define by how the Control is constructed, right? If it is\n> > > provided a value is a write, otherwise is a read.\n> >\n> > In it's initial construct yes, a ControlValue with no value (but given a\n> > type) must be a Read control ... and a ControlValue given a value (which\n> > infers it's type) will be a Write.\n> >\n> > I envisaged perhaps it would necessitate an extra call (not too fond of\n> > that, but still a WIP) to get a 'Write' control to also 'Read' afterwards.\n>\n> This still doesn't answer the question of whether application should\n> provide a list of controls they want to read, or if the pipeline handler\n> will just report all the information is has. I was envisioning the\n> latter, but if you think the former is better, please explain why :-)\n>\n> Note that the data reported by the pipeline handler is more in the form\n> of metadata (data related to how the image was captured) than in the\n> form of controls (tunable that are set to influence the image). It could\n> make sense to separate them at the API level with different names to\n> make this clear, although combining them through a single API could also\n> make sense if the implementations become too similar.\n>\n> > > I think controls should have a 'direction' type assigned as part of\n> > > their definition which should be checked against what the user tries\n> > > to do on it, and not only depend on how the user creates them.\n> >\n> > Ok - so, you see that as a third parameter in construction?\n> > \n> >\n> > > In example, the static controls that Laurent mentioned here below,\n> > > which are stored in the Camera, should be 'static'/'read-only'\n> > > controls, which provides to application non-modifiable\n> > > informations on the camera (orientation, position etc). Will the\n> > > 'direction' be part of the controls definition?\n> >\n> > I imagine if some controls are read only - then yes we would store that\n> > state information as part of the ControlInfo table/class?\n> >\n> > >>> Signed-off-by: Kieran Bingham \n> > >>> ---\n> > >>> include/libcamera/request.h | 4 ++++\n> > >>> 1 file changed, 4 insertions(+)\n> > >>>\n> > >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > >>> index 58de6f00a554..5fae0d5fc838 100644\n> > >>> --- a/include/libcamera/request.h\n> > >>> +++ b/include/libcamera/request.h\n> > >>> @@ -10,6 +10,7 @@\n> > >>> #include \n> > >>> #include \n> > >>>\n> > >>> +#include \n> > >>> #include \n> > >>>\n> > >>> namespace libcamera {\n> > >>> @@ -36,6 +37,8 @@ public:\n> > >>> \tint setBuffers(const std::map &streamMap);\n> > >>> \tBuffer *findBuffer(Stream *stream) const;\n> > >>>\n> > >>> +\tstd::set &controls() { return controls_; };\n> > >>\n> > >> I think we will need something a bit more complicated than a std::set.\n> > >> Here's what I was thinking about.\n> > >>\n> > >> - Let's split control information from control value. The former are\n> > >> static for the lifetime of the camera (or at least of the capture\n> > >> session), while the latter vary per-request.\n> >\n> > Ok - that was one of the directions I considered as well, and I think\n> > now that more thought has gone into things it could be a reasonable\n> > route. Or of course the class Control::id_ could be changed to a\n> > Control::info_ as a &ControlInfo type in an unordered_set.\n> >\n> > >> - The control values in the request could be stored in a\n> > >> std::map. This would make copies as\n> > >> cheap as possible as we won't have to duplicate the control\n> > >> information.\n> >\n> > I agree, the ControlInformation should not be duplicated, and the 'ID'\n> > refernce should be as cheap as possible.\n> >\n> > > Where do you envision them to live? I assume as they won't have to be\n> > > duplicated we'll maintain a table somewhere of ControlInfo each\n> > > request's control will reference to...\n> >\n> > More so than that, as some controls will be dynamic - and we must query\n> > the underlying kernel drivers for the properties (min/max/defaults).\n> >\n> > So would we do that at open? StreamOn? Some-other time?\n>\n> Will they be dynamic ? They certainly can't be hardcoded in the sourced\n> code, but I don't expect them to change for the lifetime of the camera\n> (feel free to prove me wrong). They could thus be created when creating\n> the camera.\n>\n> > Perhaps it will be up to the PipelineHandler to create it's ControlInfo\n> > set and populate values appropriately. This would then let the\n> > PipelineHandler decide which controls are valid to support or not.\n>\n> I think that's what will happen, yes (with the help of the IPA).\n>\n> > >> - The return value of the controls() function should be a custom class\n> > >> wrapping around the std::map. This is needed to perform control\n> > >> validation for instance (min/max/step), and will likely come handy to\n> > >> implement other custom behaviours.\n> > >\n> > > Each control should be validated against it's associated ControlInfo.\n> > > Why is wrapping the container relevant if not from the API usage\n> > > easiness, maybe?\n> >\n> > Hrm... this makes me wonder if that validation should be done by the\n> > 'container of multiple controls' ... or a container of a single control.\n> >\n> > I.e. either we create a ControlList() which stores a map > ControlValue>, and it does the validation and hides the map,\n> >\n> > Or we change the control class to store \n> > and let that be stored in any container as required.\n>\n> We have started by exposing STL containers directly (for format\n> enumeration for instance), and then moved to custom classes. I think\n> custom classes give is more flexibility and allow creation of better\n> APIs for applications. Regardless of where the validation is performed,\n> I would use a custom container class to store controls in requests.\n>\n> > >> - The control information should be exposed by the camera so that\n> > >> application can enumerate supported controls and query their\n> > >> information.\n> > >\n> > > Do we expect the camera to have anything like \"readControl()\" ?\n> > > My understanding was that all interactions with control goes through\n> > > requests, so there's not actually anything like an asynchronous\n> > > read().\n> >\n> > No I don't think we'll have readControl() - but populating any relevant\n> > ControlInfo sturctures might certainly have to be done at run-time (but\n> > that's more like queryControl() - not readControl().\n>\n> Yes, that's what I meant.\n>\n> > > I wonder if\n> > > 1) getting from the camera a list of controls with associated default\n> > > values\n>\n> I don't think we'll have global defaults, we will likely need an API to\n> query defaults for a given use case, similar to\n> Camera::generateConfiguration().\n>\n> > > 2) modify the one the app is interested into ad submit a request\n> > > 3) at request complete time verify the control value has changed to\n> > > the requested value (or is converging to it) and modify it again if\n> > > required.\n> > >\n> > > Is not a better interaction model compared to the asynchronous\n> > > \"getControl/setControl\" one. We briefly discussed that, and one point\n> > > was that the delays in completing the request might delay the read of\n> > > a control. I wonder if that's relevant or if we can do anything about it\n> >\n> > No, I think a delayed request is fine. It can only be delayed due to\n> > other actions within that request, and we guarantee (I believe) that\n> > requests complete in sequential order. Thus, the control data/state in\n> > the request will match the state of that request completion. (i.e. if\n> > any other buffers were included.).\n> >\n> > In my initial work - I expected reading of controls to be the *last*\n> > thing that a request completion does. So the control state would be as\n> > recent as possible, and no further delays should be expected.\n>\n> Among all the information reported to the application through the\n> request, we will have data coming from V4L2 controls, data coming from\n> buffers (e.g. metadata sent by the sensor as embedded data), data\n> created by the IPA (e.g. the locked state of the control loops), ... All\n> these will potentially be generated at different times.\n>\n> > > ie. the control value comes from a statistic value generated from\n> > > inspecting the pixel data: this should always be associated with a\n> > > completed request. This holds for other \"async\" controls as well: do we care\n> > > what is the lens exposure \"now\" or what is value was at the time the picture\n> > > has been taken?.\n> >\n> > I would say it's important to associate the control information with the\n> > other request content. I.e. - \"At the time 'this' picture was taken, the\n> > lens exposure 'was' $E, and the brightness was $B\".\n>\n> I agree, I don't think we need asynchronous reads.\n>\n> > > If we have to fast-track some controls with an immediate\n> > > Camera::readControl() I agree this might be needed for some case but\n> > > not pretty, as it offers an API easy to abuse.\n> >\n> > I don't yet know what 'fast tracking' we would have to do.\n> >\n> > If there is a control you want to read/write - without picture data, it\n> > has to go into a request of it's own (and the framework should handle\n> > that correctly, i.e. not block on waiting for any buffers to complete).\n>\n> I don't see a use-case for fast-tracking reads. Writes could be\n> different though, we will have to evaluate the use cases.\n>\n> > >> - We need to think of the common use cases from an application point of\n> > >> view. In particular I think applications will need an API to get a\n> > >> default set of control values from libcamera, that they will then\n> > >> possibly modify and set for the first request. Subsequent requests\n> > >> will likely just modify a small number of controls, and will likely be\n> > >> constructed from scratch. For control that applications want to read,\n> > >> I expect most requests to contain the same controls, so there should\n> > >> be an easy and efficient way to handle that. Splitting read and write\n> > >> controls may be a good idea.\n> >\n> > Laurent: I'm a bit confused as to whether you expect a request to\n> > contain all controls and all state, or just ones that are modified...\n>\n> Just the ones that are modified, but I expect the first request to\n> typically contain all/most/many controls. The camera should provide\n> default control values for a given use case, and I think that will be\n> used to create the first request. Subsequent requests will likely\n> contain less controls.\n>\n> > > I'm not clear how do you think an interaction to read controls\n> > > asynchronously from request would look like. I'm mostly concerned\n> >\n> > Asynchronously from request ..? You mean not using a request? (I don't\n> > think we would ever read controls without it going through a request?)\n> > or do you mean handling the timeing of when a control is read during a\n> > request lifecycle.\n> >\n> > > about the fact that most (some?) parameter would depend on the\n> > > statistics generated processing the most recently captured image, and\n> > > being able to provide the most recent values would require quite some\n> > > caching somewhere. Do we want to cache -all- control values to be able\n> > > to promptly reply to a \"getControl()\" at any point in time?\n> >\n> > Hrm... some control values might be 'cached' internally, becuase they\n> > might not really be a value that is queried from hardware. (As you state\n> > below). ... but any control which is represented by hardware, I can't\n> > imagine caching it - as if you read it - you want to know - what that\n> > value really is?\n> >\n> > The IPA's might likely keep track of the values they need to monitor\n> > over a time series perhaps? But that should be in the IPA - not at a\n> > core level I don't think... \n> >\n> > > See, Kieran did a great job defining the container types and their\n> > > storage, but what I would like to happen before I can really make up\n> > > my mind is a better definition of the designed interaction model and\n> > > the definition of some Libcamera controls which are not just plain\n> > > V4L2Control which Kieran has rightfully used in this series.\n> > >\n> > > -------------------------------------------------------------------------\n> > > Let me write down how I imagine interacting with a control would look\n> > > like from an application point of view. Ignore it if it's too long :)\n> > >\n> > >\n> > > Let's use in example, the lens aperture (I don't see V4L2 control that\n> > > maps to it, I might be wrong, as it is quite a fundamental parameter).\n> > >\n> > > As an application I would expect to know:\n> > > - the type of the control value (ie float, in this case)\n> > > - if I can control it or not\n> > > - the available values I can chose from, as a list of values or a range with\n> > > min, max and a step value.\n> > >\n> > > Where should this informations be stored? In the control info? How\n> >\n> > Yes, that all sounds like ControlInfo information to me ..., which is\n> > (at least runtime) constant.\n> >\n> > > would a pipeline handler create them? For each control the camera supports\n> > > should it fill up a control info with all this informations?\n> >\n> > Yes, I think so.\n> >\n> > > So I'm now an application, I have a camera, and I would like to list\n> > > the control it supports. I should call something like controls() and\n> > > get back a map of .\n> >\n> > Perhaps, or the ControlValue &default might actually just be insside the\n> > ControlInfo &\n>\n> I think the default values will be use-case-dependent, so I believe\n> we'll need a method to retrieve information about all supported\n> controls, without a default. The default should be retrieved through a\n> separate method that will take a use case (possible a set of stream\n> roles).\n>\n> > > The control info should live somewhere, if we want to cache it to\n> > > perform validations, so a * is fine. The defaultValue is a reference or\n> >\n> > Why a pointer and not a reference to the ControlInfo? It should be\n> > guaranteed to exist... (And we should guarantee it's pointer/address\n> > should not change)\n> >\n> > > pointer to a value which could be cached as well, to perform something like\n> > > \"reset to default\".\n> >\n> > Yes, somehow we want to be able to reset controls to defaults. Perhaps\n> > individually, or in bulk, or as a complete (re)set.\n>\n> As defaults are not global, we can't have a reset method, but\n> applications can simply use the defaults they have retrieved for a given\n> use case and set them in a request.\n>\n> > > Now I want to know\n> > > 1) Can I control aperture?\n> > > 2) which values are accepted?\n> > >\n> > > So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE\n> > > returns a valid pair from where get the associated controlInfo. This would\n> > > tell me:\n> >\n> > This is why I don't like std::map ... If you try to obtain a\n> > ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be\n> > created!\n>\n> std::map::find()\n>\n> > So an unordered_set might be more appropriate there.\n> >\n> > (Or perhaps only [] creates, and maybe .find() will be ok)\n> >\n> > > 1) The direction: can I set it, or just read it?\n> >\n> > Indeed, and should this be a bitfield, or enum like my ControlAction enum...\n> >\n> > > 2) The supported values as a list of floats, or max-min with steps\n> > > 3) its default value in the pair.second\n> >\n> > As the ControlInfo is a class, it can just as well contain a\n> > ControlValue default; imho.\n>\n> See above.\n>\n> > > Now, should I create a ControlValue to associate it with a request? So\n> > > I go, create a new instance, fill it with one value, and store it in\n> > > the request. The control value alone, or does it need the control\n> > > info? I guess the frameworks as a cache of control info, indexed by\n> > > ID, so a control value with an ID, is enough for the framework to\n> > > retrieve the ControlInfo and perform validations.\n> >\n> > Yes, you would create a ControlValue to put in the request somehow.\n> > \n> >\n> > That ControlValue should somehow be associated with a ControlInfo &\n> > reference, and it should be easy to obtain the ControlValue for\n> > LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or\n> > Map) stored in the request.\n> >\n> > > The request gets processed, and once completed, it returns with an\n> > > updated control value. I inspect it's content and I could either\n> > > delete it if I'm done, or re-use it in the next request and delete it\n> > > later once I'm done? It is responsability of the application to create\n> > > controls and delete them opportunely. To me this is acceptable.\n> >\n> > I 'think' when you get a new request, you would create a new set of\n> > controls.\n> >\n> > I don't think you'd necessarily use the same objects - but you might\n> > have a predefined list of controls that you could add to a request\n> > (which would create a copy of that list) for some known determined state\n> > that you desire.\n>\n> That's how I envision it too. When a request is dequeued and reused all\n> the controls it contains should be removed. Applications can then set\n> controls manually, or use a predefined list.\n>\n> > > -------------------------------------------------------------------------\n> > >\n> > > Does this match your understanding? Can you name other controls which\n> > > would require a different interaction model?\n> >\n> > Further discussion all inline, I think (/hope) we're quite aligned on\n> > most things...\n> >\n> > >>> +\n> > >>> \tStatus status() const { return status_; }\n> > >>>\n> > >>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> > >>> @@ -52,6 +55,7 @@ private:\n> > >>> \tCamera *camera_;\n> > >>> \tstd::map bufferMap_;\n> > >>> \tstd::unordered_set pending_;\n> > >>> +\tstd::set controls_;\n> > >>>\n> > >>> \tStatus status_;\n> > >>> };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB02E61BE3\n\tfor ;\n\tTue, 11 Jun 2019 15:14:53 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 15DFD240005;\n\tTue, 11 Jun 2019 13:14:52 +0000 (UTC)"],"Date":"Tue, 11 Jun 2019 15:16:06 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190611131606.2v5hbzp4nf4cb5l5@uno.localdomain>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>\n\t<20190607163719.GA14958@pendragon.ideasonboard.com>\n\t<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>\n\t\n\t<20190611120716.GG5016@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"27rdlu4mfjy25c3j\"","Content-Disposition":"inline","In-Reply-To":"<20190611120716.GG5016@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 13:14:53 -0000"}},{"id":1853,"web_url":"https://patchwork.libcamera.org/comment/1853/","msgid":"<73be01e2-71f5-eac4-794e-5a4d1341849d@ideasonboard.com>","date":"2019-06-11T14:06:06","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 11/06/2019 13:07, Laurent Pinchart wrote:\n> On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote:\n>> On 10/06/2019 11:45, Jacopo Mondi wrote:\n>>> On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote:\n>>>> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n>>>>> Provide a set to contain all controls applicable to the request.\n>>>>> The set contains all controls whether they are write, read, or write-read controls.\n>>>\n>>> Not on this patch strictly, but, the 'nature' of the control (w, r,\n>>> rw) is define by how the Control is constructed, right? If it is\n>>> provided a value is a write, otherwise is a read.\n>>\n>> In it's initial construct yes, a ControlValue with no value (but given a\n>> type) must be a Read control ... and a ControlValue given a value (which\n>> infers it's type) will be a Write.\n>>\n>> I envisaged perhaps it would necessitate an extra call (not too fond of\n>> that, but still a WIP) to get a 'Write' control to also 'Read' afterwards.\n> \n> This still doesn't answer the question of whether application should\n> provide a list of controls they want to read, or if the pipeline handler\n> will just report all the information is has. I was envisioning the\n> latter, but if you think the former is better, please explain why :-)\n\nDo you expect the pipeline handler to obtain and update all control\nvalues after every capture completes?\n\nI guess this is 'cheaper' now that we have a single ioctl with extended\ncontrols.\n\nHow about any controls in the request when it's queued are there to be\nactioned (written/updated), and the pipeline handler should then update\nthose controls at the request completion (assuming that's a reasonable\naction on that control) and along with that provide any further\ncontrol/state information.\n\nSo the request completion might find that it iterates all controls it\ncan handle - and updates the control map with everything it knows about\nthat's relevant to pass back in the request.\n\n\n> Note that the data reported by the pipeline handler is more in the form\n> of metadata (data related to how the image was captured) than in the\n> form of controls (tunable that are set to influence the image). It could\n> make sense to separate them at the API level with different names to\n> make this clear, although combining them through a single API could also\n> make sense if the implementations become too similar.\n\nYes, I think things like \"What was the exposure time when this frame was\ncaptured\" is meta data that should be provided.\n\nThat begs the question of how controls interact with each other though!\n\nFor items that could be set by an IPA or controlled manually, this could\nget complex. I.e.\n\nIf an IPA sets \"Exposure\", Requests complete with the configured\n\"Exposure\" setting.\n\nIf an Application forces \"ManualExposure\" to X, and would then expect\n'Exposure' to be returned as X (at least at some point).\n\n\n(edit: Now that I've gone through this - with the concept that the\nrequest is updated at the end - this should be fine.)\n\n>>> I think controls should have a 'direction' type assigned as part of\n>>> their definition which should be checked against what the user tries\n>>> to do on it, and not only depend on how the user creates them.\n>>\n>> Ok - so, you see that as a third parameter in construction?\n>> \n>>\n>>> In example, the static controls that Laurent mentioned here below,\n>>> which are stored in the Camera, should be 'static'/'read-only'\n>>> controls, which provides to application non-modifiable\n>>> informations on the camera (orientation, position etc). Will the\n>>> 'direction' be part of the controls definition?\n>>\n>> I imagine if some controls are read only - then yes we would store that\n>> state information as part of the ControlInfo table/class?\n>>\n>>>>> Signed-off-by: Kieran Bingham \n>>>>> ---\n>>>>> include/libcamera/request.h | 4 ++++\n>>>>> 1 file changed, 4 insertions(+)\n>>>>>\n>>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>>>>> index 58de6f00a554..5fae0d5fc838 100644\n>>>>> --- a/include/libcamera/request.h\n>>>>> +++ b/include/libcamera/request.h\n>>>>> @@ -10,6 +10,7 @@\n>>>>> #include \n>>>>> #include \n>>>>>\n>>>>> +#include \n>>>>> #include \n>>>>>\n>>>>> namespace libcamera {\n>>>>> @@ -36,6 +37,8 @@ public:\n>>>>> \tint setBuffers(const std::map &streamMap);\n>>>>> \tBuffer *findBuffer(Stream *stream) const;\n>>>>>\n>>>>> +\tstd::set &controls() { return controls_; };\n>>>>\n>>>> I think we will need something a bit more complicated than a std::set.\n>>>> Here's what I was thinking about.\n>>>>\n>>>> - Let's split control information from control value. The former are\n>>>> static for the lifetime of the camera (or at least of the capture\n>>>> session), while the latter vary per-request.\n>>\n>> Ok - that was one of the directions I considered as well, and I think\n>> now that more thought has gone into things it could be a reasonable\n>> route. Or of course the class Control::id_ could be changed to a\n>> Control::info_ as a &ControlInfo type in an unordered_set.\n>>\n>>>> - The control values in the request could be stored in a\n>>>> std::map. This would make copies as\n>>>> cheap as possible as we won't have to duplicate the control\n>>>> information.\n>>\n>> I agree, the ControlInformation should not be duplicated, and the 'ID'\n>> refernce should be as cheap as possible.\n>>\n>>> Where do you envision them to live? I assume as they won't have to be\n>>> duplicated we'll maintain a table somewhere of ControlInfo each\n>>> request's control will reference to...\n>>\n>> More so than that, as some controls will be dynamic - and we must query\n>> the underlying kernel drivers for the properties (min/max/defaults).\n>>\n>> So would we do that at open? StreamOn? Some-other time?\n> \n> Will they be dynamic ? They certainly can't be hardcoded in the sourced\n> code, but I don't expect them to change for the lifetime of the camera\n> (feel free to prove me wrong). They could thus be created when creating\n> the camera.\n\nDynamic was the wrong choice of word. I meant as in - needs to be\ndetermined at runtime (creating the camera is a fine time), and can't be\nconfigured in advance in a static table or such.\n\n\n>> Perhaps it will be up to the PipelineHandler to create it's ControlInfo\n>> set and populate values appropriately. This would then let the\n>> PipelineHandler decide which controls are valid to support or not.\n> \n> I think that's what will happen, yes (with the help of the IPA).\n\n\nSo some controls are going to be specific to the IPA perhaps? Should we\nseparate those as a different set? :S\n\n\n\n>>>> - The return value of the controls() function should be a custom class\n>>>> wrapping around the std::map. This is needed to perform control\n>>>> validation for instance (min/max/step), and will likely come handy to\n>>>> implement other custom behaviours.\n>>>\n>>> Each control should be validated against it's associated ControlInfo.\n>>> Why is wrapping the container relevant if not from the API usage\n>>> easiness, maybe?\n>>\n>> Hrm... this makes me wonder if that validation should be done by the\n>> 'container of multiple controls' ... or a container of a single control.\n>>\n>> I.e. either we create a ControlList() which stores a map > ControlValue>, and it does the validation and hides the map,\n>>\n>> Or we change the control class to store \n>> and let that be stored in any container as required.\n> \n> We have started by exposing STL containers directly (for format\n> enumeration for instance), and then moved to custom classes. I think\n> custom classes give is more flexibility and allow creation of better\n> APIs for applications. Regardless of where the validation is performed,\n> I would use a custom container class to store controls in requests.\n> \n>>>> - The control information should be exposed by the camera so that\n>>>> application can enumerate supported controls and query their\n>>>> information.\n>>>\n>>> Do we expect the camera to have anything like \"readControl()\" ?\n>>> My understanding was that all interactions with control goes through\n>>> requests, so there's not actually anything like an asynchronous\n>>> read().\n>>\n>> No I don't think we'll have readControl() - but populating any relevant\n>> ControlInfo sturctures might certainly have to be done at run-time (but\n>> that's more like queryControl() - not readControl().\n> \n> Yes, that's what I meant.\n\nThen yes, we need to have a queryControl() somewhere.\n\n\n> \n>>> I wonder if\n>>> 1) getting from the camera a list of controls with associated default\n>>> values\n> \n> I don't think we'll have global defaults, we will likely need an API to\n> query defaults for a given use case, similar to\n> Camera::generateConfiguration().\n> \n>>> 2) modify the one the app is interested into ad submit a request\n>>> 3) at request complete time verify the control value has changed to\n>>> the requested value (or is converging to it) and modify it again if\n>>> required.\n>>>\n>>> Is not a better interaction model compared to the asynchronous\n>>> \"getControl/setControl\" one. We briefly discussed that, and one point\n>>> was that the delays in completing the request might delay the read of\n>>> a control. I wonder if that's relevant or if we can do anything about it\n>>\n>> No, I think a delayed request is fine. It can only be delayed due to\n>> other actions within that request, and we guarantee (I believe) that\n>> requests complete in sequential order. Thus, the control data/state in\n>> the request will match the state of that request completion. (i.e. if\n>> any other buffers were included.).\n>>\n>> In my initial work - I expected reading of controls to be the *last*\n>> thing that a request completion does. So the control state would be as\n>> recent as possible, and no further delays should be expected.\n> \n> Among all the information reported to the application through the\n> request, we will have data coming from V4L2 controls, data coming from\n> buffers (e.g. metadata sent by the sensor as embedded data), data\n> created by the IPA (e.g. the locked state of the control loops), ... All\n> these will potentially be generated at different times.\n\nIndeed. I wonder at what level we will want to timestamp this\ninformation...I guess that will appear as it's needed.\n\n>>> ie. the control value comes from a statistic value generated from\n>>> inspecting the pixel data: this should always be associated with a\n>>> completed request. This holds for other \"async\" controls as well: do we care\n>>> what is the lens exposure \"now\" or what is value was at the time the picture\n>>> has been taken?.\n>>\n>> I would say it's important to associate the control information with the\n>> other request content. I.e. - \"At the time 'this' picture was taken, the\n>> lens exposure 'was' $E, and the brightness was $B\".\n> \n> I agree, I don't think we need asynchronous reads.\n> \n>>> If we have to fast-track some controls with an immediate\n>>> Camera::readControl() I agree this might be needed for some case but\n>>> not pretty, as it offers an API easy to abuse.\n>>\n>> I don't yet know what 'fast tracking' we would have to do.\n>>\n>> If there is a control you want to read/write - without picture data, it\n>> has to go into a request of it's own (and the framework should handle\n>> that correctly, i.e. not block on waiting for any buffers to complete).\n> \n> I don't see a use-case for fast-tracking reads. Writes could be\n> different though, we will have to evaluate the use cases.\n\nWell lets handle that when it comes up. We need to get there first :-)\n\n\n>>>> - We need to think of the common use cases from an application point of\n>>>> view. In particular I think applications will need an API to get a\n>>>> default set of control values from libcamera, that they will then\n>>>> possibly modify and set for the first request. Subsequent requests\n>>>> will likely just modify a small number of controls, and will likely be\n>>>> constructed from scratch. For control that applications want to read,\n>>>> I expect most requests to contain the same controls, so there should\n>>>> be an easy and efficient way to handle that. Splitting read and write\n>>>> controls may be a good idea.\n>>\n>> Laurent: I'm a bit confused as to whether you expect a request to\n>> contain all controls and all state, or just ones that are modified...\n> \n> Just the ones that are modified, but I expect the first request to\n> typically contain all/most/many controls. The camera should provide\n> default control values for a given use case, and I think that will be\n> used to create the first request. Subsequent requests will likely\n> contain less controls.\n\n\nOk - so the camera/pipeline can provide a function which returns a\nset/list/map/container/class of all known controls and their defaults.\n\nThen it's up to the Camera/Pipeline to prepare or update those lists for\nthe use cases if necessary.\n\n\n>>> I'm not clear how do you think an interaction to read controls\n>>> asynchronously from request would look like. I'm mostly concerned\n>>\n>> Asynchronously from request ..? You mean not using a request? (I don't\n>> think we would ever read controls without it going through a request?)\n>> or do you mean handling the timeing of when a control is read during a\n>> request lifecycle.\n>>\n>>> about the fact that most (some?) parameter would depend on the\n>>> statistics generated processing the most recently captured image, and\n>>> being able to provide the most recent values would require quite some\n>>> caching somewhere. Do we want to cache -all- control values to be able\n>>> to promptly reply to a \"getControl()\" at any point in time?\n>>\n>> Hrm... some control values might be 'cached' internally, becuase they\n>> might not really be a value that is queried from hardware. (As you state\n>> below). ... but any control which is represented by hardware, I can't\n>> imagine caching it - as if you read it - you want to know - what that\n>> value really is?\n>>\n>> The IPA's might likely keep track of the values they need to monitor\n>> over a time series perhaps? But that should be in the IPA - not at a\n>> core level I don't think... \n>>\n>>> See, Kieran did a great job defining the container types and their\n>>> storage, but what I would like to happen before I can really make up\n>>> my mind is a better definition of the designed interaction model and\n>>> the definition of some Libcamera controls which are not just plain\n>>> V4L2Control which Kieran has rightfully used in this series.\n>>>\n>>> -------------------------------------------------------------------------\n>>> Let me write down how I imagine interacting with a control would look\n>>> like from an application point of view. Ignore it if it's too long :)\n>>>\n>>>\n>>> Let's use in example, the lens aperture (I don't see V4L2 control that\n>>> maps to it, I might be wrong, as it is quite a fundamental parameter).\n>>>\n>>> As an application I would expect to know:\n>>> - the type of the control value (ie float, in this case)\n>>> - if I can control it or not\n>>> - the available values I can chose from, as a list of values or a range with\n>>> min, max and a step value.\n>>>\n>>> Where should this informations be stored? In the control info? How\n>>\n>> Yes, that all sounds like ControlInfo information to me ..., which is\n>> (at least runtime) constant.\n>>\n>>> would a pipeline handler create them? For each control the camera supports\n>>> should it fill up a control info with all this informations?\n>>\n>> Yes, I think so.\n>>\n>>> So I'm now an application, I have a camera, and I would like to list\n>>> the control it supports. I should call something like controls() and\n>>> get back a map of .\n>>\n>> Perhaps, or the ControlValue &default might actually just be insside the\n>> ControlInfo &\n> \n> I think the default values will be use-case-dependent, so I believe\n> we'll need a method to retrieve information about all supported\n> controls, without a default. The default should be retrieved through a\n> separate method that will take a use case (possible a set of stream\n> roles).\n\nOk - so the ControlInfo doesn't need a default() then, as getting\ndefaults will be a per-use-case created list of ControlInfo/ControlValue\npairs.\n\nNow I understand, and I believe that's exactly what Jacopo wrote just\nabove here :)\n\n> \n>>> The control info should live somewhere, if we want to cache it to\n>>> perform validations, so a * is fine. The defaultValue is a reference or\n>>\n>> Why a pointer and not a reference to the ControlInfo? It should be\n>> guaranteed to exist... (And we should guarantee it's pointer/address\n>> should not change)\n>>\n>>> pointer to a value which could be cached as well, to perform something like\n>>> \"reset to default\".\n>>\n>> Yes, somehow we want to be able to reset controls to defaults. Perhaps\n>> individually, or in bulk, or as a complete (re)set.\n> \n> As defaults are not global, we can't have a reset method, but\n> applications can simply use the defaults they have retrieved for a given\n> use case and set them in a request.\n\nOk.\n\n> \n>>> Now I want to know\n>>> 1) Can I control aperture?\n>>> 2) which values are accepted?\n>>>\n>>> So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE\n>>> returns a valid pair from where get the associated controlInfo. This would\n>>> tell me:\n>>\n>> This is why I don't like std::map ... If you try to obtain a\n>> ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be\n>> created!\n> \n> std::map::find()\n\nOk - that's alright then :-)\n\n\n>> So an unordered_set might be more appropriate there.\n>>\n>> (Or perhaps only [] creates, and maybe .find() will be ok)\n>>\n>>> 1) The direction: can I set it, or just read it?\n>>\n>> Indeed, and should this be a bitfield, or enum like my ControlAction enum...\n>>\n>>> 2) The supported values as a list of floats, or max-min with steps\n>>> 3) its default value in the pair.second\n>>\n>> As the ControlInfo is a class, it can just as well contain a\n>> ControlValue default; imho.\n> \n> See above.\n\nAck.\n\n\n>>> Now, should I create a ControlValue to associate it with a request? So\n>>> I go, create a new instance, fill it with one value, and store it in\n>>> the request. The control value alone, or does it need the control\n>>> info? I guess the frameworks as a cache of control info, indexed by\n>>> ID, so a control value with an ID, is enough for the framework to\n>>> retrieve the ControlInfo and perform validations.\n>>\n>> Yes, you would create a ControlValue to put in the request somehow.\n>> \n>>\n>> That ControlValue should somehow be associated with a ControlInfo &\n>> reference, and it should be easy to obtain the ControlValue for\n>> LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or\n>> Map) stored in the request.\n>>\n>>> The request gets processed, and once completed, it returns with an\n>>> updated control value. I inspect it's content and I could either\n>>> delete it if I'm done, or re-use it in the next request and delete it\n>>> later once I'm done? It is responsability of the application to create\n>>> controls and delete them opportunely. To me this is acceptable.\n>>\n>> I 'think' when you get a new request, you would create a new set of\n>> controls.\n>>\n>> I don't think you'd necessarily use the same objects - but you might\n>> have a predefined list of controls that you could add to a request\n>> (which would create a copy of that list) for some known determined state\n>> that you desire.\n> \n> That's how I envision it too. When a request is dequeued and reused all\n> the controls it contains should be removed. Applications can then set\n> controls manually, or use a predefined list.\n\n\nAgreed here.\n\n> \n>>> -------------------------------------------------------------------------\n>>>\n>>> Does this match your understanding? Can you name other controls which\n>>> would require a different interaction model?\n>>\n>> Further discussion all inline, I think (/hope) we're quite aligned on\n>> most things...\n>>\n>>>>> +\n>>>>> \tStatus status() const { return status_; }\n>>>>>\n>>>>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n>>>>> @@ -52,6 +55,7 @@ private:\n>>>>> \tCamera *camera_;\n>>>>> \tstd::map bufferMap_;\n>>>>> \tstd::unordered_set pending_;\n>>>>> +\tstd::set controls_;\n>>>>>\n>>>>> \tStatus status_;\n>>>>> };\n>","headers":{"Return-Path":"","Received":["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 81CB662F77\n\tfor ;\n\tTue, 11 Jun 2019 16:06:09 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E66CDFA0;\n\tTue, 11 Jun 2019 16:06:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560261969;\n\tbh=kenmUeb9Q+faFnyTwAaemLKlDTOsXqWlOsshw4K8ZsI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=L8YNQ88uoYBmcYhfzh0Ae9+VrcDl5ikMru3LQBlmZQ1k9IHjUsTxZFU5kn62fcOSY\n\tsP31DEBaCZikdfHJR9VEv89YoRWnUV2gOT//J3gURaP6yQaml/cyvNNj33sEUxwss9\n\txai/HXz0ZZaPhvws8DCeljXvRe98d3Q/x8DmGSA8=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tLibCamera Devel ","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>\n\t<20190607163719.GA14958@pendragon.ideasonboard.com>\n\t<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>\n\t\n\t<20190611120716.GG5016@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<73be01e2-71f5-eac4-794e-5a4d1341849d@ideasonboard.com>","Date":"Tue, 11 Jun 2019 15:06:06 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190611120716.GG5016@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 14:06:09 -0000"}},{"id":1856,"web_url":"https://patchwork.libcamera.org/comment/1856/","msgid":"<20190611144557.GM5016@pendragon.ideasonboard.com>","date":"2019-06-11T14:45:57","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Jun 11, 2019 at 03:06:06PM +0100, Kieran Bingham wrote:\n> On 11/06/2019 13:07, Laurent Pinchart wrote:\n> > On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote:\n> >> On 10/06/2019 11:45, Jacopo Mondi wrote:\n> >>> On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote:\n> >>>> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote:\n> >>>>> Provide a set to contain all controls applicable to the request.\n> >>>>> The set contains all controls whether they are write, read, or write-read controls.\n> >>>\n> >>> Not on this patch strictly, but, the 'nature' of the control (w, r,\n> >>> rw) is define by how the Control is constructed, right? If it is\n> >>> provided a value is a write, otherwise is a read.\n> >>\n> >> In it's initial construct yes, a ControlValue with no value (but given a\n> >> type) must be a Read control ... and a ControlValue given a value (which\n> >> infers it's type) will be a Write.\n> >>\n> >> I envisaged perhaps it would necessitate an extra call (not too fond of\n> >> that, but still a WIP) to get a 'Write' control to also 'Read' afterwards.\n> > \n> > This still doesn't answer the question of whether application should\n> > provide a list of controls they want to read, or if the pipeline handler\n> > will just report all the information is has. I was envisioning the\n> > latter, but if you think the former is better, please explain why :-)\n> \n> Do you expect the pipeline handler to obtain and update all control\n> values after every capture completes?\n\nNot all of them. I think we should split controls and metadata, as they\nare two different things. Applications don't need to read back controls,\nthey need to know about the context in which a frame was captured. When\nmodifying the exposure time or the lens position, when using the flash\nto expose an image or when having an action on a parameter that doesn't\ntake immediate effect, it can be import to know, for each captured\nframe, what exact settings have been used. However, when setting a\ncontrol that has immediate effect (a gamma table applied by the ISP for\ninstance), there's often no need to report it back. Most of the controls\nprovided by the hardware fall in the second category and don't have to\nbe read back. Only a handful will in typical cases.\n\n> I guess this is 'cheaper' now that we have a single ioctl with extended\n> controls.\n\nI2C access is still expensive, so we should minimise the number of V4L2\nreads on the sensor for instance. However, in many cases the metadata\nare reported through faster means, for instance as embedded data on the\nCSI-2 bus.\n\n> How about any controls in the request when it's queued are there to be\n> actioned (written/updated), and the pipeline handler should then update\n> those controls at the request completion (assuming that's a reasonable\n> action on that control) and along with that provide any further\n> control/state information.\n\nI really think we should split the two. Applications set controls in\nrequests, and the pipeline handler (and IPA) apply those controls to the\nhardware. The pipeline handler then reports metadata. It may be a useful\noptimisation for applications to tell what metadata they're interested\nin, to skip slow read operations, but I expect that in most cases the\ncontrol loop implementation would need the metadata for internal\npurpose, so they will have to be read anyway.\n\n> So the request completion might find that it iterates all controls it\n> can handle - and updates the control map with everything it knows about\n> that's relevant to pass back in the request.\n> \n> > Note that the data reported by the pipeline handler is more in the form\n> > of metadata (data related to how the image was captured) than in the\n> > form of controls (tunable that are set to influence the image). It could\n> > make sense to separate them at the API level with different names to\n> > make this clear, although combining them through a single API could also\n> > make sense if the implementations become too similar.\n> \n> Yes, I think things like \"What was the exposure time when this frame was\n> captured\" is meta data that should be provided.\n> \n> That begs the question of how controls interact with each other though!\n> \n> For items that could be set by an IPA or controlled manually, this could\n> get complex. I.e.\n> \n> If an IPA sets \"Exposure\", Requests complete with the configured\n> \"Exposure\" setting.\n> \n> If an Application forces \"ManualExposure\" to X, and would then expect\n> 'Exposure' to be returned as X (at least at some point).\n\nWe have to document precisely how controls interact. That will be a big\ndocumentation effort, but it will be worth it.\n\n> (edit: Now that I've gone through this - with the concept that the\n> request is updated at the end - this should be fine.)\n> \n> >>> I think controls should have a 'direction' type assigned as part of\n> >>> their definition which should be checked against what the user tries\n> >>> to do on it, and not only depend on how the user creates them.\n> >>\n> >> Ok - so, you see that as a third parameter in construction?\n> >> \n> >>\n> >>> In example, the static controls that Laurent mentioned here below,\n> >>> which are stored in the Camera, should be 'static'/'read-only'\n> >>> controls, which provides to application non-modifiable\n> >>> informations on the camera (orientation, position etc). Will the\n> >>> 'direction' be part of the controls definition?\n> >>\n> >> I imagine if some controls are read only - then yes we would store that\n> >> state information as part of the ControlInfo table/class?\n> >>\n> >>>>> Signed-off-by: Kieran Bingham \n> >>>>> ---\n> >>>>> include/libcamera/request.h | 4 ++++\n> >>>>> 1 file changed, 4 insertions(+)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> >>>>> index 58de6f00a554..5fae0d5fc838 100644\n> >>>>> --- a/include/libcamera/request.h\n> >>>>> +++ b/include/libcamera/request.h\n> >>>>> @@ -10,6 +10,7 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>>\n> >>>>> +#include \n> >>>>> #include \n> >>>>>\n> >>>>> namespace libcamera {\n> >>>>> @@ -36,6 +37,8 @@ public:\n> >>>>> \tint setBuffers(const std::map &streamMap);\n> >>>>> \tBuffer *findBuffer(Stream *stream) const;\n> >>>>>\n> >>>>> +\tstd::set &controls() { return controls_; };\n> >>>>\n> >>>> I think we will need something a bit more complicated than a std::set.\n> >>>> Here's what I was thinking about.\n> >>>>\n> >>>> - Let's split control information from control value. The former are\n> >>>> static for the lifetime of the camera (or at least of the capture\n> >>>> session), while the latter vary per-request.\n> >>\n> >> Ok - that was one of the directions I considered as well, and I think\n> >> now that more thought has gone into things it could be a reasonable\n> >> route. Or of course the class Control::id_ could be changed to a\n> >> Control::info_ as a &ControlInfo type in an unordered_set.\n> >>\n> >>>> - The control values in the request could be stored in a\n> >>>> std::map. This would make copies as\n> >>>> cheap as possible as we won't have to duplicate the control\n> >>>> information.\n> >>\n> >> I agree, the ControlInformation should not be duplicated, and the 'ID'\n> >> refernce should be as cheap as possible.\n> >>\n> >>> Where do you envision them to live? I assume as they won't have to be\n> >>> duplicated we'll maintain a table somewhere of ControlInfo each\n> >>> request's control will reference to...\n> >>\n> >> More so than that, as some controls will be dynamic - and we must query\n> >> the underlying kernel drivers for the properties (min/max/defaults).\n> >>\n> >> So would we do that at open? StreamOn? Some-other time?\n> > \n> > Will they be dynamic ? They certainly can't be hardcoded in the sourced\n> > code, but I don't expect them to change for the lifetime of the camera\n> > (feel free to prove me wrong). They could thus be created when creating\n> > the camera.\n> \n> Dynamic was the wrong choice of word. I meant as in - needs to be\n> determined at runtime (creating the camera is a fine time), and can't be\n> configured in advance in a static table or such.\n> \n> >> Perhaps it will be up to the PipelineHandler to create it's ControlInfo\n> >> set and populate values appropriately. This would then let the\n> >> PipelineHandler decide which controls are valid to support or not.\n> > \n> > I think that's what will happen, yes (with the help of the IPA).\n> \n> So some controls are going to be specific to the IPA perhaps? Should we\n> separate those as a different set? :S\n\n>From an application point of view I wouldn't separate them.\n\n> >>>> - The return value of the controls() function should be a custom class\n> >>>> wrapping around the std::map. This is needed to perform control\n> >>>> validation for instance (min/max/step), and will likely come handy to\n> >>>> implement other custom behaviours.\n> >>>\n> >>> Each control should be validated against it's associated ControlInfo.\n> >>> Why is wrapping the container relevant if not from the API usage\n> >>> easiness, maybe?\n> >>\n> >> Hrm... this makes me wonder if that validation should be done by the\n> >> 'container of multiple controls' ... or a container of a single control.\n> >>\n> >> I.e. either we create a ControlList() which stores a map >> ControlValue>, and it does the validation and hides the map,\n> >>\n> >> Or we change the control class to store \n> >> and let that be stored in any container as required.\n> > \n> > We have started by exposing STL containers directly (for format\n> > enumeration for instance), and then moved to custom classes. I think\n> > custom classes give is more flexibility and allow creation of better\n> > APIs for applications. Regardless of where the validation is performed,\n> > I would use a custom container class to store controls in requests.\n> > \n> >>>> - The control information should be exposed by the camera so that\n> >>>> application can enumerate supported controls and query their\n> >>>> information.\n> >>>\n> >>> Do we expect the camera to have anything like \"readControl()\" ?\n> >>> My understanding was that all interactions with control goes through\n> >>> requests, so there's not actually anything like an asynchronous\n> >>> read().\n> >>\n> >> No I don't think we'll have readControl() - but populating any relevant\n> >> ControlInfo sturctures might certainly have to be done at run-time (but\n> >> that's more like queryControl() - not readControl().\n> > \n> > Yes, that's what I meant.\n> \n> Then yes, we need to have a queryControl() somewhere.\n\nIt could be a Camera::controls() method.\n\n> >>> I wonder if\n> >>> 1) getting from the camera a list of controls with associated default\n> >>> values\n> > \n> > I don't think we'll have global defaults, we will likely need an API to\n> > query defaults for a given use case, similar to\n> > Camera::generateConfiguration().\n> > \n> >>> 2) modify the one the app is interested into ad submit a request\n> >>> 3) at request complete time verify the control value has changed to\n> >>> the requested value (or is converging to it) and modify it again if\n> >>> required.\n> >>>\n> >>> Is not a better interaction model compared to the asynchronous\n> >>> \"getControl/setControl\" one. We briefly discussed that, and one point\n> >>> was that the delays in completing the request might delay the read of\n> >>> a control. I wonder if that's relevant or if we can do anything about it\n> >>\n> >> No, I think a delayed request is fine. It can only be delayed due to\n> >> other actions within that request, and we guarantee (I believe) that\n> >> requests complete in sequential order. Thus, the control data/state in\n> >> the request will match the state of that request completion. (i.e. if\n> >> any other buffers were included.).\n> >>\n> >> In my initial work - I expected reading of controls to be the *last*\n> >> thing that a request completion does. So the control state would be as\n> >> recent as possible, and no further delays should be expected.\n> > \n> > Among all the information reported to the application through the\n> > request, we will have data coming from V4L2 controls, data coming from\n> > buffers (e.g. metadata sent by the sensor as embedded data), data\n> > created by the IPA (e.g. the locked state of the control loops), ... All\n> > these will potentially be generated at different times.\n> \n> Indeed. I wonder at what level we will want to timestamp this\n> information...I guess that will appear as it's needed.\n> \n> >>> ie. the control value comes from a statistic value generated from\n> >>> inspecting the pixel data: this should always be associated with a\n> >>> completed request. This holds for other \"async\" controls as well: do we care\n> >>> what is the lens exposure \"now\" or what is value was at the time the picture\n> >>> has been taken?.\n> >>\n> >> I would say it's important to associate the control information with the\n> >> other request content. I.e. - \"At the time 'this' picture was taken, the\n> >> lens exposure 'was' $E, and the brightness was $B\".\n> > \n> > I agree, I don't think we need asynchronous reads.\n> > \n> >>> If we have to fast-track some controls with an immediate\n> >>> Camera::readControl() I agree this might be needed for some case but\n> >>> not pretty, as it offers an API easy to abuse.\n> >>\n> >> I don't yet know what 'fast tracking' we would have to do.\n> >>\n> >> If there is a control you want to read/write - without picture data, it\n> >> has to go into a request of it's own (and the framework should handle\n> >> that correctly, i.e. not block on waiting for any buffers to complete).\n> > \n> > I don't see a use-case for fast-tracking reads. Writes could be\n> > different though, we will have to evaluate the use cases.\n> \n> Well lets handle that when it comes up. We need to get there first :-)\n> \n> >>>> - We need to think of the common use cases from an application point of\n> >>>> view. In particular I think applications will need an API to get a\n> >>>> default set of control values from libcamera, that they will then\n> >>>> possibly modify and set for the first request. Subsequent requests\n> >>>> will likely just modify a small number of controls, and will likely be\n> >>>> constructed from scratch. For control that applications want to read,\n> >>>> I expect most requests to contain the same controls, so there should\n> >>>> be an easy and efficient way to handle that. Splitting read and write\n> >>>> controls may be a good idea.\n> >>\n> >> Laurent: I'm a bit confused as to whether you expect a request to\n> >> contain all controls and all state, or just ones that are modified...\n> > \n> > Just the ones that are modified, but I expect the first request to\n> > typically contain all/most/many controls. The camera should provide\n> > default control values for a given use case, and I think that will be\n> > used to create the first request. Subsequent requests will likely\n> > contain less controls.\n> \n> Ok - so the camera/pipeline can provide a function which returns a\n> set/list/map/container/class of all known controls and their defaults.\n> \n> Then it's up to the Camera/Pipeline to prepare or update those lists for\n> the use cases if necessary.\n\nI think the pipeline handler will prepare the ControlInfo when creating\nthe camera, and will provide an API to retrieve defaults for a set of\nuse cases. If we need to change the info at runtime then we'll need to\nextend the API. I hope it won't be needed though.\n\n> >>> I'm not clear how do you think an interaction to read controls\n> >>> asynchronously from request would look like. I'm mostly concerned\n> >>\n> >> Asynchronously from request ..? You mean not using a request? (I don't\n> >> think we would ever read controls without it going through a request?)\n> >> or do you mean handling the timeing of when a control is read during a\n> >> request lifecycle.\n> >>\n> >>> about the fact that most (some?) parameter would depend on the\n> >>> statistics generated processing the most recently captured image, and\n> >>> being able to provide the most recent values would require quite some\n> >>> caching somewhere. Do we want to cache -all- control values to be able\n> >>> to promptly reply to a \"getControl()\" at any point in time?\n> >>\n> >> Hrm... some control values might be 'cached' internally, becuase they\n> >> might not really be a value that is queried from hardware. (As you state\n> >> below). ... but any control which is represented by hardware, I can't\n> >> imagine caching it - as if you read it - you want to know - what that\n> >> value really is?\n> >>\n> >> The IPA's might likely keep track of the values they need to monitor\n> >> over a time series perhaps? But that should be in the IPA - not at a\n> >> core level I don't think... \n> >>\n> >>> See, Kieran did a great job defining the container types and their\n> >>> storage, but what I would like to happen before I can really make up\n> >>> my mind is a better definition of the designed interaction model and\n> >>> the definition of some Libcamera controls which are not just plain\n> >>> V4L2Control which Kieran has rightfully used in this series.\n> >>>\n> >>> -------------------------------------------------------------------------\n> >>> Let me write down how I imagine interacting with a control would look\n> >>> like from an application point of view. Ignore it if it's too long :)\n> >>>\n> >>>\n> >>> Let's use in example, the lens aperture (I don't see V4L2 control that\n> >>> maps to it, I might be wrong, as it is quite a fundamental parameter).\n> >>>\n> >>> As an application I would expect to know:\n> >>> - the type of the control value (ie float, in this case)\n> >>> - if I can control it or not\n> >>> - the available values I can chose from, as a list of values or a range with\n> >>> min, max and a step value.\n> >>>\n> >>> Where should this informations be stored? In the control info? How\n> >>\n> >> Yes, that all sounds like ControlInfo information to me ..., which is\n> >> (at least runtime) constant.\n> >>\n> >>> would a pipeline handler create them? For each control the camera supports\n> >>> should it fill up a control info with all this informations?\n> >>\n> >> Yes, I think so.\n> >>\n> >>> So I'm now an application, I have a camera, and I would like to list\n> >>> the control it supports. I should call something like controls() and\n> >>> get back a map of .\n> >>\n> >> Perhaps, or the ControlValue &default might actually just be insside the\n> >> ControlInfo &\n> > \n> > I think the default values will be use-case-dependent, so I believe\n> > we'll need a method to retrieve information about all supported\n> > controls, without a default. The default should be retrieved through a\n> > separate method that will take a use case (possible a set of stream\n> > roles).\n> \n> Ok - so the ControlInfo doesn't need a default() then, as getting\n> defaults will be a per-use-case created list of ControlInfo/ControlValue\n> pairs.\n> \n> Now I understand, and I believe that's exactly what Jacopo wrote just\n> above here :)\n> \n> >>> The control info should live somewhere, if we want to cache it to\n> >>> perform validations, so a * is fine. The defaultValue is a reference or\n> >>\n> >> Why a pointer and not a reference to the ControlInfo? It should be\n> >> guaranteed to exist... (And we should guarantee it's pointer/address\n> >> should not change)\n> >>\n> >>> pointer to a value which could be cached as well, to perform something like\n> >>> \"reset to default\".\n> >>\n> >> Yes, somehow we want to be able to reset controls to defaults. Perhaps\n> >> individually, or in bulk, or as a complete (re)set.\n> > \n> > As defaults are not global, we can't have a reset method, but\n> > applications can simply use the defaults they have retrieved for a given\n> > use case and set them in a request.\n> \n> Ok.\n> \n> >>> Now I want to know\n> >>> 1) Can I control aperture?\n> >>> 2) which values are accepted?\n> >>>\n> >>> So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE\n> >>> returns a valid pair from where get the associated controlInfo. This would\n> >>> tell me:\n> >>\n> >> This is why I don't like std::map ... If you try to obtain a\n> >> ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be\n> >> created!\n> > \n> > std::map::find()\n> \n> Ok - that's alright then :-)\n> \n> >> So an unordered_set might be more appropriate there.\n> >>\n> >> (Or perhaps only [] creates, and maybe .find() will be ok)\n> >>\n> >>> 1) The direction: can I set it, or just read it?\n> >>\n> >> Indeed, and should this be a bitfield, or enum like my ControlAction enum...\n> >>\n> >>> 2) The supported values as a list of floats, or max-min with steps\n> >>> 3) its default value in the pair.second\n> >>\n> >> As the ControlInfo is a class, it can just as well contain a\n> >> ControlValue default; imho.\n> > \n> > See above.\n> \n> Ack.\n> \n> >>> Now, should I create a ControlValue to associate it with a request? So\n> >>> I go, create a new instance, fill it with one value, and store it in\n> >>> the request. The control value alone, or does it need the control\n> >>> info? I guess the frameworks as a cache of control info, indexed by\n> >>> ID, so a control value with an ID, is enough for the framework to\n> >>> retrieve the ControlInfo and perform validations.\n> >>\n> >> Yes, you would create a ControlValue to put in the request somehow.\n> >> \n> >>\n> >> That ControlValue should somehow be associated with a ControlInfo &\n> >> reference, and it should be easy to obtain the ControlValue for\n> >> LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or\n> >> Map) stored in the request.\n> >>\n> >>> The request gets processed, and once completed, it returns with an\n> >>> updated control value. I inspect it's content and I could either\n> >>> delete it if I'm done, or re-use it in the next request and delete it\n> >>> later once I'm done? It is responsability of the application to create\n> >>> controls and delete them opportunely. To me this is acceptable.\n> >>\n> >> I 'think' when you get a new request, you would create a new set of\n> >> controls.\n> >>\n> >> I don't think you'd necessarily use the same objects - but you might\n> >> have a predefined list of controls that you could add to a request\n> >> (which would create a copy of that list) for some known determined state\n> >> that you desire.\n> > \n> > That's how I envision it too. When a request is dequeued and reused all\n> > the controls it contains should be removed. Applications can then set\n> > controls manually, or use a predefined list.\n> \n> Agreed here.\n> \n> >>> -------------------------------------------------------------------------\n> >>>\n> >>> Does this match your understanding? Can you name other controls which\n> >>> would require a different interaction model?\n> >>\n> >> Further discussion all inline, I think (/hope) we're quite aligned on\n> >> most things...\n> >>\n> >>>>> +\n> >>>>> \tStatus status() const { return status_; }\n> >>>>>\n> >>>>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> >>>>> @@ -52,6 +55,7 @@ private:\n> >>>>> \tCamera *camera_;\n> >>>>> \tstd::map bufferMap_;\n> >>>>> \tstd::unordered_set pending_;\n> >>>>> +\tstd::set controls_;\n> >>>>>\n> >>>>> \tStatus status_;\n> >>>>> };","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C7A962F78\n\tfor ;\n\tTue, 11 Jun 2019 16:46:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9E783FA0;\n\tTue, 11 Jun 2019 16:46:12 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560264372;\n\tbh=+tHadbJb/5LFBbzVBE7d5BmVII9ItNxMCBCs+sPoq4w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hz6JJYXOeF3XdKXc73EDBtMIoktR/+PbygGO/slsE2lEYlLsujp1zIw3+bS8W7H0O\n\tB1RbSJ/gEvJujJZq64c5XZ0fO1uE01ySEWxyKQ8ZUnoJQuEaqaa9+sEufpG4MzCPxA\n\tg9GyAGaDNAIgCjC3P5b28h52C+ustt0viBQmrjJ8=","Date":"Tue, 11 Jun 2019 17:45:57 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tLibCamera Devel ","Message-ID":"<20190611144557.GM5016@pendragon.ideasonboard.com>","References":"<20190606205654.9311-1-kieran.bingham@ideasonboard.com>\n\t<20190606205654.9311-3-kieran.bingham@ideasonboard.com>\n\t<20190607163719.GA14958@pendragon.ideasonboard.com>\n\t<20190610104522.ohvl46rft4xi2vbp@uno.localdomain>\n\t\n\t<20190611120716.GG5016@pendragon.ideasonboard.com>\n\t<73be01e2-71f5-eac4-794e-5a4d1341849d@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<73be01e2-71f5-eac4-794e-5a4d1341849d@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a\n\tcontrol set","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 14:46:13 -0000"}}]