| Message ID | 20190606205654.9311-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: > Provide a set to contain all controls applicable to the request. > The set contains all controls whether they are write, read, or write-read controls. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/request.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 58de6f00a554..5fae0d5fc838 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -10,6 +10,7 @@ > #include <map> > #include <unordered_set> > > +#include <libcamera/controls.h> > #include <libcamera/signal.h> > > namespace libcamera { > @@ -36,6 +37,8 @@ public: > int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > Buffer *findBuffer(Stream *stream) const; > > + std::set<Control> &controls() { return controls_; }; I think we will need something a bit more complicated than a std::set. Here's what I was thinking about. - Let's split control information from control value. The former are static for the lifetime of the camera (or at least of the capture session), while the latter vary per-request. - The control values in the request could be stored in a std::map<const ControlInfo &, ControlValue>. This would make copies as cheap as possible as we won't have to duplicate the control information. - The return value of the controls() function should be a custom class wrapping around the std::map. This is needed to perform control validation for instance (min/max/step), and will likely come handy to implement other custom behaviours. - The control information should be exposed by the camera so that application can enumerate supported controls and query their information. - We need to think of the common use cases from an application point of view. In particular I think applications will need an API to get a default set of control values from libcamera, that they will then possibly modify and set for the first request. Subsequent requests will likely just modify a small number of controls, and will likely be constructed from scratch. For control that applications want to read, I expect most requests to contain the same controls, so there should be an easy and efficient way to handle that. Splitting read and write controls may be a good idea. > + > Status status() const { return status_; } > > bool hasPendingBuffers() const { return !pending_.empty(); } > @@ -52,6 +55,7 @@ private: > Camera *camera_; > std::map<Stream *, Buffer *> bufferMap_; > std::unordered_set<Buffer *> pending_; > + std::set<Control> controls_; > > Status status_; > };
Hi Kieran, just some more thoughts on your patch and Laurent comments.. On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: > > Provide a set to contain all controls applicable to the request. > > The set contains all controls whether they are write, read, or write-read controls. Not on this patch strictly, but, the 'nature' of the control (w, r, rw) is define by how the Control is constructed, right? If it is provided a value is a write, otherwise is a read. I think controls should have a 'direction' type assigned as part of their definition which should be checked against what the user tries to do on it, and not only depend on how the user creates them. In example, the static controls that Laurent mentioned here below, which are stored in the Camera, should be 'static'/'read-only' controls, which provides to application non-modifiable informations on the camera (orientation, position etc). Will the 'direction' be part of the controls definition? > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/request.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index 58de6f00a554..5fae0d5fc838 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -10,6 +10,7 @@ > > #include <map> > > #include <unordered_set> > > > > +#include <libcamera/controls.h> > > #include <libcamera/signal.h> > > > > namespace libcamera { > > @@ -36,6 +37,8 @@ public: > > int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > > Buffer *findBuffer(Stream *stream) const; > > > > + std::set<Control> &controls() { return controls_; }; > > I think we will need something a bit more complicated than a std::set. > Here's what I was thinking about. > > - Let's split control information from control value. The former are > static for the lifetime of the camera (or at least of the capture > session), while the latter vary per-request. > > - The control values in the request could be stored in a > std::map<const ControlInfo &, ControlValue>. This would make copies as > cheap as possible as we won't have to duplicate the control > information. Where do you envision them to live? I assume as they won't have to be duplicated we'll maintain a table somewhere of ControlInfo each request's control will reference to... > > - The return value of the controls() function should be a custom class > wrapping around the std::map. This is needed to perform control > validation for instance (min/max/step), and will likely come handy to > implement other custom behaviours. Each control should be validated against it's associated ControlInfo. Why is wrapping the container relevant if not from the API usage easiness, maybe? > > - The control information should be exposed by the camera so that > application can enumerate supported controls and query their > information. > Do we expect the camera to have anything like "readControl()" ? My understanding was that all interactions with control goes through requests, so there's not actually anything like an asynchronous read(). I wonder if 1) getting from the camera a list of controls with associated default values 2) modify the one the app is interested into ad submit a request 3) at request complete time verify the control value has changed to the requested value (or is converging to it) and modify it again if required. Is not a better interaction model compared to the asynchronous "getControl/setControl" one. We briefly discussed that, and one point was that the delays in completing the request might delay the read of a control. I wonder if that's relevant or if we can do anything about it ie. the control value comes from a statistic value generated from inspecting the pixel data: this should always be associated with a completed request. This holds for other "async" controls as well: do we care what is the lens exposure "now" or what is value was at the time the picture has been taken?. If we have to fast-track some controls with an immediate Camera::readControl() I agree this might be needed for some case but not pretty, as it offers an API easy to abuse. > - We need to think of the common use cases from an application point of > view. In particular I think applications will need an API to get a > default set of control values from libcamera, that they will then > possibly modify and set for the first request. Subsequent requests > will likely just modify a small number of controls, and will likely be > constructed from scratch. For control that applications want to read, > I expect most requests to contain the same controls, so there should > be an easy and efficient way to handle that. Splitting read and write > controls may be a good idea. > I'm not clear how do you think an interaction to read controls asynchronously from request would look like. I'm mostly concerned about the fact that most (some?) parameter would depend on the statistics generated processing the most recently captured image, and being able to provide the most recent values would require quite some caching somewhere. Do we want to cache -all- control values to be able to promptly reply to a "getControl()" at any point in time? See, Kieran did a great job defining the container types and their storage, but what I would like to happen before I can really make up my mind is a better definition of the designed interaction model and the definition of some Libcamera controls which are not just plain V4L2Control which Kieran has rightfully used in this series. ------------------------------------------------------------------------- Let me write down how I imagine interacting with a control would look like from an application point of view. Ignore it if it's too long :) Let's use in example, the lens aperture (I don't see V4L2 control that maps to it, I might be wrong, as it is quite a fundamental parameter). As an application I would expect to know: - the type of the control value (ie float, in this case) - if I can control it or not - the available values I can chose from, as a list of values or a range with min, max and a step value. Where should this informations be stored? In the control info? How would a pipeline handler create them? For each control the camera supports should it fill up a control info with all this informations? So I'm now an application, I have a camera, and I would like to list the control it supports. I should call something like controls() and get back a map of <ControlInfo *, ControlValue &default>. The control info should live somewhere, if we want to cache it to perform validations, so a * is fine. The defaultValue is a reference or pointer to a value which could be cached as well, to perform something like "reset to default". Now I want to know 1) Can I control aperture? 2) which values are accepted? So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE returns a valid pair from where get the associated controlInfo. This would tell me: 1) The direction: can I set it, or just read it? 2) The supported values as a list of floats, or max-min with steps 3) its default value in the pair.second Now, should I create a ControlValue to associate it with a request? So I go, create a new instance, fill it with one value, and store it in the request. The control value alone, or does it need the control info? I guess the frameworks as a cache of control info, indexed by ID, so a control value with an ID, is enough for the framework to retrieve the ControlInfo and perform validations. The request gets processed, and once completed, it returns with an updated control value. I inspect it's content and I could either delete it if I'm done, or re-use it in the next request and delete it later once I'm done? It is responsability of the application to create controls and delete them opportunely. To me this is acceptable. ------------------------------------------------------------------------- Does this match your understanding? Can you name other controls which would require a different interaction model? Thanks j > > + > > Status status() const { return status_; } > > > > bool hasPendingBuffers() const { return !pending_.empty(); } > > @@ -52,6 +55,7 @@ private: > > Camera *camera_; > > std::map<Stream *, Buffer *> bufferMap_; > > std::unordered_set<Buffer *> pending_; > > + std::set<Control> controls_; > > > > Status status_; > > }; > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 10/06/2019 11:45, Jacopo Mondi wrote: > Hi Kieran, > just some more thoughts on your patch and Laurent comments.. > > On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote: > > >> Hi Kieran, >> >> Thank you for the patch. >> >> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: >>> Provide a set to contain all controls applicable to the request. >>> The set contains all controls whether they are write, read, or write-read controls. > > Not on this patch strictly, but, the 'nature' of the control (w, r, > rw) is define by how the Control is constructed, right? If it is > provided a value is a write, otherwise is a read. In it's initial construct yes, a ControlValue with no value (but given a type) must be a Read control ... and a ControlValue given a value (which infers it's type) will be a Write. I envisaged perhaps it would necessitate an extra call (not too fond of that, but still a WIP) to get a 'Write' control to also 'Read' afterwards. > I think controls should have a 'direction' type assigned as part of > their definition which should be checked against what the user tries > to do on it, and not only depend on how the user creates them. Ok - so, you see that as a third parameter in construction? <id, value, direction?> > In example, the static controls that Laurent mentioned here below, > which are stored in the Camera, should be 'static'/'read-only' > controls, which provides to application non-modifiable > informations on the camera (orientation, position etc). Will the > 'direction' be part of the controls definition? I imagine if some controls are read only - then yes we would store that state information as part of the ControlInfo table/class? >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> include/libcamera/request.h | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>> index 58de6f00a554..5fae0d5fc838 100644 >>> --- a/include/libcamera/request.h >>> +++ b/include/libcamera/request.h >>> @@ -10,6 +10,7 @@ >>> #include <map> >>> #include <unordered_set> >>> >>> +#include <libcamera/controls.h> >>> #include <libcamera/signal.h> >>> >>> namespace libcamera { >>> @@ -36,6 +37,8 @@ public: >>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); >>> Buffer *findBuffer(Stream *stream) const; >>> >>> + std::set<Control> &controls() { return controls_; }; >> >> I think we will need something a bit more complicated than a std::set. >> Here's what I was thinking about. >> >> - Let's split control information from control value. The former are >> static for the lifetime of the camera (or at least of the capture >> session), while the latter vary per-request. Ok - that was one of the directions I considered as well, and I think now that more thought has gone into things it could be a reasonable route. Or of course the class Control::id_ could be changed to a Control::info_ as a &ControlInfo type in an unordered_set. >> - The control values in the request could be stored in a >> std::map<const ControlInfo &, ControlValue>. This would make copies as >> cheap as possible as we won't have to duplicate the control >> information. I agree, the ControlInformation should not be duplicated, and the 'ID' refernce should be as cheap as possible. > Where do you envision them to live? I assume as they won't have to be > duplicated we'll maintain a table somewhere of ControlInfo each > request's control will reference to... More so than that, as some controls will be dynamic - and we must query the underlying kernel drivers for the properties (min/max/defaults). So would we do that at open? StreamOn? Some-other time? Perhaps it will be up to the PipelineHandler to create it's ControlInfo set and populate values appropriately. This would then let the PipelineHandler decide which controls are valid to support or not. >> - The return value of the controls() function should be a custom class >> wrapping around the std::map. This is needed to perform control >> validation for instance (min/max/step), and will likely come handy to >> implement other custom behaviours. > > Each control should be validated against it's associated ControlInfo. > Why is wrapping the container relevant if not from the API usage > easiness, maybe? Hrm... this makes me wonder if that validation should be done by the 'container of multiple controls' ... or a container of a single control. I.e. either we create a ControlList() which stores a map <ControlInfo, ControlValue>, and it does the validation and hides the map, Or we change the control class to store <ControlInfo &, ControlValue> and let that be stored in any container as required. >> >> - The control information should be exposed by the camera so that >> application can enumerate supported controls and query their >> information. >> > > Do we expect the camera to have anything like "readControl()" ? > My understanding was that all interactions with control goes through > requests, so there's not actually anything like an asynchronous > read(). No I don't think we'll have readControl() - but populating any relevant ControlInfo sturctures might certainly have to be done at run-time (but that's more like queryControl() - not readControl(). > I wonder if > 1) getting from the camera a list of controls with associated default > values > 2) modify the one the app is interested into ad submit a request > 3) at request complete time verify the control value has changed to > the requested value (or is converging to it) and modify it again if > required. > > Is not a better interaction model compared to the asynchronous > "getControl/setControl" one. We briefly discussed that, and one point > was that the delays in completing the request might delay the read of > a control. I wonder if that's relevant or if we can do anything about it No, I think a delayed request is fine. It can only be delayed due to other actions within that request, and we guarantee (I believe) that requests complete in sequential order. Thus, the control data/state in the request will match the state of that request completion. (i.e. if any other buffers were included.). In my initial work - I expected reading of controls to be the *last* thing that a request completion does. So the control state would be as recent as possible, and no further delays should be expected. > ie. the control value comes from a statistic value generated from > inspecting the pixel data: this should always be associated with a > completed request. This holds for other "async" controls as well: do we care > what is the lens exposure "now" or what is value was at the time the picture > has been taken?. I would say it's important to associate the control information with the other request content. I.e. - "At the time 'this' picture was taken, the lens exposure 'was' $E, and the brightness was $B". > If we have to fast-track some controls with an immediate > Camera::readControl() I agree this might be needed for some case but > not pretty, as it offers an API easy to abuse. I don't yet know what 'fast tracking' we would have to do. If there is a control you want to read/write - without picture data, it has to go into a request of it's own (and the framework should handle that correctly, i.e. not block on waiting for any buffers to complete). >> - We need to think of the common use cases from an application point of >> view. In particular I think applications will need an API to get a >> default set of control values from libcamera, that they will then >> possibly modify and set for the first request. Subsequent requests >> will likely just modify a small number of controls, and will likely be >> constructed from scratch. For control that applications want to read, >> I expect most requests to contain the same controls, so there should >> be an easy and efficient way to handle that. Splitting read and write >> controls may be a good idea. Laurent: I'm a bit confused as to whether you expect a request to contain all controls and all state, or just ones that are modified... > I'm not clear how do you think an interaction to read controls > asynchronously from request would look like. I'm mostly concerned Asynchronously from request ..? You mean not using a request? (I don't think we would ever read controls without it going through a request?) or do you mean handling the timeing of when a control is read during a request lifecycle. > about the fact that most (some?) parameter would depend on the > statistics generated processing the most recently captured image, and > being able to provide the most recent values would require quite some > caching somewhere. Do we want to cache -all- control values to be able > to promptly reply to a "getControl()" at any point in time? Hrm... some control values might be 'cached' internally, becuase they might not really be a value that is queried from hardware. (As you state below). ... but any control which is represented by hardware, I can't imagine caching it - as if you read it - you want to know - what that value really is? The IPA's might likely keep track of the values they need to monitor over a time series perhaps? But that should be in the IPA - not at a core level I don't think... <I guess we'll see soon> > See, Kieran did a great job defining the container types and their > storage, but what I would like to happen before I can really make up > my mind is a better definition of the designed interaction model and > the definition of some Libcamera controls which are not just plain > V4L2Control which Kieran has rightfully used in this series. > > ------------------------------------------------------------------------- > Let me write down how I imagine interacting with a control would look > like from an application point of view. Ignore it if it's too long :) > > > Let's use in example, the lens aperture (I don't see V4L2 control that > maps to it, I might be wrong, as it is quite a fundamental parameter). > > As an application I would expect to know: > - the type of the control value (ie float, in this case) > - if I can control it or not > - the available values I can chose from, as a list of values or a range with > min, max and a step value. > > Where should this informations be stored? In the control info? How Yes, that all sounds like ControlInfo information to me ..., which is (at least runtime) constant. > would a pipeline handler create them? For each control the camera supports > should it fill up a control info with all this informations? Yes, I think so. > So I'm now an application, I have a camera, and I would like to list > the control it supports. I should call something like controls() and > get back a map of <ControlInfo *, ControlValue &default>. Perhaps, or the ControlValue &default might actually just be insside the ControlInfo & > The control info should live somewhere, if we want to cache it to > perform validations, so a * is fine. The defaultValue is a reference or Why a pointer and not a reference to the ControlInfo? It should be guaranteed to exist... (And we should guarantee it's pointer/address should not change) > pointer to a value which could be cached as well, to perform something like > "reset to default". Yes, somehow we want to be able to reset controls to defaults. Perhaps individually, or in bulk, or as a complete (re)set. > Now I want to know > 1) Can I control aperture? > 2) which values are accepted? > > So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE > returns a valid pair from where get the associated controlInfo. This would > tell me: This is why I don't like std::map ... If you try to obtain a ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be created! So an unordered_set might be more appropriate there. (Or perhaps only [] creates, and maybe .find() will be ok) > 1) The direction: can I set it, or just read it? Indeed, and should this be a bitfield, or enum like my ControlAction enum... > 2) The supported values as a list of floats, or max-min with steps > 3) its default value in the pair.second As the ControlInfo is a class, it can just as well contain a ControlValue default; imho. > Now, should I create a ControlValue to associate it with a request? So > I go, create a new instance, fill it with one value, and store it in > the request. The control value alone, or does it need the control > info? I guess the frameworks as a cache of control info, indexed by > ID, so a control value with an ID, is enough for the framework to > retrieve the ControlInfo and perform validations. Yes, you would create a ControlValue to put in the request somehow. <either in a map, or an unordered_set, or a not-yet-defined ControlList> That ControlValue should somehow be associated with a ControlInfo & reference, and it should be easy to obtain the ControlValue for LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or Map) stored in the request. > The request gets processed, and once completed, it returns with an > updated control value. I inspect it's content and I could either > delete it if I'm done, or re-use it in the next request and delete it > later once I'm done? It is responsability of the application to create > controls and delete them opportunely. To me this is acceptable. I 'think' when you get a new request, you would create a new set of controls. I don't think you'd necessarily use the same objects - but you might have a predefined list of controls that you could add to a request (which would create a copy of that list) for some known determined state that you desire. > > ------------------------------------------------------------------------- > > Does this match your understanding? Can you name other controls which > would require a different interaction model? Further discussion all inline, I think (/hope) we're quite aligned on most things... > > Thanks > j > > >>> + >>> Status status() const { return status_; } >>> >>> bool hasPendingBuffers() const { return !pending_.empty(); } >>> @@ -52,6 +55,7 @@ private: >>> Camera *camera_; >>> std::map<Stream *, Buffer *> bufferMap_; >>> std::unordered_set<Buffer *> pending_; >>> + std::set<Control> controls_; >>> >>> Status status_; >>> }; >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote: > On 10/06/2019 11:45, Jacopo Mondi wrote: > > On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote: > >> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: > >>> Provide a set to contain all controls applicable to the request. > >>> The set contains all controls whether they are write, read, or write-read controls. > > > > Not on this patch strictly, but, the 'nature' of the control (w, r, > > rw) is define by how the Control is constructed, right? If it is > > provided a value is a write, otherwise is a read. > > In it's initial construct yes, a ControlValue with no value (but given a > type) must be a Read control ... and a ControlValue given a value (which > infers it's type) will be a Write. > > I envisaged perhaps it would necessitate an extra call (not too fond of > that, but still a WIP) to get a 'Write' control to also 'Read' afterwards. This still doesn't answer the question of whether application should provide a list of controls they want to read, or if the pipeline handler will just report all the information is has. I was envisioning the latter, but if you think the former is better, please explain why :-) Note that the data reported by the pipeline handler is more in the form of metadata (data related to how the image was captured) than in the form of controls (tunable that are set to influence the image). It could make sense to separate them at the API level with different names to make this clear, although combining them through a single API could also make sense if the implementations become too similar. > > I think controls should have a 'direction' type assigned as part of > > their definition which should be checked against what the user tries > > to do on it, and not only depend on how the user creates them. > > Ok - so, you see that as a third parameter in construction? > <id, value, direction?> > > > In example, the static controls that Laurent mentioned here below, > > which are stored in the Camera, should be 'static'/'read-only' > > controls, which provides to application non-modifiable > > informations on the camera (orientation, position etc). Will the > > 'direction' be part of the controls definition? > > I imagine if some controls are read only - then yes we would store that > state information as part of the ControlInfo table/class? > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> include/libcamera/request.h | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > >>> index 58de6f00a554..5fae0d5fc838 100644 > >>> --- a/include/libcamera/request.h > >>> +++ b/include/libcamera/request.h > >>> @@ -10,6 +10,7 @@ > >>> #include <map> > >>> #include <unordered_set> > >>> > >>> +#include <libcamera/controls.h> > >>> #include <libcamera/signal.h> > >>> > >>> namespace libcamera { > >>> @@ -36,6 +37,8 @@ public: > >>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > >>> Buffer *findBuffer(Stream *stream) const; > >>> > >>> + std::set<Control> &controls() { return controls_; }; > >> > >> I think we will need something a bit more complicated than a std::set. > >> Here's what I was thinking about. > >> > >> - Let's split control information from control value. The former are > >> static for the lifetime of the camera (or at least of the capture > >> session), while the latter vary per-request. > > Ok - that was one of the directions I considered as well, and I think > now that more thought has gone into things it could be a reasonable > route. Or of course the class Control::id_ could be changed to a > Control::info_ as a &ControlInfo type in an unordered_set. > > >> - The control values in the request could be stored in a > >> std::map<const ControlInfo &, ControlValue>. This would make copies as > >> cheap as possible as we won't have to duplicate the control > >> information. > > I agree, the ControlInformation should not be duplicated, and the 'ID' > refernce should be as cheap as possible. > > > Where do you envision them to live? I assume as they won't have to be > > duplicated we'll maintain a table somewhere of ControlInfo each > > request's control will reference to... > > More so than that, as some controls will be dynamic - and we must query > the underlying kernel drivers for the properties (min/max/defaults). > > So would we do that at open? StreamOn? Some-other time? Will they be dynamic ? They certainly can't be hardcoded in the sourced code, but I don't expect them to change for the lifetime of the camera (feel free to prove me wrong). They could thus be created when creating the camera. > Perhaps it will be up to the PipelineHandler to create it's ControlInfo > set and populate values appropriately. This would then let the > PipelineHandler decide which controls are valid to support or not. I think that's what will happen, yes (with the help of the IPA). > >> - The return value of the controls() function should be a custom class > >> wrapping around the std::map. This is needed to perform control > >> validation for instance (min/max/step), and will likely come handy to > >> implement other custom behaviours. > > > > Each control should be validated against it's associated ControlInfo. > > Why is wrapping the container relevant if not from the API usage > > easiness, maybe? > > Hrm... this makes me wonder if that validation should be done by the > 'container of multiple controls' ... or a container of a single control. > > I.e. either we create a ControlList() which stores a map <ControlInfo, > ControlValue>, and it does the validation and hides the map, > > Or we change the control class to store <ControlInfo &, ControlValue> > and let that be stored in any container as required. We have started by exposing STL containers directly (for format enumeration for instance), and then moved to custom classes. I think custom classes give is more flexibility and allow creation of better APIs for applications. Regardless of where the validation is performed, I would use a custom container class to store controls in requests. > >> - The control information should be exposed by the camera so that > >> application can enumerate supported controls and query their > >> information. > > > > Do we expect the camera to have anything like "readControl()" ? > > My understanding was that all interactions with control goes through > > requests, so there's not actually anything like an asynchronous > > read(). > > No I don't think we'll have readControl() - but populating any relevant > ControlInfo sturctures might certainly have to be done at run-time (but > that's more like queryControl() - not readControl(). Yes, that's what I meant. > > I wonder if > > 1) getting from the camera a list of controls with associated default > > values I don't think we'll have global defaults, we will likely need an API to query defaults for a given use case, similar to Camera::generateConfiguration(). > > 2) modify the one the app is interested into ad submit a request > > 3) at request complete time verify the control value has changed to > > the requested value (or is converging to it) and modify it again if > > required. > > > > Is not a better interaction model compared to the asynchronous > > "getControl/setControl" one. We briefly discussed that, and one point > > was that the delays in completing the request might delay the read of > > a control. I wonder if that's relevant or if we can do anything about it > > No, I think a delayed request is fine. It can only be delayed due to > other actions within that request, and we guarantee (I believe) that > requests complete in sequential order. Thus, the control data/state in > the request will match the state of that request completion. (i.e. if > any other buffers were included.). > > In my initial work - I expected reading of controls to be the *last* > thing that a request completion does. So the control state would be as > recent as possible, and no further delays should be expected. Among all the information reported to the application through the request, we will have data coming from V4L2 controls, data coming from buffers (e.g. metadata sent by the sensor as embedded data), data created by the IPA (e.g. the locked state of the control loops), ... All these will potentially be generated at different times. > > ie. the control value comes from a statistic value generated from > > inspecting the pixel data: this should always be associated with a > > completed request. This holds for other "async" controls as well: do we care > > what is the lens exposure "now" or what is value was at the time the picture > > has been taken?. > > I would say it's important to associate the control information with the > other request content. I.e. - "At the time 'this' picture was taken, the > lens exposure 'was' $E, and the brightness was $B". I agree, I don't think we need asynchronous reads. > > If we have to fast-track some controls with an immediate > > Camera::readControl() I agree this might be needed for some case but > > not pretty, as it offers an API easy to abuse. > > I don't yet know what 'fast tracking' we would have to do. > > If there is a control you want to read/write - without picture data, it > has to go into a request of it's own (and the framework should handle > that correctly, i.e. not block on waiting for any buffers to complete). I don't see a use-case for fast-tracking reads. Writes could be different though, we will have to evaluate the use cases. > >> - We need to think of the common use cases from an application point of > >> view. In particular I think applications will need an API to get a > >> default set of control values from libcamera, that they will then > >> possibly modify and set for the first request. Subsequent requests > >> will likely just modify a small number of controls, and will likely be > >> constructed from scratch. For control that applications want to read, > >> I expect most requests to contain the same controls, so there should > >> be an easy and efficient way to handle that. Splitting read and write > >> controls may be a good idea. > > Laurent: I'm a bit confused as to whether you expect a request to > contain all controls and all state, or just ones that are modified... Just the ones that are modified, but I expect the first request to typically contain all/most/many controls. The camera should provide default control values for a given use case, and I think that will be used to create the first request. Subsequent requests will likely contain less controls. > > I'm not clear how do you think an interaction to read controls > > asynchronously from request would look like. I'm mostly concerned > > Asynchronously from request ..? You mean not using a request? (I don't > think we would ever read controls without it going through a request?) > or do you mean handling the timeing of when a control is read during a > request lifecycle. > > > about the fact that most (some?) parameter would depend on the > > statistics generated processing the most recently captured image, and > > being able to provide the most recent values would require quite some > > caching somewhere. Do we want to cache -all- control values to be able > > to promptly reply to a "getControl()" at any point in time? > > Hrm... some control values might be 'cached' internally, becuase they > might not really be a value that is queried from hardware. (As you state > below). ... but any control which is represented by hardware, I can't > imagine caching it - as if you read it - you want to know - what that > value really is? > > The IPA's might likely keep track of the values they need to monitor > over a time series perhaps? But that should be in the IPA - not at a > core level I don't think... <I guess we'll see soon> > > > See, Kieran did a great job defining the container types and their > > storage, but what I would like to happen before I can really make up > > my mind is a better definition of the designed interaction model and > > the definition of some Libcamera controls which are not just plain > > V4L2Control which Kieran has rightfully used in this series. > > > > ------------------------------------------------------------------------- > > Let me write down how I imagine interacting with a control would look > > like from an application point of view. Ignore it if it's too long :) > > > > > > Let's use in example, the lens aperture (I don't see V4L2 control that > > maps to it, I might be wrong, as it is quite a fundamental parameter). > > > > As an application I would expect to know: > > - the type of the control value (ie float, in this case) > > - if I can control it or not > > - the available values I can chose from, as a list of values or a range with > > min, max and a step value. > > > > Where should this informations be stored? In the control info? How > > Yes, that all sounds like ControlInfo information to me ..., which is > (at least runtime) constant. > > > would a pipeline handler create them? For each control the camera supports > > should it fill up a control info with all this informations? > > Yes, I think so. > > > So I'm now an application, I have a camera, and I would like to list > > the control it supports. I should call something like controls() and > > get back a map of <ControlInfo *, ControlValue &default>. > > Perhaps, or the ControlValue &default might actually just be insside the > ControlInfo & I think the default values will be use-case-dependent, so I believe we'll need a method to retrieve information about all supported controls, without a default. The default should be retrieved through a separate method that will take a use case (possible a set of stream roles). > > The control info should live somewhere, if we want to cache it to > > perform validations, so a * is fine. The defaultValue is a reference or > > Why a pointer and not a reference to the ControlInfo? It should be > guaranteed to exist... (And we should guarantee it's pointer/address > should not change) > > > pointer to a value which could be cached as well, to perform something like > > "reset to default". > > Yes, somehow we want to be able to reset controls to defaults. Perhaps > individually, or in bulk, or as a complete (re)set. As defaults are not global, we can't have a reset method, but applications can simply use the defaults they have retrieved for a given use case and set them in a request. > > Now I want to know > > 1) Can I control aperture? > > 2) which values are accepted? > > > > So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE > > returns a valid pair from where get the associated controlInfo. This would > > tell me: > > This is why I don't like std::map ... If you try to obtain a > ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be > created! std::map::find() > So an unordered_set might be more appropriate there. > > (Or perhaps only [] creates, and maybe .find() will be ok) > > > 1) The direction: can I set it, or just read it? > > Indeed, and should this be a bitfield, or enum like my ControlAction enum... > > > 2) The supported values as a list of floats, or max-min with steps > > 3) its default value in the pair.second > > As the ControlInfo is a class, it can just as well contain a > ControlValue default; imho. See above. > > Now, should I create a ControlValue to associate it with a request? So > > I go, create a new instance, fill it with one value, and store it in > > the request. The control value alone, or does it need the control > > info? I guess the frameworks as a cache of control info, indexed by > > ID, so a control value with an ID, is enough for the framework to > > retrieve the ControlInfo and perform validations. > > Yes, you would create a ControlValue to put in the request somehow. > <either in a map, or an unordered_set, or a not-yet-defined ControlList> > > That ControlValue should somehow be associated with a ControlInfo & > reference, and it should be easy to obtain the ControlValue for > LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or > Map) stored in the request. > > > The request gets processed, and once completed, it returns with an > > updated control value. I inspect it's content and I could either > > delete it if I'm done, or re-use it in the next request and delete it > > later once I'm done? It is responsability of the application to create > > controls and delete them opportunely. To me this is acceptable. > > I 'think' when you get a new request, you would create a new set of > controls. > > I don't think you'd necessarily use the same objects - but you might > have a predefined list of controls that you could add to a request > (which would create a copy of that list) for some known determined state > that you desire. That's how I envision it too. When a request is dequeued and reused all the controls it contains should be removed. Applications can then set controls manually, or use a predefined list. > > ------------------------------------------------------------------------- > > > > Does this match your understanding? Can you name other controls which > > would require a different interaction model? > > Further discussion all inline, I think (/hope) we're quite aligned on > most things... > > >>> + > >>> Status status() const { return status_; } > >>> > >>> bool hasPendingBuffers() const { return !pending_.empty(); } > >>> @@ -52,6 +55,7 @@ private: > >>> Camera *camera_; > >>> std::map<Stream *, Buffer *> bufferMap_; > >>> std::unordered_set<Buffer *> pending_; > >>> + std::set<Control> controls_; > >>> > >>> Status status_; > >>> };
Hi Laurent, Kieran, On Tue, Jun 11, 2019 at 03:07:16PM +0300, Laurent Pinchart wrote: Thank you both for your replies. Now it's all much clear to me (let's for how long). Thanks j > On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote: > > On 10/06/2019 11:45, Jacopo Mondi wrote: > > > On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote: > > >> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: > > >>> Provide a set to contain all controls applicable to the request. > > >>> The set contains all controls whether they are write, read, or write-read controls. > > > > > > Not on this patch strictly, but, the 'nature' of the control (w, r, > > > rw) is define by how the Control is constructed, right? If it is > > > provided a value is a write, otherwise is a read. > > > > In it's initial construct yes, a ControlValue with no value (but given a > > type) must be a Read control ... and a ControlValue given a value (which > > infers it's type) will be a Write. > > > > I envisaged perhaps it would necessitate an extra call (not too fond of > > that, but still a WIP) to get a 'Write' control to also 'Read' afterwards. > > This still doesn't answer the question of whether application should > provide a list of controls they want to read, or if the pipeline handler > will just report all the information is has. I was envisioning the > latter, but if you think the former is better, please explain why :-) > > Note that the data reported by the pipeline handler is more in the form > of metadata (data related to how the image was captured) than in the > form of controls (tunable that are set to influence the image). It could > make sense to separate them at the API level with different names to > make this clear, although combining them through a single API could also > make sense if the implementations become too similar. > > > > I think controls should have a 'direction' type assigned as part of > > > their definition which should be checked against what the user tries > > > to do on it, and not only depend on how the user creates them. > > > > Ok - so, you see that as a third parameter in construction? > > <id, value, direction?> > > > > > In example, the static controls that Laurent mentioned here below, > > > which are stored in the Camera, should be 'static'/'read-only' > > > controls, which provides to application non-modifiable > > > informations on the camera (orientation, position etc). Will the > > > 'direction' be part of the controls definition? > > > > I imagine if some controls are read only - then yes we would store that > > state information as part of the ControlInfo table/class? > > > > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>> --- > > >>> include/libcamera/request.h | 4 ++++ > > >>> 1 file changed, 4 insertions(+) > > >>> > > >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > >>> index 58de6f00a554..5fae0d5fc838 100644 > > >>> --- a/include/libcamera/request.h > > >>> +++ b/include/libcamera/request.h > > >>> @@ -10,6 +10,7 @@ > > >>> #include <map> > > >>> #include <unordered_set> > > >>> > > >>> +#include <libcamera/controls.h> > > >>> #include <libcamera/signal.h> > > >>> > > >>> namespace libcamera { > > >>> @@ -36,6 +37,8 @@ public: > > >>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > > >>> Buffer *findBuffer(Stream *stream) const; > > >>> > > >>> + std::set<Control> &controls() { return controls_; }; > > >> > > >> I think we will need something a bit more complicated than a std::set. > > >> Here's what I was thinking about. > > >> > > >> - Let's split control information from control value. The former are > > >> static for the lifetime of the camera (or at least of the capture > > >> session), while the latter vary per-request. > > > > Ok - that was one of the directions I considered as well, and I think > > now that more thought has gone into things it could be a reasonable > > route. Or of course the class Control::id_ could be changed to a > > Control::info_ as a &ControlInfo type in an unordered_set. > > > > >> - The control values in the request could be stored in a > > >> std::map<const ControlInfo &, ControlValue>. This would make copies as > > >> cheap as possible as we won't have to duplicate the control > > >> information. > > > > I agree, the ControlInformation should not be duplicated, and the 'ID' > > refernce should be as cheap as possible. > > > > > Where do you envision them to live? I assume as they won't have to be > > > duplicated we'll maintain a table somewhere of ControlInfo each > > > request's control will reference to... > > > > More so than that, as some controls will be dynamic - and we must query > > the underlying kernel drivers for the properties (min/max/defaults). > > > > So would we do that at open? StreamOn? Some-other time? > > Will they be dynamic ? They certainly can't be hardcoded in the sourced > code, but I don't expect them to change for the lifetime of the camera > (feel free to prove me wrong). They could thus be created when creating > the camera. > > > Perhaps it will be up to the PipelineHandler to create it's ControlInfo > > set and populate values appropriately. This would then let the > > PipelineHandler decide which controls are valid to support or not. > > I think that's what will happen, yes (with the help of the IPA). > > > >> - The return value of the controls() function should be a custom class > > >> wrapping around the std::map. This is needed to perform control > > >> validation for instance (min/max/step), and will likely come handy to > > >> implement other custom behaviours. > > > > > > Each control should be validated against it's associated ControlInfo. > > > Why is wrapping the container relevant if not from the API usage > > > easiness, maybe? > > > > Hrm... this makes me wonder if that validation should be done by the > > 'container of multiple controls' ... or a container of a single control. > > > > I.e. either we create a ControlList() which stores a map <ControlInfo, > > ControlValue>, and it does the validation and hides the map, > > > > Or we change the control class to store <ControlInfo &, ControlValue> > > and let that be stored in any container as required. > > We have started by exposing STL containers directly (for format > enumeration for instance), and then moved to custom classes. I think > custom classes give is more flexibility and allow creation of better > APIs for applications. Regardless of where the validation is performed, > I would use a custom container class to store controls in requests. > > > >> - The control information should be exposed by the camera so that > > >> application can enumerate supported controls and query their > > >> information. > > > > > > Do we expect the camera to have anything like "readControl()" ? > > > My understanding was that all interactions with control goes through > > > requests, so there's not actually anything like an asynchronous > > > read(). > > > > No I don't think we'll have readControl() - but populating any relevant > > ControlInfo sturctures might certainly have to be done at run-time (but > > that's more like queryControl() - not readControl(). > > Yes, that's what I meant. > > > > I wonder if > > > 1) getting from the camera a list of controls with associated default > > > values > > I don't think we'll have global defaults, we will likely need an API to > query defaults for a given use case, similar to > Camera::generateConfiguration(). > > > > 2) modify the one the app is interested into ad submit a request > > > 3) at request complete time verify the control value has changed to > > > the requested value (or is converging to it) and modify it again if > > > required. > > > > > > Is not a better interaction model compared to the asynchronous > > > "getControl/setControl" one. We briefly discussed that, and one point > > > was that the delays in completing the request might delay the read of > > > a control. I wonder if that's relevant or if we can do anything about it > > > > No, I think a delayed request is fine. It can only be delayed due to > > other actions within that request, and we guarantee (I believe) that > > requests complete in sequential order. Thus, the control data/state in > > the request will match the state of that request completion. (i.e. if > > any other buffers were included.). > > > > In my initial work - I expected reading of controls to be the *last* > > thing that a request completion does. So the control state would be as > > recent as possible, and no further delays should be expected. > > Among all the information reported to the application through the > request, we will have data coming from V4L2 controls, data coming from > buffers (e.g. metadata sent by the sensor as embedded data), data > created by the IPA (e.g. the locked state of the control loops), ... All > these will potentially be generated at different times. > > > > ie. the control value comes from a statistic value generated from > > > inspecting the pixel data: this should always be associated with a > > > completed request. This holds for other "async" controls as well: do we care > > > what is the lens exposure "now" or what is value was at the time the picture > > > has been taken?. > > > > I would say it's important to associate the control information with the > > other request content. I.e. - "At the time 'this' picture was taken, the > > lens exposure 'was' $E, and the brightness was $B". > > I agree, I don't think we need asynchronous reads. > > > > If we have to fast-track some controls with an immediate > > > Camera::readControl() I agree this might be needed for some case but > > > not pretty, as it offers an API easy to abuse. > > > > I don't yet know what 'fast tracking' we would have to do. > > > > If there is a control you want to read/write - without picture data, it > > has to go into a request of it's own (and the framework should handle > > that correctly, i.e. not block on waiting for any buffers to complete). > > I don't see a use-case for fast-tracking reads. Writes could be > different though, we will have to evaluate the use cases. > > > >> - We need to think of the common use cases from an application point of > > >> view. In particular I think applications will need an API to get a > > >> default set of control values from libcamera, that they will then > > >> possibly modify and set for the first request. Subsequent requests > > >> will likely just modify a small number of controls, and will likely be > > >> constructed from scratch. For control that applications want to read, > > >> I expect most requests to contain the same controls, so there should > > >> be an easy and efficient way to handle that. Splitting read and write > > >> controls may be a good idea. > > > > Laurent: I'm a bit confused as to whether you expect a request to > > contain all controls and all state, or just ones that are modified... > > Just the ones that are modified, but I expect the first request to > typically contain all/most/many controls. The camera should provide > default control values for a given use case, and I think that will be > used to create the first request. Subsequent requests will likely > contain less controls. > > > > I'm not clear how do you think an interaction to read controls > > > asynchronously from request would look like. I'm mostly concerned > > > > Asynchronously from request ..? You mean not using a request? (I don't > > think we would ever read controls without it going through a request?) > > or do you mean handling the timeing of when a control is read during a > > request lifecycle. > > > > > about the fact that most (some?) parameter would depend on the > > > statistics generated processing the most recently captured image, and > > > being able to provide the most recent values would require quite some > > > caching somewhere. Do we want to cache -all- control values to be able > > > to promptly reply to a "getControl()" at any point in time? > > > > Hrm... some control values might be 'cached' internally, becuase they > > might not really be a value that is queried from hardware. (As you state > > below). ... but any control which is represented by hardware, I can't > > imagine caching it - as if you read it - you want to know - what that > > value really is? > > > > The IPA's might likely keep track of the values they need to monitor > > over a time series perhaps? But that should be in the IPA - not at a > > core level I don't think... <I guess we'll see soon> > > > > > See, Kieran did a great job defining the container types and their > > > storage, but what I would like to happen before I can really make up > > > my mind is a better definition of the designed interaction model and > > > the definition of some Libcamera controls which are not just plain > > > V4L2Control which Kieran has rightfully used in this series. > > > > > > ------------------------------------------------------------------------- > > > Let me write down how I imagine interacting with a control would look > > > like from an application point of view. Ignore it if it's too long :) > > > > > > > > > Let's use in example, the lens aperture (I don't see V4L2 control that > > > maps to it, I might be wrong, as it is quite a fundamental parameter). > > > > > > As an application I would expect to know: > > > - the type of the control value (ie float, in this case) > > > - if I can control it or not > > > - the available values I can chose from, as a list of values or a range with > > > min, max and a step value. > > > > > > Where should this informations be stored? In the control info? How > > > > Yes, that all sounds like ControlInfo information to me ..., which is > > (at least runtime) constant. > > > > > would a pipeline handler create them? For each control the camera supports > > > should it fill up a control info with all this informations? > > > > Yes, I think so. > > > > > So I'm now an application, I have a camera, and I would like to list > > > the control it supports. I should call something like controls() and > > > get back a map of <ControlInfo *, ControlValue &default>. > > > > Perhaps, or the ControlValue &default might actually just be insside the > > ControlInfo & > > I think the default values will be use-case-dependent, so I believe > we'll need a method to retrieve information about all supported > controls, without a default. The default should be retrieved through a > separate method that will take a use case (possible a set of stream > roles). > > > > The control info should live somewhere, if we want to cache it to > > > perform validations, so a * is fine. The defaultValue is a reference or > > > > Why a pointer and not a reference to the ControlInfo? It should be > > guaranteed to exist... (And we should guarantee it's pointer/address > > should not change) > > > > > pointer to a value which could be cached as well, to perform something like > > > "reset to default". > > > > Yes, somehow we want to be able to reset controls to defaults. Perhaps > > individually, or in bulk, or as a complete (re)set. > > As defaults are not global, we can't have a reset method, but > applications can simply use the defaults they have retrieved for a given > use case and set them in a request. > > > > Now I want to know > > > 1) Can I control aperture? > > > 2) which values are accepted? > > > > > > So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE > > > returns a valid pair from where get the associated controlInfo. This would > > > tell me: > > > > This is why I don't like std::map ... If you try to obtain a > > ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be > > created! > > std::map::find() > > > So an unordered_set might be more appropriate there. > > > > (Or perhaps only [] creates, and maybe .find() will be ok) > > > > > 1) The direction: can I set it, or just read it? > > > > Indeed, and should this be a bitfield, or enum like my ControlAction enum... > > > > > 2) The supported values as a list of floats, or max-min with steps > > > 3) its default value in the pair.second > > > > As the ControlInfo is a class, it can just as well contain a > > ControlValue default; imho. > > See above. > > > > Now, should I create a ControlValue to associate it with a request? So > > > I go, create a new instance, fill it with one value, and store it in > > > the request. The control value alone, or does it need the control > > > info? I guess the frameworks as a cache of control info, indexed by > > > ID, so a control value with an ID, is enough for the framework to > > > retrieve the ControlInfo and perform validations. > > > > Yes, you would create a ControlValue to put in the request somehow. > > <either in a map, or an unordered_set, or a not-yet-defined ControlList> > > > > That ControlValue should somehow be associated with a ControlInfo & > > reference, and it should be easy to obtain the ControlValue for > > LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or > > Map) stored in the request. > > > > > The request gets processed, and once completed, it returns with an > > > updated control value. I inspect it's content and I could either > > > delete it if I'm done, or re-use it in the next request and delete it > > > later once I'm done? It is responsability of the application to create > > > controls and delete them opportunely. To me this is acceptable. > > > > I 'think' when you get a new request, you would create a new set of > > controls. > > > > I don't think you'd necessarily use the same objects - but you might > > have a predefined list of controls that you could add to a request > > (which would create a copy of that list) for some known determined state > > that you desire. > > That's how I envision it too. When a request is dequeued and reused all > the controls it contains should be removed. Applications can then set > controls manually, or use a predefined list. > > > > ------------------------------------------------------------------------- > > > > > > Does this match your understanding? Can you name other controls which > > > would require a different interaction model? > > > > Further discussion all inline, I think (/hope) we're quite aligned on > > most things... > > > > >>> + > > >>> Status status() const { return status_; } > > >>> > > >>> bool hasPendingBuffers() const { return !pending_.empty(); } > > >>> @@ -52,6 +55,7 @@ private: > > >>> Camera *camera_; > > >>> std::map<Stream *, Buffer *> bufferMap_; > > >>> std::unordered_set<Buffer *> pending_; > > >>> + std::set<Control> controls_; > > >>> > > >>> Status status_; > > >>> }; > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On 11/06/2019 13:07, Laurent Pinchart wrote: > On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote: >> On 10/06/2019 11:45, Jacopo Mondi wrote: >>> On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote: >>>> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: >>>>> Provide a set to contain all controls applicable to the request. >>>>> The set contains all controls whether they are write, read, or write-read controls. >>> >>> Not on this patch strictly, but, the 'nature' of the control (w, r, >>> rw) is define by how the Control is constructed, right? If it is >>> provided a value is a write, otherwise is a read. >> >> In it's initial construct yes, a ControlValue with no value (but given a >> type) must be a Read control ... and a ControlValue given a value (which >> infers it's type) will be a Write. >> >> I envisaged perhaps it would necessitate an extra call (not too fond of >> that, but still a WIP) to get a 'Write' control to also 'Read' afterwards. > > This still doesn't answer the question of whether application should > provide a list of controls they want to read, or if the pipeline handler > will just report all the information is has. I was envisioning the > latter, but if you think the former is better, please explain why :-) Do you expect the pipeline handler to obtain and update all control values after every capture completes? I guess this is 'cheaper' now that we have a single ioctl with extended controls. How about any controls in the request when it's queued are there to be actioned (written/updated), and the pipeline handler should then update those controls at the request completion (assuming that's a reasonable action on that control) and along with that provide any further control/state information. So the request completion might find that it iterates all controls it can handle - and updates the control map with everything it knows about that's relevant to pass back in the request. > Note that the data reported by the pipeline handler is more in the form > of metadata (data related to how the image was captured) than in the > form of controls (tunable that are set to influence the image). It could > make sense to separate them at the API level with different names to > make this clear, although combining them through a single API could also > make sense if the implementations become too similar. Yes, I think things like "What was the exposure time when this frame was captured" is meta data that should be provided. That begs the question of how controls interact with each other though! For items that could be set by an IPA or controlled manually, this could get complex. I.e. If an IPA sets "Exposure", Requests complete with the configured "Exposure" setting. If an Application forces "ManualExposure" to X, and would then expect 'Exposure' to be returned as X (at least at some point). (edit: Now that I've gone through this - with the concept that the request is updated at the end - this should be fine.) >>> I think controls should have a 'direction' type assigned as part of >>> their definition which should be checked against what the user tries >>> to do on it, and not only depend on how the user creates them. >> >> Ok - so, you see that as a third parameter in construction? >> <id, value, direction?> >> >>> In example, the static controls that Laurent mentioned here below, >>> which are stored in the Camera, should be 'static'/'read-only' >>> controls, which provides to application non-modifiable >>> informations on the camera (orientation, position etc). Will the >>> 'direction' be part of the controls definition? >> >> I imagine if some controls are read only - then yes we would store that >> state information as part of the ControlInfo table/class? >> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> --- >>>>> include/libcamera/request.h | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>>>> index 58de6f00a554..5fae0d5fc838 100644 >>>>> --- a/include/libcamera/request.h >>>>> +++ b/include/libcamera/request.h >>>>> @@ -10,6 +10,7 @@ >>>>> #include <map> >>>>> #include <unordered_set> >>>>> >>>>> +#include <libcamera/controls.h> >>>>> #include <libcamera/signal.h> >>>>> >>>>> namespace libcamera { >>>>> @@ -36,6 +37,8 @@ public: >>>>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); >>>>> Buffer *findBuffer(Stream *stream) const; >>>>> >>>>> + std::set<Control> &controls() { return controls_; }; >>>> >>>> I think we will need something a bit more complicated than a std::set. >>>> Here's what I was thinking about. >>>> >>>> - Let's split control information from control value. The former are >>>> static for the lifetime of the camera (or at least of the capture >>>> session), while the latter vary per-request. >> >> Ok - that was one of the directions I considered as well, and I think >> now that more thought has gone into things it could be a reasonable >> route. Or of course the class Control::id_ could be changed to a >> Control::info_ as a &ControlInfo type in an unordered_set. >> >>>> - The control values in the request could be stored in a >>>> std::map<const ControlInfo &, ControlValue>. This would make copies as >>>> cheap as possible as we won't have to duplicate the control >>>> information. >> >> I agree, the ControlInformation should not be duplicated, and the 'ID' >> refernce should be as cheap as possible. >> >>> Where do you envision them to live? I assume as they won't have to be >>> duplicated we'll maintain a table somewhere of ControlInfo each >>> request's control will reference to... >> >> More so than that, as some controls will be dynamic - and we must query >> the underlying kernel drivers for the properties (min/max/defaults). >> >> So would we do that at open? StreamOn? Some-other time? > > Will they be dynamic ? They certainly can't be hardcoded in the sourced > code, but I don't expect them to change for the lifetime of the camera > (feel free to prove me wrong). They could thus be created when creating > the camera. Dynamic was the wrong choice of word. I meant as in - needs to be determined at runtime (creating the camera is a fine time), and can't be configured in advance in a static table or such. >> Perhaps it will be up to the PipelineHandler to create it's ControlInfo >> set and populate values appropriately. This would then let the >> PipelineHandler decide which controls are valid to support or not. > > I think that's what will happen, yes (with the help of the IPA). So some controls are going to be specific to the IPA perhaps? Should we separate those as a different set? :S >>>> - The return value of the controls() function should be a custom class >>>> wrapping around the std::map. This is needed to perform control >>>> validation for instance (min/max/step), and will likely come handy to >>>> implement other custom behaviours. >>> >>> Each control should be validated against it's associated ControlInfo. >>> Why is wrapping the container relevant if not from the API usage >>> easiness, maybe? >> >> Hrm... this makes me wonder if that validation should be done by the >> 'container of multiple controls' ... or a container of a single control. >> >> I.e. either we create a ControlList() which stores a map <ControlInfo, >> ControlValue>, and it does the validation and hides the map, >> >> Or we change the control class to store <ControlInfo &, ControlValue> >> and let that be stored in any container as required. > > We have started by exposing STL containers directly (for format > enumeration for instance), and then moved to custom classes. I think > custom classes give is more flexibility and allow creation of better > APIs for applications. Regardless of where the validation is performed, > I would use a custom container class to store controls in requests. > >>>> - The control information should be exposed by the camera so that >>>> application can enumerate supported controls and query their >>>> information. >>> >>> Do we expect the camera to have anything like "readControl()" ? >>> My understanding was that all interactions with control goes through >>> requests, so there's not actually anything like an asynchronous >>> read(). >> >> No I don't think we'll have readControl() - but populating any relevant >> ControlInfo sturctures might certainly have to be done at run-time (but >> that's more like queryControl() - not readControl(). > > Yes, that's what I meant. Then yes, we need to have a queryControl() somewhere. > >>> I wonder if >>> 1) getting from the camera a list of controls with associated default >>> values > > I don't think we'll have global defaults, we will likely need an API to > query defaults for a given use case, similar to > Camera::generateConfiguration(). > >>> 2) modify the one the app is interested into ad submit a request >>> 3) at request complete time verify the control value has changed to >>> the requested value (or is converging to it) and modify it again if >>> required. >>> >>> Is not a better interaction model compared to the asynchronous >>> "getControl/setControl" one. We briefly discussed that, and one point >>> was that the delays in completing the request might delay the read of >>> a control. I wonder if that's relevant or if we can do anything about it >> >> No, I think a delayed request is fine. It can only be delayed due to >> other actions within that request, and we guarantee (I believe) that >> requests complete in sequential order. Thus, the control data/state in >> the request will match the state of that request completion. (i.e. if >> any other buffers were included.). >> >> In my initial work - I expected reading of controls to be the *last* >> thing that a request completion does. So the control state would be as >> recent as possible, and no further delays should be expected. > > Among all the information reported to the application through the > request, we will have data coming from V4L2 controls, data coming from > buffers (e.g. metadata sent by the sensor as embedded data), data > created by the IPA (e.g. the locked state of the control loops), ... All > these will potentially be generated at different times. Indeed. I wonder at what level we will want to timestamp this information...I guess that will appear as it's needed. >>> ie. the control value comes from a statistic value generated from >>> inspecting the pixel data: this should always be associated with a >>> completed request. This holds for other "async" controls as well: do we care >>> what is the lens exposure "now" or what is value was at the time the picture >>> has been taken?. >> >> I would say it's important to associate the control information with the >> other request content. I.e. - "At the time 'this' picture was taken, the >> lens exposure 'was' $E, and the brightness was $B". > > I agree, I don't think we need asynchronous reads. > >>> If we have to fast-track some controls with an immediate >>> Camera::readControl() I agree this might be needed for some case but >>> not pretty, as it offers an API easy to abuse. >> >> I don't yet know what 'fast tracking' we would have to do. >> >> If there is a control you want to read/write - without picture data, it >> has to go into a request of it's own (and the framework should handle >> that correctly, i.e. not block on waiting for any buffers to complete). > > I don't see a use-case for fast-tracking reads. Writes could be > different though, we will have to evaluate the use cases. Well lets handle that when it comes up. We need to get there first :-) >>>> - We need to think of the common use cases from an application point of >>>> view. In particular I think applications will need an API to get a >>>> default set of control values from libcamera, that they will then >>>> possibly modify and set for the first request. Subsequent requests >>>> will likely just modify a small number of controls, and will likely be >>>> constructed from scratch. For control that applications want to read, >>>> I expect most requests to contain the same controls, so there should >>>> be an easy and efficient way to handle that. Splitting read and write >>>> controls may be a good idea. >> >> Laurent: I'm a bit confused as to whether you expect a request to >> contain all controls and all state, or just ones that are modified... > > Just the ones that are modified, but I expect the first request to > typically contain all/most/many controls. The camera should provide > default control values for a given use case, and I think that will be > used to create the first request. Subsequent requests will likely > contain less controls. Ok - so the camera/pipeline can provide a function which returns a set/list/map/container/class of all known controls and their defaults. Then it's up to the Camera/Pipeline to prepare or update those lists for the use cases if necessary. >>> I'm not clear how do you think an interaction to read controls >>> asynchronously from request would look like. I'm mostly concerned >> >> Asynchronously from request ..? You mean not using a request? (I don't >> think we would ever read controls without it going through a request?) >> or do you mean handling the timeing of when a control is read during a >> request lifecycle. >> >>> about the fact that most (some?) parameter would depend on the >>> statistics generated processing the most recently captured image, and >>> being able to provide the most recent values would require quite some >>> caching somewhere. Do we want to cache -all- control values to be able >>> to promptly reply to a "getControl()" at any point in time? >> >> Hrm... some control values might be 'cached' internally, becuase they >> might not really be a value that is queried from hardware. (As you state >> below). ... but any control which is represented by hardware, I can't >> imagine caching it - as if you read it - you want to know - what that >> value really is? >> >> The IPA's might likely keep track of the values they need to monitor >> over a time series perhaps? But that should be in the IPA - not at a >> core level I don't think... <I guess we'll see soon> >> >>> See, Kieran did a great job defining the container types and their >>> storage, but what I would like to happen before I can really make up >>> my mind is a better definition of the designed interaction model and >>> the definition of some Libcamera controls which are not just plain >>> V4L2Control which Kieran has rightfully used in this series. >>> >>> ------------------------------------------------------------------------- >>> Let me write down how I imagine interacting with a control would look >>> like from an application point of view. Ignore it if it's too long :) >>> >>> >>> Let's use in example, the lens aperture (I don't see V4L2 control that >>> maps to it, I might be wrong, as it is quite a fundamental parameter). >>> >>> As an application I would expect to know: >>> - the type of the control value (ie float, in this case) >>> - if I can control it or not >>> - the available values I can chose from, as a list of values or a range with >>> min, max and a step value. >>> >>> Where should this informations be stored? In the control info? How >> >> Yes, that all sounds like ControlInfo information to me ..., which is >> (at least runtime) constant. >> >>> would a pipeline handler create them? For each control the camera supports >>> should it fill up a control info with all this informations? >> >> Yes, I think so. >> >>> So I'm now an application, I have a camera, and I would like to list >>> the control it supports. I should call something like controls() and >>> get back a map of <ControlInfo *, ControlValue &default>. >> >> Perhaps, or the ControlValue &default might actually just be insside the >> ControlInfo & > > I think the default values will be use-case-dependent, so I believe > we'll need a method to retrieve information about all supported > controls, without a default. The default should be retrieved through a > separate method that will take a use case (possible a set of stream > roles). Ok - so the ControlInfo doesn't need a default() then, as getting defaults will be a per-use-case created list of ControlInfo/ControlValue pairs. Now I understand, and I believe that's exactly what Jacopo wrote just above here :) > >>> The control info should live somewhere, if we want to cache it to >>> perform validations, so a * is fine. The defaultValue is a reference or >> >> Why a pointer and not a reference to the ControlInfo? It should be >> guaranteed to exist... (And we should guarantee it's pointer/address >> should not change) >> >>> pointer to a value which could be cached as well, to perform something like >>> "reset to default". >> >> Yes, somehow we want to be able to reset controls to defaults. Perhaps >> individually, or in bulk, or as a complete (re)set. > > As defaults are not global, we can't have a reset method, but > applications can simply use the defaults they have retrieved for a given > use case and set them in a request. Ok. > >>> Now I want to know >>> 1) Can I control aperture? >>> 2) which values are accepted? >>> >>> So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE >>> returns a valid pair from where get the associated controlInfo. This would >>> tell me: >> >> This is why I don't like std::map ... If you try to obtain a >> ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be >> created! > > std::map::find() Ok - that's alright then :-) >> So an unordered_set might be more appropriate there. >> >> (Or perhaps only [] creates, and maybe .find() will be ok) >> >>> 1) The direction: can I set it, or just read it? >> >> Indeed, and should this be a bitfield, or enum like my ControlAction enum... >> >>> 2) The supported values as a list of floats, or max-min with steps >>> 3) its default value in the pair.second >> >> As the ControlInfo is a class, it can just as well contain a >> ControlValue default; imho. > > See above. Ack. >>> Now, should I create a ControlValue to associate it with a request? So >>> I go, create a new instance, fill it with one value, and store it in >>> the request. The control value alone, or does it need the control >>> info? I guess the frameworks as a cache of control info, indexed by >>> ID, so a control value with an ID, is enough for the framework to >>> retrieve the ControlInfo and perform validations. >> >> Yes, you would create a ControlValue to put in the request somehow. >> <either in a map, or an unordered_set, or a not-yet-defined ControlList> >> >> That ControlValue should somehow be associated with a ControlInfo & >> reference, and it should be easy to obtain the ControlValue for >> LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or >> Map) stored in the request. >> >>> The request gets processed, and once completed, it returns with an >>> updated control value. I inspect it's content and I could either >>> delete it if I'm done, or re-use it in the next request and delete it >>> later once I'm done? It is responsability of the application to create >>> controls and delete them opportunely. To me this is acceptable. >> >> I 'think' when you get a new request, you would create a new set of >> controls. >> >> I don't think you'd necessarily use the same objects - but you might >> have a predefined list of controls that you could add to a request >> (which would create a copy of that list) for some known determined state >> that you desire. > > That's how I envision it too. When a request is dequeued and reused all > the controls it contains should be removed. Applications can then set > controls manually, or use a predefined list. Agreed here. > >>> ------------------------------------------------------------------------- >>> >>> Does this match your understanding? Can you name other controls which >>> would require a different interaction model? >> >> Further discussion all inline, I think (/hope) we're quite aligned on >> most things... >> >>>>> + >>>>> Status status() const { return status_; } >>>>> >>>>> bool hasPendingBuffers() const { return !pending_.empty(); } >>>>> @@ -52,6 +55,7 @@ private: >>>>> Camera *camera_; >>>>> std::map<Stream *, Buffer *> bufferMap_; >>>>> std::unordered_set<Buffer *> pending_; >>>>> + std::set<Control> controls_; >>>>> >>>>> Status status_; >>>>> }; >
Hi Kieran, On Tue, Jun 11, 2019 at 03:06:06PM +0100, Kieran Bingham wrote: > On 11/06/2019 13:07, Laurent Pinchart wrote: > > On Mon, Jun 10, 2019 at 01:11:02PM +0100, Kieran Bingham wrote: > >> On 10/06/2019 11:45, Jacopo Mondi wrote: > >>> On Fri, Jun 07, 2019 at 07:37:19PM +0300, Laurent Pinchart wrote: > >>>> On Thu, Jun 06, 2019 at 09:56:51PM +0100, Kieran Bingham wrote: > >>>>> Provide a set to contain all controls applicable to the request. > >>>>> The set contains all controls whether they are write, read, or write-read controls. > >>> > >>> Not on this patch strictly, but, the 'nature' of the control (w, r, > >>> rw) is define by how the Control is constructed, right? If it is > >>> provided a value is a write, otherwise is a read. > >> > >> In it's initial construct yes, a ControlValue with no value (but given a > >> type) must be a Read control ... and a ControlValue given a value (which > >> infers it's type) will be a Write. > >> > >> I envisaged perhaps it would necessitate an extra call (not too fond of > >> that, but still a WIP) to get a 'Write' control to also 'Read' afterwards. > > > > This still doesn't answer the question of whether application should > > provide a list of controls they want to read, or if the pipeline handler > > will just report all the information is has. I was envisioning the > > latter, but if you think the former is better, please explain why :-) > > Do you expect the pipeline handler to obtain and update all control > values after every capture completes? Not all of them. I think we should split controls and metadata, as they are two different things. Applications don't need to read back controls, they need to know about the context in which a frame was captured. When modifying the exposure time or the lens position, when using the flash to expose an image or when having an action on a parameter that doesn't take immediate effect, it can be import to know, for each captured frame, what exact settings have been used. However, when setting a control that has immediate effect (a gamma table applied by the ISP for instance), there's often no need to report it back. Most of the controls provided by the hardware fall in the second category and don't have to be read back. Only a handful will in typical cases. > I guess this is 'cheaper' now that we have a single ioctl with extended > controls. I2C access is still expensive, so we should minimise the number of V4L2 reads on the sensor for instance. However, in many cases the metadata are reported through faster means, for instance as embedded data on the CSI-2 bus. > How about any controls in the request when it's queued are there to be > actioned (written/updated), and the pipeline handler should then update > those controls at the request completion (assuming that's a reasonable > action on that control) and along with that provide any further > control/state information. I really think we should split the two. Applications set controls in requests, and the pipeline handler (and IPA) apply those controls to the hardware. The pipeline handler then reports metadata. It may be a useful optimisation for applications to tell what metadata they're interested in, to skip slow read operations, but I expect that in most cases the control loop implementation would need the metadata for internal purpose, so they will have to be read anyway. > So the request completion might find that it iterates all controls it > can handle - and updates the control map with everything it knows about > that's relevant to pass back in the request. > > > Note that the data reported by the pipeline handler is more in the form > > of metadata (data related to how the image was captured) than in the > > form of controls (tunable that are set to influence the image). It could > > make sense to separate them at the API level with different names to > > make this clear, although combining them through a single API could also > > make sense if the implementations become too similar. > > Yes, I think things like "What was the exposure time when this frame was > captured" is meta data that should be provided. > > That begs the question of how controls interact with each other though! > > For items that could be set by an IPA or controlled manually, this could > get complex. I.e. > > If an IPA sets "Exposure", Requests complete with the configured > "Exposure" setting. > > If an Application forces "ManualExposure" to X, and would then expect > 'Exposure' to be returned as X (at least at some point). We have to document precisely how controls interact. That will be a big documentation effort, but it will be worth it. > (edit: Now that I've gone through this - with the concept that the > request is updated at the end - this should be fine.) > > >>> I think controls should have a 'direction' type assigned as part of > >>> their definition which should be checked against what the user tries > >>> to do on it, and not only depend on how the user creates them. > >> > >> Ok - so, you see that as a third parameter in construction? > >> <id, value, direction?> > >> > >>> In example, the static controls that Laurent mentioned here below, > >>> which are stored in the Camera, should be 'static'/'read-only' > >>> controls, which provides to application non-modifiable > >>> informations on the camera (orientation, position etc). Will the > >>> 'direction' be part of the controls definition? > >> > >> I imagine if some controls are read only - then yes we would store that > >> state information as part of the ControlInfo table/class? > >> > >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>> --- > >>>>> include/libcamera/request.h | 4 ++++ > >>>>> 1 file changed, 4 insertions(+) > >>>>> > >>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > >>>>> index 58de6f00a554..5fae0d5fc838 100644 > >>>>> --- a/include/libcamera/request.h > >>>>> +++ b/include/libcamera/request.h > >>>>> @@ -10,6 +10,7 @@ > >>>>> #include <map> > >>>>> #include <unordered_set> > >>>>> > >>>>> +#include <libcamera/controls.h> > >>>>> #include <libcamera/signal.h> > >>>>> > >>>>> namespace libcamera { > >>>>> @@ -36,6 +37,8 @@ public: > >>>>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > >>>>> Buffer *findBuffer(Stream *stream) const; > >>>>> > >>>>> + std::set<Control> &controls() { return controls_; }; > >>>> > >>>> I think we will need something a bit more complicated than a std::set. > >>>> Here's what I was thinking about. > >>>> > >>>> - Let's split control information from control value. The former are > >>>> static for the lifetime of the camera (or at least of the capture > >>>> session), while the latter vary per-request. > >> > >> Ok - that was one of the directions I considered as well, and I think > >> now that more thought has gone into things it could be a reasonable > >> route. Or of course the class Control::id_ could be changed to a > >> Control::info_ as a &ControlInfo type in an unordered_set. > >> > >>>> - The control values in the request could be stored in a > >>>> std::map<const ControlInfo &, ControlValue>. This would make copies as > >>>> cheap as possible as we won't have to duplicate the control > >>>> information. > >> > >> I agree, the ControlInformation should not be duplicated, and the 'ID' > >> refernce should be as cheap as possible. > >> > >>> Where do you envision them to live? I assume as they won't have to be > >>> duplicated we'll maintain a table somewhere of ControlInfo each > >>> request's control will reference to... > >> > >> More so than that, as some controls will be dynamic - and we must query > >> the underlying kernel drivers for the properties (min/max/defaults). > >> > >> So would we do that at open? StreamOn? Some-other time? > > > > Will they be dynamic ? They certainly can't be hardcoded in the sourced > > code, but I don't expect them to change for the lifetime of the camera > > (feel free to prove me wrong). They could thus be created when creating > > the camera. > > Dynamic was the wrong choice of word. I meant as in - needs to be > determined at runtime (creating the camera is a fine time), and can't be > configured in advance in a static table or such. > > >> Perhaps it will be up to the PipelineHandler to create it's ControlInfo > >> set and populate values appropriately. This would then let the > >> PipelineHandler decide which controls are valid to support or not. > > > > I think that's what will happen, yes (with the help of the IPA). > > So some controls are going to be specific to the IPA perhaps? Should we > separate those as a different set? :S >From an application point of view I wouldn't separate them. > >>>> - The return value of the controls() function should be a custom class > >>>> wrapping around the std::map. This is needed to perform control > >>>> validation for instance (min/max/step), and will likely come handy to > >>>> implement other custom behaviours. > >>> > >>> Each control should be validated against it's associated ControlInfo. > >>> Why is wrapping the container relevant if not from the API usage > >>> easiness, maybe? > >> > >> Hrm... this makes me wonder if that validation should be done by the > >> 'container of multiple controls' ... or a container of a single control. > >> > >> I.e. either we create a ControlList() which stores a map <ControlInfo, > >> ControlValue>, and it does the validation and hides the map, > >> > >> Or we change the control class to store <ControlInfo &, ControlValue> > >> and let that be stored in any container as required. > > > > We have started by exposing STL containers directly (for format > > enumeration for instance), and then moved to custom classes. I think > > custom classes give is more flexibility and allow creation of better > > APIs for applications. Regardless of where the validation is performed, > > I would use a custom container class to store controls in requests. > > > >>>> - The control information should be exposed by the camera so that > >>>> application can enumerate supported controls and query their > >>>> information. > >>> > >>> Do we expect the camera to have anything like "readControl()" ? > >>> My understanding was that all interactions with control goes through > >>> requests, so there's not actually anything like an asynchronous > >>> read(). > >> > >> No I don't think we'll have readControl() - but populating any relevant > >> ControlInfo sturctures might certainly have to be done at run-time (but > >> that's more like queryControl() - not readControl(). > > > > Yes, that's what I meant. > > Then yes, we need to have a queryControl() somewhere. It could be a Camera::controls() method. > >>> I wonder if > >>> 1) getting from the camera a list of controls with associated default > >>> values > > > > I don't think we'll have global defaults, we will likely need an API to > > query defaults for a given use case, similar to > > Camera::generateConfiguration(). > > > >>> 2) modify the one the app is interested into ad submit a request > >>> 3) at request complete time verify the control value has changed to > >>> the requested value (or is converging to it) and modify it again if > >>> required. > >>> > >>> Is not a better interaction model compared to the asynchronous > >>> "getControl/setControl" one. We briefly discussed that, and one point > >>> was that the delays in completing the request might delay the read of > >>> a control. I wonder if that's relevant or if we can do anything about it > >> > >> No, I think a delayed request is fine. It can only be delayed due to > >> other actions within that request, and we guarantee (I believe) that > >> requests complete in sequential order. Thus, the control data/state in > >> the request will match the state of that request completion. (i.e. if > >> any other buffers were included.). > >> > >> In my initial work - I expected reading of controls to be the *last* > >> thing that a request completion does. So the control state would be as > >> recent as possible, and no further delays should be expected. > > > > Among all the information reported to the application through the > > request, we will have data coming from V4L2 controls, data coming from > > buffers (e.g. metadata sent by the sensor as embedded data), data > > created by the IPA (e.g. the locked state of the control loops), ... All > > these will potentially be generated at different times. > > Indeed. I wonder at what level we will want to timestamp this > information...I guess that will appear as it's needed. > > >>> ie. the control value comes from a statistic value generated from > >>> inspecting the pixel data: this should always be associated with a > >>> completed request. This holds for other "async" controls as well: do we care > >>> what is the lens exposure "now" or what is value was at the time the picture > >>> has been taken?. > >> > >> I would say it's important to associate the control information with the > >> other request content. I.e. - "At the time 'this' picture was taken, the > >> lens exposure 'was' $E, and the brightness was $B". > > > > I agree, I don't think we need asynchronous reads. > > > >>> If we have to fast-track some controls with an immediate > >>> Camera::readControl() I agree this might be needed for some case but > >>> not pretty, as it offers an API easy to abuse. > >> > >> I don't yet know what 'fast tracking' we would have to do. > >> > >> If there is a control you want to read/write - without picture data, it > >> has to go into a request of it's own (and the framework should handle > >> that correctly, i.e. not block on waiting for any buffers to complete). > > > > I don't see a use-case for fast-tracking reads. Writes could be > > different though, we will have to evaluate the use cases. > > Well lets handle that when it comes up. We need to get there first :-) > > >>>> - We need to think of the common use cases from an application point of > >>>> view. In particular I think applications will need an API to get a > >>>> default set of control values from libcamera, that they will then > >>>> possibly modify and set for the first request. Subsequent requests > >>>> will likely just modify a small number of controls, and will likely be > >>>> constructed from scratch. For control that applications want to read, > >>>> I expect most requests to contain the same controls, so there should > >>>> be an easy and efficient way to handle that. Splitting read and write > >>>> controls may be a good idea. > >> > >> Laurent: I'm a bit confused as to whether you expect a request to > >> contain all controls and all state, or just ones that are modified... > > > > Just the ones that are modified, but I expect the first request to > > typically contain all/most/many controls. The camera should provide > > default control values for a given use case, and I think that will be > > used to create the first request. Subsequent requests will likely > > contain less controls. > > Ok - so the camera/pipeline can provide a function which returns a > set/list/map/container/class of all known controls and their defaults. > > Then it's up to the Camera/Pipeline to prepare or update those lists for > the use cases if necessary. I think the pipeline handler will prepare the ControlInfo when creating the camera, and will provide an API to retrieve defaults for a set of use cases. If we need to change the info at runtime then we'll need to extend the API. I hope it won't be needed though. > >>> I'm not clear how do you think an interaction to read controls > >>> asynchronously from request would look like. I'm mostly concerned > >> > >> Asynchronously from request ..? You mean not using a request? (I don't > >> think we would ever read controls without it going through a request?) > >> or do you mean handling the timeing of when a control is read during a > >> request lifecycle. > >> > >>> about the fact that most (some?) parameter would depend on the > >>> statistics generated processing the most recently captured image, and > >>> being able to provide the most recent values would require quite some > >>> caching somewhere. Do we want to cache -all- control values to be able > >>> to promptly reply to a "getControl()" at any point in time? > >> > >> Hrm... some control values might be 'cached' internally, becuase they > >> might not really be a value that is queried from hardware. (As you state > >> below). ... but any control which is represented by hardware, I can't > >> imagine caching it - as if you read it - you want to know - what that > >> value really is? > >> > >> The IPA's might likely keep track of the values they need to monitor > >> over a time series perhaps? But that should be in the IPA - not at a > >> core level I don't think... <I guess we'll see soon> > >> > >>> See, Kieran did a great job defining the container types and their > >>> storage, but what I would like to happen before I can really make up > >>> my mind is a better definition of the designed interaction model and > >>> the definition of some Libcamera controls which are not just plain > >>> V4L2Control which Kieran has rightfully used in this series. > >>> > >>> ------------------------------------------------------------------------- > >>> Let me write down how I imagine interacting with a control would look > >>> like from an application point of view. Ignore it if it's too long :) > >>> > >>> > >>> Let's use in example, the lens aperture (I don't see V4L2 control that > >>> maps to it, I might be wrong, as it is quite a fundamental parameter). > >>> > >>> As an application I would expect to know: > >>> - the type of the control value (ie float, in this case) > >>> - if I can control it or not > >>> - the available values I can chose from, as a list of values or a range with > >>> min, max and a step value. > >>> > >>> Where should this informations be stored? In the control info? How > >> > >> Yes, that all sounds like ControlInfo information to me ..., which is > >> (at least runtime) constant. > >> > >>> would a pipeline handler create them? For each control the camera supports > >>> should it fill up a control info with all this informations? > >> > >> Yes, I think so. > >> > >>> So I'm now an application, I have a camera, and I would like to list > >>> the control it supports. I should call something like controls() and > >>> get back a map of <ControlInfo *, ControlValue &default>. > >> > >> Perhaps, or the ControlValue &default might actually just be insside the > >> ControlInfo & > > > > I think the default values will be use-case-dependent, so I believe > > we'll need a method to retrieve information about all supported > > controls, without a default. The default should be retrieved through a > > separate method that will take a use case (possible a set of stream > > roles). > > Ok - so the ControlInfo doesn't need a default() then, as getting > defaults will be a per-use-case created list of ControlInfo/ControlValue > pairs. > > Now I understand, and I believe that's exactly what Jacopo wrote just > above here :) > > >>> The control info should live somewhere, if we want to cache it to > >>> perform validations, so a * is fine. The defaultValue is a reference or > >> > >> Why a pointer and not a reference to the ControlInfo? It should be > >> guaranteed to exist... (And we should guarantee it's pointer/address > >> should not change) > >> > >>> pointer to a value which could be cached as well, to perform something like > >>> "reset to default". > >> > >> Yes, somehow we want to be able to reset controls to defaults. Perhaps > >> individually, or in bulk, or as a complete (re)set. > > > > As defaults are not global, we can't have a reset method, but > > applications can simply use the defaults they have retrieved for a given > > use case and set them in a request. > > Ok. > > >>> Now I want to know > >>> 1) Can I control aperture? > >>> 2) which values are accepted? > >>> > >>> So I should find() on the map the map, make sure LIBCAMERA_LENS_APERTURE > >>> returns a valid pair from where get the associated controlInfo. This would > >>> tell me: > >> > >> This is why I don't like std::map ... If you try to obtain a > >> ControlInfo(LIBCAMERA_LENS_APERTURE) which isn't in the map - it will be > >> created! > > > > std::map::find() > > Ok - that's alright then :-) > > >> So an unordered_set might be more appropriate there. > >> > >> (Or perhaps only [] creates, and maybe .find() will be ok) > >> > >>> 1) The direction: can I set it, or just read it? > >> > >> Indeed, and should this be a bitfield, or enum like my ControlAction enum... > >> > >>> 2) The supported values as a list of floats, or max-min with steps > >>> 3) its default value in the pair.second > >> > >> As the ControlInfo is a class, it can just as well contain a > >> ControlValue default; imho. > > > > See above. > > Ack. > > >>> Now, should I create a ControlValue to associate it with a request? So > >>> I go, create a new instance, fill it with one value, and store it in > >>> the request. The control value alone, or does it need the control > >>> info? I guess the frameworks as a cache of control info, indexed by > >>> ID, so a control value with an ID, is enough for the framework to > >>> retrieve the ControlInfo and perform validations. > >> > >> Yes, you would create a ControlValue to put in the request somehow. > >> <either in a map, or an unordered_set, or a not-yet-defined ControlList> > >> > >> That ControlValue should somehow be associated with a ControlInfo & > >> reference, and it should be easy to obtain the ControlValue for > >> LIBCAMERA_LENS_APERTURE within the container (either the List, Set, or > >> Map) stored in the request. > >> > >>> The request gets processed, and once completed, it returns with an > >>> updated control value. I inspect it's content and I could either > >>> delete it if I'm done, or re-use it in the next request and delete it > >>> later once I'm done? It is responsability of the application to create > >>> controls and delete them opportunely. To me this is acceptable. > >> > >> I 'think' when you get a new request, you would create a new set of > >> controls. > >> > >> I don't think you'd necessarily use the same objects - but you might > >> have a predefined list of controls that you could add to a request > >> (which would create a copy of that list) for some known determined state > >> that you desire. > > > > That's how I envision it too. When a request is dequeued and reused all > > the controls it contains should be removed. Applications can then set > > controls manually, or use a predefined list. > > Agreed here. > > >>> ------------------------------------------------------------------------- > >>> > >>> Does this match your understanding? Can you name other controls which > >>> would require a different interaction model? > >> > >> Further discussion all inline, I think (/hope) we're quite aligned on > >> most things... > >> > >>>>> + > >>>>> Status status() const { return status_; } > >>>>> > >>>>> bool hasPendingBuffers() const { return !pending_.empty(); } > >>>>> @@ -52,6 +55,7 @@ private: > >>>>> Camera *camera_; > >>>>> std::map<Stream *, Buffer *> bufferMap_; > >>>>> std::unordered_set<Buffer *> pending_; > >>>>> + std::set<Control> controls_; > >>>>> > >>>>> Status status_; > >>>>> };
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 58de6f00a554..5fae0d5fc838 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -10,6 +10,7 @@ #include <map> #include <unordered_set> +#include <libcamera/controls.h> #include <libcamera/signal.h> namespace libcamera { @@ -36,6 +37,8 @@ public: int setBuffers(const std::map<Stream *, Buffer *> &streamMap); Buffer *findBuffer(Stream *stream) const; + std::set<Control> &controls() { return controls_; }; + Status status() const { return status_; } bool hasPendingBuffers() const { return !pending_.empty(); } @@ -52,6 +55,7 @@ private: Camera *camera_; std::map<Stream *, Buffer *> bufferMap_; std::unordered_set<Buffer *> pending_; + std::set<Control> controls_; Status status_; };
Provide a set to contain all controls applicable to the request. The set contains all controls whether they are write, read, or write-read controls. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/request.h | 4 ++++ 1 file changed, 4 insertions(+)