| Message ID | 20210322104242.31107-2-m.cichy@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Marian, On Mon, Mar 22, 2021 at 11:42:40AM +0100, Marian Cichy wrote: > By adding a controlList to the stream attributes, we have access to > several more configuration parameters for a stream. In this patch > series, it is for now only used to access the frame duration of a sensor. > > 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..a6347e31 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -13,6 +13,7 @@ > #include <vector> > > #include <libcamera/buffer.h> > +#include <libcamera/controls.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > > @@ -44,6 +45,7 @@ struct StreamConfiguration { > Size size; > unsigned int stride; > unsigned int frameSize; > + ControlList controls; Documentation is missing. In general, as discussed on irc, I guess this is an acceptable first step to be able to retrieve per-stream information. > > unsigned int bufferCount; > > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 21-03-22 18:43, Jacopo Mondi wrote: > Hi Marian, > > On Mon, Mar 22, 2021 at 11:42:40AM +0100, Marian Cichy wrote: > > By adding a controlList to the stream attributes, we have access to > > several more configuration parameters for a stream. In this patch > > series, it is for now only used to access the frame duration of a sensor. > > > > 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..a6347e31 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -13,6 +13,7 @@ > > #include <vector> > > > > #include <libcamera/buffer.h> > > +#include <libcamera/controls.h> > > #include <libcamera/geometry.h> > > #include <libcamera/pixel_format.h> > > > > @@ -44,6 +45,7 @@ struct StreamConfiguration { > > Size size; > > unsigned int stride; > > unsigned int frameSize; > > + ControlList controls; > > Documentation is missing. > > In general, as discussed on irc, I guess this is an acceptable first > step to be able to retrieve per-stream information. I picked up Marian's work to become familar with libcamera and now I have my first question on this topic. Why is this necessary here? It think what we wanna do (retrieve the framerate) can be done by the pipeline_handler ControlInfoMap. So we only need to extended the simple-pipeline rather than adding it here, like all other pipeline handlers does. Am I right or did I miss something? Regards, Marco > > > > > unsigned int bufferCount; > > > > -- > > 2.29.2 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Marco, On Wed, Apr 28, 2021 at 03:47:05PM +0200, Marco Felsch wrote: > Hi Jacopo, > > On 21-03-22 18:43, Jacopo Mondi wrote: > > Hi Marian, > > > > On Mon, Mar 22, 2021 at 11:42:40AM +0100, Marian Cichy wrote: > > > By adding a controlList to the stream attributes, we have access to > > > several more configuration parameters for a stream. In this patch > > > series, it is for now only used to access the frame duration of a sensor. > > > > > > 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..a6347e31 100644 > > > --- a/include/libcamera/stream.h > > > +++ b/include/libcamera/stream.h > > > @@ -13,6 +13,7 @@ > > > #include <vector> > > > > > > #include <libcamera/buffer.h> > > > +#include <libcamera/controls.h> > > > #include <libcamera/geometry.h> > > > #include <libcamera/pixel_format.h> > > > > > > @@ -44,6 +45,7 @@ struct StreamConfiguration { > > > Size size; > > > unsigned int stride; > > > unsigned int frameSize; > > > + ControlList controls; > > > > Documentation is missing. > > > > In general, as discussed on irc, I guess this is an acceptable first > > step to be able to retrieve per-stream information. > > I picked up Marian's work to become familar with libcamera and now I > have my first question on this topic. Welcome on board :) > > Why is this necessary here? It think what we wanna do (retrieve the > framerate) can be done by the pipeline_handler ControlInfoMap. So we > only need to extended the simple-pipeline rather than adding it here, > like all other pipeline handlers does. Do you mean populating the Camera::contorls() info maps ? I tried to explain my understanding in my last reply to patch [2/3] https://patchwork.libcamera.org/patch/11639/ I'll try to summarize it here: - The current frame duration (as well as other parameters) depend on the currently applied configuration as they depend on what sizes/formats are requested and how the sensor is configured. - The ControlInfoMap instances access through Camera::controls() are initialized at pipeline creation time. We have to pick and euhristic to decide which sensor configuration to use, as the Camera's ControlInfoMap instances are populated using (also) the sensor's v4l2 controls, whose values and limits depend on the current configuration [*] - Ideally, we would need a mechanism to update the Camera's ControlInfoMaps when a new configuration is applied. It's a bit of work, and would need to be a mchanism that all pipeline handler can use [**] - As we don't have the above mechanism in place at the moment, the easiest way to do allow libcamera to expose to applications information that are configuration-dependent is to augment the CameraConfiguration (or StreamConfiguration as in this case) with a list of controls that report the current values - The 'issue' with this series is that Marian wanted to do at Camera::generateConfiguration() time, which for the reasons I tried to outline in https://patchwork.libcamera.org/patch/11639/ is not really possible. So yes, we're missing a mechanism to 1) Update the Camera::controls() when a Configuration is applied to the Camera 2) It could be worked around by adding controls to the configuration-dependent components (CameraConfiguration and StreamConfiguration) 3) However this can only be done after a configuration has been applied and the sensor driver has been configured and its controls values and limits updated. Maybe there's an easier way out I might have missed, what do you think ? Thanks j [*] The most notable example is the VBLANK limits, that change depending on the selected sensor's output sizes [**] RPi in example has a series that would allow to update the V4L2 controls on format change https://patchwork.libcamera.org/patch/11797/ This is a first step. With an updated v4l2 control info, also the Camera's controls info can be updated. > > Am I right or did I miss something? > > Regards, > Marco > > > > > > > > > > unsigned int bufferCount; > > > > > > -- > > > 2.29.2 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Jacopo, On 21-04-29 10:52, Jacopo Mondi wrote: > Hi Marco, > > On Wed, Apr 28, 2021 at 03:47:05PM +0200, Marco Felsch wrote: > > Hi Jacopo, > > > > On 21-03-22 18:43, Jacopo Mondi wrote: > > > Hi Marian, > > > > > > On Mon, Mar 22, 2021 at 11:42:40AM +0100, Marian Cichy wrote: > > > > By adding a controlList to the stream attributes, we have access to > > > > several more configuration parameters for a stream. In this patch > > > > series, it is for now only used to access the frame duration of a sensor. > > > > > > > > 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..a6347e31 100644 > > > > --- a/include/libcamera/stream.h > > > > +++ b/include/libcamera/stream.h > > > > @@ -13,6 +13,7 @@ > > > > #include <vector> > > > > > > > > #include <libcamera/buffer.h> > > > > +#include <libcamera/controls.h> > > > > #include <libcamera/geometry.h> > > > > #include <libcamera/pixel_format.h> > > > > > > > > @@ -44,6 +45,7 @@ struct StreamConfiguration { > > > > Size size; > > > > unsigned int stride; > > > > unsigned int frameSize; > > > > + ControlList controls; > > > > > > Documentation is missing. > > > > > > In general, as discussed on irc, I guess this is an acceptable first > > > step to be able to retrieve per-stream information. > > > > I picked up Marian's work to become familar with libcamera and now I > > have my first question on this topic. > > Welcome on board :) > > > > > Why is this necessary here? It think what we wanna do (retrieve the > > framerate) can be done by the pipeline_handler ControlInfoMap. So we > > only need to extended the simple-pipeline rather than adding it here, > > like all other pipeline handlers does. > > Do you mean populating the Camera::contorls() info maps ? Yep. > I tried to explain my understanding in my last reply to patch [2/3] > https://patchwork.libcamera.org/patch/11639/ > > I'll try to summarize it here: > > - The current frame duration (as well as other parameters) depend on > the currently applied configuration as they depend on what sizes/formats > are requested and how the sensor is configured. Yes. > - The ControlInfoMap instances access through Camera::controls() are > initialized at pipeline creation time. We have to pick and euhristic > to decide which sensor configuration to use, as the Camera's ControlInfoMap > instances are populated using (also) the sensor's v4l2 controls, whose values > and limits depend on the current configuration [*] Yes, this was the next topic I thought about. Currently there is no way to defer the initializiation for such controls? > - Ideally, we would need a mechanism to update the Camera's > ControlInfoMaps when a new configuration is applied. It's a bit of > work, and would need to be a mchanism that all pipeline handler can > use [**] Thanks for that pointer. > - As we don't have the above mechanism in place at the moment, the > easiest way to do allow libcamera to expose to applications > information that are configuration-dependent is to augment the > CameraConfiguration (or StreamConfiguration as in this case) with a > list of controls that report the current values But in that case we should mark it as RO so users are not trying to change the controls by StreamConfiguration. In other words, it should only mirror the values. In that case the controls are frame based too. > - The 'issue' with this series is that Marian wanted to do at > Camera::generateConfiguration() time, which for the reasons > I tried to outline in https://patchwork.libcamera.org/patch/11639/ > is not really possible. I've just wanted to gather more information, why this solution was taken. Thanks for this :) > So yes, we're missing a mechanism to > 1) Update the Camera::controls() when a Configuration is applied to > the Camera > 2) It could be worked around by adding controls to the > configuration-dependent components (CameraConfiguration and > StreamConfiguration) > 3) However this can only be done after a configuration has been > applied and the sensor driver has been configured and its controls > values and limits updated. > > Maybe there's an easier way out I might have missed, what do you think ? Currently I'm not that deep into libcamera.. so I'm maybe the wrong person. I would try to avoid duplications for the users. I'm now know the problem. Currently we can't init to some value which let the user know that it has to be initialized after the first configuration happend. Then we also need something like [**] to update the it control if there is a need to. Regards, Marco > > Thanks > j > > [*] The most notable example is the VBLANK limits, that change > depending on the selected sensor's output sizes > > [**] RPi in example has a series that would allow to update the V4L2 > controls on format change https://patchwork.libcamera.org/patch/11797/ > This is a first step. With an updated v4l2 control info, also the > Camera's controls info can be updated. > > > > Am I right or did I miss something? > > > > Regards, > > Marco > > > > > > > > > > > > > > > unsigned int bufferCount; > > > > > > > > -- > > > > 2.29.2 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > -- > > Pengutronix e.K. | | > > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index bb47c390..a6347e31 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -13,6 +13,7 @@ #include <vector> #include <libcamera/buffer.h> +#include <libcamera/controls.h> #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> @@ -44,6 +45,7 @@ struct StreamConfiguration { Size size; unsigned int stride; unsigned int frameSize; + ControlList controls; unsigned int bufferCount;
By adding a controlList to the stream attributes, we have access to several more configuration parameters for a stream. In this patch series, it is for now only used to access the frame duration of a sensor. Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> --- include/libcamera/stream.h | 2 ++ 1 file changed, 2 insertions(+)