<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your comments.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 11 Jan 2021 at 23:07, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Wed, Jan 06, 2021 at 10:06:57AM +0000, Naushir Patuck wrote:<br>
> Add an int64_t array control (controls::FrameDurations) to specify the<br>
> minimum and maximum (in that order) frame duration to be used by the<br>
> camera sensor.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ---<br>
> src/libcamera/control_ids.yaml | 38 ++++++++++++++++++++++++++++++++++<br>
> 1 file changed, 38 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml<br>
> index 6d6f0fee..d1240beb 100644<br>
> --- a/src/libcamera/control_ids.yaml<br>
> +++ b/src/libcamera/control_ids.yaml<br>
> @@ -306,6 +306,44 @@ controls:<br>
> maximum valid value is given by the properties::ScalerCropMaximum<br>
> property, and the two can be used to implement digital zoom.<br>
> <br>
> + - FrameDurations:<br>
> + type: int64_t<br>
> + description: |<br>
> + The minimum and maximum (in that order) frame duration,<br>
> + expressed in micro-seconds.<br>
<br>
s/micro-seconds/microseconds/<br>
<br>
(The indentation should be 2 spaces btw)<br>
<br>
> +<br>
> + When provided by applications, the control specifies the sensor frame<br>
> + duration interval the pipeline has to use. This limits the largest<br>
> + exposure time the sensor can use. For example, if a maximum frame<br>
> + duration of 33ms is requested (corresponding to 30 frames per second),<br>
> + the sensor will not be able to raise the exposure time above 33ms.<br>
> + A fixed frame duration is achieved by setting the minimum and maximum<br>
> + values to be the same. Setting both values to 0 reverts to using the<br>
> + IPA provided defaults.<br>
> +<br>
> + The maximum frame duration provides the absolute limit to the shutter<br>
> + speed computed by the AE algorithm and it overrides any exposure mode<br>
> + setting specified with controls::AeExposureMode. Similarly, when a<br>
> + manual exposure time is set through controls::ExposureTime, it also<br>
> + gets clipped to the limits set by this control.<br>
<br>
This looks good.<br>
<br>
> When reported in<br>
> + metadata, the control expresses the minimum and maximum frame<br>
> + durations used after being clipped to these limits.<br>
> +<br>
<br>
But this sounds weird. The previous part states that FrameDurations has<br>
higher priority than all other parameters, but then this sentence says<br>
it's clipped by "these limits".<br></blockquote><div><br></div><div>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:</div><div><br></div><div>When reported in metadata, the control expresses the minimum and maximum frame durations used after being clipped to the sensor provided frame duration limits.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + \sa AeExposureMode<br>
> + \sa ExposureTime<br>
> +<br>
> + \todo Refer to the frame duration limits property to describe how<br>
> + application-provided values gets clipped and reset.<br>
<br>
It hasn't been long, and the context is already starting to escape me.<br>
Would it be possible to expand this just a little bit so that we'll know<br>
what it means in 3 months time ?<br></blockquote><div><br></div><div>Perhaps this makes more sense given the rewording above? Or maybe a reword as follows:</div><div><br></div><div>\todo Refer to the frame duration limits property to describe how application-provided values get clipped to what is supported by the sensor mode.<br></div><div><br></div><div>Hopefully that makes things more readable?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + \todo Define how to calculate the capture frame rate by<br>
> + defining controls to report additional delays introduced by<br>
> + the capture pipeline or post-processing stages (ie JPEG<br>
> + conversion, frame scaling).<br>
> +<br>
> + \todo Provide an explicit definition of default control values, for<br>
> + this and all other controls.<br>
> + size: [2]<br>
> +<br>
> # ----------------------------------------------------------------------------<br>
> # Draft controls section<br>
> <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>