[libcamera-devel] [PATCH v3 9/9] ipa: ipu3: Move IPU3 agc into algorithms
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Aug 19 00:09:03 CEST 2021
On 18/08/2021 23:05, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Wed, Aug 18, 2021 at 05:54:03PM +0200, Jean-Michel Hautbois wrote:
>> Now that the interface is properly used by the AGC class, move it into
>> ipa::ipu3::algorithms and let the loops do the calls.
>> As we need to exchange the exposure_ and gain_ by passing them through the
>> FrameContext, use the calculated values in setControls() function to
>> ease the reading.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 21 +++++++++----------
>> src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 +++++++++---------
>> src/ipa/ipu3/algorithms/meson.build | 1 +
>> src/ipa/ipu3/ipu3.cpp | 15 +++++--------
>> src/ipa/ipu3/meson.build | 1 -
>> 5 files changed, 26 insertions(+), 32 deletions(-)
>> rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (93%)
>> rename src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} (73%)
>>
>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> similarity index 93%
>> rename from src/ipa/ipu3/ipu3_agc.cpp
>> rename to src/ipa/ipu3/algorithms/agc.cpp
>> index 1c1f5fb5..bb119cb4 100644
>> --- a/src/ipa/ipu3/ipu3_agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -5,7 +5,7 @@
>> * ipu3_agc.cpp - AGC/AEC control algorithm
>> */
>>
>> -#include "ipu3_agc.h"
>> +#include "agc.h"
>>
>> #include <algorithm>
>> #include <chrono>
>> @@ -21,7 +21,7 @@ namespace libcamera {
>>
>> using namespace std::literals::chrono_literals;
>>
>> -namespace ipa::ipu3 {
>> +namespace ipa::ipu3::algorithms {
>>
>> LOG_DEFINE_CATEGORY(IPU3Agc)
>>
>> @@ -50,14 +50,13 @@ static constexpr double kEvGainTarget = 0.5;
>> /* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
>> static constexpr uint8_t kCellSize = 8;
>>
>> -IPU3Agc::IPU3Agc()
>> +Agc::Agc()
>> : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
>> - maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
>> - currentExposure_(0s), currentExposureNoDg_(0s)
>
> Why can those two fields not be initialized anymore ?
>
>> + maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s)
>> {
>> }
>>
>> -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>> +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>> {
>> aeGrid_ = context.configuration.grid.bdsGrid;
>>
>> @@ -67,7 +66,7 @@ int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>> return 0;
>> }
>>
>> -void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>> +void Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>> {
>> const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
>> Rectangle aeRegion = { statsAeGrid.x_start,
>> @@ -108,7 +107,7 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>> iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>> }
>>
>> -void IPU3Agc::filterExposure()
>> +void Agc::filterExposure()
>> {
>> double speed = 0.2;
>> if (prevExposure_ == 0s) {
>> @@ -142,7 +141,7 @@ void IPU3Agc::filterExposure()
>> LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
>> }
>>
>> -void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>> +void Agc::lockExposureGain(uint32_t &exposure, double &gain)
>> {
>> /* Algorithm initialization should wait for first valid frames */
>> /* \todo - have a number of frames given by DelayedControls ?
>> @@ -185,7 +184,7 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>> lastFrame_ = frameCount_;
>> }
>>
>> -void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>> +void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>> {
>> uint32_t &exposure = context.frameContext.agc.exposure;
>> double &gain = context.frameContext.agc.gain;
>> @@ -194,6 +193,6 @@ void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>> frameCount_++;
>> }
>>
>> -} /* namespace ipa::ipu3 */
>> +} /* namespace ipa::ipu3::algorithms */
>>
>> } /* namespace libcamera */
>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/algorithms/agc.h
>> similarity index 73%
>> rename from src/ipa/ipu3/ipu3_agc.h
>> rename to src/ipa/ipu3/algorithms/agc.h
>> index 0e922664..1739d2fd 100644
>> --- a/src/ipa/ipu3/ipu3_agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -2,10 +2,10 @@
>> /*
>> * Copyright (C) 2021, Ideas On Board
>> *
>> - * ipu3_agc.h - IPU3 AGC/AEC control algorithm
>> + * agc.h - IPU3 AGC/AEC control algorithm
>> */
>> -#ifndef __LIBCAMERA_IPU3_AGC_H__
>> -#define __LIBCAMERA_IPU3_AGC_H__
>> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>> +#define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>>
>> #include <linux/intel-ipu3.h>
>>
>> @@ -13,21 +13,21 @@
>>
>> #include <libcamera/geometry.h>
>>
>> -#include "algorithms/algorithm.h"
>> +#include "algorithm.h"
>>
>> namespace libcamera {
>>
>> struct IPACameraSensorInfo;
>>
>> -namespace ipa::ipu3 {
>> +namespace ipa::ipu3::algorithms {
>>
>> using utils::Duration;
>>
>> -class IPU3Agc : public Algorithm
>> +class Agc : public Algorithm
>> {
>> public:
>> - IPU3Agc();
>> - ~IPU3Agc() = default;
>> + Agc();
>> + ~Agc() = default;
>>
>> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>> @@ -53,8 +53,8 @@ private:
>> Duration currentExposureNoDg_;
>> };
>>
>> -} /* namespace ipa::ipu3 */
>> +} /* namespace ipa::ipu3::algorithms */
>>
>> } /* namespace libcamera */
>>
>> -#endif /* __LIBCAMERA_IPU3_AGC_H__ */
>> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__ */
>> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
>> index 87377f4e..acaadd90 100644
>> --- a/src/ipa/ipu3/algorithms/meson.build
>> +++ b/src/ipa/ipu3/algorithms/meson.build
>> @@ -2,6 +2,7 @@
>>
>> ipu3_ipa_algorithms = files([
>> 'algorithm.cpp',
>> + 'agc.cpp',
>
> Wrong alphabetical order.
This one doesn't worry me too much if it's sorted alphabetically...
>
>> 'awb.cpp',
>> 'contrast.cpp',
>> ])
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index f4f49025..d73ec79d 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -30,9 +30,9 @@
>> #include "libcamera/internal/mapped_framebuffer.h"
>>
>> #include "algorithms/algorithm.h"
>> +#include "algorithms/agc.h"
>
> Here too.
>
But this one does.
If agc.h comes before algorithm.h, then by definition we're relying upon
the inclusion of algorithm.h from agc.h, so there's no point it being
here at all.
But I'd personally keep it, and keep it at the beginning of the
inclusions, as an exception to the alphabetical ordering.
The other algorithms can be sorted alphabetically though
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> #include "algorithms/awb.h"
>> #include "algorithms/contrast.h"
>> -#include "ipu3_agc.h"
>> #include "libipa/camera_sensor_helper.h"
>>
>> static constexpr uint32_t kMaxCellWidthPerSet = 160;
>> @@ -136,8 +136,6 @@ private:
>> uint32_t minGain_;
>> uint32_t maxGain_;
>>
>> - /* Interface to the AEC/AGC algorithm */
>> - std::unique_ptr<IPU3Agc> agcAlgo_;
>> /* Interface to the Camera Helper */
>> std::unique_ptr<CameraSensorHelper> camHelper_;
>>
>> @@ -218,6 +216,7 @@ int IPAIPU3::init(const IPASettings &settings,
>> *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>>
>> /* Construct our Algorithms */
>> + algorithms_.emplace_back(new algorithms::Agc());
>> algorithms_.emplace_back(new algorithms::Awb());
>> algorithms_.emplace_back(new algorithms::Contrast());
>>
>> @@ -337,10 +336,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>> return ret;
>> }
>>
>> -
>> - agcAlgo_ = std::make_unique<IPU3Agc>();
>> - agcAlgo_->configure(context_, configInfo);
>> -
>> return 0;
>> }
>>
>> @@ -436,9 +431,6 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>> for (auto const &algo : algorithms_)
>> algo->process(context_, stats);
>>
>> - agcAlgo_->process(context_, stats);
>> - exposure_ = context_.frameContext.agc.exposure;
>> - gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
>> setControls(frame);
>>
>> /* \todo Use VBlank value calculated from each frame exposure. */
>> @@ -458,6 +450,9 @@ void IPAIPU3::setControls(unsigned int frame)
>> IPU3Action op;
>> op.op = ActionSetSensorControls;
>>
>> + exposure_ = context_.frameContext.agc.exposure;
>> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
>> +
>> ControlList ctrls(ctrls_);
>> ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>> index d1126947..39320980 100644
>> --- a/src/ipa/ipu3/meson.build
>> +++ b/src/ipa/ipu3/meson.build
>> @@ -6,7 +6,6 @@ ipa_name = 'ipa_ipu3'
>>
>> ipu3_ipa_sources = files([
>> 'ipu3.cpp',
>> - 'ipu3_agc.cpp',
>> ])
>>
>> ipu3_ipa_sources += ipu3_ipa_algorithms
>
More information about the libcamera-devel
mailing list