[libcamera-devel] [RFC v4 1/1] ipa: ipu3: af: Auto focus for dw9719 Surface Go2 VCM
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Thu Dec 16 16:32:04 CET 2021
Hi Kate,
Thanks for the patch :-) !
I have a few comments below, and I have to tell you I couldn't make it
work here... I don't know how you are testing it ? I can see the lens
position changing but it passes over the correct focus and never locks
on it.
On 13/12/2021 13:28, Kate Hsuan wrote:
> Since VCM for surface Go 2 (dw9719) had been successfully
> driven, this Af module can be used to control the VCM and
> determine the focus value based on the IPU3 AF state.
>
> The variance of each focus step is determined and a greedy
> approah is used to find the maximum variance of the AF
> state and a appropriate focus value.
>
> Changes in v2:
> 1. Add grid configuration interface.
> 2. Add AF accelerator configuration.
> 3. Move default focus area to the center of the image.
>
> Changes in v3:
> 1. Squash Daniel's commit- Remove v4l2 interaction from AF
> algorithm.
> 2. Fix greedy AF algorithm since V4l2 interface had been
> removed.
> 3. Simplify default grid config for AF.
> 4. AF start area is determined by bdsOutputSize.
>
> Changes in v4:
> In v4, it significant improves the AF scan time using a two pass
> scaning method and the AF scan will be terminated when it finds
> a negative gradient.
> 1. Introduce 2 pass AF scan (coarse and fine scan) to increase
> the AF success rate.
> 2. The low pass filter convolution results are used to determine
> a coarse AF result.
> 3. The high pass convolution result is used to find a good lens
> step in a given AF range determined in pass 1.
>
The lines above describe the changes between the versions, which is
great ! But those should go after the "---" as we don't want those in
the final commit message.
> Signed-off-by: Kate Hsuan <hpa at redhat.com>
> ---
Changes in ...
---
> src/ipa/ipu3/algorithms/af.cpp | 334 ++++++++++++++++++++++++++++
> src/ipa/ipu3/algorithms/af.h | 66 ++++++
> src/ipa/ipu3/algorithms/meson.build | 3 +-
> src/ipa/ipu3/ipa_context.cpp | 27 +++
> src/ipa/ipu3/ipa_context.h | 11 +
> src/ipa/ipu3/ipu3.cpp | 2 +
> 6 files changed, 442 insertions(+), 1 deletion(-)
> create mode 100644 src/ipa/ipu3/algorithms/af.cpp
> create mode 100644 src/ipa/ipu3/algorithms/af.h
>
I applied your patch, and I have compilation issues:
../src/ipa/ipu3/algorithms/af.cpp:89:1: error: missing initializer for
member ‘ipu3_uapi_af_config_s::padding’ [-Werror=missing-field-initializers]
89 | };
| ^
../src/ipa/ipu3/algorithms/af.cpp:89:1: error: missing initializer for
member ‘ipu3_uapi_grid_config::height_per_slice’
[-Werror=missing-field-initializers]
../src/ipa/ipu3/algorithms/af.cpp:89:1: error: missing initializer for
member ‘ipu3_uapi_grid_config::x_end’ [-Werror=missing-field-initializers]
../src/ipa/ipu3/algorithms/af.cpp:89:1: error: missing initializer for
member ‘ipu3_uapi_grid_config::y_end’ [-Werror=missing-field-initializers]
../src/ipa/ipu3/algorithms/af.cpp: In member function ‘virtual void
libcamera::ipa::ipu3::algorithms::Af::process(libcamera::ipa::ipu3::IPAContext&,
const ipu3_uapi_stats_3a*)’:
../src/ipa/ipu3/algorithms/af.cpp:265:50: error: taking address of
packed member of ‘ipu3_uapi_stats_3a’ may result in an unaligned pointer
value [-Werror=address-of-packed-member]
265 | y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;
I solved them locally (with a quick and dirty patch) how did you not
have those ?
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> new file mode 100644
> index 00000000..620be9fa
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -0,0 +1,334 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Red Hat
> + *
> + * af.cpp - IPU3 auto focus control
> + */
> +
> +#include "af.h"
> +
> +#include <algorithm>
> +#include <chrono>
> +#include <cmath>
> +#include <fcntl.h>
> +#include <numeric>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <linux/videodev2.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +#include "libipa/histogram.h"
> +
> +/**
> + * \file af.h
> + */
> +
> +namespace libcamera {
> +
> +using namespace std::literals::chrono_literals;
> +
> +namespace ipa::ipu3::algorithms {
> +
> +/**
> + * \class Af
> + * \brief A IPU3 auto-focus accelerator based auto focus algorthim
> + *
> + * This algorithm is used to determine the position of the lens and get a
> + * focused image. The IPU3 AF accelerator computes the statistics, composed
> + * by high pass and low pass filtered value and stores in a AF buffer.
> + * Typically, for a focused image, it has relative high contrast than a
> + * blurred image, i.e. an out of focus image. Therefore, if an image with the
> + * highest contrast can be found from the AF scan, the lens' position is the
> + * best step of the focus.
> + *
> + */
> +
> +LOG_DEFINE_CATEGORY(IPU3Af)
> +
> +/**
> + * Maximum focus value of the VCM control
> + * \todo should be obtained from the VCM driver
> + */
> +static constexpr uint32_t MaxFocusSteps_ = 1023;
> +
> +/* minimum focus step for searching appropriate focus*/
> +static constexpr uint32_t coarseSearchStep_ = 10;
> +static constexpr uint32_t fineSearchStep_ = 1;
> +
> +/* max ratio of variance change, 0.0 < MaxChange_ < 1.0*/
> +static constexpr double MaxChange_ = 0.8;
Why 0.8 ? Any explanation on how to set it ?
> +
> +/* settings for Auto Focus from the kernel */
> +static struct ipu3_uapi_af_config_s imgu_css_af_defaults = {
> + .filter_config = {
> + { 0, 0, 0, 0 },
> + { 0, 0, 0, 0 },
> + { 0, 0, 0, 128 },
> + 0,
> + { 0, 0, 0, 0 },
> + { 0, 0, 0, 0 },
> + { 0, 0, 0, 128 },
> + 0,
> + .y_calc = { 8, 8, 8, 8 },
> + .nf = { 0, 7, 0, 7, 0 },
> + },
> + .grid_cfg = {
> + .width = 16,
> + .height = 16,
> + .block_width_log2 = 3,
> + .block_height_log2 = 3,
> + .x_start = 10,
> + .y_start = 2 | IPU3_UAPI_GRID_Y_START_EN,
> + },
> +};
As mentionned, you missed some fields:
'''
@@ -78,13 +78,17 @@ static struct ipu3_uapi_af_config_s
imgu_css_af_defaults = {
.y_calc = { 8, 8, 8, 8 },
.nf = { 0, 7, 0, 7, 0 },
},
+ .padding = { 0, 0, 0, 0 },
.grid_cfg = {
.width = 16,
.height = 16,
.block_width_log2 = 3,
.block_height_log2 = 3,
+ .height_per_slice = 1,
.x_start = 10,
.y_start = 2 | IPU3_UAPI_GRID_Y_START_EN,
+ .x_end = 138,
+ .y_end = 130,
},
};
'''
> +
> +Af::Af()
> + : focus_(0), goodFocus_(0), currentVariance_(0.0), previousVariance_(0.0),
> + pass1Done_(false), pass2Done_(false)
> +{
> + maxStep_ = MaxFocusSteps_;
> +}
> +
> +Af::~Af()
> +{
> +}
> +
> +void Af::prepare(IPAContext &context, ipu3_uapi_params *params)
Missing documentation for prepare() => you should build the doc ;-)
> +{
> + params->use.acc_af = 1;
> + params->acc_param.af = imgu_css_af_defaults;
> + params->acc_param.af.grid_cfg.x_start = context.configuration.af.start_x;
> + params->acc_param.af.grid_cfg.y_start = context.configuration.af.start_y | IPU3_UAPI_GRID_Y_START_EN;
You are never changing the grid size, is it intended ?
> +}
> +
> +/**
> + * \brief Configure the Af given a configInfo
> + * \param[in] context The shared IPA context
> + * \param[in] configInfo The IPA configuration data
> + *
> + * \return 0
> + */
> +int Af::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> +{
> + /* determined focus value i.e. current focus value */
> + context.frameContext.af.focus = 0;
> + /* maximum variance of the AF statistics */
> + context.frameContext.af.maxVariance = 0;
double type, maybe s/= 0/= 0.0/ ?
> + /* is focused? if it is true, the AF should be in a stable state. */
> + context.frameContext.af.stable = false;
> + /* frame to be ignored before start to estimate AF variance. */
> + ignoreFrame_ = 10;
Could be a constexpr ?
> +
> + /*
> + * AF default area configuration
> + * Move AF area to the center of the image.
> + */
> + /* Default AF width is 16x8 = 128 */
> + context.configuration.af.start_x = (configInfo.bdsOutputSize.width / 2) - 64;
Why are you using a fixed value for the grid ? You could use the value
stored in imgu_css_af_defaults directly instead of applying 128/2 manualy ?
> + context.configuration.af.start_y = (configInfo.bdsOutputSize.height / 2) - 64;
> +
> + LOG(IPU3Af, Debug) << "BDS X: "
> + << configInfo.bdsOutputSize.width
> + << " Y: "
> + << configInfo.bdsOutputSize.height;
> + LOG(IPU3Af, Debug) << "AF start from X: "
> + << context.configuration.af.start_x
> + << " Y: "
> + << context.configuration.af.start_y;
> +
> + return 0;
> +}
> +
> +/**
> + * \brief AF coarse scan
> + * \param[in] context The shared IPA context
> + *
> + */
> +void Af::af_coarse_scan(IPAContext &context)
Bad naming (camelCase only). Same below.
> +{
> + if (pass1Done_ == true)
> + return;
> +
> + if (af_scan(context, coarseSearchStep_)) {
> + pass1Done_ = true;
> + context.frameContext.af.maxVariance = 0;
> + focus_ = context.frameContext.af.focus - (context.frameContext.af.focus * 0.1);
> + context.frameContext.af.focus = focus_;
> + previousVariance_ = 0;
> + maxStep_ = focus_ + (focus_ * 0.2);
> + }
> +}
> +
> +/**
> + * \brief AF fine scan
> + * \param[in] context The shared IPA context
> + *
> + */
> +void Af::af_fine_scan(IPAContext &context)
> +{
> + if (pass1Done_ != true)
> + return;
> +
> + if (af_scan(context, fineSearchStep_)) {
> + context.frameContext.af.stable = true;
> + pass2Done_ = true;
> + }
> +}
I am not a big fan of these pass1Done_ and pass2Done_ variables, let's
seee where it is used...
> +
> +/**
> + * \brief AF reset
> + * \param[in] context The shared IPA context
> + *
> + */
> +void Af::af_reset(IPAContext &context)
> +{
> + context.frameContext.af.maxVariance = 0;
> + context.frameContext.af.focus = 0;
> + focus_ = 0;
> + context.frameContext.af.stable = false;
> + ignoreFrame_ = 60;
Why 60 after reset and 10 at configure ? It seems to be a lot ?
> + previousVariance_ = 0.0;
> + pass1Done_ = false;
> + pass2Done_ = false;
> + maxStep_ = MaxFocusSteps_;
> +}
> +
> +/**
> + * \brief AF scan
> + * \param[in] context The shared IPA context
> + *
> + * \return True, if it finds a AF value.
> + */
> +bool Af::af_scan(IPAContext &context, int min_step)
> +{
> + /* find the maximum variance during the AF scan using a greedy strategy */
> + if (currentVariance_ > context.frameContext.af.maxVariance) {
> + context.frameContext.af.maxVariance = currentVariance_;
> + goodFocus_ = focus_;
> + }
> +
> + if (focus_ > maxStep_) {
> + /* if reach the max step, move lens to the position and set "focus stable". */
> + context.frameContext.af.focus = goodFocus_;
> + return true;
> + } else {
> + /* check negative gradient */
> + if ((currentVariance_ - context.frameContext.af.maxVariance) > -(context.frameContext.af.maxVariance * 0.15)) {
Where is the 0.15 coming from ?
> + focus_ += min_step;
> + context.frameContext.af.focus = focus_;
> + } else {
> + context.frameContext.af.focus = goodFocus_;
> + previousVariance_ = currentVariance_;
> + return true;
> + }
> + }
> + LOG(IPU3Af, Debug) << "Variance prevrious: "
s/prevrious/previous/
> + << previousVariance_
> + << " current: "
> + << currentVariance_
> + << " Diff: "
> + << (currentVariance_ - context.frameContext.af.maxVariance);
Interestingly, it seems there is an issue here, according to the log I get ?
[0:58:23.867136346] [6811] DEBUG IPU3Af af.cpp:238 Variance prevrious: 0
current: 20012.4 Diff: 0
Shouldn't diff be 20012.4 ?
I am not sure the algorithm is very strong though... You may never
converge and stay in a local minimum without knowing ?
> + previousVariance_ = currentVariance_;
> + LOG(IPU3Af, Debug) << "Focus searching max variance is: "
> + << context.frameContext.af.maxVariance
> + << " Focus step is "
> + << goodFocus_
> + << " Current scan is "
> + << focus_;
> + return false;
> +}
I am not sure to understand how this function should behave... Is it a
minimum searching ? Are you looking for argmin(currentVariance -
maxVariance) ?
> +
> +/**
> + * \brief Determine the max contrast image and lens position. y_table is the
> + * statictic data from IPU3 and is composed of low pass and high pass filtered
> + * value. High pass filtered value also represents the sharpness of the image.
> + * Based on this, if the image with highest variance of the high pass filtered
> + * value (contrast) during the AF scan, the position of the len should be the
> + * best focus.
> + * \param[in] context The shared IPA context.
> + * \param[in] stats The statistic buffer of 3A from the IPU3.
> + */
> +void Af::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> +{
> + uint32_t total = 0;
> + double mean;
> + uint64_t var_sum = 0;
> + y_table_item_t *y_item;
> + int z = 0;
> +
> + y_item = (y_table_item_t *)stats->af_raw_buffer.y_table;
> +
> + /**
> + * Calculate the mean and the varience of each non-zero AF statistics, since IPU3 only determine the AF value
> + * for a given grid.
> + * For pass1: low pass results are used.
> + * For pass2: high pass results are used.
> + */
> + if (pass1Done_) {
> + for (z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE) / 4; z++) {
> + total = total + y_item[z].y2_avg;
> + if (y_item[z].y2_avg == 0)
> + break;
> + }
> + mean = total / z;
> +
> + for (z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE) / 4 && y_item[z].y2_avg != 0; z++) {
> + var_sum = var_sum + ((y_item[z].y2_avg - mean) * (y_item[z].y2_avg - mean));
> + if (y_item[z].y2_avg == 0)
> + break;
> + }
> + } else {
/* Calculate the mean of the low pass filter values. */
> + for (z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE) / 4; z++) {
> + total = total + y_item[z].y1_avg;
> + if (y_item[z].y1_avg == 0)
> + break;
Are you sure of that ? Shouldn't you use the grid you set to parse the
right number of data ? If you have a local dark area you may stop even
if it is not yet the end of the grid ?
> + }
> + mean = total / z;
> +
/* Calulate the deviation from the mean. */
> + for (z = 0; z < (IPU3_UAPI_AF_Y_TABLE_MAX_SIZE) / 4 && y_item[z].y1_avg != 0; z++) {
> + var_sum = var_sum + ((y_item[z].y1_avg - mean) * (y_item[z].y1_avg - mean));
> + if (y_item[z].y1_avg == 0)
> + break;
> + }
> + }
The two if() blocks are very similar, the only exception beeing the item
of the table used (y_item[z].y1_avg in the first pass, y_item[z].y2_avg
in the second one). This is not easy to read (according to me).
> + /* Determine the average variance of the frame. */
> + currentVariance_ = static_cast<double>(var_sum) / static_cast<double>(z);
Side note:
If my stats are still correct, I think it should be divided by (z-1) and
not z, as we have a sample and not a full population ?
> + LOG(IPU3Af, Debug) << "variance: " << currentVariance_;
> +
> + if (context.frameContext.af.stable == true) {
> + const uint32_t diff_var = std::abs(currentVariance_ - context.frameContext.af.maxVariance);
> + const double var_ratio = diff_var / context.frameContext.af.maxVariance;
> + LOG(IPU3Af, Debug) << "Change ratio: "
> + << var_ratio
> + << " current focus: "
> + << context.frameContext.af.focus;
> + /**
> + * If the change ratio of contrast is over Maxchange_ (out of focus),
> + * trigger AF again.
> + */
> + if (var_ratio > MaxChange_) {
> + if (ignoreFrame_ == 0) {
> + af_reset(context);
> + } else
> + ignoreFrame_--;
> + } else
> + ignoreFrame_ = 10;
> + } else {
> + if (ignoreFrame_ != 0)
> + ignoreFrame_--;
> + else {
> + af_coarse_scan(context);
> + af_fine_scan(context);
> + }
> + }
> +}
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> new file mode 100644
> index 00000000..b9295b19
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Red Hat
> + *
> + * af.h - IPU3 Af control
> + */
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_AF_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +class Af : public Algorithm
> +{
> + /* The format of y_table. From ipu3-ipa repo */
> + typedef struct y_table_item {
> + uint16_t y1_avg;
> + uint16_t y2_avg;
> + } y_table_item_t;
> +
> +public:
> + Af();
> + ~Af();
> +
> + void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> + void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> +
> +private:
> + void af_coarse_scan(IPAContext &context);
> + void af_fine_scan(IPAContext &context);
> + bool af_scan(IPAContext &context, int min_step);
> + void af_reset(IPAContext &context);
> +
> + /* Used for focus scan. */
> + uint32_t focus_;
> + /* Focus good */
> + uint32_t goodFocus_;
> + /* Recent AF statistic variance. */
> + double currentVariance_;
> + /* The frames to be ignore before starting measuring. */
> + uint32_t ignoreFrame_;
> + /* previous variance. it is used to determine the gradient */
> + double previousVariance_;
> + /* Max scan steps of each pass of AF scaning */
> + uint32_t maxStep_;
> + /* Pass 1 stable. Complete low pass search (coarse) scan) */
> + bool pass1Done_;
> + /* Pass 2 stable. Complete high pass scan (fine scan) */
> + bool pass2Done_;
> +};
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AF_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index 4db6ae1d..e1099169 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,8 +1,9 @@
> # SPDX-License-Identifier: CC0-1.0
>
> ipu3_ipa_algorithms = files([
> + 'af.cpp',
> 'agc.cpp',
> 'awb.cpp',
> 'blc.cpp',
> - 'tone_mapping.cpp',
> + 'tone_mapping.cpp'
> ])
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 86794ac1..ee644d3c 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -67,6 +67,33 @@ namespace libcamera::ipa::ipu3 {
> *
> * \var IPASessionConfiguration::grid.stride
> * \brief Number of cells on one line including the ImgU padding
> + *
> + */
> +
> +/**
> + * \var IPASessionConfiguration::af
> + * \brief AF parameters configuration of the IPA
> + *
> + * \var IPASessionConfiguration::af.start_x
> + * \brief The start X position of the AF area
> + *
> + * \var IPASessionConfiguration::af.start_y
> + * \brief The start Y position of the AF area
> + */
> +
> +/**
> + * \var IPAFrameContext::af
> + * \brief Context for the Automatic Focus algorithm
> + *
> + * \struct IPAFrameContext::af
> + * \var IPAFrameContext::af.focus
> + * \brief Current position of the lens
> + *
> + * \var IPAFrameContext::af.maxVariance
> + * \brief The maximum variance of the current image.
> + *
> + * \var IPAFrameContext::af.stable
> + * \brief is the image focused?
> */
>
> /**
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index c6dc0814..aa5bf97f 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -31,6 +31,11 @@ struct IPASessionConfiguration {
> double minAnalogueGain;
> double maxAnalogueGain;
> } agc;
> +
> + struct {
> + uint16_t start_x;
> + uint16_t start_y;
> + } af;
> };
>
> struct IPAFrameContext {
> @@ -49,6 +54,12 @@ struct IPAFrameContext {
> double temperatureK;
> } awb;
>
> + struct {
> + uint32_t focus;
> + double maxVariance;
> + bool stable;
> + } af;
> +
> struct {
> uint32_t exposure;
> double gain;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 3d307708..93966c6f 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -30,6 +30,7 @@
>
> #include "libcamera/internal/mapped_framebuffer.h"
>
> +#include "algorithms/af.h"
> #include "algorithms/agc.h"
> #include "algorithms/algorithm.h"
> #include "algorithms/awb.h"
> @@ -294,6 +295,7 @@ int IPAIPU3::init(const IPASettings &settings,
> }
>
> /* Construct our Algorithms */
> + algorithms_.push_back(std::make_unique<algorithms::Af>());
> algorithms_.push_back(std::make_unique<algorithms::Agc>());
> algorithms_.push_back(std::make_unique<algorithms::Awb>());
> algorithms_.push_back(std::make_unique<algorithms::BlackLevelCorrection>());
>
More information about the libcamera-devel
mailing list