| Message ID | 20210316155211.6679-3-m.cichy@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Marian, Thank you for the patch. 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@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. > > unsigned int bufferCount; >
Hello, On Tue, Mar 16, 2021 at 09:28:47PM +0200, Laurent Pinchart wrote: > Hi Marian, > > Thank you for the patch. > > 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@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?) :/ 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 ? Thanks j > > > > unsigned int bufferCount; > > > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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). > 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;
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@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
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@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;
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; unsigned int bufferCount;
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@pengutronix.de> --- include/libcamera/stream.h | 2 ++ 1 file changed, 2 insertions(+)