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

Jacopo Mondi jacopo at jmondi.org
Mon Jun 10 12:45:22 CEST 2019 

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 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.
>
> - 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20190610/8c2847fc/attachment.sig>

More information about the libcamera-devel mailing list