| Message ID | 20210106100659.8363-2-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Wed, Jan 06, 2021 at 10:06:57AM +0000, Naushir Patuck wrote: > Add an int64_t array control (controls::FrameDurations) to specify the > minimum and maximum (in that order) frame duration to be used by the > camera sensor. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > Tested-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_ids.yaml | 38 ++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index 6d6f0fee..d1240beb 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -306,6 +306,44 @@ controls: > maximum valid value is given by the properties::ScalerCropMaximum > property, and the two can be used to implement digital zoom. > > + - FrameDurations: > + type: int64_t > + description: | > + The minimum and maximum (in that order) frame duration, > + expressed in micro-seconds. s/micro-seconds/microseconds/ (The indentation should be 2 spaces btw) > + > + When provided by applications, the control specifies the sensor frame > + duration interval the pipeline has to use. This limits the largest > + exposure time the sensor can use. For example, if a maximum frame > + duration of 33ms is requested (corresponding to 30 frames per second), > + the sensor will not be able to raise the exposure time above 33ms. > + A fixed frame duration is achieved by setting the minimum and maximum > + values to be the same. Setting both values to 0 reverts to using the > + IPA provided defaults. > + > + The maximum frame duration provides the absolute limit to the shutter > + speed computed by the AE algorithm and it overrides any exposure mode > + setting specified with controls::AeExposureMode. Similarly, when a > + manual exposure time is set through controls::ExposureTime, it also > + gets clipped to the limits set by this control. This looks good. > When reported in > + metadata, the control expresses the minimum and maximum frame > + durations used after being clipped to these limits. > + But this sounds weird. The previous part states that FrameDurations has higher priority than all other parameters, but then this sentence says it's clipped by "these limits". > + \sa AeExposureMode > + \sa ExposureTime > + > + \todo Refer to the frame duration limits property to describe how > + application-provided values gets clipped and reset. It hasn't been long, and the context is already starting to escape me. Would it be possible to expand this just a little bit so that we'll know what it means in 3 months time ? > + > + \todo Define how to calculate the capture frame rate by > + defining controls to report additional delays introduced by > + the capture pipeline or post-processing stages (ie JPEG > + conversion, frame scaling). > + > + \todo Provide an explicit definition of default control values, for > + this and all other controls. > + size: [2] > + > # ---------------------------------------------------------------------------- > # Draft controls section >
Hi Laurent, Thank you for your comments. On Mon, 11 Jan 2021 at 23:07, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Wed, Jan 06, 2021 at 10:06:57AM +0000, Naushir Patuck wrote: > > Add an int64_t array control (controls::FrameDurations) to specify the > > minimum and maximum (in that order) frame duration to be used by the > > camera sensor. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/control_ids.yaml | 38 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/src/libcamera/control_ids.yaml > b/src/libcamera/control_ids.yaml > > index 6d6f0fee..d1240beb 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -306,6 +306,44 @@ controls: > > maximum valid value is given by the > properties::ScalerCropMaximum > > property, and the two can be used to implement digital zoom. > > > > + - FrameDurations: > > + type: int64_t > > + description: | > > + The minimum and maximum (in that order) frame duration, > > + expressed in micro-seconds. > > s/micro-seconds/microseconds/ > > (The indentation should be 2 spaces btw) > > > + > > + When provided by applications, the control specifies the > sensor frame > > + duration interval the pipeline has to use. This limits the > largest > > + exposure time the sensor can use. For example, if a maximum > frame > > + duration of 33ms is requested (corresponding to 30 frames per > second), > > + the sensor will not be able to raise the exposure time above > 33ms. > > + A fixed frame duration is achieved by setting the minimum and > maximum > > + values to be the same. Setting both values to 0 reverts to > using the > > + IPA provided defaults. > > + > > + The maximum frame duration provides the absolute limit to the > shutter > > + speed computed by the AE algorithm and it overrides any > exposure mode > > + setting specified with controls::AeExposureMode. Similarly, > when a > > + manual exposure time is set through controls::ExposureTime, > it also > > + gets clipped to the limits set by this control. > > This looks good. > > > When reported in > > + metadata, the control expresses the minimum and maximum frame > > + durations used after being clipped to these limits. > > + > > But this sounds weird. The previous part states that FrameDurations has > higher priority than all other parameters, but then this sentence says > it's clipped by "these limits". > You are right, this does not read correct. I wanted to express that the frame durations provided may be further limited by what the sensor mode can actually achieve. How about replacing the above paragraph of text with the following: When reported in metadata, the control expresses the minimum and maximum frame durations used after being clipped to the sensor provided frame duration limits. > > > + \sa AeExposureMode > > + \sa ExposureTime > > + > > + \todo Refer to the frame duration limits property to describe > how > > + application-provided values gets clipped and reset. > > It hasn't been long, and the context is already starting to escape me. > Would it be possible to expand this just a little bit so that we'll know > what it means in 3 months time ? > Perhaps this makes more sense given the rewording above? Or maybe a reword as follows: \todo Refer to the frame duration limits property to describe how application-provided values get clipped to what is supported by the sensor mode. Hopefully that makes things more readable? Regards, Naush > > + > > + \todo Define how to calculate the capture frame rate by > > + defining controls to report additional delays introduced by > > + the capture pipeline or post-processing stages (ie JPEG > > + conversion, frame scaling). > > + > > + \todo Provide an explicit definition of default control > values, for > > + this and all other controls. > > + size: [2] > > + > > # > ---------------------------------------------------------------------------- > > # Draft controls section > > > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Tue, Jan 12, 2021 at 07:17:14AM +0000, Naushir Patuck wrote: > On Mon, 11 Jan 2021 at 23:07, Laurent Pinchart wrote: > > On Wed, Jan 06, 2021 at 10:06:57AM +0000, Naushir Patuck wrote: > > > Add an int64_t array control (controls::FrameDurations) to specify the > > > minimum and maximum (in that order) frame duration to be used by the > > > camera sensor. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/control_ids.yaml | 38 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 38 insertions(+) > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > index 6d6f0fee..d1240beb 100644 > > > --- a/src/libcamera/control_ids.yaml > > > +++ b/src/libcamera/control_ids.yaml > > > @@ -306,6 +306,44 @@ controls: > > > maximum valid value is given by the properties::ScalerCropMaximum > > > property, and the two can be used to implement digital zoom. > > > > > > + - FrameDurations: > > > + type: int64_t > > > + description: | > > > + The minimum and maximum (in that order) frame duration, > > > + expressed in micro-seconds. > > > > s/micro-seconds/microseconds/ > > > > (The indentation should be 2 spaces btw) > > > > > + > > > + When provided by applications, the control specifies the sensor frame > > > + duration interval the pipeline has to use. This limits the largest > > > + exposure time the sensor can use. For example, if a maximum frame > > > + duration of 33ms is requested (corresponding to 30 frames per second), > > > + the sensor will not be able to raise the exposure time above 33ms. > > > + A fixed frame duration is achieved by setting the minimum and maximum > > > + values to be the same. Setting both values to 0 reverts to using the > > > + IPA provided defaults. > > > + > > > + The maximum frame duration provides the absolute limit to the shutter > > > + speed computed by the AE algorithm and it overrides any exposure mode > > > + setting specified with controls::AeExposureMode. Similarly, when a > > > + manual exposure time is set through controls::ExposureTime, it also > > > + gets clipped to the limits set by this control. > > > > This looks good. > > > > > When reported in > > > + metadata, the control expresses the minimum and maximum frame > > > + durations used after being clipped to these limits. > > > + > > > > But this sounds weird. The previous part states that FrameDurations has > > higher priority than all other parameters, but then this sentence says > > it's clipped by "these limits". > > You are right, this does not read correct. I wanted to express that the > frame durations provided may be further limited by what the sensor mode can > actually achieve. How about replacing the above paragraph of text with the > following: > > When reported in metadata, the control expresses the minimum and maximum > frame durations used after being clipped to the sensor provided frame > duration limits. Sounds good to me. > > > + \sa AeExposureMode > > > + \sa ExposureTime > > > + > > > + \todo Refer to the frame duration limits property to describe how > > > + application-provided values gets clipped and reset. > > > > It hasn't been long, and the context is already starting to escape me. > > Would it be possible to expand this just a little bit so that we'll know > > what it means in 3 months time ? > > Perhaps this makes more sense given the rewording above? Or maybe a reword > as follows: > > \todo Refer to the frame duration limits property to describe how > application-provided values get clipped to what is supported by the sensor > mode. > > Hopefully that makes things more readable? Not quite I'm afraid, but maybe it's just too early in the morning :-) Is this about documenting how other properties also get clipped by the sensor mode ? Or something else ? > > > + > > > + \todo Define how to calculate the capture frame rate by > > > + defining controls to report additional delays introduced by > > > + the capture pipeline or post-processing stages (ie JPEG > > > + conversion, frame scaling). > > > + > > > + \todo Provide an explicit definition of default control values, for > > > + this and all other controls. > > > + size: [2] > > > + > > > # ---------------------------------------------------------------------------- > > > # Draft controls section > > >
Hi Laurent, On Thu, 14 Jan 2021 at 06:04, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, Jan 12, 2021 at 07:17:14AM +0000, Naushir Patuck wrote: > > On Mon, 11 Jan 2021 at 23:07, Laurent Pinchart wrote: > > > On Wed, Jan 06, 2021 at 10:06:57AM +0000, Naushir Patuck wrote: > > > > Add an int64_t array control (controls::FrameDurations) to specify > the > > > > minimum and maximum (in that order) frame duration to be used by the > > > > camera sensor. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/control_ids.yaml | 38 > ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 38 insertions(+) > > > > > > > > diff --git a/src/libcamera/control_ids.yaml > b/src/libcamera/control_ids.yaml > > > > index 6d6f0fee..d1240beb 100644 > > > > --- a/src/libcamera/control_ids.yaml > > > > +++ b/src/libcamera/control_ids.yaml > > > > @@ -306,6 +306,44 @@ controls: > > > > maximum valid value is given by the > properties::ScalerCropMaximum > > > > property, and the two can be used to implement digital zoom. > > > > > > > > + - FrameDurations: > > > > + type: int64_t > > > > + description: | > > > > + The minimum and maximum (in that order) frame duration, > > > > + expressed in micro-seconds. > > > > > > s/micro-seconds/microseconds/ > > > > > > (The indentation should be 2 spaces btw) > > > > > > > + > > > > + When provided by applications, the control specifies the > sensor frame > > > > + duration interval the pipeline has to use. This limits > the largest > > > > + exposure time the sensor can use. For example, if a > maximum frame > > > > + duration of 33ms is requested (corresponding to 30 frames > per second), > > > > + the sensor will not be able to raise the exposure time > above 33ms. > > > > + A fixed frame duration is achieved by setting the minimum > and maximum > > > > + values to be the same. Setting both values to 0 reverts > to using the > > > > + IPA provided defaults. > > > > + > > > > + The maximum frame duration provides the absolute limit to > the shutter > > > > + speed computed by the AE algorithm and it overrides any > exposure mode > > > > + setting specified with controls::AeExposureMode. > Similarly, when a > > > > + manual exposure time is set through > controls::ExposureTime, it also > > > > + gets clipped to the limits set by this control. > > > > > > This looks good. > > > > > > > When reported in > > > > + metadata, the control expresses the minimum and maximum > frame > > > > + durations used after being clipped to these limits. > > > > + > > > > > > But this sounds weird. The previous part states that FrameDurations has > > > higher priority than all other parameters, but then this sentence says > > > it's clipped by "these limits". > > > > You are right, this does not read correct. I wanted to express that the > > frame durations provided may be further limited by what the sensor mode > can > > actually achieve. How about replacing the above paragraph of text with > the > > following: > > > > When reported in metadata, the control expresses the minimum and maximum > > frame durations used after being clipped to the sensor provided frame > > duration limits. > > Sounds good to me. > > > > > + \sa AeExposureMode > > > > + \sa ExposureTime > > > > + > > > > + \todo Refer to the frame duration limits property to > describe how > > > > + application-provided values gets clipped and reset. > > > > > > It hasn't been long, and the context is already starting to escape me. > > > Would it be possible to expand this just a little bit so that we'll > know > > > what it means in 3 months time ? > > > > Perhaps this makes more sense given the rewording above? Or maybe a > reword > > as follows: > > > > \todo Refer to the frame duration limits property to describe how > > application-provided values get clipped to what is supported by the > sensor > > mode. > > > > Hopefully that makes things more readable? > > Not quite I'm afraid, but maybe it's just too early in the morning :-) > > Is this about documenting how other properties also get clipped by the > sensor mode ? Or something else ? > It's about how the frame durations are clipped by the sensor mode limits - as advertised by the sensor properties in the future. We can remove this statement entirely if you do not think it's appropriate, or a rewording as follows: \todo Refer to the frame duration limits property (when available) to obtain sensor mode limits used for clipping the application-provided values. Regards, Naush > > > > > + > > > > + \todo Define how to calculate the capture frame rate by > > > > + defining controls to report additional delays introduced > by > > > > + the capture pipeline or post-processing stages (ie JPEG > > > > + conversion, frame scaling). > > > > + > > > > + \todo Provide an explicit definition of default control > values, for > > > > + this and all other controls. > > > > + size: [2] > > > > + > > > > # > ---------------------------------------------------------------------------- > > > > # Draft controls section > > > > > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Thu, Jan 14, 2021 at 07:34:20AM +0000, Naushir Patuck wrote: > Hi Laurent, > > > On Thu, 14 Jan 2021 at 06:04, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > > > Hi Naush, > > [snip] > > > > > > > > > When reported in > > > > > + metadata, the control expresses the minimum and maximum > > frame > > > > > + durations used after being clipped to these limits. > > > > > + > > > > > > > > But this sounds weird. The previous part states that FrameDurations has > > > > higher priority than all other parameters, but then this sentence says > > > > it's clipped by "these limits". > > > > > > You are right, this does not read correct. I wanted to express that the > > > frame durations provided may be further limited by what the sensor mode > > can > > > actually achieve. How about replacing the above paragraph of text with > > the > > > following: > > > > > > When reported in metadata, the control expresses the minimum and maximum > > > frame durations used after being clipped to the sensor provided frame > > > duration limits. > > > > Sounds good to me. > > > > > > > + \sa AeExposureMode > > > > > + \sa ExposureTime > > > > > + > > > > > + \todo Refer to the frame duration limits property to > > describe how > > > > > + application-provided values gets clipped and reset. > > > > > > > > It hasn't been long, and the context is already starting to escape me. > > > > Would it be possible to expand this just a little bit so that we'll > > know > > > > what it means in 3 months time ? > > > > > > Perhaps this makes more sense given the rewording above? Or maybe a > > reword > > > as follows: > > > > > > \todo Refer to the frame duration limits property to describe how > > > application-provided values get clipped to what is supported by the > > sensor > > > mode. > > > > > > Hopefully that makes things more readable? I know where this last statment came from, as I had a frame durations limit property on its way. But as it has not been finalized, and I'm currently questioning if it is really required or not, can we drop this last part so that the last obstacle for this series to be merged is removed ? Thanks j > > > > Not quite I'm afraid, but maybe it's just too early in the morning :-) > > > > Is this about documenting how other properties also get clipped by the > > sensor mode ? Or something else ? > > > > It's about how the frame durations are clipped by the sensor mode limits - > as advertised by the sensor properties in the future. > We can remove this statement entirely if you do not think it's appropriate, > or a rewording as follows: > > \todo Refer to the frame duration limits property (when available) to > obtain sensor > mode limits used for clipping the application-provided values. > > Regards, > Naush > > > > > > > > > > + > > > > > + \todo Define how to calculate the capture frame rate by > > > > > + defining controls to report additional delays introduced > > by > > > > > + the capture pipeline or post-processing stages (ie JPEG > > > > > + conversion, frame scaling). > > > > > + > > > > > + \todo Provide an explicit definition of default control > > values, for > > > > > + this and all other controls. > > > > > + size: [2] > > > > > + > > > > > # > > ---------------------------------------------------------------------------- > > > > > # Draft controls section > > > > > > > > > -- > > Regards, > > > > Laurent Pinchart > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Tue, 19 Jan 2021 at 15:15, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Naush, > > On Thu, Jan 14, 2021 at 07:34:20AM +0000, Naushir Patuck wrote: > > Hi Laurent, > > > > > > On Thu, 14 Jan 2021 at 06:04, Laurent Pinchart < > > laurent.pinchart@ideasonboard.com> wrote: > > > > > Hi Naush, > > > > > [snip] > > > > > > > > > > > > When reported in > > > > > > + metadata, the control expresses the minimum and > maximum > > > frame > > > > > > + durations used after being clipped to these limits. > > > > > > + > > > > > > > > > > But this sounds weird. The previous part states that > FrameDurations has > > > > > higher priority than all other parameters, but then this sentence > says > > > > > it's clipped by "these limits". > > > > > > > > You are right, this does not read correct. I wanted to express that > the > > > > frame durations provided may be further limited by what the sensor > mode > > > can > > > > actually achieve. How about replacing the above paragraph of text > with > > > the > > > > following: > > > > > > > > When reported in metadata, the control expresses the minimum and > maximum > > > > frame durations used after being clipped to the sensor provided frame > > > > duration limits. > > > > > > Sounds good to me. > > > > > > > > > + \sa AeExposureMode > > > > > > + \sa ExposureTime > > > > > > + > > > > > > + \todo Refer to the frame duration limits property to > > > describe how > > > > > > + application-provided values gets clipped and reset. > > > > > > > > > > It hasn't been long, and the context is already starting to escape > me. > > > > > Would it be possible to expand this just a little bit so that we'll > > > know > > > > > what it means in 3 months time ? > > > > > > > > Perhaps this makes more sense given the rewording above? Or maybe a > > > reword > > > > as follows: > > > > > > > > \todo Refer to the frame duration limits property to describe how > > > > application-provided values get clipped to what is supported by the > > > sensor > > > > mode. > > > > > > > > Hopefully that makes things more readable? > > I know where this last statment came from, as I had a frame durations > limit property on its way. But as it has not been finalized, and I'm > currently questioning if it is really required or not, can we drop > this last part so that the last obstacle for this series to be merged > is removed ? > Yes, I am fine with removing this todo statement if this property is not confirmed to be introduced. I will post an update with it removed, as well as the previous wording changes as discussed with Laurent. Hopefully it should then be good for merging. Regards, Naush > > Thanks > j > > > > > > > Not quite I'm afraid, but maybe it's just too early in the morning :-) > > > > > > Is this about documenting how other properties also get clipped by the > > > sensor mode ? Or something else ? > > > > > > > It's about how the frame durations are clipped by the sensor mode limits > - > > as advertised by the sensor properties in the future. > > We can remove this statement entirely if you do not think it's > appropriate, > > or a rewording as follows: > > > > \todo Refer to the frame duration limits property (when available) to > > obtain sensor > > mode limits used for clipping the application-provided values. > > > > Regards, > > Naush > > > > > > > > > > > > > > > + > > > > > > + \todo Define how to calculate the capture frame rate > by > > > > > > + defining controls to report additional delays > introduced > > > by > > > > > > + the capture pipeline or post-processing stages (ie > JPEG > > > > > > + conversion, frame scaling). > > > > > > + > > > > > > + \todo Provide an explicit definition of default > control > > > values, for > > > > > > + this and all other controls. > > > > > > + size: [2] > > > > > > + > > > > > > # > > > > ---------------------------------------------------------------------------- > > > > > > # Draft controls section > > > > > > > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index 6d6f0fee..d1240beb 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -306,6 +306,44 @@ controls: maximum valid value is given by the properties::ScalerCropMaximum property, and the two can be used to implement digital zoom. + - FrameDurations: + type: int64_t + description: | + The minimum and maximum (in that order) frame duration, + expressed in micro-seconds. + + When provided by applications, the control specifies the sensor frame + duration interval the pipeline has to use. This limits the largest + exposure time the sensor can use. For example, if a maximum frame + duration of 33ms is requested (corresponding to 30 frames per second), + the sensor will not be able to raise the exposure time above 33ms. + A fixed frame duration is achieved by setting the minimum and maximum + values to be the same. Setting both values to 0 reverts to using the + IPA provided defaults. + + The maximum frame duration provides the absolute limit to the shutter + speed computed by the AE algorithm and it overrides any exposure mode + setting specified with controls::AeExposureMode. Similarly, when a + manual exposure time is set through controls::ExposureTime, it also + gets clipped to the limits set by this control. When reported in + metadata, the control expresses the minimum and maximum frame + durations used after being clipped to these limits. + + \sa AeExposureMode + \sa ExposureTime + + \todo Refer to the frame duration limits property to describe how + application-provided values gets clipped and reset. + + \todo Define how to calculate the capture frame rate by + defining controls to report additional delays introduced by + the capture pipeline or post-processing stages (ie JPEG + conversion, frame scaling). + + \todo Provide an explicit definition of default control values, for + this and all other controls. + size: [2] + # ---------------------------------------------------------------------------- # Draft controls section