[libcamera-devel] [RFC PATCH 2/5] libcamera: request: add a control set

[Jacopo Mondi]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190611/bc7bc593/attachment-0001.sig>