<div dir="ltr"><div dir="ltr">Hi all,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 28 Oct 2022 at 10:09, Jacopo Mondi via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</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 Nicholas<br>
<br>
subject as<br>
<br>
ipa: rkisp1: Add FrameDurationLimits control<br>
<br>
This is not just for Android<br>
<br>
On Thu, Oct 27, 2022 at 10:17:19PM -0500, Nicholas Roth via libcamera-devel wrote:<br>
> From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
><br>
> Currently, the Android HAL does not work on rkisp1-based devices because<br>
> required FrameDurationLimits metadata is missing from the ISP<br>
<br>
IPA = Image Processing Algorithm<br>
it's the software component part of libcamera that drives the image<br>
enhancement algorithms. IPA is libcamera lingo, in other context it<br>
might be simply called "3A library" even if it does way more than just<br>
Auto[exposure, white-balance, focus].<br>
<br>
ISP = Image Signal Processor<br>
it's the HW component that performs image enhancement computations and<br>
produces statistics on the processed image<br>
<br>
The two terms are not interchangeable<br>
<br>
> implementation.<br>
><br>
> This change introduces sensible defaults for FrameDurationLimits that<br>
> should generally work most of the time.<br>
><br>
> In theory, it should be possible to implement more sophisticated logic<br>
> to detect frame durations. The appropriate API hooks for doing so exist<br>
> and are used in IPAIPU3::updateControls(), although the implementation<br>
> may be suspect. This is left as future work.<br>
><br>
> Signed-off-by: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
> ---<br>
>  src/ipa/rkisp1/rkisp1.cpp | 6 ++++++<br>
>  1 file changed, 6 insertions(+)<br>
><br>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp<br>
> index ba3c547e..dd12c341 100644<br>
> --- a/src/ipa/rkisp1/rkisp1.cpp<br>
> +++ b/src/ipa/rkisp1/rkisp1.cpp<br>
> @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{<br>
>       { &controls::Contrast, ControlInfo(0.0f, 1.993f) },<br>
>       { &controls::Saturation, ControlInfo(0.0f, 1.993f) },<br>
>       { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },<br>
> +     /* libcamera requires a fixed value for minimum frame duration,<br>
> +      * but this depends on the frame size and the rkisp1 device datasheets<br>
> +      * measure this in pixels per second. Neither the datasheets nor the driver<br>
<br>
What are you referring to as "the rkisp1 device datasheet measures<br>
this in pixels per second" ? Is it the ISP bandwidth ? Where is this<br>
information available from ?<br>
<br>
I'm still of opinion that we should report the sensor's duration<br>
limits, but maybe I'm missing a point here.<br>
<br>
<br>
> +      * specify a maximum. The minimum below is for 1920x1920. The maximum<br>
<br>
Is 1920x1920 is the RkISP1 self-path maximum output resolution that<br>
you have used ? If the self-path has a scaler (something I'm not 100%<br>
sure) images in such resolution might be obtained by scaling the frame<br>
as produced by the sensor, or by simply cropping them.<br>
<br>
If you connect to the ISP a sensor whose minimum resolution and<br>
max framerate is 2592x1944@15FPS you would get a min frame duration of:<br>
<br>
                (2592 + hblank) * (1944 + vblank) / pixel_rate  = 66666 usec<br>
<br>
which is larger than what you report here, as the sensor would be<br>
clocking out frames at such rate, regardless of the final output<br>
resolution. Moreover I'm not sure how you got to 48505 from 1920x1920,<br>
have you taken into account the pixel rate of the sensor you're using ?<br>
This can't be hardcoded but should come from the CameraSensor,<br>
different platforms use different sensors and we can't assume anything<br>
about it in the IPA module.<br>
<br>
I would say that the ISP output size limits should not be taken into<br>
account. It would be different if we had a clear documentation about<br>
how bandwidth limitations constraints the durations, but I'm afraid<br>
the relationship between the two parameters is hard to measure, if<br>
ever documented.<br></blockquote><div><br></div><div>I agree with Jacopo on this - FrameDurationLimits ought to specify the sensor<br>limits and not the ISP.  I can't speak for other platforms, but on a<br>Raspberry Pi the ISP throughput depends on many many things, such as<br>pipeline configuration, hardware clock rate, memory bandwidth availability to<br>name a few.  It would be wholly impractical to provide such a number in the<br>FrameDurationLimits control.<br></div><div><br></div><div>Naush</div><div><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>
> +      * corresponds to two seconds. */<br>
<br>
Multi-line comments should be<br>
<br>
        /*<br>
         * comment on multiple<br>
         * lines<br>
         */<br>
<br>
> +     { &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },<br>
>       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },<br>
>  };<br>
><br>
> --<br>
> 2.34.1<br>
><br>
</blockquote></div></div>