[libcamera-devel] [PATCH v6 07/10] ipa: rkisp1: af: Add "Windows" Metering mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 31 20:07:11 CEST 2023
Hi Daniel,
On Fri, May 26, 2023 at 12:54:38PM +0200, Daniel Semkowicz wrote:
> On Wed, Apr 26, 2023 at 9:04 AM Laurent Pinchart wrote:
> > 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.
>
> I was not aware this field maps directly to the register... The comment
> in rkisp1-config.h about this field is a bit misleading, as it does not
> mention there are actually two values. I should look into the driver
> code more often instead of making assumptions :P
> It would be good to have two different fields in the
> rkisp1_cif_isp_afc_config structure for lum and afm var shift, but I
> guess it is not easy to change now, as it breaks the interface.
Yes, it would be difficult to change in a backward-compatible way. If we
could assume libcamera is the only user of the rkisp1 AF we could
possibly modify the interface, but that wouldn't be a very humble
assumption :-)
> > 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.
>
> Thank you for the details! I made my own calculations basing on your
> description and I achieved the same result of value 4261478400 for the
> 2x2 chessboard.
>
> I made some additional calculations, and it looks that 2x2 chessboard is
> not the worst case for the Tenengrad function.
> What I found to be the worst case is the stripes pattern with 2 black
> pixels and two white pixels. The following 4x4 block of pixels
> represents the stripes pattern:
>
> [ 255 255 0 0
> 255 255 0 0
> 255 255 0 0
> 255 255 0 0 ]
>
> This gives:
>
> G_{x}(2,2) = (-1*255) + (0*255) + (1*0)
> + (-2*255) + (0*255) + (2*0)
> + (-1*255) + (0*255) + (1*0)
> = 1020
That's -1020 actually, but not relevant as the value is then squared.
>
> G_{x}(i,j) will be 1020 for all positions.
>
> G_{y}(i,j) will be always 0 for this pattern as there are changes only
> along the X axis.
>
> The sum of the squares in this case will be equal to:
>
> 1020^{2} + 0^{2} = 1040400
Good catch, I had missed that !
> Basing on your description, for 128x128 this pattern should give the
> sharpness value:
>
> 1040400 * (128/2) * 128 = 8522956800
>
> This value is two times bigger than what you mentioned to be found
> in the documentation, however, it is in the same order of magnitude.
>
> There are a lot of uncertainties of how exactly the ISP calculates the
> sharpness value. If documentation states 4227989504 for the 128x128
> window, then maybe it would be safer to base the maximum value on
> the documentation? Something like this:
We could actually test it, by using a test pattern from the sensor, and
carefully selecting the AF window, but that's a bit of work.
> 4227989504 / (128 * 128) = 258056
>
> maxSharpness = window.width * window.height * 258056
We can start with this, that's fine with me. Please capture the above
explanation in a comment, and add a \todo to measure the sharpness value
with a vertical bar test pattern from the sensor, to double-check the
calculation.
> > The threshold value should likely be dynamic too, as it should depend on
> > the noise level, but that can be done later.
>
> Do I understand correctly that threshold here means that if calculated
> sharpness value give the value lower than threshold, the sharpness will
> be set to 0?
That's a very good question. I'm afraid I don't have an answer, I
haven't been able to find documentation about this.
> If this is true, the threshold can be hardcoded to 0,
> because the Af algorithm already includes safety margin when looking
> for max sharpness value.
I'm fine setting it to 0 to start with (please add a \todo comment to
tell that it should likely be computed based on the noise level).
> > > Did you ever experienced overflow ?
>
> No, I did not. I set the bit shift just for safety.
>
> > > > > > * .
> > > > > > * \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 ?
>
> I would suggest doing this in a separate change as it requires
> additional testing to make sure changing the state does not affect
> sharpness calculations.
>
> > > > > > +
> > > > > > + 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 ?
>
> Yes, in my experiments it took one frame. However, I am not 100% this
> will be the same for all cases.
>
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > /**
> > > > > > * \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