[libcamera-devel] [RFC PATCH 2/6] libcamera: stream: Add frame interval attribute

[Jacopo Mondi]

On Tue, Mar 16, 2021 at 11:24:16PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Mar 16, 2021 at 10:15:00PM +0100, Jacopo Mondi wrote:
> > On Tue, Mar 16, 2021 at 09:28:47PM +0200, Laurent Pinchart wrote:
> > > On Tue, Mar 16, 2021 at 04:52:07PM +0100, Marian Cichy wrote:
> > > > The frame interval can be get and set from and on v4l2-subdevices and is
> > > > part of the stream configuration.
> > > >
> > > > Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> > > > ---
> > > >  include/libcamera/stream.h | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > > index bb47c390..14bfbf44 100644
> > > > --- a/include/libcamera/stream.h
> > > > +++ b/include/libcamera/stream.h
> > > > @@ -13,6 +13,7 @@
> > > >  #include <vector>
> > > >
> > > >  #include <libcamera/buffer.h>
> > > > +#include <libcamera/fraction.h>
> > > >  #include <libcamera/geometry.h>
> > > >  #include <libcamera/pixel_format.h>
> > > >
> > > > @@ -44,6 +45,7 @@ struct StreamConfiguration {
> > > >  	Size size;
> > > >  	unsigned int stride;
> > > >  	unsigned int frameSize;
> > > > +	Fraction frameInterval;
> > >
> > > This should use the FrameDurations control instead.
> >
> > This might turn out to be rather nasty as we don't support per-stream
> > controls (yet?) :/
>
> This patch series targets a single stream as it configures the frame
> duration on the sensor, so I think that's fine (at least for now).
>

Yeah, I meant that it could get nasty as that's a missing feature of
the library (I thought this required per-stream controls).

If it's about reading a property, shouldn't it be registered as a
Camera::property by the pipeline handler (as it's done in IPU3 and RPI
iirc) ?

Or does the information depend on the current configuration ? Wouldn't, in that
case, adding a list of control to CameraConfiguration
a good fit ?

Camera currently provides a list of available controls and their
limits and a list of read-only constants as properties. We
don't really have a way to represent controls (or properties) that
depend on the current configuration.

We discussed updating the ContrlInfos stored in the Camera to reflect
the current limits and default and it seems to cover the same use-case
to me ? Wouldn't it be less intrusive adding a list of controls to the
configuration and updated that at Camera::configure() time ?

> > What are you trying to achieve here ? Setting the frame duration
> > during a capture session ? In that case the setting should go through
> > the Request::controls control list (and should be reported per-request
> > through Request::metadata).
> >
> > Or should the frame duration be made a property of the
> > CameraConfiguration and set at configure() time ? What is the use case
> > for this last option ?
> >
> > > >  	unsigned int bufferCount;
>
> --
> Regards,
>
> Laurent Pinchart