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

[Kieran Bingham]

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 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?


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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran