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

[Laurent Pinchart]

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 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.
> 
> 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_;
> >>>>>  };

-- 
Regards,

Laurent Pinchart