<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for the review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 20 Dec 2020 at 05:38, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">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 Fri, Dec 18, 2020 at 10:06:25AM +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 | 41 ++++++++++++++++++++++++++++++++++<br>
> 1 file changed, 41 insertions(+)<br>
> <br>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml<br>
> index 6d6f0fee..a58bff18 100644<br>
> --- a/src/libcamera/control_ids.yaml<br>
> +++ b/src/libcamera/control_ids.yaml<br>
> @@ -306,6 +306,47 @@ 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>
> + When provided by applications, the control specifies the sensor frame<br>
> + duration interval the pipeline has to use. This could also limit the<br>
<br>
s/could also limit/limits/ ?<br></blockquote><div><br></div><div>Ack</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>
> + largest exposure time the sensor can use. For example, if a maximum<br>
> + frame duration of 33ms is requested (corresponding to 30 frames per<br>
> + second), the sensor will not be able to raise the exposure time above<br>
> + 33ms. A fixed frame duration is achieved by setting the minimum and<br>
> + maximum values to be the same.<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>
<br>
I think we should talk about exposure time instead of shutter speed, but<br>
we'll need to go through the whole documentation and code base to ensure<br>
consistency, so this can be done later.<br>
<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>
> + \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>
> + When reported by pipelines, the control expresses the minimum and<br>
<br>
Is this about Request::metadata(), or Camera::properties() ? I assume<br>
the former as it's defined in control_ids.yaml.<br></blockquote><div><br></div><div>This is through Request::metadata(). Should I perhaps reword to make this more explicit?</div><div><br></div><div>"When reported by pipelines through metadata, the control expresses...." ?<br></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>
> + maximum frame durations used after being clipped to what the current<br>
> + sensor mode supports, and what is achievable based on the exposure<br>
> + mode setting specified with controls::AeExposureMode or manual<br>
> + exposure time set through controls::ExposureTime. The sensor frame<br>
<br>
I have a hard time parsing this, and a hard time seeing how it should be<br>
used by applications :-S Do you need this for your use cases ?<br></blockquote><div><br></div><div>Yes, this was also a point of confusion between Jacopo and me :-)</div><div><br></div><div>What I was trying to convey is that the values returned through the metadata is the actual limits that will be used (after being constrained by the exposure mode and/or shutter speed), and may not be the same as what the application requested. I thought this might be useful for the application to know the actual limits used. If you think this may not be needed, I could remove this control from the metadata?</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>
> + duration is one of the parameter that defines the capture frame rate<br>
> + but it does not alone provide enough information to fully calculate it<br>
> + as it does not account for pipeline processing delays.<br>
<br>
Delays in the pipeline don't affect the frame duration, do they ? We<br>
could drop frames when processing streams in software (for instance for<br>
JPEG compression), but that's out of scope as libcamera doesn't support<br>
that.<br></blockquote><div><br></div><div>If you have, e.g. a slow software denoise running in the IPA whose processing time limits your frame throughput, it could be limiting the overall pipeline framerate (increasing frame duration). Do you think perhaps this should not be mentioned here to avoid complicating things for now?</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>
> +<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>
> + size: [2]<br>
> +<br>
> # ----------------------------------------------------------------------------<br>
> # Draft controls section<br>
> <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>