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

[Laurent Pinchart]

Hi Jacopo,

On Tue, Mar 16, 2021 at 11:03:38PM +0100, Jacopo Mondi wrote:
> On Tue, Mar 16, 2021 at 11:24:16PM +0200, Laurent Pinchart wrote:
> > 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 per-stream frame intervals were needed, yes, but this patch series is
about controlling the frame interval for all streams.

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

Yes, that's exactly what I meant when I said that the FrameDurations
control should be used instead.

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

This has been discussed in the context of adding controls to
Camera::start(), and I'm sure we'll resurect that discussion at some
point.

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

Something will be needed for sure, but I think a bit more effort will be
required to design a proper solution.

In the meantime, this series could move to using the existing
FrameDurations controls, I don't really see any blocker.

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