[libcamera-devel] [PATCH v6 05/10] ipa: af_hill_climbing: Add "Windows" metering mode
Daniel Semkowicz
dse at thaumatec.com
Mon Apr 24 16:15:22 CEST 2023
Hi Jacopo,
On Wed, Apr 5, 2023 at 4:39 PM Jacopo Mondi
<jacopo.mondi at 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 at 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 at 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 at 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
More information about the libcamera-devel
mailing list