| Message ID | 20230331081930.19289-6-dse@thaumatec.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel sorry I have neglected the Windows related patches in previous reviews as I hoped someone would have shared an opinion on the Signal-based mechanism. I don't have better ideas to propose, so let's go with it On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > Add support for setting user defined auto focus window in the > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > Each time, there is a need for changing the window configuration in the > ISP, the signal is emitted. Platform layer that wants to use > the "Windows" metering mode, needs to connect to this signal > and configure the ISP on each emission. > > Currently only one window is supported. > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > --- > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > 2 files changed, 66 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > index 244b8803..0fb17df3 100644 > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > * movement range rather than coarse search result. > * \todo Implement setRange. > * \todo Implement setSpeed. > - * \todo Implement setMeteringMode. > - * \todo Implement setWindows. > * \todo Implement the AfPauseDeferred mode. > + * \todo Implement support for multiple AF windows. > */ > > /** > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > return 0; > } > > +/** > + * \brief Configure the AfHillClimbing with sensor information > + * \param[in] outputSize Camera sensor output size > + * > + * This method should be called in the libcamera::ipa::Algorithm::configure() > + * method of the platform layer. > + */ > +void AfHillClimbing::configure(const Size &outputSize) According to the AfWindows control definition Sets the focus windows used by the AF algorithm when AfMetering is set to AfMeteringWindows. The units used are pixels within the rectangle returned by the ScalerCropMaximum property. The final sensor's output size can be obtained by downscaling/cropping a larger portion of the pixel array. The portion of the pixel array processed to obtain the final image is named AnalogueCropRectangle and all valid crop rectangles lie inside it. Most of our controls, such as ScalerCrop, are expressed with the AnalogueCrop as reference, to allow expressing them regardless of the current output size. It's up to the pipeline handler to re-scale any valid control in the current configured output. I'm not 100% sure why we decided to use ScalerCropMaximum here, most probably to allow pipeline handler express any additional processing margins required by any cropping/scaling that happens on the ISP. However as the RkISP1 pipeline doesn't express any of those margin yet, I would simply use here the sensor's analogue crop, which is part of the IPACameraSensorInfo you already use in the platform layer. To remove ambiguities I would call the parameter here "maxWindow" or something similar. I would also initialize userWindow_ to the same value, so that if an application switches to AfMeteringWindows mode the rectangle is at least initialized to something. > +{ > + /* > + * Default AF window of 3/4 size of the camera sensor output, > + * placed at the center > + */ > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > + static_cast<int>(outputSize.height / 8), > + 3 * outputSize.width / 4, > + 3 * outputSize.height / 4); > + > + windowUpdateRequested.emit(defaultWindow_); > +} > + > /** > * \brief Run the auto focus algorithm loop > * \param[in] currentContrast New value of contrast measured for current frame > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > framesToSkip_ = n; > } > > +/** > + * \var AfHillClimbing::windowUpdateRequested > + * \brief Signal emitted when change in AF window was requested > + * > + * Platform layer supporting AF windows should connect to this signal > + * and configure the ISP with new window on each emition. > + */ > + > void AfHillClimbing::setMode(controls::AfModeEnum mode) > { > if (mode == mode_) > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > LOG(Af, Error) << "setSpeed() not implemented!"; > } > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > { > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > + if (metering == meteringMode_) > + return; > + > + meteringMode_ = metering; > + > + switch (metering) { > + case controls::AfMeteringAuto: > + windowUpdateRequested.emit(defaultWindow_); > + break; > + case controls::AfMeteringWindows: > + windowUpdateRequested.emit(userWindow_); > + break; > + } > } > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > { > - LOG(Af, Error) << "setWindows() not implemented!"; > + if (windows.size() != 1) { > + LOG(Af, Error) << "Only one AF window is supported"; > + return; > + } Should this be an hard error or should we just want and continue by only considering the first available rectangle ? > + > + LOG(Af, Debug) << "setWindows: " << windows[0]; > + > + userWindow_ = windows[0]; > + > + if (meteringMode_ == controls::AfMeteringWindows) > + windowUpdateRequested.emit(userWindow_); > } > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > index 2147939b..0f7c65db 100644 > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > @@ -10,6 +10,7 @@ > #pragma once > > #include <libcamera/base/log.h> > +#include <libcamera/base/signal.h> > > #include "af.h" > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > public: > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > const YamlObject &tuningData); > + void configure(const Size &outputSize); > int32_t process(double currentContrast); > void skipFrames(uint32_t n); > > controls::AfStateEnum state() override { return state_; } > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > + Signal<const Rectangle &> windowUpdateRequested; > + > private: > void setMode(controls::AfModeEnum mode) override; > void setRange(controls::AfRangeEnum range) override; > @@ -54,6 +58,7 @@ private: > controls::AfModeEnum mode_ = controls::AfModeManual; > controls::AfStateEnum state_ = controls::AfStateIdle; > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > /* Current focus lens position. */ > int32_t lensPosition_ = 0; > @@ -84,6 +89,11 @@ private: > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > double maxChange_; > + > + /* Default AF window. */ > + Rectangle defaultWindow_; > + /* AF window set by user using AF_WINDOWS control. */ > + Rectangle userWindow_; > }; > > } /* namespace ipa::algorithms */ > -- > 2.39.2 >
Hi Jacopo, On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > sorry I have neglected the Windows related patches in previous > reviews as I hoped someone would have shared an opinion on the > Signal-based mechanism. > > I don't have better ideas to propose, so let's go with it > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Add support for setting user defined auto focus window in the > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > Each time, there is a need for changing the window configuration in the > > ISP, the signal is emitted. Platform layer that wants to use > > the "Windows" metering mode, needs to connect to this signal > > and configure the ISP on each emission. > > > > Currently only one window is supported. > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > index 244b8803..0fb17df3 100644 > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > * movement range rather than coarse search result. > > * \todo Implement setRange. > > * \todo Implement setSpeed. > > - * \todo Implement setMeteringMode. > > - * \todo Implement setWindows. > > * \todo Implement the AfPauseDeferred mode. > > + * \todo Implement support for multiple AF windows. > > */ > > > > /** > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > return 0; > > } > > > > +/** > > + * \brief Configure the AfHillClimbing with sensor information > > + * \param[in] outputSize Camera sensor output size > > + * > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > + * method of the platform layer. > > + */ > > +void AfHillClimbing::configure(const Size &outputSize) > > According to the AfWindows control definition > > Sets the focus windows used by the AF algorithm when AfMetering is set > to AfMeteringWindows. The units used are pixels within the rectangle > returned by the ScalerCropMaximum property. > > The final sensor's output size can be obtained by downscaling/cropping > a larger portion of the pixel array. The portion of the pixel array > processed to obtain the final image is named AnalogueCropRectangle and > all valid crop rectangles lie inside it. > > Most of our controls, such as ScalerCrop, are expressed with the > AnalogueCrop as reference, to allow expressing them regardless of the > current output size. It's up to the pipeline handler to re-scale any > valid control in the current configured output. For the 1024x768 stream, I have the following parameters: - activeAreaSize: 2592x1944 - analogCrop: (0, 0)/2592x1944 - outputSize: 1296x972 When using analogCrop, I will also need to scale the AF window size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes values in reference to the ISP input size (IPACameraSensorInfo::outputSize). Based on what you wrote earlier, I understand that it is supposed to be like that? > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > probably to allow pipeline handler express any additional processing > margins required by any cropping/scaling that happens on the ISP. > > However as the RkISP1 pipeline doesn't express any of those margin > yet, I would simply use here the sensor's analogue crop, which is part > of the IPACameraSensorInfo you already use in the platform layer. > I have some concerns about the ScalerCropMaximum. What if the ISP have different size margins for cropping and auto-focus windows? I see there are margins defined for the AF window, but nothing about the cropping margins in the RK3399 documentation. In this case, it will not be a problem, but what if some ISP will have margins for cropping, but no margins for AF window? > To remove ambiguities I would call the parameter here "maxWindow" or > something similar. > > I would also initialize userWindow_ to the same value, so that if an > application switches to AfMeteringWindows mode the rectangle is at > least initialized to something. Good point! > > > > +{ > > + /* > > + * Default AF window of 3/4 size of the camera sensor output, > > + * placed at the center > > + */ > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > + static_cast<int>(outputSize.height / 8), > > + 3 * outputSize.width / 4, > > + 3 * outputSize.height / 4); > > + > > + windowUpdateRequested.emit(defaultWindow_); > > +} > > + > > /** > > * \brief Run the auto focus algorithm loop > > * \param[in] currentContrast New value of contrast measured for current frame > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > framesToSkip_ = n; > > } > > > > +/** > > + * \var AfHillClimbing::windowUpdateRequested > > + * \brief Signal emitted when change in AF window was requested > > + * > > + * Platform layer supporting AF windows should connect to this signal > > + * and configure the ISP with new window on each emition. > > + */ > > + > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > { > > if (mode == mode_) > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > LOG(Af, Error) << "setSpeed() not implemented!"; > > } > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > { > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > + if (metering == meteringMode_) > > + return; > > + > > + meteringMode_ = metering; > > + > > + switch (metering) { > > + case controls::AfMeteringAuto: > > + windowUpdateRequested.emit(defaultWindow_); > > + break; > > + case controls::AfMeteringWindows: > > + windowUpdateRequested.emit(userWindow_); > > + break; > > + } > > } > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > { > > - LOG(Af, Error) << "setWindows() not implemented!"; > > + if (windows.size() != 1) { > > + LOG(Af, Error) << "Only one AF window is supported"; > > + return; > > + } > > > Should this be an hard error or should we just want and continue by > only considering the first available rectangle ? I will change it to a warning that only the first window was used. > > > + > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > + > > + userWindow_ = windows[0]; > > + > > + if (meteringMode_ == controls::AfMeteringWindows) > > + windowUpdateRequested.emit(userWindow_); > > } > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > index 2147939b..0f7c65db 100644 > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > @@ -10,6 +10,7 @@ > > #pragma once > > > > #include <libcamera/base/log.h> > > +#include <libcamera/base/signal.h> > > > > #include "af.h" > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > public: > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > const YamlObject &tuningData); > > + void configure(const Size &outputSize); > > int32_t process(double currentContrast); > > void skipFrames(uint32_t n); > > > > controls::AfStateEnum state() override { return state_; } > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > + Signal<const Rectangle &> windowUpdateRequested; > > + > > private: > > void setMode(controls::AfModeEnum mode) override; > > void setRange(controls::AfRangeEnum range) override; > > @@ -54,6 +58,7 @@ private: > > controls::AfModeEnum mode_ = controls::AfModeManual; > > controls::AfStateEnum state_ = controls::AfStateIdle; > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > /* Current focus lens position. */ > > int32_t lensPosition_ = 0; > > @@ -84,6 +89,11 @@ private: > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > double maxChange_; > > + > > + /* Default AF window. */ > > + Rectangle defaultWindow_; > > + /* AF window set by user using AF_WINDOWS control. */ > > + Rectangle userWindow_; > > }; > > > > } /* namespace ipa::algorithms */ > > -- > > 2.39.2 > > Best regards Daniel
Hi Daniel cc RPi folks which originally introduced the AF controls and which have recently been working a new implementation On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > Hi Jacopo, > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Daniel > > sorry I have neglected the Windows related patches in previous > > reviews as I hoped someone would have shared an opinion on the > > Signal-based mechanism. > > > > I don't have better ideas to propose, so let's go with it > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > Add support for setting user defined auto focus window in the > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > Each time, there is a need for changing the window configuration in the > > > ISP, the signal is emitted. Platform layer that wants to use > > > the "Windows" metering mode, needs to connect to this signal > > > and configure the ISP on each emission. > > > > > > Currently only one window is supported. > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > --- > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > index 244b8803..0fb17df3 100644 > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > * movement range rather than coarse search result. > > > * \todo Implement setRange. > > > * \todo Implement setSpeed. > > > - * \todo Implement setMeteringMode. > > > - * \todo Implement setWindows. > > > * \todo Implement the AfPauseDeferred mode. > > > + * \todo Implement support for multiple AF windows. > > > */ > > > > > > /** > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > return 0; > > > } > > > > > > +/** > > > + * \brief Configure the AfHillClimbing with sensor information > > > + * \param[in] outputSize Camera sensor output size > > > + * > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > + * method of the platform layer. > > > + */ > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > According to the AfWindows control definition > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > to AfMeteringWindows. The units used are pixels within the rectangle > > returned by the ScalerCropMaximum property. > > > > The final sensor's output size can be obtained by downscaling/cropping > > a larger portion of the pixel array. The portion of the pixel array > > processed to obtain the final image is named AnalogueCropRectangle and > > all valid crop rectangles lie inside it. > > > > Most of our controls, such as ScalerCrop, are expressed with the > > AnalogueCrop as reference, to allow expressing them regardless of the > > current output size. It's up to the pipeline handler to re-scale any > > valid control in the current configured output. > > For the 1024x768 stream, I have the following parameters: > - activeAreaSize: 2592x1944 > - analogCrop: (0, 0)/2592x1944 > - outputSize: 1296x972 > > When using analogCrop, I will also need to scale the AF window > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > values in reference to the ISP input size > (IPACameraSensorInfo::outputSize). > Based on what you wrote earlier, I understand that it is supposed to be > like that? > I presume so. I should check how RPi handles windowing > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > probably to allow pipeline handler express any additional processing > > margins required by any cropping/scaling that happens on the ISP. > > > > However as the RkISP1 pipeline doesn't express any of those margin > > yet, I would simply use here the sensor's analogue crop, which is part > > of the IPACameraSensorInfo you already use in the platform layer. > > > > I have some concerns about the ScalerCropMaximum. > What if the ISP have different size margins for cropping and auto-focus > windows? I see there are margins defined for the AF window, but nothing > about the cropping margins in the RK3399 documentation. In this case, > it will not be a problem, but what if some ISP will have margins for > cropping, but no margins for AF window? > Good point. Do we need an AfWindowMaximum ? RPi any opinions here ? > > To remove ambiguities I would call the parameter here "maxWindow" or > > something similar. > > > > I would also initialize userWindow_ to the same value, so that if an > > application switches to AfMeteringWindows mode the rectangle is at > > least initialized to something. > > Good point! > > > > > > > > +{ > > > + /* > > > + * Default AF window of 3/4 size of the camera sensor output, > > > + * placed at the center > > > + */ > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > + static_cast<int>(outputSize.height / 8), > > > + 3 * outputSize.width / 4, > > > + 3 * outputSize.height / 4); > > > + > > > + windowUpdateRequested.emit(defaultWindow_); > > > +} > > > + > > > /** > > > * \brief Run the auto focus algorithm loop > > > * \param[in] currentContrast New value of contrast measured for current frame > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > framesToSkip_ = n; > > > } > > > > > > +/** > > > + * \var AfHillClimbing::windowUpdateRequested > > > + * \brief Signal emitted when change in AF window was requested > > > + * > > > + * Platform layer supporting AF windows should connect to this signal > > > + * and configure the ISP with new window on each emition. > > > + */ > > > + > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > { > > > if (mode == mode_) > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > } > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > { > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > + if (metering == meteringMode_) > > > + return; > > > + > > > + meteringMode_ = metering; > > > + > > > + switch (metering) { > > > + case controls::AfMeteringAuto: > > > + windowUpdateRequested.emit(defaultWindow_); > > > + break; > > > + case controls::AfMeteringWindows: > > > + windowUpdateRequested.emit(userWindow_); > > > + break; > > > + } > > > } > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > { > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > + if (windows.size() != 1) { > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > + return; > > > + } > > > > > > Should this be an hard error or should we just want and continue by > > only considering the first available rectangle ? > > I will change it to a warning that only the first window was used. > > > > > > + > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > + > > > + userWindow_ = windows[0]; > > > + > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > + windowUpdateRequested.emit(userWindow_); > > > } > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > index 2147939b..0f7c65db 100644 > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > @@ -10,6 +10,7 @@ > > > #pragma once > > > > > > #include <libcamera/base/log.h> > > > +#include <libcamera/base/signal.h> > > > > > > #include "af.h" > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > public: > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > const YamlObject &tuningData); > > > + void configure(const Size &outputSize); > > > int32_t process(double currentContrast); > > > void skipFrames(uint32_t n); > > > > > > controls::AfStateEnum state() override { return state_; } > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > + > > > private: > > > void setMode(controls::AfModeEnum mode) override; > > > void setRange(controls::AfRangeEnum range) override; > > > @@ -54,6 +58,7 @@ private: > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > /* Current focus lens position. */ > > > int32_t lensPosition_ = 0; > > > @@ -84,6 +89,11 @@ private: > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > double maxChange_; > > > + > > > + /* Default AF window. */ > > > + Rectangle defaultWindow_; > > > + /* AF window set by user using AF_WINDOWS control. */ > > > + Rectangle userWindow_; > > > }; > > > > > > } /* namespace ipa::algorithms */ > > > -- > > > 2.39.2 > > > > > Best regards > Daniel
On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > cc RPi folks which originally introduced the AF controls and which > have recently been working a new implementation > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Hi Jacopo, > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Daniel > > > sorry I have neglected the Windows related patches in previous > > > reviews as I hoped someone would have shared an opinion on the > > > Signal-based mechanism. > > > > > > I don't have better ideas to propose, so let's go with it > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > Add support for setting user defined auto focus window in the > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > Each time, there is a need for changing the window configuration in the > > > > ISP, the signal is emitted. Platform layer that wants to use > > > > the "Windows" metering mode, needs to connect to this signal > > > > and configure the ISP on each emission. > > > > > > > > Currently only one window is supported. > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > --- > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > index 244b8803..0fb17df3 100644 > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > * movement range rather than coarse search result. > > > > * \todo Implement setRange. > > > > * \todo Implement setSpeed. > > > > - * \todo Implement setMeteringMode. > > > > - * \todo Implement setWindows. > > > > * \todo Implement the AfPauseDeferred mode. > > > > + * \todo Implement support for multiple AF windows. > > > > */ > > > > > > > > /** > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > return 0; > > > > } > > > > > > > > +/** > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > + * \param[in] outputSize Camera sensor output size > > > > + * > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > > + * method of the platform layer. > > > > + */ > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > According to the AfWindows control definition > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > returned by the ScalerCropMaximum property. > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > a larger portion of the pixel array. The portion of the pixel array > > > processed to obtain the final image is named AnalogueCropRectangle and > > > all valid crop rectangles lie inside it. > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > current output size. It's up to the pipeline handler to re-scale any > > > valid control in the current configured output. > > > > For the 1024x768 stream, I have the following parameters: > > - activeAreaSize: 2592x1944 > > - analogCrop: (0, 0)/2592x1944 > > - outputSize: 1296x972 > > > > When using analogCrop, I will also need to scale the AF window > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > values in reference to the ISP input size > > (IPACameraSensorInfo::outputSize). > > Based on what you wrote earlier, I understand that it is supposed to be > > like that? > > > > I presume so. > > I should check how RPi handles windowing > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > probably to allow pipeline handler express any additional processing > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > yet, I would simply use here the sensor's analogue crop, which is part > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > I have some concerns about the ScalerCropMaximum. > > What if the ISP have different size margins for cropping and auto-focus > > windows? I see there are margins defined for the AF window, but nothing > > about the cropping margins in the RK3399 documentation. In this case, > > it will not be a problem, but what if some ISP will have margins for > > cropping, but no margins for AF window? > > > > Good point. Do we need an AfWindowMaximum ? To make sense, AfWindowMaximum would need to be expressed in reference to how ISP handles that (sensor output size on rkisp) to allow precise AF window configuration. AF window margins on rkisp are small: 2px and 5px. I would like to avoid scaling, so small values compared to the output size. I am not sure if AfWindowMaximum as additional property is actually needed. Maximum value for AfWindows would not be enough? Is there a need to expose the window margins to the user at all? We could allow the window size of the whole ISP input size to be set by the user and clamp the size in the IPA before configuring the ISP? In summary, I see two options: 1. Maximum AF window expressed in units that are directly set to ISP (output size for rkisp). Maximum AF window will include margins. 2. Keep the current approach and express the AF window in reference to Analogue crop size. Just allow the full size instead of the ScalerCropMaximum. Clamp the margins inside the IPA if user requested larger window. > > RPi any opinions here ? > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > something similar. > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > application switches to AfMeteringWindows mode the rectangle is at > > > least initialized to something. > > > > Good point! > > > > > > > > > > > > +{ > > > > + /* > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > + * placed at the center > > > > + */ > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > + static_cast<int>(outputSize.height / 8), > > > > + 3 * outputSize.width / 4, > > > > + 3 * outputSize.height / 4); > > > > + > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > +} > > > > + > > > > /** > > > > * \brief Run the auto focus algorithm loop > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > framesToSkip_ = n; > > > > } > > > > > > > > +/** > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > + * \brief Signal emitted when change in AF window was requested > > > > + * > > > > + * Platform layer supporting AF windows should connect to this signal > > > > + * and configure the ISP with new window on each emition. > > > > + */ > > > > + > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > { > > > > if (mode == mode_) > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > } > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > { > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > + if (metering == meteringMode_) > > > > + return; > > > > + > > > > + meteringMode_ = metering; > > > > + > > > > + switch (metering) { > > > > + case controls::AfMeteringAuto: > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > + break; > > > > + case controls::AfMeteringWindows: > > > > + windowUpdateRequested.emit(userWindow_); > > > > + break; > > > > + } > > > > } > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > { > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > + if (windows.size() != 1) { > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > + return; > > > > + } > > > > > > > > > Should this be an hard error or should we just want and continue by > > > only considering the first available rectangle ? > > > > I will change it to a warning that only the first window was used. > > > > > > > > > + > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > + > > > > + userWindow_ = windows[0]; > > > > + > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > + windowUpdateRequested.emit(userWindow_); > > > > } > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > index 2147939b..0f7c65db 100644 > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > @@ -10,6 +10,7 @@ > > > > #pragma once > > > > > > > > #include <libcamera/base/log.h> > > > > +#include <libcamera/base/signal.h> > > > > > > > > #include "af.h" > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > public: > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > const YamlObject &tuningData); > > > > + void configure(const Size &outputSize); > > > > int32_t process(double currentContrast); > > > > void skipFrames(uint32_t n); > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > + > > > > private: > > > > void setMode(controls::AfModeEnum mode) override; > > > > void setRange(controls::AfRangeEnum range) override; > > > > @@ -54,6 +58,7 @@ private: > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > /* Current focus lens position. */ > > > > int32_t lensPosition_ = 0; > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > double maxChange_; > > > > + > > > > + /* Default AF window. */ > > > > + Rectangle defaultWindow_; > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > + Rectangle userWindow_; > > > > }; > > > > > > > > } /* namespace ipa::algorithms */ > > > > -- > > > > 2.39.2 > > > > > > > > Best regards > > Daniel
Hi Daniel On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote: > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Daniel > > cc RPi folks which originally introduced the AF controls and which > > have recently been working a new implementation > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > Hi Jacopo, > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi Daniel > > > > sorry I have neglected the Windows related patches in previous > > > > reviews as I hoped someone would have shared an opinion on the > > > > Signal-based mechanism. > > > > > > > > I don't have better ideas to propose, so let's go with it > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > Add support for setting user defined auto focus window in the > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > > Each time, there is a need for changing the window configuration in the > > > > > ISP, the signal is emitted. Platform layer that wants to use > > > > > the "Windows" metering mode, needs to connect to this signal > > > > > and configure the ISP on each emission. > > > > > > > > > > Currently only one window is supported. > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > --- > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > index 244b8803..0fb17df3 100644 > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > > * movement range rather than coarse search result. > > > > > * \todo Implement setRange. > > > > > * \todo Implement setSpeed. > > > > > - * \todo Implement setMeteringMode. > > > > > - * \todo Implement setWindows. > > > > > * \todo Implement the AfPauseDeferred mode. > > > > > + * \todo Implement support for multiple AF windows. > > > > > */ > > > > > > > > > > /** > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > return 0; > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > > + * \param[in] outputSize Camera sensor output size > > > > > + * > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > > > + * method of the platform layer. > > > > > + */ > > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > > > According to the AfWindows control definition > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > > returned by the ScalerCropMaximum property. > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > > a larger portion of the pixel array. The portion of the pixel array > > > > processed to obtain the final image is named AnalogueCropRectangle and > > > > all valid crop rectangles lie inside it. > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > > current output size. It's up to the pipeline handler to re-scale any > > > > valid control in the current configured output. > > > > > > For the 1024x768 stream, I have the following parameters: > > > - activeAreaSize: 2592x1944 > > > - analogCrop: (0, 0)/2592x1944 > > > - outputSize: 1296x972 > > > > > > When using analogCrop, I will also need to scale the AF window > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > > values in reference to the ISP input size > > > (IPACameraSensorInfo::outputSize). > > > Based on what you wrote earlier, I understand that it is supposed to be > > > like that? > > > > > > > I presume so. > > > > I should check how RPi handles windowing > > > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > > probably to allow pipeline handler express any additional processing > > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > > yet, I would simply use here the sensor's analogue crop, which is part > > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > > > > I have some concerns about the ScalerCropMaximum. > > > What if the ISP have different size margins for cropping and auto-focus > > > windows? I see there are margins defined for the AF window, but nothing > > > about the cropping margins in the RK3399 documentation. In this case, > > > it will not be a problem, but what if some ISP will have margins for > > > cropping, but no margins for AF window? > > > > > > > Good point. Do we need an AfWindowMaximum ? > > To make sense, AfWindowMaximum would need to be expressed in reference > to how ISP handles that (sensor output size on rkisp) to allow precise > AF window configuration. AF window margins on rkisp are small: 2px and > 5px. I would like to avoid scaling, so small values compared to > the output size. For ScalerCrop we decided to have a ScalerCropMaximum property which basically reports the AnalogueCrop rectangle, as this is the maximum rectangle an application can set as a cropping area. The reference is the PixelArrayActiveAreas size. For AfWindows we don't care about AnalogCrop but only about the sensor's output as, if my understanding is correct, this is the rectangle on which the ISP performs windowing on. As we don't expose the sensor's output size to applications, the only reference we can use is full PixelArrayActiveAreas and re-scale before setting the windows. > I am not sure if AfWindowMaximum as additional property is actually > needed. Maximum value for AfWindows would not be enough? > As a comment on the ScalerCropMaximum property reports \todo Turn this property into a "maximum control value" for the ScalerCrop control once "dynamic" controls have been implemented. I presume this is very old, as right now (at least for RkISP1) I see the "ControlInfoMap *ipaControls" parameter passed in to configure() and being overwritten completely with new control limits. So my suggestion for a new property was probably not correct. > Is there a need to expose the window margins to the user at all? I think so, it would avoid setting controls which can't be applied as they are. I presume you can initialize the maximum value of AfWindows as { 0, 0, PixelArrayActiveAreas.width - 4, PixelArrayActiveAreas.height - 10 } When re-scaling, you first need to translate it to take into consideration the processing margins then re-scale it using the activeArea-to-outputSize ratio. (only compiled, not run-time tested) Span<const Rectangle> userWindows = *request->controls().get<Span<const Rectangle>>(controls::AfWindows); std::vector<Rectangle> afWindows; for (auto userWindow : userWindows) { Rectangle window = userWindow.translatedBy({2, 5}); window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize); afWindows.push_back(window); } (as you only support one window, this could be even simplified) Now, I would need to do the re-scaling in the RkISP1 Af algorithm implementation, and you would need to pass to the algorithm the active area size. One way to do so, as algorithms have access to the context_, is to add the active area size to IPASessionConfiguration::sensor which already contains the sensor's output size. What do you think ? > We could allow the window size of the whole ISP input size to be set > by the user and clamp the size in the IPA before configuring the ISP? > > In summary, I see two options: > 1. Maximum AF window expressed in units that are directly set to ISP > (output size for rkisp). Maximum AF window will include margins. > 2. Keep the current approach and express the AF window in reference to > Analogue crop size. Just allow the full size instead of the > ScalerCropMaximum. Clamp the margins inside the IPA if user > requested larger window. > > > > > RPi any opinions here ? > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > > something similar. > > > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > > application switches to AfMeteringWindows mode the rectangle is at > > > > least initialized to something. > > > > > > Good point! > > > > > > > > > > > > > > > > +{ > > > > > + /* > > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > > + * placed at the center > > > > > + */ > > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > > + static_cast<int>(outputSize.height / 8), > > > > > + 3 * outputSize.width / 4, > > > > > + 3 * outputSize.height / 4); > > > > > + > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Run the auto focus algorithm loop > > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > > framesToSkip_ = n; > > > > > } > > > > > > > > > > +/** > > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > > + * \brief Signal emitted when change in AF window was requested > > > > > + * > > > > > + * Platform layer supporting AF windows should connect to this signal > > > > > + * and configure the ISP with new window on each emition. > > > > > + */ > > > > > + > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > > { > > > > > if (mode == mode_) > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > > } > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > > { > > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > > + if (metering == meteringMode_) > > > > > + return; > > > > > + > > > > > + meteringMode_ = metering; > > > > > + > > > > > + switch (metering) { > > > > > + case controls::AfMeteringAuto: > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > + break; > > > > > + case controls::AfMeteringWindows: > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > + break; > > > > > + } > > > > > } > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > > { > > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > > + if (windows.size() != 1) { > > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > > + return; > > > > > + } > > > > > > > > > > > > Should this be an hard error or should we just want and continue by > > > > only considering the first available rectangle ? > > > > > > I will change it to a warning that only the first window was used. > > > > > > > > > > > > + > > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > > + > > > > > + userWindow_ = windows[0]; > > > > > + > > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > } > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > index 2147939b..0f7c65db 100644 > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > @@ -10,6 +10,7 @@ > > > > > #pragma once > > > > > > > > > > #include <libcamera/base/log.h> > > > > > +#include <libcamera/base/signal.h> > > > > > > > > > > #include "af.h" > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > > public: > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > const YamlObject &tuningData); > > > > > + void configure(const Size &outputSize); > > > > > int32_t process(double currentContrast); > > > > > void skipFrames(uint32_t n); > > > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > > + > > > > > private: > > > > > void setMode(controls::AfModeEnum mode) override; > > > > > void setRange(controls::AfRangeEnum range) override; > > > > > @@ -54,6 +58,7 @@ private: > > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > > > /* Current focus lens position. */ > > > > > int32_t lensPosition_ = 0; > > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > > double maxChange_; > > > > > + > > > > > + /* Default AF window. */ > > > > > + Rectangle defaultWindow_; > > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > > + Rectangle userWindow_; > > > > > }; > > > > > > > > > > } /* namespace ipa::algorithms */ > > > > > -- > > > > > 2.39.2 > > > > > > > > > > > Best regards > > > Daniel
Hi Jacopo, On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote: > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Daniel > > > cc RPi folks which originally introduced the AF controls and which > > > have recently been working a new implementation > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > Hi Jacopo, > > > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > Hi Daniel > > > > > sorry I have neglected the Windows related patches in previous > > > > > reviews as I hoped someone would have shared an opinion on the > > > > > Signal-based mechanism. > > > > > > > > > > I don't have better ideas to propose, so let's go with it > > > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > Add support for setting user defined auto focus window in the > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > > > Each time, there is a need for changing the window configuration in the > > > > > > ISP, the signal is emitted. Platform layer that wants to use > > > > > > the "Windows" metering mode, needs to connect to this signal > > > > > > and configure the ISP on each emission. > > > > > > > > > > > > Currently only one window is supported. > > > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > > --- > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > index 244b8803..0fb17df3 100644 > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > > > * movement range rather than coarse search result. > > > > > > * \todo Implement setRange. > > > > > > * \todo Implement setSpeed. > > > > > > - * \todo Implement setMeteringMode. > > > > > > - * \todo Implement setWindows. > > > > > > * \todo Implement the AfPauseDeferred mode. > > > > > > + * \todo Implement support for multiple AF windows. > > > > > > */ > > > > > > > > > > > > /** > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > > > + * \param[in] outputSize Camera sensor output size > > > > > > + * > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > > > > + * method of the platform layer. > > > > > > + */ > > > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > > > > > According to the AfWindows control definition > > > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > > > returned by the ScalerCropMaximum property. > > > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > > > a larger portion of the pixel array. The portion of the pixel array > > > > > processed to obtain the final image is named AnalogueCropRectangle and > > > > > all valid crop rectangles lie inside it. > > > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > > > current output size. It's up to the pipeline handler to re-scale any > > > > > valid control in the current configured output. > > > > > > > > For the 1024x768 stream, I have the following parameters: > > > > - activeAreaSize: 2592x1944 > > > > - analogCrop: (0, 0)/2592x1944 > > > > - outputSize: 1296x972 > > > > > > > > When using analogCrop, I will also need to scale the AF window > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > > > values in reference to the ISP input size > > > > (IPACameraSensorInfo::outputSize). > > > > Based on what you wrote earlier, I understand that it is supposed to be > > > > like that? > > > > > > > > > > I presume so. > > > > > > I should check how RPi handles windowing > > > > > > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > > > probably to allow pipeline handler express any additional processing > > > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > > > yet, I would simply use here the sensor's analogue crop, which is part > > > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > > > > > > > I have some concerns about the ScalerCropMaximum. > > > > What if the ISP have different size margins for cropping and auto-focus > > > > windows? I see there are margins defined for the AF window, but nothing > > > > about the cropping margins in the RK3399 documentation. In this case, > > > > it will not be a problem, but what if some ISP will have margins for > > > > cropping, but no margins for AF window? > > > > > > > > > > Good point. Do we need an AfWindowMaximum ? > > > > To make sense, AfWindowMaximum would need to be expressed in reference > > to how ISP handles that (sensor output size on rkisp) to allow precise > > AF window configuration. AF window margins on rkisp are small: 2px and > > 5px. I would like to avoid scaling, so small values compared to > > the output size. > > For ScalerCrop we decided to have a ScalerCropMaximum property which > basically reports the AnalogueCrop rectangle, as this is the maximum > rectangle an application can set as a cropping area. The reference is > the PixelArrayActiveAreas size. > > For AfWindows we don't care about AnalogCrop but only about the > sensor's output as, if my understanding is correct, this is the > rectangle on which the ISP performs windowing on. > > As we don't expose the sensor's output size to applications, the only > reference we can use is full PixelArrayActiveAreas and re-scale before > setting the windows. > > > I am not sure if AfWindowMaximum as additional property is actually > > needed. Maximum value for AfWindows would not be enough? > > > > As a comment on the ScalerCropMaximum property reports > > \todo Turn this property into a "maximum control value" for the > ScalerCrop control once "dynamic" controls have been implemented. > > I presume this is very old, as right now (at least for RkISP1) I see > the "ControlInfoMap *ipaControls" parameter passed in to configure() and > being overwritten completely with new control limits. > > So my suggestion for a new property was probably not correct. > > > Is there a need to expose the window margins to the user at all? > > I think so, it would avoid setting controls which can't be applied as > they are. > > I presume you can initialize the maximum value of AfWindows as > > { 0, 0, PixelArrayActiveAreas.width - 4, > PixelArrayActiveAreas.height - 10 } > > When re-scaling, you first need to translate it to take into > consideration the processing margins then re-scale it using the > activeArea-to-outputSize ratio. > > (only compiled, not run-time tested) > > Span<const Rectangle> userWindows = > *request->controls().get<Span<const Rectangle>>(controls::AfWindows); > std::vector<Rectangle> afWindows; > > for (auto userWindow : userWindows) { > Rectangle window = userWindow.translatedBy({2, 5}); > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize); > afWindows.push_back(window); > } > > (as you only support one window, this could be even > simplified) > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm > implementation, and you would need to pass to the algorithm the active > area size. One way to do so, as algorithms have access to the > context_, is to add the active area size to > IPASessionConfiguration::sensor which already contains the sensor's > output size. > > What do you think ? I am not fully happy with this approach as this requires scaling the values two times, but as you said, expressing it directly in the output size would require exposing additional controls. The other way, I was thinking of, is to express the AF window in the final output image size (1024x768 in my already mentioned example). This way, it is easier to think about from the user perspective. However, this is an opposite approach than the already existing one for the ScalerCrop. Maybe I am not 100% happy with this, but it looks that expressing the AF window as you show above (in reference to the current PixelArrayActiveAreas) makes the most sense for the existing code, because it will be the common approach with the ScalerCrop. I can change my implementation to this approach. Do we need to change also the documentation related to the AF Windows? Best regards Daniel > > > We could allow the window size of the whole ISP input size to be set > > by the user and clamp the size in the IPA before configuring the ISP? > > > > In summary, I see two options: > > 1. Maximum AF window expressed in units that are directly set to ISP > > (output size for rkisp). Maximum AF window will include margins. > > 2. Keep the current approach and express the AF window in reference to > > Analogue crop size. Just allow the full size instead of the > > ScalerCropMaximum. Clamp the margins inside the IPA if user > > requested larger window. > > > > > > > > RPi any opinions here ? > > > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > > > something similar. > > > > > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > > > application switches to AfMeteringWindows mode the rectangle is at > > > > > least initialized to something. > > > > > > > > Good point! > > > > > > > > > > > > > > > > > > > > +{ > > > > > > + /* > > > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > > > + * placed at the center > > > > > > + */ > > > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > > > + static_cast<int>(outputSize.height / 8), > > > > > > + 3 * outputSize.width / 4, > > > > > > + 3 * outputSize.height / 4); > > > > > > + > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * \brief Run the auto focus algorithm loop > > > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > > > framesToSkip_ = n; > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > > > + * \brief Signal emitted when change in AF window was requested > > > > > > + * > > > > > > + * Platform layer supporting AF windows should connect to this signal > > > > > > + * and configure the ISP with new window on each emition. > > > > > > + */ > > > > > > + > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > > > { > > > > > > if (mode == mode_) > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > > > } > > > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > > > { > > > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > > > + if (metering == meteringMode_) > > > > > > + return; > > > > > > + > > > > > > + meteringMode_ = metering; > > > > > > + > > > > > > + switch (metering) { > > > > > > + case controls::AfMeteringAuto: > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > + break; > > > > > > + case controls::AfMeteringWindows: > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > + break; > > > > > > + } > > > > > > } > > > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > > > { > > > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > > > + if (windows.size() != 1) { > > > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > > > + return; > > > > > > + } > > > > > > > > > > > > > > > Should this be an hard error or should we just want and continue by > > > > > only considering the first available rectangle ? > > > > > > > > I will change it to a warning that only the first window was used. > > > > > > > > > > > > > > > + > > > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > > > + > > > > > > + userWindow_ = windows[0]; > > > > > > + > > > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > } > > > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > index 2147939b..0f7c65db 100644 > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > @@ -10,6 +10,7 @@ > > > > > > #pragma once > > > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > +#include <libcamera/base/signal.h> > > > > > > > > > > > > #include "af.h" > > > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > > > public: > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > const YamlObject &tuningData); > > > > > > + void configure(const Size &outputSize); > > > > > > int32_t process(double currentContrast); > > > > > > void skipFrames(uint32_t n); > > > > > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > > > + > > > > > > private: > > > > > > void setMode(controls::AfModeEnum mode) override; > > > > > > void setRange(controls::AfRangeEnum range) override; > > > > > > @@ -54,6 +58,7 @@ private: > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > > > > > /* Current focus lens position. */ > > > > > > int32_t lensPosition_ = 0; > > > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > > > double maxChange_; > > > > > > + > > > > > > + /* Default AF window. */ > > > > > > + Rectangle defaultWindow_; > > > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > > > + Rectangle userWindow_; > > > > > > }; > > > > > > > > > > > > } /* namespace ipa::algorithms */ > > > > > > -- > > > > > > 2.39.2 > > > > > > > > > > > > > > Best regards > > > > Daniel
Hi Laurent, Daniel, On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > (Nick, there's a question for you below) > > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote: > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote: > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote: > > > > > > > > > > Hi Daniel > > > > > cc RPi folks which originally introduced the AF controls and which > > > > > have recently been working a new implementation > > > > > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote: > > > > > > > > > > > > > > Hi Daniel > > > > > > > sorry I have neglected the Windows related patches in previous > > > > > > > reviews as I hoped someone would have shared an opinion on the > > > > > > > Signal-based mechanism. > > > > > > > > > > > > > > I don't have better ideas to propose, so let's go with it > > I don't like the signal much either. We could simply implement a > function to retrieve the window. > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > > Add support for setting user defined auto focus window in the > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > > > > > Each time, there is a need for changing the window configuration in the > > s/Each time,/Each time/ > > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use > > s/layer that wants/Layers that want/ > > > > > > > > > the "Windows" metering mode, needs to connect to this signal > > s/mode, needs/mode need/ > > > > > > > > > and configure the ISP on each emission. > > > > > > > > > > > > > > > > Currently only one window is supported. > > > > > > > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > > > > --- > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > index 244b8803..0fb17df3 100644 > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > > > > > * movement range rather than coarse search result. > > > > > > > > * \todo Implement setRange. > > > > > > > > * \todo Implement setSpeed. > > > > > > > > - * \todo Implement setMeteringMode. > > > > > > > > - * \todo Implement setWindows. > > > > > > > > * \todo Implement the AfPauseDeferred mode. > > > > > > > > + * \todo Implement support for multiple AF windows. > > > > > > > > */ > > > > > > > > > > > > > > > > /** > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > return 0; > > > > > > > > } > > > > > > > > > > > > > > > > +/** > > > > > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > > > > > + * \param[in] outputSize Camera sensor output size > > > > > > > > + * > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > s/method/function/ > > > > > > > > > + * method of the platform layer. > > > > > > > > + */ > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > > > > > > > > > According to the AfWindows control definition > > > > > > > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > > > > > returned by the ScalerCropMaximum property. > > > > > > > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > > > > > a larger portion of the pixel array. The portion of the pixel array > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and > > > > > > > all valid crop rectangles lie inside it. > > > > > > > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > > > > > current output size. It's up to the pipeline handler to re-scale any > > > > > > > valid control in the current configured output. > > > > > > > > > > > > For the 1024x768 stream, I have the following parameters: > > > > > > - activeAreaSize: 2592x1944 > > > > > > - analogCrop: (0, 0)/2592x1944 > > > > > > - outputSize: 1296x972 > > > > > > > > > > > > When using analogCrop, I will also need to scale the AF window > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > > > > > values in reference to the ISP input size > > > > > > (IPACameraSensorInfo::outputSize). > > > > > > Based on what you wrote earlier, I understand that it is supposed to be > > > > > > like that? > > > > > > > > > > I presume so. > > > > > > > > > > I should check how RPi handles windowing > > Nick, that's the first question for you. Our implementation is based on what we understand to be the current libcamera specification: That AF windows are in the same coordinate system as ScalerCropMaximum, i.e. raw sensor pixels. I found the documentation phrase "The units used are pixels within the rectangle..." ambiguous -- does it suggest that the origin of the AfWIndows coordinate system is the top-left corner of ScalerCropMaximum? I took the answer to be "no", that those rectangles were in a common coordinate system, without offsetting. This in turn means that Af Windows will stick to the same objects in the scene, regardless of mode or digital zoom. > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > > > > > probably to allow pipeline handler express any additional processing > > > > > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part > > > > > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > > > > > > > > > > > > > I have some concerns about the ScalerCropMaximum. > > > > > > What if the ISP have different size margins for cropping and auto-focus > > > > > > windows? I see there are margins defined for the AF window, but nothing > > > > > > about the cropping margins in the RK3399 documentation. In this case, > > > > > > it will not be a problem, but what if some ISP will have margins for > > > > > > cropping, but no margins for AF window? > > > > > > > > > > Good point. Do we need an AfWindowMaximum ? > > > > > > > > To make sense, AfWindowMaximum would need to be expressed in reference > > > > to how ISP handles that (sensor output size on rkisp) to allow precise > > > > AF window configuration. AF window margins on rkisp are small: 2px and > > > > 5px. I would like to avoid scaling, so small values compared to > > > > the output size. > > > > > > For ScalerCrop we decided to have a ScalerCropMaximum property which > > > basically reports the AnalogueCrop rectangle, as this is the maximum > > > rectangle an application can set as a cropping area. The reference is > > > the PixelArrayActiveAreas size. > > > > > > For AfWindows we don't care about AnalogCrop but only about the > > > sensor's output as, if my understanding is correct, this is the > > > rectangle on which the ISP performs windowing on. > > > > > > As we don't expose the sensor's output size to applications, the only > > > reference we can use is full PixelArrayActiveAreas and re-scale before > > > setting the windows. > > > > > > > I am not sure if AfWindowMaximum as additional property is actually > > > > needed. Maximum value for AfWindows would not be enough? Encoding the largest useful window as the control maximum value (where this means minimum X, Y and maximum width,height?) sounds useful. BTW is there any way to encode the maximum number of windows? > > > > > > As a comment on the ScalerCropMaximum property reports > > > > > > \todo Turn this property into a "maximum control value" for the > > > ScalerCrop control once "dynamic" controls have been implemented. > > > > > > I presume this is very old, as right now (at least for RkISP1) I see > > > the "ControlInfoMap *ipaControls" parameter passed in to configure() and > > > being overwritten completely with new control limits. > > > > > > So my suggestion for a new property was probably not correct. > > > > > > > Is there a need to expose the window margins to the user at all? > > > > > > I think so, it would avoid setting controls which can't be applied as > > > they are. > > > > > > I presume you can initialize the maximum value of AfWindows as > > > > > > { 0, 0, PixelArrayActiveAreas.width - 4, > > > PixelArrayActiveAreas.height - 10 } > > > > > > When re-scaling, you first need to translate it to take into > > > consideration the processing margins then re-scale it using the > > > activeArea-to-outputSize ratio. > > > > > > (only compiled, not run-time tested) > > > > > > Span<const Rectangle> userWindows = > > > *request->controls().get<Span<const Rectangle>>(controls::AfWindows); > > > std::vector<Rectangle> afWindows; > > > > > > for (auto userWindow : userWindows) { > > > Rectangle window = userWindow.translatedBy({2, 5}); > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize); > > > afWindows.push_back(window); > > > } > > > > > > (as you only support one window, this could be even > > > simplified) > > > > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm > > > implementation, and you would need to pass to the algorithm the active > > > area size. One way to do so, as algorithms have access to the > > > context_, is to add the active area size to > > > IPASessionConfiguration::sensor which already contains the sensor's > > > output size. > > > > > > What do you think ? > > > > I am not fully happy with this approach as this requires scaling > > the values two times, but as you said, expressing it directly > > in the output size would require exposing additional controls. > > > > The other way, I was thinking of, is to express the AF window > > in the final output image size (1024x768 in my already mentioned > > example). This way, it is easier to think about from the user > > perspective. However, this is an opposite approach than the already > > existing one for the ScalerCrop. > > > > Maybe I am not 100% happy with this, but it looks that expressing > > the AF window as you show above (in reference to the current > > PixelArrayActiveAreas) makes the most sense for the existing code, > > because it will be the common approach with the ScalerCrop. > > I can change my implementation to this approach. > > > > Do we need to change also the documentation related to the AF Windows? > > > > > > We could allow the window size of the whole ISP input size to be set > > > > by the user and clamp the size in the IPA before configuring the ISP? > > > > > > > > In summary, I see two options: > > > > 1. Maximum AF window expressed in units that are directly set to ISP > > > > (output size for rkisp). Maximum AF window will include margins. > > > > 2. Keep the current approach and express the AF window in reference to > > > > Analogue crop size. Just allow the full size instead of the > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user > > > > requested larger window. > > > > > > > > > RPi any opinions here ? > > And this is the second question for Nick. I don't have very strong opinions. Option 2 seems the most general. (Again, I've been assuming there's no implicit offsetting for the "reference" rectangle -- so the only difference between those definitions is a question of cropping?) I suspect there is not a one-size-fits-all approach. Some sensors may be able to generate focus information outside the cropped region; others may not. Some platforms may only be able to measure focus within the final image. Cropping "as required" sounds like a reasonable way to resolve that -- but we ought to give the user some way to predict when a window might end up being cropped to nothing (and thus ignored)! > > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > > > > > something similar. > > > > > > > > > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > > > > > application switches to AfMeteringWindows mode the rectangle is at > > > > > > > least initialized to something. > > > > > > > > > > > > Good point! > > > > > > > > > > > > > > +{ > > > > > > > > + /* > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > > > > > + * placed at the center > > > > > > > > + */ > > > > > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > > > > > + static_cast<int>(outputSize.height / 8), > > > > > > > > + 3 * outputSize.width / 4, > > > > > > > > + 3 * outputSize.height / 4); > > > > > > > > + > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > +} > > > > > > > > + > > > > > > > > /** > > > > > > > > * \brief Run the auto focus algorithm loop > > > > > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > > > > > framesToSkip_ = n; > > > > > > > > } > > > > > > > > > > > > > > > > +/** > > > > > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > > > > > + * \brief Signal emitted when change in AF window was requested > > > > > > > > + * > > > > > > > > + * Platform layer supporting AF windows should connect to this signal > > s/layer/layers/ > s/connect to/connect/ > > > > > > > > > + * and configure the ISP with new window on each emition. > > s/new window/the new window/ > s/emition/emission/ > > The new window will likely need a few frames to become effective. I'm > worried that the AF algorithm doesn't take that into account at all (it > also doesn't take the lens movement delays into account, which is > another of my worries). This can affect the performance and stability of > the auto-focus algorithm. > > How have you tested this ? > > > > > > > > > + */ > > > > > > > > + > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > > > > > { > > > > > > > > if (mode == mode_) > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > > > > > } > > > > > > > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > > > > > { > > > > > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > > > > > + if (metering == meteringMode_) > > > > > > > > + return; > > > > > > > > + > > > > > > > > + meteringMode_ = metering; > > > > > > > > + > > > > > > > > + switch (metering) { > > > > > > > > + case controls::AfMeteringAuto: > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > + break; > > > > > > > > + case controls::AfMeteringWindows: > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > + break; > > This needs a default case to avoid modifying meteringMode_. We have two > modes currently defined, if we add a third one, the implementation will > break. > > When the window is changed the contrast value may drastically change. > The optimal focus point will likely change too. I think you need to > reset the AF and possibly trigger a rescan for continuous AF mode. > > > > > > > > > + } > > > > > > > > } > > > > > > > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > > > > > { > > > > > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > > > > > + if (windows.size() != 1) { > > > > > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > > > > > + return; > > > > > > > > + } > > > > > > > > > > > > > > Should this be an hard error or should we just want and continue by > > > > > > > only considering the first available rectangle ? > > > > > > > > > > > > I will change it to a warning that only the first window was used. > > > > > > > > > > > > > > + > > > > > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > > > > > + > > > > > > > > + userWindow_ = windows[0]; > > > > > > > > + > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > } > > > > > > > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > index 2147939b..0f7c65db 100644 > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > @@ -10,6 +10,7 @@ > > > > > > > > #pragma once > > > > > > > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > +#include <libcamera/base/signal.h> > > > > > > > > > > > > > > > > #include "af.h" > > > > > > > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > > > > > public: > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > const YamlObject &tuningData); > > > > > > > > + void configure(const Size &outputSize); > > > > > > > > int32_t process(double currentContrast); > > > > > > > > void skipFrames(uint32_t n); > > > > > > > > > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > > > > > + > > > > > > > > private: > > > > > > > > void setMode(controls::AfModeEnum mode) override; > > > > > > > > void setRange(controls::AfRangeEnum range) override; > > > > > > > > @@ -54,6 +58,7 @@ private: > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > > > > > > > > > /* Current focus lens position. */ > > > > > > > > int32_t lensPosition_ = 0; > > > > > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > > > > > double maxChange_; > > > > > > > > + > > > > > > > > + /* Default AF window. */ > > > > > > > > + Rectangle defaultWindow_; > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > > > > > + Rectangle userWindow_; > > > > > > > > }; > > > > > > > > > > > > > > > > } /* namespace ipa::algorithms */ > > -- > Regards, > > Laurent Pinchart Regards, Nick
Hello Laurent, Hello Nick, On Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> wrote: > > Hi Laurent, Daniel, > > On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Daniel, > > > > (Nick, there's a question for you below) > > > > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote: > > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote: > > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote: > > > > > > > > > > > > Hi Daniel > > > > > > cc RPi folks which originally introduced the AF controls and which > > > > > > have recently been working a new implementation > > > > > > > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote: > > > > > > > > > > > > > > > > Hi Daniel > > > > > > > > sorry I have neglected the Windows related patches in previous > > > > > > > > reviews as I hoped someone would have shared an opinion on the > > > > > > > > Signal-based mechanism. > > > > > > > > > > > > > > > > I don't have better ideas to propose, so let's go with it > > > > I don't like the signal much either. We could simply implement a > > function to retrieve the window. Signal has the advantage that it will be invoked only when the window has changed. With regular function, the function will need to be called on each frame to check if there is a new window. What are the disadvantages of using the signal? > > > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > > > Add support for setting user defined auto focus window in the > > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > > > > > > Each time, there is a need for changing the window configuration in the > > > > s/Each time,/Each time/ > > > > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use > > > > s/layer that wants/Layers that want/ > > > > > > > > > > > the "Windows" metering mode, needs to connect to this signal > > > > s/mode, needs/mode need/ > > > > > > > > > > > and configure the ISP on each emission. > > > > > > > > > > > > > > > > > > Currently only one window is supported. > > > > > > > > > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > > > > > --- > > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > index 244b8803..0fb17df3 100644 > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > > > > > > * movement range rather than coarse search result. > > > > > > > > > * \todo Implement setRange. > > > > > > > > > * \todo Implement setSpeed. > > > > > > > > > - * \todo Implement setMeteringMode. > > > > > > > > > - * \todo Implement setWindows. > > > > > > > > > * \todo Implement the AfPauseDeferred mode. > > > > > > > > > + * \todo Implement support for multiple AF windows. > > > > > > > > > */ > > > > > > > > > > > > > > > > > > /** > > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > > return 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > > > > > > + * \param[in] outputSize Camera sensor output size > > > > > > > > > + * > > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > > s/method/function/ > > > > > > > > > > > + * method of the platform layer. > > > > > > > > > + */ > > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > > > > > > > > > > > According to the AfWindows control definition > > > > > > > > > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > > > > > > returned by the ScalerCropMaximum property. > > > > > > > > > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > > > > > > a larger portion of the pixel array. The portion of the pixel array > > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and > > > > > > > > all valid crop rectangles lie inside it. > > > > > > > > > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > > > > > > current output size. It's up to the pipeline handler to re-scale any > > > > > > > > valid control in the current configured output. > > > > > > > > > > > > > > For the 1024x768 stream, I have the following parameters: > > > > > > > - activeAreaSize: 2592x1944 > > > > > > > - analogCrop: (0, 0)/2592x1944 > > > > > > > - outputSize: 1296x972 > > > > > > > > > > > > > > When using analogCrop, I will also need to scale the AF window > > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > > > > > > values in reference to the ISP input size > > > > > > > (IPACameraSensorInfo::outputSize). > > > > > > > Based on what you wrote earlier, I understand that it is supposed to be > > > > > > > like that? > > > > > > > > > > > > I presume so. > > > > > > > > > > > > I should check how RPi handles windowing > > > > Nick, that's the first question for you. > > Our implementation is based on what we understand to be the current > libcamera specification: That AF windows are in the same coordinate > system as ScalerCropMaximum, i.e. raw sensor pixels. > > I found the documentation phrase "The units used are pixels within the > rectangle..." ambiguous -- does it suggest that the origin of the > AfWIndows coordinate system is the top-left corner of > ScalerCropMaximum? I took the answer to be "no", that those rectangles > were in a common coordinate system, without offsetting. This in turn > means that Af Windows will stick to the same objects in the scene, > regardless of mode or digital zoom. I had the same problems with understanding this documentation part, so probably it should be rephrased to clearly state what is the reference coordination system for AfWindows. > > > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > > > > > > probably to allow pipeline handler express any additional processing > > > > > > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > > > > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part > > > > > > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > > > > > > > > > > > > > > > > I have some concerns about the ScalerCropMaximum. > > > > > > > What if the ISP have different size margins for cropping and auto-focus > > > > > > > windows? I see there are margins defined for the AF window, but nothing > > > > > > > about the cropping margins in the RK3399 documentation. In this case, > > > > > > > it will not be a problem, but what if some ISP will have margins for > > > > > > > cropping, but no margins for AF window? > > > > > > > > > > > > Good point. Do we need an AfWindowMaximum ? > > > > > > > > > > To make sense, AfWindowMaximum would need to be expressed in reference > > > > > to how ISP handles that (sensor output size on rkisp) to allow precise > > > > > AF window configuration. AF window margins on rkisp are small: 2px and > > > > > 5px. I would like to avoid scaling, so small values compared to > > > > > the output size. > > > > > > > > For ScalerCrop we decided to have a ScalerCropMaximum property which > > > > basically reports the AnalogueCrop rectangle, as this is the maximum > > > > rectangle an application can set as a cropping area. The reference is > > > > the PixelArrayActiveAreas size. > > > > > > > > For AfWindows we don't care about AnalogCrop but only about the > > > > sensor's output as, if my understanding is correct, this is the > > > > rectangle on which the ISP performs windowing on. > > > > > > > > As we don't expose the sensor's output size to applications, the only > > > > reference we can use is full PixelArrayActiveAreas and re-scale before > > > > setting the windows. > > > > > > > > > I am not sure if AfWindowMaximum as additional property is actually > > > > > needed. Maximum value for AfWindows would not be enough? > > Encoding the largest useful window as the control maximum value (where > this means minimum X, Y and maximum width,height?) sounds useful. BTW > is there any way to encode the maximum number of windows? That is a good point. If we specify the maximum windows size, we should also specify the maximum number of windows. The only solution I can think of is something like this: Rectangle minWindow(0, 0, 0, 0); ControlValue min(Span<const Rectangle>({minWindow})); Rectangle maxWindow(0, 0, 640, 480); ControlValue max(Span<const Rectangle>({maxWindow, maxWindow, maxWindow})); Rectangle defWindow(100, 100, 200, 200); ControlValue def(Span<const Rectangle>({defWindow})); ctrls[&controls::AfWindows] = ControlInfo(min, max,def); cam lists such control as: Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0, 0)/640x480, (0, 0)/640x480 ]] I am not convinced this is a readable solution for the user... Especially, if there will be bigger number of windows supported. I expect that minimum and maximum size of will be the same for all windows, regardless of the number of windows. From this, it can be deduced that we could expose only the simple pair: { window size, number of windows } for each min, max and def ControlInfo value. > > > > > > > > > As a comment on the ScalerCropMaximum property reports > > > > > > > > \todo Turn this property into a "maximum control value" for the > > > > ScalerCrop control once "dynamic" controls have been implemented. > > > > > > > > I presume this is very old, as right now (at least for RkISP1) I see > > > > the "ControlInfoMap *ipaControls" parameter passed in to configure() and > > > > being overwritten completely with new control limits. > > > > > > > > So my suggestion for a new property was probably not correct. > > > > > > > > > Is there a need to expose the window margins to the user at all? > > > > > > > > I think so, it would avoid setting controls which can't be applied as > > > > they are. > > > > > > > > I presume you can initialize the maximum value of AfWindows as > > > > > > > > { 0, 0, PixelArrayActiveAreas.width - 4, > > > > PixelArrayActiveAreas.height - 10 } > > > > > > > > When re-scaling, you first need to translate it to take into > > > > consideration the processing margins then re-scale it using the > > > > activeArea-to-outputSize ratio. > > > > > > > > (only compiled, not run-time tested) > > > > > > > > Span<const Rectangle> userWindows = > > > > *request->controls().get<Span<const Rectangle>>(controls::AfWindows); > > > > std::vector<Rectangle> afWindows; > > > > > > > > for (auto userWindow : userWindows) { > > > > Rectangle window = userWindow.translatedBy({2, 5}); > > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize); > > > > afWindows.push_back(window); > > > > } > > > > > > > > (as you only support one window, this could be even > > > > simplified) > > > > > > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm > > > > implementation, and you would need to pass to the algorithm the active > > > > area size. One way to do so, as algorithms have access to the > > > > context_, is to add the active area size to > > > > IPASessionConfiguration::sensor which already contains the sensor's > > > > output size. > > > > > > > > What do you think ? > > > > > > I am not fully happy with this approach as this requires scaling > > > the values two times, but as you said, expressing it directly > > > in the output size would require exposing additional controls. > > > > > > The other way, I was thinking of, is to express the AF window > > > in the final output image size (1024x768 in my already mentioned > > > example). This way, it is easier to think about from the user > > > perspective. However, this is an opposite approach than the already > > > existing one for the ScalerCrop. > > > > > > Maybe I am not 100% happy with this, but it looks that expressing > > > the AF window as you show above (in reference to the current > > > PixelArrayActiveAreas) makes the most sense for the existing code, > > > because it will be the common approach with the ScalerCrop. > > > I can change my implementation to this approach. > > > > > > Do we need to change also the documentation related to the AF Windows? > > > > > > > > We could allow the window size of the whole ISP input size to be set > > > > > by the user and clamp the size in the IPA before configuring the ISP? > > > > > > > > > > In summary, I see two options: > > > > > 1. Maximum AF window expressed in units that are directly set to ISP > > > > > (output size for rkisp). Maximum AF window will include margins. > > > > > 2. Keep the current approach and express the AF window in reference to > > > > > Analogue crop size. Just allow the full size instead of the > > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user > > > > > requested larger window. > > > > > > > > > > > RPi any opinions here ? > > > > And this is the second question for Nick. > > I don't have very strong opinions. Option 2 seems the most general. > (Again, I've been assuming there's no implicit offsetting for the > "reference" rectangle -- so the only difference between those > definitions is a question of cropping?) > > I suspect there is not a one-size-fits-all approach. Some sensors may > be able to generate focus information outside the cropped region; > others may not. Some platforms may only be able to measure focus > within the final image. Cropping "as required" sounds like a > reasonable way to resolve that -- but we ought to give the user some > way to predict when a window might end up being cropped to nothing > (and thus ignored)! Ok, so my idea with cropping the margins without the user knowledge is not a good solution... Then, probably We end up with something similar to the ScalerCrop that is already implemented in the Raspberry implementation, if I understand it correctly: The (x,y) location of the AfWindow is relative to the PixelArrayActiveAreas that is being used. The units remain native sensor pixels. Maximum value of the AfWindows ControlInfo (position + size), already includes all the margins that need to be considered (e.g. required by the ISP). Would you agree with the above definition? > > > > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > > > > > > something similar. > > > > > > > > > > > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > > > > > > application switches to AfMeteringWindows mode the rectangle is at > > > > > > > > least initialized to something. > > > > > > > > > > > > > > Good point! > > > > > > > > > > > > > > > > +{ > > > > > > > > > + /* > > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > > > > > > + * placed at the center > > > > > > > > > + */ > > > > > > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > > > > > > + static_cast<int>(outputSize.height / 8), > > > > > > > > > + 3 * outputSize.width / 4, > > > > > > > > > + 3 * outputSize.height / 4); > > > > > > > > > + > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > /** > > > > > > > > > * \brief Run the auto focus algorithm loop > > > > > > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > > > > > > framesToSkip_ = n; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > > > > > > + * \brief Signal emitted when change in AF window was requested > > > > > > > > > + * > > > > > > > > > + * Platform layer supporting AF windows should connect to this signal > > > > s/layer/layers/ > > s/connect to/connect/ > > > > > > > > > > > + * and configure the ISP with new window on each emition. > > > > s/new window/the new window/ > > s/emition/emission/ > > > > The new window will likely need a few frames to become effective. I'm > > worried that the AF algorithm doesn't take that into account at all (it > > also doesn't take the lens movement delays into account, which is > > another of my worries). This can affect the performance and stability of > > the auto-focus algorithm. The delay when configuring new window is set in the platform layer in the patch 07/10. The delay can be different for different ISPs, so I decided it will be better to leave the delay control to the platform layer. Lens delay is a more complex problem and there were discussions previously about it. We need a proper solution for that. As a workaround, I introduced the "wait-frames-lens" tuning file parameter in 06/10. > > > > How have you tested this ? Yes, and most of the delays configured using AfHillClimbing::skipFrames() base on my experiments on RK3399 and camera lens available on my device. Unfortunately, the documentation I have access to, says very little about the delays in ISP. > > > > > > > > > > > + */ > > > > > > > > > + > > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > > > > > > { > > > > > > > > > if (mode == mode_) > > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > > > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > > > > > > } > > > > > > > > > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > > > > > > { > > > > > > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > > > > > > + if (metering == meteringMode_) > > > > > > > > > + return; > > > > > > > > > + > > > > > > > > > + meteringMode_ = metering; > > > > > > > > > + > > > > > > > > > + switch (metering) { > > > > > > > > > + case controls::AfMeteringAuto: > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > > + break; > > > > > > > > > + case controls::AfMeteringWindows: > > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > > + break; > > > > This needs a default case to avoid modifying meteringMode_. We have two > > modes currently defined, if we add a third one, the implementation will > > break. Well, in v4 there was a suggestion to remove the default case from switch statements, so the compiler will complain early about the missing implementation. > > > > When the window is changed the contrast value may drastically change. > > The optimal focus point will likely change too. I think you need to > > reset the AF and possibly trigger a rescan for continuous AF mode. This is true and sounds like a general problem. Do you think this behaviour should be documented, so other platforms will also follow this rule? > > > > > > > > > > > + } > > > > > > > > > } > > > > > > > > > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > > > > > > { > > > > > > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > > > > > > + if (windows.size() != 1) { > > > > > > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > > > > > > + return; > > > > > > > > > + } > > > > > > > > > > > > > > > > Should this be an hard error or should we just want and continue by > > > > > > > > only considering the first available rectangle ? > > > > > > > > > > > > > > I will change it to a warning that only the first window was used. > > > > > > > > > > > > > > > > + > > > > > > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > > > > > > + > > > > > > > > > + userWindow_ = windows[0]; > > > > > > > > > + > > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > > } > > > > > > > > > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > index 2147939b..0f7c65db 100644 > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > @@ -10,6 +10,7 @@ > > > > > > > > > #pragma once > > > > > > > > > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > > +#include <libcamera/base/signal.h> > > > > > > > > > > > > > > > > > > #include "af.h" > > > > > > > > > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > > > > > > public: > > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > > const YamlObject &tuningData); > > > > > > > > > + void configure(const Size &outputSize); > > > > > > > > > int32_t process(double currentContrast); > > > > > > > > > void skipFrames(uint32_t n); > > > > > > > > > > > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > > > > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > > > > > > + > > > > > > > > > private: > > > > > > > > > void setMode(controls::AfModeEnum mode) override; > > > > > > > > > void setRange(controls::AfRangeEnum range) override; > > > > > > > > > @@ -54,6 +58,7 @@ private: > > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > > > > > > > > > > > /* Current focus lens position. */ > > > > > > > > > int32_t lensPosition_ = 0; > > > > > > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > > > > > > double maxChange_; > > > > > > > > > + > > > > > > > > > + /* Default AF window. */ > > > > > > > > > + Rectangle defaultWindow_; > > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > > > > > > + Rectangle userWindow_; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > } /* namespace ipa::algorithms */ > > > > -- > > Regards, > > > > Laurent Pinchart > > Regards, > > Nick Best regards, Daniel
Hello, (CC'ing David) On Thu, May 18, 2023 at 03:14:04PM +0200, Daniel Semkowicz wrote: > On Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst wrote: > > On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart wrote: > > > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote: > > > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote: > > > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote: > > > > > > > > > > > > > > Hi Daniel > > > > > > > cc RPi folks which originally introduced the AF controls and which > > > > > > > have recently been working a new implementation > > > > > > > > > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote: > > > > > > > > > > > > > > > > > > Hi Daniel > > > > > > > > > sorry I have neglected the Windows related patches in previous > > > > > > > > > reviews as I hoped someone would have shared an opinion on the > > > > > > > > > Signal-based mechanism. > > > > > > > > > > > > > > > > > > I don't have better ideas to propose, so let's go with it > > > > > > I don't like the signal much either. We could simply implement a > > > function to retrieve the window. > > Signal has the advantage that it will be invoked only when the window > has changed. With regular function, the function will need to be called > on each frame to check if there is a new window. > What are the disadvantages of using the signal? I was initially thinking we could always set the AF parameters in the ISP configuration buffer, but that would burn extra CPU cycles in the kernel in interrupt context (for rkisp1 at least) to reprogram registers, which should be avoided. The issue with a signal is that it creates a mid-layer, in the sense that the AF implementation mandates a particular implementation of the platform-specific code. You would need to clearly specify where the signal could be emitted from, as the platform-specific code may not operate properly when getting the window changed signalled at random times. For instance, you're currently emitting the signal from - AfHillClimbing::configure() - AfHillClimbing::setMeteringMode() - AfHillClimbing::setWindows() The last two functions are called from algorithms::Af::queueRequest(). The signal is connect to the rkisp1::algorithms::Af::updateCurrentWindow() function, which just stores the rectangle in the updateWindow_ member variable, and that variable is used in rkisp1::algorithms::Af::prepare() to set the ISP parameters buffer. This means that rkisp1::algorithms::Af::prepare() operates with the parameters set by the very latest request, which isn't right. It should operate with the parameters set for the request that corresponds to the frame being prepared. Fixing that is possible, using the frame context (we do so in the AWB algorithm for instance), but that will require the signal handler to know exactly where the signal can be emitted from, and get the right context. The context isn't passed to the signal handler (it doesn't get told for which request the window is set), so we'll have a problem. The rkisp1::algorithms::Af class could still implement this correctly by knowing the signal is emitted from the algorithms::Af::queueRequest() function, and using updateWindow_ in rkisp1::algorithms::Af::queueRequest() after calling algorithms::Af::queueRequest(), but that's implicit knowledge of the internal algorithms::Af helper implementation, which could change in the future. I think it will be simpler if the platform queries the AF window manually, as it will then be in full control of when it retrieves the value and how it uses it. This means the new value will need to be compared with a cached value to decide whether or not to configure the ISP, but that's simpler than going through a signal in the end. > > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > > > > Add support for setting user defined auto focus window in the > > > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > > > > > > > Each time, there is a need for changing the window configuration in the > > > > > > s/Each time,/Each time/ > > > > > > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use > > > > > > s/layer that wants/Layers that want/ > > > > > > > > > > > > > the "Windows" metering mode, needs to connect to this signal > > > > > > s/mode, needs/mode need/ > > > > > > > > > > > > > and configure the ISP on each emission. > > > > > > > > > > > > > > > > > > > > Currently only one window is supported. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > > > > > > --- > > > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > > index 244b8803..0fb17df3 100644 > > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > > > > > > > * movement range rather than coarse search result. > > > > > > > > > > * \todo Implement setRange. > > > > > > > > > > * \todo Implement setSpeed. > > > > > > > > > > - * \todo Implement setMeteringMode. > > > > > > > > > > - * \todo Implement setWindows. > > > > > > > > > > * \todo Implement the AfPauseDeferred mode. > > > > > > > > > > + * \todo Implement support for multiple AF windows. > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > > > return 0; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > > > > > > > + * \param[in] outputSize Camera sensor output size > > > > > > > > > > + * > > > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > > > > s/method/function/ > > > > > > > > > > > > > + * method of the platform layer. > > > > > > > > > > + */ > > > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > > > > > > > > > > > > > According to the AfWindows control definition > > > > > > > > > > > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > > > > > > > returned by the ScalerCropMaximum property. > > > > > > > > > > > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > > > > > > > a larger portion of the pixel array. The portion of the pixel array > > > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and > > > > > > > > > all valid crop rectangles lie inside it. > > > > > > > > > > > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > > > > > > > current output size. It's up to the pipeline handler to re-scale any > > > > > > > > > valid control in the current configured output. > > > > > > > > > > > > > > > > For the 1024x768 stream, I have the following parameters: > > > > > > > > - activeAreaSize: 2592x1944 > > > > > > > > - analogCrop: (0, 0)/2592x1944 > > > > > > > > - outputSize: 1296x972 > > > > > > > > > > > > > > > > When using analogCrop, I will also need to scale the AF window > > > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > > > > > > > values in reference to the ISP input size > > > > > > > > (IPACameraSensorInfo::outputSize). > > > > > > > > Based on what you wrote earlier, I understand that it is supposed to be > > > > > > > > like that? > > > > > > > > > > > > > > I presume so. > > > > > > > > > > > > > > I should check how RPi handles windowing > > > > > > Nick, that's the first question for you. > > > > Our implementation is based on what we understand to be the current > > libcamera specification: That AF windows are in the same coordinate > > system as ScalerCropMaximum, i.e. raw sensor pixels. > > > > I found the documentation phrase "The units used are pixels within the > > rectangle..." ambiguous -- does it suggest that the origin of the > > AfWIndows coordinate system is the top-left corner of > > ScalerCropMaximum? I took the answer to be "no", that those rectangles > > were in a common coordinate system, without offsetting. This in turn > > means that Af Windows will stick to the same objects in the scene, > > regardless of mode or digital zoom. > > I had the same problems with understanding this documentation part, so > probably it should be rephrased to clearly state what is the reference > coordination system for AfWindows. David, git pointed to you as the author of the text :-) I share Nick's understanding here, what is yours ? The documentation should be clarified, and I would like to propose changing the wording to make AfWindows relative to the PixelArrayActiveAreas property, not the ScalerCropMaximum. The coordinate system for ScalerCropMaximum isn't defined, but the coordinate system for ScalerCrop is documented as relative to PixelArrayActiveAreas. Having both ScalerCrop and AfWindows expressed in the same coordinate system would make sense to me. There's still a question of what to do with windows outside (or partially outside) of the ScalerCrop. Does it make sense to allow the user to focus on something that isn't visible in the output image ? Use cases seem dubious to me, and well-written applications will likely ensure that the AF windows full within the visible image, but should we enforce this in libcamera ? > > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > > > > > > > probably to allow pipeline handler express any additional processing > > > > > > > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > > > > > > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part > > > > > > > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > > > > > > > > > > > > > > > > > > > I have some concerns about the ScalerCropMaximum. > > > > > > > > What if the ISP have different size margins for cropping and auto-focus > > > > > > > > windows? I see there are margins defined for the AF window, but nothing > > > > > > > > about the cropping margins in the RK3399 documentation. In this case, > > > > > > > > it will not be a problem, but what if some ISP will have margins for > > > > > > > > cropping, but no margins for AF window? > > > > > > > > > > > > > > Good point. Do we need an AfWindowMaximum ? > > > > > > > > > > > > To make sense, AfWindowMaximum would need to be expressed in reference > > > > > > to how ISP handles that (sensor output size on rkisp) to allow precise > > > > > > AF window configuration. AF window margins on rkisp are small: 2px and > > > > > > 5px. I would like to avoid scaling, so small values compared to > > > > > > the output size. > > > > > > > > > > For ScalerCrop we decided to have a ScalerCropMaximum property which > > > > > basically reports the AnalogueCrop rectangle, as this is the maximum > > > > > rectangle an application can set as a cropping area. The reference is > > > > > the PixelArrayActiveAreas size. > > > > > > > > > > For AfWindows we don't care about AnalogCrop but only about the > > > > > sensor's output as, if my understanding is correct, this is the > > > > > rectangle on which the ISP performs windowing on. > > > > > > > > > > As we don't expose the sensor's output size to applications, the only > > > > > reference we can use is full PixelArrayActiveAreas and re-scale before > > > > > setting the windows. > > > > > > > > > > > I am not sure if AfWindowMaximum as additional property is actually > > > > > > needed. Maximum value for AfWindows would not be enough? > > > > Encoding the largest useful window as the control maximum value (where > > this means minimum X, Y and maximum width,height?) sounds useful. BTW > > is there any way to encode the maximum number of windows? > > That is a good point. If we specify the maximum windows size, we should > also specify the maximum number of windows. > The only solution I can think of is something like this: > > Rectangle minWindow(0, 0, 0, 0); > ControlValue min(Span<const Rectangle>({minWindow})); > > Rectangle maxWindow(0, 0, 640, 480); > ControlValue max(Span<const Rectangle>({maxWindow, maxWindow, maxWindow})); > > Rectangle defWindow(100, 100, 200, 200); > ControlValue def(Span<const Rectangle>({defWindow})); > > ctrls[&controls::AfWindows] = ControlInfo(min, max,def); > > cam lists such control as: > > Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0, > 0)/640x480, (0, 0)/640x480 ]] > > I am not convinced this is a readable solution for the user... > Especially, if there will be bigger number of windows supported. > I expect that minimum and maximum size of will be the same for all > windows, regardless of the number of windows. > From this, it can be deduced that we could expose only the simple pair: > { window size, number of windows } for each min, max and def ControlInfo > value. How about simply adding a AfMaxWindows property ? > > > > > > > > > > As a comment on the ScalerCropMaximum property reports > > > > > > > > > > \todo Turn this property into a "maximum control value" for the > > > > > ScalerCrop control once "dynamic" controls have been implemented. > > > > > > > > > > I presume this is very old, as right now (at least for RkISP1) I see > > > > > the "ControlInfoMap *ipaControls" parameter passed in to configure() and > > > > > being overwritten completely with new control limits. > > > > > > > > > > So my suggestion for a new property was probably not correct. > > > > > > > > > > > Is there a need to expose the window margins to the user at all? > > > > > > > > > > I think so, it would avoid setting controls which can't be applied as > > > > > they are. > > > > > > > > > > I presume you can initialize the maximum value of AfWindows as > > > > > > > > > > { 0, 0, PixelArrayActiveAreas.width - 4, > > > > > PixelArrayActiveAreas.height - 10 } > > > > > > > > > > When re-scaling, you first need to translate it to take into > > > > > consideration the processing margins then re-scale it using the > > > > > activeArea-to-outputSize ratio. > > > > > > > > > > (only compiled, not run-time tested) > > > > > > > > > > Span<const Rectangle> userWindows = > > > > > *request->controls().get<Span<const Rectangle>>(controls::AfWindows); > > > > > std::vector<Rectangle> afWindows; > > > > > > > > > > for (auto userWindow : userWindows) { > > > > > Rectangle window = userWindow.translatedBy({2, 5}); > > > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize); > > > > > afWindows.push_back(window); > > > > > } > > > > > > > > > > (as you only support one window, this could be even > > > > > simplified) > > > > > > > > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm > > > > > implementation, and you would need to pass to the algorithm the active > > > > > area size. One way to do so, as algorithms have access to the > > > > > context_, is to add the active area size to > > > > > IPASessionConfiguration::sensor which already contains the sensor's > > > > > output size. > > > > > > > > > > What do you think ? > > > > > > > > I am not fully happy with this approach as this requires scaling > > > > the values two times, but as you said, expressing it directly > > > > in the output size would require exposing additional controls. > > > > > > > > The other way, I was thinking of, is to express the AF window > > > > in the final output image size (1024x768 in my already mentioned > > > > example). This way, it is easier to think about from the user > > > > perspective. However, this is an opposite approach than the already > > > > existing one for the ScalerCrop. > > > > > > > > Maybe I am not 100% happy with this, but it looks that expressing > > > > the AF window as you show above (in reference to the current > > > > PixelArrayActiveAreas) makes the most sense for the existing code, > > > > because it will be the common approach with the ScalerCrop. > > > > I can change my implementation to this approach. > > > > > > > > Do we need to change also the documentation related to the AF Windows? > > > > > > > > > > We could allow the window size of the whole ISP input size to be set > > > > > > by the user and clamp the size in the IPA before configuring the ISP? > > > > > > > > > > > > In summary, I see two options: > > > > > > 1. Maximum AF window expressed in units that are directly set to ISP > > > > > > (output size for rkisp). Maximum AF window will include margins. > > > > > > 2. Keep the current approach and express the AF window in reference to > > > > > > Analogue crop size. Just allow the full size instead of the > > > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user > > > > > > requested larger window. > > > > > > > > > > > > > RPi any opinions here ? > > > > > > And this is the second question for Nick. > > > > I don't have very strong opinions. Option 2 seems the most general. > > (Again, I've been assuming there's no implicit offsetting for the > > "reference" rectangle -- so the only difference between those > > definitions is a question of cropping?) > > > > I suspect there is not a one-size-fits-all approach. Some sensors may > > be able to generate focus information outside the cropped region; > > others may not. Some platforms may only be able to measure focus > > within the final image. Cropping "as required" sounds like a > > reasonable way to resolve that -- but we ought to give the user some > > way to predict when a window might end up being cropped to nothing > > (and thus ignored)! > > Ok, so my idea with cropping the margins without the user knowledge > is not a good solution... Then, probably We end up with something > similar to the ScalerCrop that is already implemented in the Raspberry > implementation, if I understand it correctly: > > The (x,y) location of the AfWindow is relative to the > PixelArrayActiveAreas that is being used. The units remain native sensor > pixels. Maximum value of the AfWindows ControlInfo (position + size), > already includes all the margins that need to be considered > (e.g. required by the ISP). > > Would you agree with the above definition? I think so. Depending on the platform, the AF statistics can be computed right away on the image received from the sensor, or after some ISP processing steps that will crop a few lines and columns on the edges. I would like to avoid a need for applications to be aware of that cropping, as it should be small. If the window(s) need to be expressed, at the hardware level, in a cropped coordinate system, the IPA module should perform the translation between PixelArrayActiveAreas and the hardware coordinates. If analog crop and/or digital crop gets applied in the sensor, there may be significant cropping, and the application may need to be aware of this. I think you can ignore this for now, we'll need to revisit the processing pipeline model exposed to applications at some point. For platforms that calculate the AF statistics directly on the sensor (or more precisely, where the sensor generates AF data, consumed by the algorithms directly or after processing, as in the PDAF case), the windows could even exceed the sensor digital crop (I doubt they could exceed the analog crop, as pixels outside the analog crop rectangle are not read out by definition). We will have to take that into account too I suppose, but I think you can ignore it for now as you're not dealing with sensor-side AF data. > > > > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > > > > > > > something similar. > > > > > > > > > > > > > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > > > > > > > application switches to AfMeteringWindows mode the rectangle is at > > > > > > > > > least initialized to something. > > > > > > > > > > > > > > > > Good point! > > > > > > > > > > > > > > > > > > +{ > > > > > > > > > > + /* > > > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > > > > > > > + * placed at the center > > > > > > > > > > + */ > > > > > > > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > > > > > > > + static_cast<int>(outputSize.height / 8), > > > > > > > > > > + 3 * outputSize.width / 4, > > > > > > > > > > + 3 * outputSize.height / 4); > > > > > > > > > > + > > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > /** > > > > > > > > > > * \brief Run the auto focus algorithm loop > > > > > > > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > > > > > > > framesToSkip_ = n; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > > > > > > > + * \brief Signal emitted when change in AF window was requested > > > > > > > > > > + * > > > > > > > > > > + * Platform layer supporting AF windows should connect to this signal > > > > > > s/layer/layers/ > > > s/connect to/connect/ > > > > > > > > > > > > > + * and configure the ISP with new window on each emition. > > > > > > s/new window/the new window/ > > > s/emition/emission/ > > > > > > The new window will likely need a few frames to become effective. I'm > > > worried that the AF algorithm doesn't take that into account at all (it > > > also doesn't take the lens movement delays into account, which is > > > another of my worries). This can affect the performance and stability of > > > the auto-focus algorithm. > > The delay when configuring new window is set in the platform layer in > the patch 07/10. The delay can be different for different ISPs, so I > decided it will be better to leave the delay control to the platform > layer. The ISP doesn't add a delay as such. What happens is that a set of ISP parameters buffers are queued to the ISP, and they are applied one by one to consecutive frames. There's no internal ISP delay, the delay comes from the fact that we queue parameters buffers, so the parameters in the last queued buffer will only be applied after all other buffers get processed. This should be handled by programming the AF window in an ISP parameters buffer not right away when it gets queued in a request, but at the right time so that it will be applied to the correct frame. > Lens delay is a more complex problem and there were discussions > previously about it. We need a proper solution for that. > As a workaround, I introduced the "wait-frames-lens" tuning file > parameter in 06/10. The physical properties of the lens movement is indeed a separate and more complex question, which I'm fine handling separately (and later). > > > How have you tested this ? > > Yes, and most of the delays configured using AfHillClimbing::skipFrames() > base on my experiments on RK3399 and camera lens available on my device. > Unfortunately, the documentation I have access to, says very little > about the delays in ISP. Good news, there's no delay in the ISP itself :-) > > > > > > > > > > + */ > > > > > > > > > > + > > > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > > > > > > > { > > > > > > > > > > if (mode == mode_) > > > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > > > > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > > > > > > > { > > > > > > > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > > > > > > > + if (metering == meteringMode_) > > > > > > > > > > + return; > > > > > > > > > > + > > > > > > > > > > + meteringMode_ = metering; > > > > > > > > > > + > > > > > > > > > > + switch (metering) { > > > > > > > > > > + case controls::AfMeteringAuto: > > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > > > + break; > > > > > > > > > > + case controls::AfMeteringWindows: > > > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > > > + break; > > > > > > This needs a default case to avoid modifying meteringMode_. We have two > > > modes currently defined, if we add a third one, the implementation will > > > break. > > Well, in v4 there was a suggestion to remove the default case from > switch statements, so the compiler will complain early about the missing > implementation. Good point, please ignore my comment :-) > > > When the window is changed the contrast value may drastically change. > > > The optimal focus point will likely change too. I think you need to > > > reset the AF and possibly trigger a rescan for continuous AF mode. > > This is true and sounds like a general problem. Do you think this > behaviour should be documented, so other platforms will also follow this > rule? That's a good idea I think. I'm not sure where to best document it though, the documentation in control_ids.yaml shouldn't assume a particular type of AF implementation, and PDAF-based platforms don't need a rescan as such as they don't really scan (well, they may as a fallback...). We could write the requirement in a generic way that doesn't imply a particular AF method, or document it elsewhere. David, any opinion ? > > > > > > > > > > + } > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > > > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > > > > > > > { > > > > > > > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > > > > > > > + if (windows.size() != 1) { > > > > > > > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > > > > > > > + return; > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > Should this be an hard error or should we just want and continue by > > > > > > > > > only considering the first available rectangle ? > > > > > > > > > > > > > > > > I will change it to a warning that only the first window was used. > > > > > > > > > > > > > > > > > > + > > > > > > > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > > > > > > > + > > > > > > > > > > + userWindow_ = windows[0]; > > > > > > > > > > + > > > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > > index 2147939b..0f7c65db 100644 > > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > > @@ -10,6 +10,7 @@ > > > > > > > > > > #pragma once > > > > > > > > > > > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > > > +#include <libcamera/base/signal.h> > > > > > > > > > > > > > > > > > > > > #include "af.h" > > > > > > > > > > > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > > > > > > > public: > > > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > > > const YamlObject &tuningData); > > > > > > > > > > + void configure(const Size &outputSize); > > > > > > > > > > int32_t process(double currentContrast); > > > > > > > > > > void skipFrames(uint32_t n); > > > > > > > > > > > > > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > > > > > > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > > > > > > > + > > > > > > > > > > private: > > > > > > > > > > void setMode(controls::AfModeEnum mode) override; > > > > > > > > > > void setRange(controls::AfRangeEnum range) override; > > > > > > > > > > @@ -54,6 +58,7 @@ private: > > > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > > > > > > > > > > > > > /* Current focus lens position. */ > > > > > > > > > > int32_t lensPosition_ = 0; > > > > > > > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > > > > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > > > > > > > double maxChange_; > > > > > > > > > > + > > > > > > > > > > + /* Default AF window. */ > > > > > > > > > > + Rectangle defaultWindow_; > > > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > > > > > > > + Rectangle userWindow_; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > } /* namespace ipa::algorithms */
Hi Laurent, On Wed, May 31, 2023 at 7:31 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > (CC'ing David) > > On Thu, May 18, 2023 at 03:14:04PM +0200, Daniel Semkowicz wrote: > > On Wed, Apr 26, 2023 at 6:30 PM Nick Hollinghurst wrote: > > > On Wed, 26 Apr 2023 at 04:12, Laurent Pinchart wrote: > > > > On Mon, Apr 24, 2023 at 04:15:22PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi wrote: > > > > > > On Wed, Apr 05, 2023 at 08:02:54AM +0200, Daniel Semkowicz wrote: > > > > > > > On Tue, Apr 4, 2023 at 12:59 PM Jacopo Mondi wrote: > > > > > > > > > > > > > > > > Hi Daniel > > > > > > > > cc RPi folks which originally introduced the AF controls and which > > > > > > > > have recently been working a new implementation > > > > > > > > > > > > > > > > On Tue, Apr 04, 2023 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > > > On Mon, Apr 3, 2023 at 3:07 PM Jacopo Mondi wrote: > > > > > > > > > > > > > > > > > > > > Hi Daniel > > > > > > > > > > sorry I have neglected the Windows related patches in previous > > > > > > > > > > reviews as I hoped someone would have shared an opinion on the > > > > > > > > > > Signal-based mechanism. > > > > > > > > > > > > > > > > > > > > I don't have better ideas to propose, so let's go with it > > > > > > > > I don't like the signal much either. We could simply implement a > > > > function to retrieve the window. > > > > Signal has the advantage that it will be invoked only when the window > > has changed. With regular function, the function will need to be called > > on each frame to check if there is a new window. > > What are the disadvantages of using the signal? > > I was initially thinking we could always set the AF parameters in the > ISP configuration buffer, but that would burn extra CPU cycles in the > kernel in interrupt context (for rkisp1 at least) to reprogram > registers, which should be avoided. > > The issue with a signal is that it creates a mid-layer, in the sense > that the AF implementation mandates a particular implementation of the > platform-specific code. You would need to clearly specify where the > signal could be emitted from, as the platform-specific code may not > operate properly when getting the window changed signalled at random > times. > > For instance, you're currently emitting the signal from > > - AfHillClimbing::configure() > - AfHillClimbing::setMeteringMode() > - AfHillClimbing::setWindows() > > The last two functions are called from algorithms::Af::queueRequest(). > The signal is connect to the > rkisp1::algorithms::Af::updateCurrentWindow() function, which just > stores the rectangle in the updateWindow_ member variable, and that > variable is used in rkisp1::algorithms::Af::prepare() to set the ISP > parameters buffer. This means that rkisp1::algorithms::Af::prepare() > operates with the parameters set by the very latest request, which isn't > right. It should operate with the parameters set for the request that > corresponds to the frame being prepared. Fixing that is possible, using > the frame context (we do so in the AWB algorithm for instance), but that > will require the signal handler to know exactly where the signal can be > emitted from, and get the right context. The context isn't passed to the > signal handler (it doesn't get told for which request the window is > set), so we'll have a problem. The rkisp1::algorithms::Af class could > still implement this correctly by knowing the signal is emitted from the > algorithms::Af::queueRequest() function, and using updateWindow_ in > rkisp1::algorithms::Af::queueRequest() after calling > algorithms::Af::queueRequest(), but that's implicit knowledge of the > internal algorithms::Af helper implementation, which could change in the > future. > > I think it will be simpler if the platform queries the AF window > manually, as it will then be in full control of when it retrieves the > value and how it uses it. This means the new value will need to be > compared with a cached value to decide whether or not to configure the > ISP, but that's simpler than going through a signal in the end. Alright, the situation looks a bit more complex... I initially thought that queueRequest(), prepare() and process() are just executed sequentially for each frame, but now I see there is an optimisation implemented, where queueRequest() and prepare() are called early for each request and buffered. And later, process() is called for each frame with the previously buffered data. Now I understand your worries about signal and delays. Thank you for the explanation! It looks that I should make use of the IPAFrameContext and move the Af state there, configured for each frame, instead of keeping just the current state internally in the Af algorithm classes. Keeping the class state outside the class is not very pretty to me, but I see this is the only reasonable solution there. Is my understanding correct? > > > > > > > > > > > On Fri, Mar 31, 2023 at 10:19:25AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > > > > > > Add support for setting user defined auto focus window in the > > > > > > > > > > > AfHillClimbing. This enables usage of AfMetering and AfWindows controls. > > > > > > > > > > > Each time, there is a need for changing the window configuration in the > > > > > > > > s/Each time,/Each time/ > > > > > > > > > > > > > > > ISP, the signal is emitted. Platform layer that wants to use > > > > > > > > s/layer that wants/Layers that want/ > > > > > > > > > > > > > > > the "Windows" metering mode, needs to connect to this signal > > > > > > > > s/mode, needs/mode need/ > > > > > > > > > > > > > > > and configure the ISP on each emission. > > > > > > > > > > > > > > > > > > > > > > Currently only one window is supported. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > > > > > > > --- > > > > > > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- > > > > > > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ > > > > > > > > > > > 2 files changed, 66 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > > > index 244b8803..0fb17df3 100644 > > > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp > > > > > > > > > > > @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) > > > > > > > > > > > * movement range rather than coarse search result. > > > > > > > > > > > * \todo Implement setRange. > > > > > > > > > > > * \todo Implement setSpeed. > > > > > > > > > > > - * \todo Implement setMeteringMode. > > > > > > > > > > > - * \todo Implement setWindows. > > > > > > > > > > > * \todo Implement the AfPauseDeferred mode. > > > > > > > > > > > + * \todo Implement support for multiple AF windows. > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > > > /** > > > > > > > > > > > @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > > > > return 0; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > + * \brief Configure the AfHillClimbing with sensor information > > > > > > > > > > > + * \param[in] outputSize Camera sensor output size > > > > > > > > > > > + * > > > > > > > > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure() > > > > > > > > s/method/function/ > > > > > > > > > > > > > > > + * method of the platform layer. > > > > > > > > > > > + */ > > > > > > > > > > > +void AfHillClimbing::configure(const Size &outputSize) > > > > > > > > > > > > > > > > > > > > According to the AfWindows control definition > > > > > > > > > > > > > > > > > > > > Sets the focus windows used by the AF algorithm when AfMetering is set > > > > > > > > > > to AfMeteringWindows. The units used are pixels within the rectangle > > > > > > > > > > returned by the ScalerCropMaximum property. > > > > > > > > > > > > > > > > > > > > The final sensor's output size can be obtained by downscaling/cropping > > > > > > > > > > a larger portion of the pixel array. The portion of the pixel array > > > > > > > > > > processed to obtain the final image is named AnalogueCropRectangle and > > > > > > > > > > all valid crop rectangles lie inside it. > > > > > > > > > > > > > > > > > > > > Most of our controls, such as ScalerCrop, are expressed with the > > > > > > > > > > AnalogueCrop as reference, to allow expressing them regardless of the > > > > > > > > > > current output size. It's up to the pipeline handler to re-scale any > > > > > > > > > > valid control in the current configured output. > > > > > > > > > > > > > > > > > > For the 1024x768 stream, I have the following parameters: > > > > > > > > > - activeAreaSize: 2592x1944 > > > > > > > > > - analogCrop: (0, 0)/2592x1944 > > > > > > > > > - outputSize: 1296x972 > > > > > > > > > > > > > > > > > > When using analogCrop, I will also need to scale the AF window > > > > > > > > > size when configuring the rkisp1_params_cfg. rkisp1_params_cfg takes > > > > > > > > > values in reference to the ISP input size > > > > > > > > > (IPACameraSensorInfo::outputSize). > > > > > > > > > Based on what you wrote earlier, I understand that it is supposed to be > > > > > > > > > like that? > > > > > > > > > > > > > > > > I presume so. > > > > > > > > > > > > > > > > I should check how RPi handles windowing > > > > > > > > Nick, that's the first question for you. > > > > > > Our implementation is based on what we understand to be the current > > > libcamera specification: That AF windows are in the same coordinate > > > system as ScalerCropMaximum, i.e. raw sensor pixels. > > > > > > I found the documentation phrase "The units used are pixels within the > > > rectangle..." ambiguous -- does it suggest that the origin of the > > > AfWIndows coordinate system is the top-left corner of > > > ScalerCropMaximum? I took the answer to be "no", that those rectangles > > > were in a common coordinate system, without offsetting. This in turn > > > means that Af Windows will stick to the same objects in the scene, > > > regardless of mode or digital zoom. > > > > I had the same problems with understanding this documentation part, so > > probably it should be rephrased to clearly state what is the reference > > coordination system for AfWindows. > > David, git pointed to you as the author of the text :-) I share Nick's > understanding here, what is yours ? > > The documentation should be clarified, and I would like to propose > changing the wording to make AfWindows relative to the > PixelArrayActiveAreas property, not the ScalerCropMaximum. The > coordinate system for ScalerCropMaximum isn't defined, but the > coordinate system for ScalerCrop is documented as relative to > PixelArrayActiveAreas. Having both ScalerCrop and AfWindows expressed > in the same coordinate system would make sense to me. > > There's still a question of what to do with windows outside (or > partially outside) of the ScalerCrop. Does it make sense to allow the > user to focus on something that isn't visible in the output image ? Use > cases seem dubious to me, and well-written applications will likely > ensure that the AF windows full within the visible image, but should we > enforce this in libcamera ? If such a situation is possible, then why not allow this? I would not add more constraints than hardware already requires. It does not make implementation easier :) > > > > > > > > > > > I'm not 100% sure why we decided to use ScalerCropMaximum here, most > > > > > > > > > > probably to allow pipeline handler express any additional processing > > > > > > > > > > margins required by any cropping/scaling that happens on the ISP. > > > > > > > > > > > > > > > > > > > > However as the RkISP1 pipeline doesn't express any of those margin > > > > > > > > > > yet, I would simply use here the sensor's analogue crop, which is part > > > > > > > > > > of the IPACameraSensorInfo you already use in the platform layer. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have some concerns about the ScalerCropMaximum. > > > > > > > > > What if the ISP have different size margins for cropping and auto-focus > > > > > > > > > windows? I see there are margins defined for the AF window, but nothing > > > > > > > > > about the cropping margins in the RK3399 documentation. In this case, > > > > > > > > > it will not be a problem, but what if some ISP will have margins for > > > > > > > > > cropping, but no margins for AF window? > > > > > > > > > > > > > > > > Good point. Do we need an AfWindowMaximum ? > > > > > > > > > > > > > > To make sense, AfWindowMaximum would need to be expressed in reference > > > > > > > to how ISP handles that (sensor output size on rkisp) to allow precise > > > > > > > AF window configuration. AF window margins on rkisp are small: 2px and > > > > > > > 5px. I would like to avoid scaling, so small values compared to > > > > > > > the output size. > > > > > > > > > > > > For ScalerCrop we decided to have a ScalerCropMaximum property which > > > > > > basically reports the AnalogueCrop rectangle, as this is the maximum > > > > > > rectangle an application can set as a cropping area. The reference is > > > > > > the PixelArrayActiveAreas size. > > > > > > > > > > > > For AfWindows we don't care about AnalogCrop but only about the > > > > > > sensor's output as, if my understanding is correct, this is the > > > > > > rectangle on which the ISP performs windowing on. > > > > > > > > > > > > As we don't expose the sensor's output size to applications, the only > > > > > > reference we can use is full PixelArrayActiveAreas and re-scale before > > > > > > setting the windows. > > > > > > > > > > > > > I am not sure if AfWindowMaximum as additional property is actually > > > > > > > needed. Maximum value for AfWindows would not be enough? > > > > > > Encoding the largest useful window as the control maximum value (where > > > this means minimum X, Y and maximum width,height?) sounds useful. BTW > > > is there any way to encode the maximum number of windows? > > > > That is a good point. If we specify the maximum windows size, we should > > also specify the maximum number of windows. > > The only solution I can think of is something like this: > > > > Rectangle minWindow(0, 0, 0, 0); > > ControlValue min(Span<const Rectangle>({minWindow})); > > > > Rectangle maxWindow(0, 0, 640, 480); > > ControlValue max(Span<const Rectangle>({maxWindow, maxWindow, maxWindow})); > > > > Rectangle defWindow(100, 100, 200, 200); > > ControlValue def(Span<const Rectangle>({defWindow})); > > > > ctrls[&controls::AfWindows] = ControlInfo(min, max,def); > > > > cam lists such control as: > > > > Control: AfWindows: [[ (0, 0)/0x0 ]..[ (0, 0)/640x480, (0, > > 0)/640x480, (0, 0)/640x480 ]] > > > > I am not convinced this is a readable solution for the user... > > Especially, if there will be bigger number of windows supported. > > I expect that minimum and maximum size of will be the same for all > > windows, regardless of the number of windows. > > From this, it can be deduced that we could expose only the simple pair: > > { window size, number of windows } for each min, max and def ControlInfo > > value. > > How about simply adding a AfMaxWindows property ? I still do not understand the reason for existence of both "maximum value" properties and maximum values in the controls. Isn't it a duplication? Am I missing something? > > > > > > > > > > > > > As a comment on the ScalerCropMaximum property reports > > > > > > > > > > > > \todo Turn this property into a "maximum control value" for the > > > > > > ScalerCrop control once "dynamic" controls have been implemented. > > > > > > > > > > > > I presume this is very old, as right now (at least for RkISP1) I see > > > > > > the "ControlInfoMap *ipaControls" parameter passed in to configure() and > > > > > > being overwritten completely with new control limits. > > > > > > > > > > > > So my suggestion for a new property was probably not correct. > > > > > > > > > > > > > Is there a need to expose the window margins to the user at all? > > > > > > > > > > > > I think so, it would avoid setting controls which can't be applied as > > > > > > they are. > > > > > > > > > > > > I presume you can initialize the maximum value of AfWindows as > > > > > > > > > > > > { 0, 0, PixelArrayActiveAreas.width - 4, > > > > > > PixelArrayActiveAreas.height - 10 } > > > > > > > > > > > > When re-scaling, you first need to translate it to take into > > > > > > consideration the processing margins then re-scale it using the > > > > > > activeArea-to-outputSize ratio. > > > > > > > > > > > > (only compiled, not run-time tested) > > > > > > > > > > > > Span<const Rectangle> userWindows = > > > > > > *request->controls().get<Span<const Rectangle>>(controls::AfWindows); > > > > > > std::vector<Rectangle> afWindows; > > > > > > > > > > > > for (auto userWindow : userWindows) { > > > > > > Rectangle window = userWindow.translatedBy({2, 5}); > > > > > > window.scaleBy(sensorInfo.outputSize, sensorInfo.activeAreaSize); > > > > > > afWindows.push_back(window); > > > > > > } > > > > > > > > > > > > (as you only support one window, this could be even > > > > > > simplified) > > > > > > > > > > > > Now, I would need to do the re-scaling in the RkISP1 Af algorithm > > > > > > implementation, and you would need to pass to the algorithm the active > > > > > > area size. One way to do so, as algorithms have access to the > > > > > > context_, is to add the active area size to > > > > > > IPASessionConfiguration::sensor which already contains the sensor's > > > > > > output size. > > > > > > > > > > > > What do you think ? > > > > > > > > > > I am not fully happy with this approach as this requires scaling > > > > > the values two times, but as you said, expressing it directly > > > > > in the output size would require exposing additional controls. > > > > > > > > > > The other way, I was thinking of, is to express the AF window > > > > > in the final output image size (1024x768 in my already mentioned > > > > > example). This way, it is easier to think about from the user > > > > > perspective. However, this is an opposite approach than the already > > > > > existing one for the ScalerCrop. > > > > > > > > > > Maybe I am not 100% happy with this, but it looks that expressing > > > > > the AF window as you show above (in reference to the current > > > > > PixelArrayActiveAreas) makes the most sense for the existing code, > > > > > because it will be the common approach with the ScalerCrop. > > > > > I can change my implementation to this approach. > > > > > > > > > > Do we need to change also the documentation related to the AF Windows? > > > > > > > > > > > > We could allow the window size of the whole ISP input size to be set > > > > > > > by the user and clamp the size in the IPA before configuring the ISP? > > > > > > > > > > > > > > In summary, I see two options: > > > > > > > 1. Maximum AF window expressed in units that are directly set to ISP > > > > > > > (output size for rkisp). Maximum AF window will include margins. > > > > > > > 2. Keep the current approach and express the AF window in reference to > > > > > > > Analogue crop size. Just allow the full size instead of the > > > > > > > ScalerCropMaximum. Clamp the margins inside the IPA if user > > > > > > > requested larger window. > > > > > > > > > > > > > > > RPi any opinions here ? > > > > > > > > And this is the second question for Nick. > > > > > > I don't have very strong opinions. Option 2 seems the most general. > > > (Again, I've been assuming there's no implicit offsetting for the > > > "reference" rectangle -- so the only difference between those > > > definitions is a question of cropping?) > > > > > > I suspect there is not a one-size-fits-all approach. Some sensors may > > > be able to generate focus information outside the cropped region; > > > others may not. Some platforms may only be able to measure focus > > > within the final image. Cropping "as required" sounds like a > > > reasonable way to resolve that -- but we ought to give the user some > > > way to predict when a window might end up being cropped to nothing > > > (and thus ignored)! > > > > Ok, so my idea with cropping the margins without the user knowledge > > is not a good solution... Then, probably We end up with something > > similar to the ScalerCrop that is already implemented in the Raspberry > > implementation, if I understand it correctly: > > > > The (x,y) location of the AfWindow is relative to the > > PixelArrayActiveAreas that is being used. The units remain native sensor > > pixels. Maximum value of the AfWindows ControlInfo (position + size), > > already includes all the margins that need to be considered > > (e.g. required by the ISP). > > > > Would you agree with the above definition? > > I think so. Depending on the platform, the AF statistics can be computed > right away on the image received from the sensor, or after some ISP > processing steps that will crop a few lines and columns on the edges. I > would like to avoid a need for applications to be aware of that > cropping, as it should be small. If the window(s) need to be expressed, > at the hardware level, in a cropped coordinate system, the IPA module > should perform the translation between PixelArrayActiveAreas and the > hardware coordinates. > > If analog crop and/or digital crop gets applied in the sensor, there may > be significant cropping, and the application may need to be aware of > this. I think you can ignore this for now, we'll need to revisit the > processing pipeline model exposed to applications at some point. > > For platforms that calculate the AF statistics directly on the sensor > (or more precisely, where the sensor generates AF data, consumed by the > algorithms directly or after processing, as in the PDAF case), the > windows could even exceed the sensor digital crop (I doubt they could > exceed the analog crop, as pixels outside the analog crop rectangle are > not read out by definition). We will have to take that into account too > I suppose, but I think you can ignore it for now as you're not dealing > with sensor-side AF data. > > > > > > > > > > > To remove ambiguities I would call the parameter here "maxWindow" or > > > > > > > > > > something similar. > > > > > > > > > > > > > > > > > > > > I would also initialize userWindow_ to the same value, so that if an > > > > > > > > > > application switches to AfMeteringWindows mode the rectangle is at > > > > > > > > > > least initialized to something. > > > > > > > > > > > > > > > > > > Good point! > > > > > > > > > > > > > > > > > > > > +{ > > > > > > > > > > > + /* > > > > > > > > > > > + * Default AF window of 3/4 size of the camera sensor output, > > > > > > > > > > > + * placed at the center > > > > > > > > > > > + */ > > > > > > > > > > > + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), > > > > > > > > > > > + static_cast<int>(outputSize.height / 8), > > > > > > > > > > > + 3 * outputSize.width / 4, > > > > > > > > > > > + 3 * outputSize.height / 4); > > > > > > > > > > > + > > > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > > > > +} > > > > > > > > > > > + > > > > > > > > > > > /** > > > > > > > > > > > * \brief Run the auto focus algorithm loop > > > > > > > > > > > * \param[in] currentContrast New value of contrast measured for current frame > > > > > > > > > > > @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) > > > > > > > > > > > framesToSkip_ = n; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > + * \var AfHillClimbing::windowUpdateRequested > > > > > > > > > > > + * \brief Signal emitted when change in AF window was requested > > > > > > > > > > > + * > > > > > > > > > > > + * Platform layer supporting AF windows should connect to this signal > > > > > > > > s/layer/layers/ > > > > s/connect to/connect/ > > > > > > > > > > > > > > > + * and configure the ISP with new window on each emition. > > > > > > > > s/new window/the new window/ > > > > s/emition/emission/ > > > > > > > > The new window will likely need a few frames to become effective. I'm > > > > worried that the AF algorithm doesn't take that into account at all (it > > > > also doesn't take the lens movement delays into account, which is > > > > another of my worries). This can affect the performance and stability of > > > > the auto-focus algorithm. > > > > The delay when configuring new window is set in the platform layer in > > the patch 07/10. The delay can be different for different ISPs, so I > > decided it will be better to leave the delay control to the platform > > layer. > > The ISP doesn't add a delay as such. What happens is that a set of ISP > parameters buffers are queued to the ISP, and they are applied one by > one to consecutive frames. There's no internal ISP delay, the delay > comes from the fact that we queue parameters buffers, so the parameters > in the last queued buffer will only be applied after all other buffers > get processed. This should be handled by programming the AF window in an > ISP parameters buffer not right away when it gets queued in a request, > but at the right time so that it will be applied to the correct frame. > > > Lens delay is a more complex problem and there were discussions > > previously about it. We need a proper solution for that. > > As a workaround, I introduced the "wait-frames-lens" tuning file > > parameter in 06/10. > > The physical properties of the lens movement is indeed a separate and > more complex question, which I'm fine handling separately (and later). > > > > > How have you tested this ? > > > > Yes, and most of the delays configured using AfHillClimbing::skipFrames() > > base on my experiments on RK3399 and camera lens available on my device. > > Unfortunately, the documentation I have access to, says very little > > about the delays in ISP. > > Good news, there's no delay in the ISP itself :-) > > > > > > > > > > > > + */ > > > > > > > > > > > + > > > > > > > > > > > void AfHillClimbing::setMode(controls::AfModeEnum mode) > > > > > > > > > > > { > > > > > > > > > > > if (mode == mode_) > > > > > > > > > > > @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) > > > > > > > > > > > LOG(Af, Error) << "setSpeed() not implemented!"; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) > > > > > > > > > > > +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) > > > > > > > > > > > { > > > > > > > > > > > - LOG(Af, Error) << "setMeteringMode() not implemented!"; > > > > > > > > > > > + if (metering == meteringMode_) > > > > > > > > > > > + return; > > > > > > > > > > > + > > > > > > > > > > > + meteringMode_ = metering; > > > > > > > > > > > + > > > > > > > > > > > + switch (metering) { > > > > > > > > > > > + case controls::AfMeteringAuto: > > > > > > > > > > > + windowUpdateRequested.emit(defaultWindow_); > > > > > > > > > > > + break; > > > > > > > > > > > + case controls::AfMeteringWindows: > > > > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > > > > + break; > > > > > > > > This needs a default case to avoid modifying meteringMode_. We have two > > > > modes currently defined, if we add a third one, the implementation will > > > > break. > > > > Well, in v4 there was a suggestion to remove the default case from > > switch statements, so the compiler will complain early about the missing > > implementation. > > Good point, please ignore my comment :-) > > > > > When the window is changed the contrast value may drastically change. > > > > The optimal focus point will likely change too. I think you need to > > > > reset the AF and possibly trigger a rescan for continuous AF mode. > > > > This is true and sounds like a general problem. Do you think this > > behaviour should be documented, so other platforms will also follow this > > rule? > > That's a good idea I think. I'm not sure where to best document it > though, the documentation in control_ids.yaml shouldn't assume a > particular type of AF implementation, and PDAF-based platforms don't > need a rescan as such as they don't really scan (well, they may as a > fallback...). We could write the requirement in a generic way that > doesn't imply a particular AF method, or document it elsewhere. David, > any opinion ? > > > > > > > > > > > > + } > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) > > > > > > > > > > > +void AfHillClimbing::setWindows(Span<const Rectangle> windows) > > > > > > > > > > > { > > > > > > > > > > > - LOG(Af, Error) << "setWindows() not implemented!"; > > > > > > > > > > > + if (windows.size() != 1) { > > > > > > > > > > > + LOG(Af, Error) << "Only one AF window is supported"; > > > > > > > > > > > + return; > > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > > Should this be an hard error or should we just want and continue by > > > > > > > > > > only considering the first available rectangle ? > > > > > > > > > > > > > > > > > > I will change it to a warning that only the first window was used. > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > + LOG(Af, Debug) << "setWindows: " << windows[0]; > > > > > > > > > > > + > > > > > > > > > > > + userWindow_ = windows[0]; > > > > > > > > > > > + > > > > > > > > > > > + if (meteringMode_ == controls::AfMeteringWindows) > > > > > > > > > > > + windowUpdateRequested.emit(userWindow_); > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) > > > > > > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > > > index 2147939b..0f7c65db 100644 > > > > > > > > > > > --- a/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h > > > > > > > > > > > @@ -10,6 +10,7 @@ > > > > > > > > > > > #pragma once > > > > > > > > > > > > > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > > > > +#include <libcamera/base/signal.h> > > > > > > > > > > > > > > > > > > > > > > #include "af.h" > > > > > > > > > > > > > > > > > > > > > > @@ -26,12 +27,15 @@ class AfHillClimbing : public Af > > > > > > > > > > > public: > > > > > > > > > > > int init(int32_t minFocusPosition, int32_t maxFocusPosition, > > > > > > > > > > > const YamlObject &tuningData); > > > > > > > > > > > + void configure(const Size &outputSize); > > > > > > > > > > > int32_t process(double currentContrast); > > > > > > > > > > > void skipFrames(uint32_t n); > > > > > > > > > > > > > > > > > > > > > > controls::AfStateEnum state() override { return state_; } > > > > > > > > > > > controls::AfPauseStateEnum pauseState() override { return pauseState_; } > > > > > > > > > > > > > > > > > > > > > > + Signal<const Rectangle &> windowUpdateRequested; > > > > > > > > > > > + > > > > > > > > > > > private: > > > > > > > > > > > void setMode(controls::AfModeEnum mode) override; > > > > > > > > > > > void setRange(controls::AfRangeEnum range) override; > > > > > > > > > > > @@ -54,6 +58,7 @@ private: > > > > > > > > > > > controls::AfModeEnum mode_ = controls::AfModeManual; > > > > > > > > > > > controls::AfStateEnum state_ = controls::AfStateIdle; > > > > > > > > > > > controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; > > > > > > > > > > > + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; > > > > > > > > > > > > > > > > > > > > > > /* Current focus lens position. */ > > > > > > > > > > > int32_t lensPosition_ = 0; > > > > > > > > > > > @@ -84,6 +89,11 @@ private: > > > > > > > > > > > > > > > > > > > > > > /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ > > > > > > > > > > > double maxChange_; > > > > > > > > > > > + > > > > > > > > > > > + /* Default AF window. */ > > > > > > > > > > > + Rectangle defaultWindow_; > > > > > > > > > > > + /* AF window set by user using AF_WINDOWS control. */ > > > > > > > > > > > + Rectangle userWindow_; > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > } /* namespace ipa::algorithms */ > > -- > Regards, > > Laurent Pinchart Best regards Daniel
diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp index 244b8803..0fb17df3 100644 --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp @@ -63,9 +63,8 @@ LOG_DEFINE_CATEGORY(Af) * movement range rather than coarse search result. * \todo Implement setRange. * \todo Implement setSpeed. - * \todo Implement setMeteringMode. - * \todo Implement setWindows. * \todo Implement the AfPauseDeferred mode. + * \todo Implement support for multiple AF windows. */ /** @@ -99,6 +98,27 @@ int AfHillClimbing::init(int32_t minFocusPosition, int32_t maxFocusPosition, return 0; } +/** + * \brief Configure the AfHillClimbing with sensor information + * \param[in] outputSize Camera sensor output size + * + * This method should be called in the libcamera::ipa::Algorithm::configure() + * method of the platform layer. + */ +void AfHillClimbing::configure(const Size &outputSize) +{ + /* + * Default AF window of 3/4 size of the camera sensor output, + * placed at the center + */ + defaultWindow_ = Rectangle(static_cast<int>(outputSize.width / 8), + static_cast<int>(outputSize.height / 8), + 3 * outputSize.width / 4, + 3 * outputSize.height / 4); + + windowUpdateRequested.emit(defaultWindow_); +} + /** * \brief Run the auto focus algorithm loop * \param[in] currentContrast New value of contrast measured for current frame @@ -185,6 +205,14 @@ void AfHillClimbing::skipFrames(uint32_t n) framesToSkip_ = n; } +/** + * \var AfHillClimbing::windowUpdateRequested + * \brief Signal emitted when change in AF window was requested + * + * Platform layer supporting AF windows should connect to this signal + * and configure the ISP with new window on each emition. + */ + void AfHillClimbing::setMode(controls::AfModeEnum mode) { if (mode == mode_) @@ -210,14 +238,36 @@ void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) LOG(Af, Error) << "setSpeed() not implemented!"; } -void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering) +void AfHillClimbing::setMeteringMode(controls::AfMeteringEnum metering) { - LOG(Af, Error) << "setMeteringMode() not implemented!"; + if (metering == meteringMode_) + return; + + meteringMode_ = metering; + + switch (metering) { + case controls::AfMeteringAuto: + windowUpdateRequested.emit(defaultWindow_); + break; + case controls::AfMeteringWindows: + windowUpdateRequested.emit(userWindow_); + break; + } } -void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows) +void AfHillClimbing::setWindows(Span<const Rectangle> windows) { - LOG(Af, Error) << "setWindows() not implemented!"; + if (windows.size() != 1) { + LOG(Af, Error) << "Only one AF window is supported"; + return; + } + + LOG(Af, Debug) << "setWindows: " << windows[0]; + + userWindow_ = windows[0]; + + if (meteringMode_ == controls::AfMeteringWindows) + windowUpdateRequested.emit(userWindow_); } void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger) diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h index 2147939b..0f7c65db 100644 --- a/src/ipa/libipa/algorithms/af_hill_climbing.h +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h @@ -10,6 +10,7 @@ #pragma once #include <libcamera/base/log.h> +#include <libcamera/base/signal.h> #include "af.h" @@ -26,12 +27,15 @@ class AfHillClimbing : public Af public: int init(int32_t minFocusPosition, int32_t maxFocusPosition, const YamlObject &tuningData); + void configure(const Size &outputSize); int32_t process(double currentContrast); void skipFrames(uint32_t n); controls::AfStateEnum state() override { return state_; } controls::AfPauseStateEnum pauseState() override { return pauseState_; } + Signal<const Rectangle &> windowUpdateRequested; + private: void setMode(controls::AfModeEnum mode) override; void setRange(controls::AfRangeEnum range) override; @@ -54,6 +58,7 @@ private: controls::AfModeEnum mode_ = controls::AfModeManual; controls::AfStateEnum state_ = controls::AfStateIdle; controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning; + controls::AfMeteringEnum meteringMode_ = controls::AfMeteringAuto; /* Current focus lens position. */ int32_t lensPosition_ = 0; @@ -84,6 +89,11 @@ private: /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */ double maxChange_; + + /* Default AF window. */ + Rectangle defaultWindow_; + /* AF window set by user using AF_WINDOWS control. */ + Rectangle userWindow_; }; } /* namespace ipa::algorithms */
Add support for setting user defined auto focus window in the AfHillClimbing. This enables usage of AfMetering and AfWindows controls. Each time, there is a need for changing the window configuration in the ISP, the signal is emitted. Platform layer that wants to use the "Windows" metering mode, needs to connect to this signal and configure the ISP on each emission. Currently only one window is supported. Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- .../libipa/algorithms/af_hill_climbing.cpp | 62 +++++++++++++++++-- src/ipa/libipa/algorithms/af_hill_climbing.h | 10 +++ 2 files changed, 66 insertions(+), 6 deletions(-)