| Message ID | 20230331081930.19289-8-dse@thaumatec.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel 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@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 :) 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 ? > * . > * \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) > + }; > +} > + > +} /* 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; > + > + updateWindow_.reset(); > + > + /* Wait one frame for the ISP to apply changes. */ > + af.skipFrames(1); > + } > +} > + > /** > * \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; > }; > > -- > 2.39.2 >
Hi Jacopo, On Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > 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@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. > > > * . > > * \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) > > + }; > > +} > > + > > +} /* 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; > > + > > + updateWindow_.reset(); > > + > > + /* Wait one frame for the ISP to apply changes. */ > > + af.skipFrames(1); > > + } > > +} > > + > > /** > > * \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; > > }; > > > > -- > > 2.39.2 > > Best regards Daniel
Hi Daniel On Tue, Apr 04, 2023 at 11:39:46AM +0200, Daniel Semkowicz wrote: > Hi Jacopo, > > On Mon, Apr 3, 2023 at 3:59 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Daniel > > > > 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@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). 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) > > > + }; > > > +} > > > + > > > +} /* 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; > > > + > > > + updateWindow_.reset(); > > > + > > > + /* Wait one frame for the ISP to apply changes. */ > > > + af.skipFrames(1); > > > + } > > > +} > > > + > > > /** > > > * \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; > > > }; > > > > > > -- > > > 2.39.2 > > > > > Best regards > Daniel
Hi Jacopo, Hi Laurent, On Wed, Apr 26, 2023 at 9:04 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@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. > > 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 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 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: 4227989504 / (128 * 128) = 258056 maxSharpness = window.width * window.height * 258056 > > 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? 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. > > > 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 Best regards Daniel
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@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; > > > > > > }; > > > > > >
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. * . * \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) + }; +} + +} /* 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_) { + 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; + + updateWindow_.reset(); + + /* Wait one frame for the ISP to apply changes. */ + af.skipFrames(1); + } +} + /** * \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; };
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@thaumatec.com> --- src/ipa/rkisp1/algorithms/af.cpp | 64 +++++++++++++++++++++++++++++++- src/ipa/rkisp1/algorithms/af.h | 13 ++++++- 2 files changed, 75 insertions(+), 2 deletions(-)