[libcamera-devel] [PATCH v5 03/10] ipa: add rkisp1 metadata to fix Android HAL

[Jacopo Mondi]

Hi Nicholas

subject as

ipa: rkisp1: Add FrameDurationLimits control

This is not just for Android

On Thu, Oct 27, 2022 at 10:17:19PM -0500, Nicholas Roth via libcamera-devel wrote:
> From: Nicholas Roth <nicholas at rothemail.net>
>
> Currently, the Android HAL does not work on rkisp1-based devices because
> required FrameDurationLimits metadata is missing from the ISP

IPA = Image Processing Algorithm
it's the software component part of libcamera that drives the image
enhancement algorithms. IPA is libcamera lingo, in other context it
might be simply called "3A library" even if it does way more than just
Auto[exposure, white-balance, focus].

ISP = Image Signal Processor
it's the HW component that performs image enhancement computations and
produces statistics on the processed image

The two terms are not interchangeable

> implementation.
>
> This change introduces sensible defaults for FrameDurationLimits that
> should generally work most of the time.
>
> In theory, it should be possible to implement more sophisticated logic
> to detect frame durations. The appropriate API hooks for doing so exist
> and are used in IPAIPU3::updateControls(), although the implementation
> may be suspect. This is left as future work.
>
> Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
> ---
>  src/ipa/rkisp1/rkisp1.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index ba3c547e..dd12c341 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
>  	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> +	/* libcamera requires a fixed value for minimum frame duration,
> +	 * but this depends on the frame size and the rkisp1 device datasheets
> +	 * measure this in pixels per second. Neither the datasheets nor the driver

What are you referring to as "the rkisp1 device datasheet measures
this in pixels per second" ? Is it the ISP bandwidth ? Where is this
information available from ?

I'm still of opinion that we should report the sensor's duration
limits, but maybe I'm missing a point here.


> +	 * specify a maximum. The minimum below is for 1920x1920. The maximum

Is 1920x1920 is the RkISP1 self-path maximum output resolution that
you have used ? If the self-path has a scaler (something I'm not 100%
sure) images in such resolution might be obtained by scaling the frame
as produced by the sensor, or by simply cropping them.

If you connect to the ISP a sensor whose minimum resolution and
max framerate is 2592x1944 at 15FPS you would get a min frame duration of:

                (2592 + hblank) * (1944 + vblank) / pixel_rate  = 66666 usec

which is larger than what you report here, as the sensor would be
clocking out frames at such rate, regardless of the final output
resolution. Moreover I'm not sure how you got to 48505 from 1920x1920,
have you taken into account the pixel rate of the sensor you're using ?
This can't be hardcoded but should come from the CameraSensor,
different platforms use different sensors and we can't assume anything
about it in the IPA module.

I would say that the ISP output size limits should not be taken into
account. It would be different if we had a clear documentation about
how bandwidth limitations constraints the durations, but I'm afraid
the relationship between the two parameters is hard to measure, if
ever documented.

> +	 * corresponds to two seconds. */

Multi-line comments should be

        /*
         * comment on multiple
         * lines
         */

> +	{ &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },
>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
>
> --
> 2.34.1
>