[libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add "Windows" Metering mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Apr 26 09:04:16 CEST 2023
Hello,
On Tue, Apr 04, 2023 at 12:53:26PM +0200, Jacopo Mondi via libcamera-devel wrote:
> On Tue, Apr 04, 2023 at 11:39:46AM +0200, Daniel Semkowicz wrote:
> > On Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi wrote:
> > > On Fri, Mar 31, 2023 at 10:19:27AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > > Add platform related code for configuring auto focus window on the
> > > > rkisp1. Connect to the windowUpdateRequested() signal exposed by the
> > > > AfHillClimbing and configure the window on each signal emission.
> > > > This enables support of AfMetering and AfWindows controls on the rkisp1
> > > > platform.
> > > >
> > > > Currently, only one window is enabled, but ISP allows up to three
> > > > of them.
> > > >
> > > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > > ---
> > > > src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++-
> > > > src/ipa/rkisp1/algorithms/af.h | 13 ++++++-
> > > > 2 files changed, 75 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp
> > > > index fde924d4..b6f6eee4 100644
> > > > --- a/src/ipa/rkisp1/algorithms/af.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/af.cpp
> > > > @@ -32,16 +32,43 @@ namespace ipa::rkisp1::algorithms {
> > > > * amount of time on each movement. This parameter should be set according
> > > > * to the worst case - the number of frames it takes to move lens between
> > > > * limit positions.
> > > > + * - **isp-threshold:** Threshold used for minimizing the influence of noise.
> > > > + * This affects the ISP sharpness calculation.
> > > > + * - **isp-var-shift:** The number of bits for the shift operation at the end
> > > > + * of the calculation chain. This affects the ISP sharpness calculation.
> > >
> > > I wonder if the introduction of these values belong to this patch or
> > > not. I guess they affect the AF algorithm globally, as a default
> > > window has to be programmed to have it working (something that happens in this
> > > patch, you might say :)
> >
> > You are right, these parameters should be moved to the previous commit.
> > I found that if no AF window was configured, ISP has a default one
> > (probably maximum AF window size), but this is not documented. This
> > way rkisp1 AF algo should work even without this commit.
> >
> > > Regardless of that, have you been able to identify with a little more
> > > accuracy how these values should be computed ? For the threshold I
> > > guess the explanation is somewhat clear, but the var-shift parameter I
> > > wonder how one should compute it ? As I read it, the var-shift value
> > > represents the number number of bits to right-shift the pixel values
> > > before summing them to avoid overflows of the afm_sum register (32
> > > bits). How did you come up with a value of 4 in your configuration
> > > file ? Does this value depend on the window size or does it depend on
> > > the sensor in use ?
> >
> > I came up with 4 just by experimenting with my setup. It worked even
> > with 0, but I shifted it a little bit for safety.
> > There is very little documentation on this, so unfortunately I know
> > only as much as you.
>
> a 4 position bit-shift does halve the information for an 8-bit Bayer
> sensor... I presume this is safe when it comes to overflow but reduces
> the accuracy.
>
> Ideally this value would be tuned according to the number of pixels
> that will be summed (per window ??) and the sensor's bit depth. By
> dividing (2^32-1 / num_pixels) you would get what the maximum allowed
> value per pixel would be and then you would have to adust right shift
> enough to make sure all your pixel values staty in that limit.
>
> I'm not asking to do this right now, but I wonder if these values
> should really come from configuration file or should be computed here
> (hard-coded for the moment).
The shift value should be computed, it shouldn't come from the tuning
file.
Both the sharpness and luminance are computed on the 8 MSBs of the pixel
value as far as I can tell, so you don't need to take the bit depth of
the sensor into account, only the window size.
The var_shift field actually combines two shift values, one for the
sharpness (in bits [2:0], you can call it afm_var_shift) and one for the
luminance (in bits [18:16], you can call it lum_var_shift). The reason
why two different shifts are needed is that the luminance value is
linear, while the sharpness value is quadratic.
The luminance is stored in a 24-bit register value. It's simple to
compute the shift needed to avoid overflows:
uint32_t pixelCount = window.width * window.height;
unsigned int div = (pixelCount + 255 * 255 - 1) / (255 * 255);
unsigned int shift = div > 1 ? 32 - __builtin_clz(div - 1) : 0;
The hardware computes the sharpness value by summing the square of the
gradients in the window, using the Tenengrad function.
\[ \sum_{i=1}^{M} \sum_{j=1}^{N} G(i,j) \]
G is the gradient squared:
G(i,j) = G_{x}(i,j}^{2} + G_{y}(i,j}^{2}
G_{x} and G_{y} are the gradients in the horizontal and vertical
directions respectively, computed using Sobel filters (see
https://en.wikipedia.org/wiki/Sobel_operator for those).
If I'm not mistaken, the worst case input is a black and white
chessboard pattern with squares of 2x2 pixels. A sample 3x3 block of
pixels would have the following values:
[ 255 0 0
0 255 255
0 255 255 ]
This gives
G_{x}(i,j) = 1*255 + 0*0 + (-1)*0
+ 2*0 + 0*255 + (-2)*255
+ 1*0 + 0*255 + (-1)*255
= 510
Similarly, G_{y}(i,j) is also equal to 510. I'll leave to the reader the
exercise of checking that the value is the same for all (i,j). The
square of the gradient for a pixel is thus equal to
510^{2} + 510^{2} = 520200
The gradient is calculated on the green pixels only, so the image is
effectively subsampled by a factor of 2 horizontally. For a window of
128x128 pixels, we would thus get an accumulated sharpness value of
520200 * (128/2) * 128 = 4261478400
Now, I'm slightly annoyed, because documentation I have access to
mentions a maximum value of 4227989504 for a window size of 128x128. The
difference is small, but it shows I'm missing something somewhere
(there's also a chance the documentation is wrong, but that wouldn't be
my first bet). We could start by using the above calculation to be on
the safe side, as it gives a very slightly worst case than the value
from the documentation. The calculations should be captured in a comment
in the code.
The threshold value should likely be dynamic too, as it should depend on
the noise level, but that can be done later.
> Did you ever experienced overflow ?
>
> > > > * .
> > > > * \sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning
> > > > * parameters.
> > > > *
> > > > * \todo Model the lens delay as number of frames required for the lens position
> > > > * to stabilize in the CameraLens class.
> > > > + * \todo Check if requested window size is valid. RkISP supports AF window size
> > > > + * few pixels smaller than sensor output size.
> > > > + * \todo Implement support for all available AF windows. RkISP supports up to 3
> > > > + * AF windows.
> > > > */
> > > >
> > > > LOG_DEFINE_CATEGORY(RkISP1Af)
> > > >
> > > > +namespace {
> > > > +
> > > > +constexpr rkisp1_cif_isp_window rectangleToIspWindow(const Rectangle &rectangle)
> > > > +{
> > > > + return rkisp1_cif_isp_window{
> > > > + .h_offs = static_cast<uint16_t>(rectangle.x),
> > > > + .v_offs = static_cast<uint16_t>(rectangle.y),
> > > > + .h_size = static_cast<uint16_t>(rectangle.width),
> > > > + .v_size = static_cast<uint16_t>(rectangle.height)
> > > > + };
> > > > +}
This function could be shared with other algorithm, as
rkisp1_cif_isp_window isn't specific to AF. We can address that later,
or move the function to algorithm.h already.
> > > > +
> > > > +} /* namespace */
> > > > +
> > > > +Af::Af()
> > > > +{
> > > > + af.windowUpdateRequested.connect(this, &Af::updateCurrentWindow);
> > > > +}
> > > > +
> > > > /**
> > > > * \copydoc libcamera::ipa::Algorithm::init
> > > > */
> > > > @@ -54,8 +81,12 @@ int Af::init(IPAContext &context, const YamlObject &tuningData)
> > > > }
> > > >
> > > > waitFramesLens_ = tuningData["wait-frames-lens"].get<uint32_t>(1);
> > > > + ispThreshold_ = tuningData["isp-threshold"].get<uint32_t>(128);
> > > > + ispVarShift_ = tuningData["isp-var-shift"].get<uint32_t>(4);
> > > >
> > > > - LOG(RkISP1Af, Debug) << "waitFramesLens_: " << waitFramesLens_;
> > > > + LOG(RkISP1Af, Debug) << "waitFramesLens_: " << waitFramesLens_
> > > > + << ", ispThreshold_: " << ispThreshold_
> > > > + << ", ispVarShift_: " << ispVarShift_;
> > > >
> > > > return af.init(lensConfig->minFocusPosition,
> > > > lensConfig->maxFocusPosition, tuningData);
> > > > @@ -89,6 +120,32 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,
> > > > af.queueRequest(controls);
> > > > }
> > > >
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::prepare
> > > > + */
> > > > +void Af::prepare([[maybe_unused]] IPAContext &context,
> > > > + [[maybe_unused]] const uint32_t frame,
> > > > + [[maybe_unused]] IPAFrameContext &frameContext,
> > > > + rkisp1_params_cfg *params)
> > > > +{
> > > > + if (updateWindow_) {
> > >
> > > or
> > > if (!updateWindow_)
> > > return;
> > >
> > > > + params->meas.afc_config.num_afm_win = 1;
> > > > + params->meas.afc_config.thres = ispThreshold_;
> > > > + params->meas.afc_config.var_shift = ispVarShift_;
> > > > + params->meas.afc_config.afm_win[0] =
> > > > + rectangleToIspWindow(*updateWindow_);
> > > > +
> > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AFC;
> > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_AFC;
> > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_AFC;
Should we disable the AF stats calculation when in manual focus mode to
save power, or is that a useless microoptimization ?
> > > > +
> > > > + updateWindow_.reset();
> > > > +
> > > > + /* Wait one frame for the ISP to apply changes. */
> > > > + af.skipFrames(1);
This delay sounds dodgy, have you verified that it takes exactly one
frame from here ?
> > > > + }
> > > > +}
> > > > +
> > > > /**
> > > > * \copydoc libcamera::ipa::Algorithm::process
> > > > */
> > > > @@ -114,6 +171,11 @@ void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > }
> > > > }
> > > >
> > > > +void Af::updateCurrentWindow(const Rectangle &window)
> > > > +{
> > > > + updateWindow_ = window;
> > > > +}
> > > > +
> > > > REGISTER_IPA_ALGORITHM(Af, "Af")
> > > >
> > > > } /* namespace ipa::rkisp1::algorithms */
> > > > diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h
> > > > index 3ba66d38..6f5adb19 100644
> > > > --- a/src/ipa/rkisp1/algorithms/af.h
> > > > +++ b/src/ipa/rkisp1/algorithms/af.h
> > > > @@ -20,21 +20,32 @@ namespace ipa::rkisp1::algorithms {
> > > > class Af : public Algorithm
> > > > {
> > > > public:
> > > > + Af();
> > > > +
> > > > int init(IPAContext &context, const YamlObject &tuningData) override;
> > > > int configure(IPAContext &context,
> > > > const IPACameraSensorInfo &configInfo) override;
> > > > void queueRequest(IPAContext &context, uint32_t frame,
> > > > IPAFrameContext &frameContext,
> > > > const ControlList &controls) override;
> > > > + void prepare(IPAContext &context, uint32_t frame,
> > > > + IPAFrameContext &frameContext,
> > > > + rkisp1_params_cfg *params) override;
> > > > void process(IPAContext &context, uint32_t frame,
> > > > IPAFrameContext &frameContext,
> > > > const rkisp1_stat_buffer *stats,
> > > > ControlList &metadata) override;
> > > >
> > > > private:
> > > > + void updateCurrentWindow(const Rectangle &window);
> > > > +
> > > > ipa::algorithms::AfHillClimbing af;
> > > >
> > > > - /* Wait number of frames after changing lens position */
> > > > + std::optional<Rectangle> updateWindow_;
> > > > + uint32_t ispThreshold_ = 0;
> > > > + uint32_t ispVarShift_ = 0;
> > > > +
> > > > + /* Wait number of frames after changing lens position. */
> > > > uint32_t waitFramesLens_ = 0;
> > > > };
> > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list