| Message ID | 20210812165243.276977-11-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On 12/08/2021 17:52, 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@ideasonboard.com> > --- > .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 18 +++++++-------- > src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 ++++++++--------- > src/ipa/ipu3/algorithms/meson.build | 1 + > src/ipa/ipu3/ipu3.cpp | 22 +++++++------------ > src/ipa/ipu3/meson.build | 1 - > 5 files changed, 28 insertions(+), 34 deletions(-) > rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (94%) > 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 94% > rename from src/ipa/ipu3/ipu3_agc.cpp > rename to src/ipa/ipu3/algorithms/agc.cpp > index 0d0584a8..ee3d3bc2 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,7 +50,7 @@ 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), > @@ -58,7 +58,7 @@ IPU3Agc::IPU3Agc() > { > } > > -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > { > aeGrid_ = context.configuration.grid.bdsGrid; > > @@ -68,7 +68,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, > @@ -109,7 +109,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) { > @@ -143,7 +143,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 ? > @@ -186,7 +186,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; > @@ -195,6 +195,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 9ef2dd41..e75d92e0 100644 > --- a/src/ipa/ipu3/algorithms/meson.build > +++ b/src/ipa/ipu3/algorithms/meson.build > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: CC0-1.0 > > ipu3_ipa_algorithms = files([ > + 'agc.cpp', > 'awb.cpp', > 'contrast.cpp', > 'grid.cpp', > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index bea2a372..a99fbcac 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -30,10 +30,10 @@ > #include "libcamera/internal/mapped_framebuffer.h" > > #include "algorithms/algorithm.h" > +#include "algorithms/agc.h" > #include "algorithms/awb.h" > #include "algorithms/contrast.h" > #include "algorithms/grid.h" > -#include "ipu3_agc.h" > #include "libipa/camera_sensor_helper.h" > > namespace libcamera { > @@ -84,8 +84,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_; > > @@ -167,6 +165,7 @@ int IPAIPU3::init(const IPASettings &settings, > /* Construct our Algorithms */ > algorithms_.emplace_back(new algorithms::Grid()); > /* Grid needs to be prepared before AWB */ > + algorithms_.emplace_back(new algorithms::Agc()); > algorithms_.emplace_back(new algorithms::Awb()); > algorithms_.emplace_back(new algorithms::Contrast()); Ultimately, I would expect this 'table' of algorithms to look like the diagram in (or at least match the dependencies in): https://www.kernel.org/doc/html/latest/admin-guide/media/ipu3.html?highlight=ipu3#overview-of-ipu3-pipeline I wonder if it's worth referencing that link here. Probably when we add the first algorithms_.emplace_back(). > > @@ -229,9 +228,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) > return ret; > } > > - agcAlgo_ = std::make_unique<IPU3Agc>(); > - agcAlgo_->configure(context_, configInfo); > - > return 0; > } > > @@ -320,17 +316,12 @@ void IPAIPU3::parseStatistics(unsigned int frame, > [[maybe_unused]] const ipu3_uapi_stats_3a *stats) > { > ControlList ctrls(controls::controls); At least a new line required here. And as commented before, this needs some explicit documentation/reasoning/comments as to why the IPU3 layer is setting agc specific parameters outside of the AGC module. Of course, if those are added in the earlier patch, they'll then move here with the code move. Are these lines even actually required? Are you expecting those values to have changed such that you need to reset them? > + context_.frameContext.agc.gain = camHelper_->gain(gain_); > + context_.frameContext.agc.exposure = exposure_; > + > for (auto const &algo : algorithms_) > algo->process(context_, stats); > > - double gain = camHelper_->gain(gain_); > - context_.frameContext.agc.exposure = exposure_; > - context_.frameContext.agc.gain = gain; > - agcAlgo_->process(context_, stats); > - exposure_ = context_.frameContext.agc.exposure; > - gain = context_.frameContext.agc.gain; > - gain_ = camHelper_->gainCode(gain); > - > setControls(frame); > > /* \todo Use VBlank value calculated from each frame exposure. */ > @@ -350,6 +341,9 @@ void IPAIPU3::setControls(unsigned int frame) > IPU3Action op; > op.op = ActionSetSensorControls; > > + exposure_ = context_.frameContext.agc.exposure; > + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); Perhaps these should be local variables, and the gain_ and exposure_ variables in the IPU3 class can be removed? There's plenty more we can clean up on the IPU3 now that the interfaces are more modular, but I think this is all going in the right direction. With all relevant comments addressed, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > 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 >
Hi Jean-Michel, Thank you for the patch. On Fri, Aug 13, 2021 at 11:27:01AM +0100, Kieran Bingham wrote: > On 12/08/2021 17:52, 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@ideasonboard.com> > > --- > > .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 18 +++++++-------- > > src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 ++++++++--------- > > src/ipa/ipu3/algorithms/meson.build | 1 + > > src/ipa/ipu3/ipu3.cpp | 22 +++++++------------ > > src/ipa/ipu3/meson.build | 1 - > > 5 files changed, 28 insertions(+), 34 deletions(-) > > rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (94%) > > 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 94% > > rename from src/ipa/ipu3/ipu3_agc.cpp > > rename to src/ipa/ipu3/algorithms/agc.cpp > > index 0d0584a8..ee3d3bc2 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,7 +50,7 @@ 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), > > @@ -58,7 +58,7 @@ IPU3Agc::IPU3Agc() > > { > > } > > > > -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > > +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > > { > > aeGrid_ = context.configuration.grid.bdsGrid; > > > > @@ -68,7 +68,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, > > @@ -109,7 +109,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) { > > @@ -143,7 +143,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 ? > > @@ -186,7 +186,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; > > @@ -195,6 +195,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 9ef2dd41..e75d92e0 100644 > > --- a/src/ipa/ipu3/algorithms/meson.build > > +++ b/src/ipa/ipu3/algorithms/meson.build > > @@ -1,6 +1,7 @@ > > # SPDX-License-Identifier: CC0-1.0 > > > > ipu3_ipa_algorithms = files([ > > + 'agc.cpp', > > 'awb.cpp', > > 'contrast.cpp', > > 'grid.cpp', > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index bea2a372..a99fbcac 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -30,10 +30,10 @@ > > #include "libcamera/internal/mapped_framebuffer.h" > > > > #include "algorithms/algorithm.h" > > +#include "algorithms/agc.h" > > #include "algorithms/awb.h" > > #include "algorithms/contrast.h" > > #include "algorithms/grid.h" > > -#include "ipu3_agc.h" > > #include "libipa/camera_sensor_helper.h" > > > > namespace libcamera { > > @@ -84,8 +84,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_; > > > > @@ -167,6 +165,7 @@ int IPAIPU3::init(const IPASettings &settings, > > /* Construct our Algorithms */ > > algorithms_.emplace_back(new algorithms::Grid()); > > /* Grid needs to be prepared before AWB */ > > + algorithms_.emplace_back(new algorithms::Agc()); > > algorithms_.emplace_back(new algorithms::Awb()); > > algorithms_.emplace_back(new algorithms::Contrast()); > > Ultimately, I would expect this 'table' of algorithms to look like the > diagram in (or at least match the dependencies in): > > https://www.kernel.org/doc/html/latest/admin-guide/media/ipu3.html?highlight=ipu3#overview-of-ipu3-pipeline > > I wonder if it's worth referencing that link here. > > Probably when we add the first algorithms_.emplace_back(). > > > > > @@ -229,9 +228,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) > > return ret; > > } > > > > - agcAlgo_ = std::make_unique<IPU3Agc>(); > > - agcAlgo_->configure(context_, configInfo); > > - > > return 0; > > } > > > > @@ -320,17 +316,12 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > [[maybe_unused]] const ipu3_uapi_stats_3a *stats) > > { > > ControlList ctrls(controls::controls); > > At least a new line required here. > > And as commented before, this needs some explicit > documentation/reasoning/comments as to why the IPU3 layer is setting agc > specific parameters outside of the AGC module. I'm also interested in understanding the rationale for this, and the next steps. > Of course, if those are added in the earlier patch, they'll then move > here with the code move. > > Are these lines even actually required? > Are you expecting those values to have changed such that you need to > reset them? > > > > + context_.frameContext.agc.gain = camHelper_->gain(gain_); > > + context_.frameContext.agc.exposure = exposure_; > > + If I understand correctly, this is related to providing historical data (as in the gain and exposure for the previous frame) to the AGC implementation. We'll need a more generic mechanism to access the history, and this can come in subsequent patches. Care should be taken, when designing that, to make it easy to use for algorithms. If an algorithm needs to access values it has computed for the last frame, that's fairly simple, it can always look at the last frame. Things get more complicated when an algorithm needs to access the latest values computed by another algorithm, as in that case where the latest value is stored will depend on the order in which algorithms are run. The implementation of the algorithms should as much as possible not depend on any particular order (any departure from this rule needs to be discussed), and they should also not need to manually look at values for the current frame and for the last frame to find the most recent one. We need either helper functions, or a data storage design that ensures that the latest value is always available from the same place. > > for (auto const &algo : algorithms_) > > algo->process(context_, stats); > > > > - double gain = camHelper_->gain(gain_); > > - context_.frameContext.agc.exposure = exposure_; > > - context_.frameContext.agc.gain = gain; > > - agcAlgo_->process(context_, stats); > > - exposure_ = context_.frameContext.agc.exposure; > > - gain = context_.frameContext.agc.gain; > > - gain_ = camHelper_->gainCode(gain); > > - > > setControls(frame); > > > > /* \todo Use VBlank value calculated from each frame exposure. */ > > @@ -350,6 +341,9 @@ void IPAIPU3::setControls(unsigned int frame) > > IPU3Action op; > > op.op = ActionSetSensorControls; > > > > + exposure_ = context_.frameContext.agc.exposure; > > + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); > > Perhaps these should be local variables, and the gain_ and exposure_ > variables in the IPU3 class can be removed? > > > There's plenty more we can clean up on the IPU3 now that the interfaces > are more modular, but I think this is all going in the right direction. > > With all relevant comments addressed, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > 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
On 16/08/2021 01:21, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Fri, Aug 13, 2021 at 11:27:01AM +0100, Kieran Bingham wrote: >> On 12/08/2021 17:52, 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@ideasonboard.com> >>> --- >>> .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 18 +++++++-------- >>> src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 ++++++++--------- >>> src/ipa/ipu3/algorithms/meson.build | 1 + >>> src/ipa/ipu3/ipu3.cpp | 22 +++++++------------ >>> src/ipa/ipu3/meson.build | 1 - >>> 5 files changed, 28 insertions(+), 34 deletions(-) >>> rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (94%) >>> 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 94% >>> rename from src/ipa/ipu3/ipu3_agc.cpp >>> rename to src/ipa/ipu3/algorithms/agc.cpp >>> index 0d0584a8..ee3d3bc2 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,7 +50,7 @@ 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), >>> @@ -58,7 +58,7 @@ IPU3Agc::IPU3Agc() >>> { >>> } >>> >>> -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >>> +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >>> { >>> aeGrid_ = context.configuration.grid.bdsGrid; >>> >>> @@ -68,7 +68,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, >>> @@ -109,7 +109,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) { >>> @@ -143,7 +143,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 ? >>> @@ -186,7 +186,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; >>> @@ -195,6 +195,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 9ef2dd41..e75d92e0 100644 >>> --- a/src/ipa/ipu3/algorithms/meson.build >>> +++ b/src/ipa/ipu3/algorithms/meson.build >>> @@ -1,6 +1,7 @@ >>> # SPDX-License-Identifier: CC0-1.0 >>> >>> ipu3_ipa_algorithms = files([ >>> + 'agc.cpp', >>> 'awb.cpp', >>> 'contrast.cpp', >>> 'grid.cpp', >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>> index bea2a372..a99fbcac 100644 >>> --- a/src/ipa/ipu3/ipu3.cpp >>> +++ b/src/ipa/ipu3/ipu3.cpp >>> @@ -30,10 +30,10 @@ >>> #include "libcamera/internal/mapped_framebuffer.h" >>> >>> #include "algorithms/algorithm.h" >>> +#include "algorithms/agc.h" >>> #include "algorithms/awb.h" >>> #include "algorithms/contrast.h" >>> #include "algorithms/grid.h" >>> -#include "ipu3_agc.h" >>> #include "libipa/camera_sensor_helper.h" >>> >>> namespace libcamera { >>> @@ -84,8 +84,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_; >>> >>> @@ -167,6 +165,7 @@ int IPAIPU3::init(const IPASettings &settings, >>> /* Construct our Algorithms */ >>> algorithms_.emplace_back(new algorithms::Grid()); >>> /* Grid needs to be prepared before AWB */ >>> + algorithms_.emplace_back(new algorithms::Agc()); >>> algorithms_.emplace_back(new algorithms::Awb()); >>> algorithms_.emplace_back(new algorithms::Contrast()); >> >> Ultimately, I would expect this 'table' of algorithms to look like the >> diagram in (or at least match the dependencies in): >> >> https://www.kernel.org/doc/html/latest/admin-guide/media/ipu3.html?highlight=ipu3#overview-of-ipu3-pipeline >> >> I wonder if it's worth referencing that link here. >> >> Probably when we add the first algorithms_.emplace_back(). >> >>> >>> @@ -229,9 +228,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) >>> return ret; >>> } >>> >>> - agcAlgo_ = std::make_unique<IPU3Agc>(); >>> - agcAlgo_->configure(context_, configInfo); >>> - >>> return 0; >>> } >>> >>> @@ -320,17 +316,12 @@ void IPAIPU3::parseStatistics(unsigned int frame, >>> [[maybe_unused]] const ipu3_uapi_stats_3a *stats) >>> { >>> ControlList ctrls(controls::controls); >> >> At least a new line required here. >> >> And as commented before, this needs some explicit >> documentation/reasoning/comments as to why the IPU3 layer is setting agc >> specific parameters outside of the AGC module. > > I'm also interested in understanding the rationale for this, and the > next steps. > >> Of course, if those are added in the earlier patch, they'll then move >> here with the code move. >> >> Are these lines even actually required? >> Are you expecting those values to have changed such that you need to >> reset them? >> >> >>> + context_.frameContext.agc.gain = camHelper_->gain(gain_); >>> + context_.frameContext.agc.exposure = exposure_; >>> + > > If I understand correctly, this is related to providing historical data > (as in the gain and exposure for the previous frame) to the AGC > implementation. We'll need a more generic mechanism to access the > history, and this can come in subsequent patches. Care should be taken, > when designing that, to make it easy to use for algorithms. If an > algorithm needs to access values it has computed for the last frame, > that's fairly simple, it can always look at the last frame. Things get > more complicated when an algorithm needs to access the latest values > computed by another algorithm, as in that case where the latest value is > stored will depend on the order in which algorithms are run. The > implementation of the algorithms should as much as possible not depend > on any particular order (any departure from this rule needs to be > discussed), and they should also not need to manually look at values for > the current frame and for the last frame to find the most recent one. We > need either helper functions, or a data storage design that ensures that > the latest value is always available from the same place. I foresee two types of data we might want from the history (perhaps). Either ... what was the last value set, that could also be stored privately to an algorithm. Or ... what was the value of the exposure that was set on the frame to which we are now parsing the statistics (which if we have a pipeline of 4 buffers, may be 4 runs ago, or such for example). So we either likely want 'the last value' or 'the frameContext that was associated with the parameter buffer that was used when queuing the Request/sequence number of which we are now processing the statistics for. I.e. that should be a map from either the sequence number or Request back to a (now read-only) FrameContext. But here, currently we have only one frameContext, and we are copying the gain and exposure out of it - into the gain_ and exposure_, simply to copy it back in again ... But that's still essentially what is already happening anyway, so this is still part of the move to modularity, and any topics about dealing with historical data, or reworking how we set and adapt gain and exposure can be done on top IMO. -- Kieran > >>> for (auto const &algo : algorithms_) >>> algo->process(context_, stats); >>> >>> - double gain = camHelper_->gain(gain_); >>> - context_.frameContext.agc.exposure = exposure_; >>> - context_.frameContext.agc.gain = gain; >>> - agcAlgo_->process(context_, stats); >>> - exposure_ = context_.frameContext.agc.exposure; >>> - gain = context_.frameContext.agc.gain; >>> - gain_ = camHelper_->gainCode(gain); >>> - >>> setControls(frame); >>> >>> /* \todo Use VBlank value calculated from each frame exposure. */ >>> @@ -350,6 +341,9 @@ void IPAIPU3::setControls(unsigned int frame) >>> IPU3Action op; >>> op.op = ActionSetSensorControls; >>> >>> + exposure_ = context_.frameContext.agc.exposure; >>> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); >> >> Perhaps these should be local variables, and the gain_ and exposure_ >> variables in the IPU3 class can be removed? >> >> >> There's plenty more we can clean up on the IPU3 now that the interfaces >> are more modular, but I think this is all going in the right direction. >> >> With all relevant comments addressed, >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> + >>> 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 >
Hi Kieran, On Mon, Aug 16, 2021 at 03:54:32PM +0100, Kieran Bingham wrote: > On 16/08/2021 01:21, Laurent Pinchart wrote: > > On Fri, Aug 13, 2021 at 11:27:01AM +0100, Kieran Bingham wrote: > >> On 12/08/2021 17:52, 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@ideasonboard.com> > >>> --- > >>> .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 18 +++++++-------- > >>> src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 ++++++++--------- > >>> src/ipa/ipu3/algorithms/meson.build | 1 + > >>> src/ipa/ipu3/ipu3.cpp | 22 +++++++------------ > >>> src/ipa/ipu3/meson.build | 1 - > >>> 5 files changed, 28 insertions(+), 34 deletions(-) > >>> rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (94%) > >>> 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 94% > >>> rename from src/ipa/ipu3/ipu3_agc.cpp > >>> rename to src/ipa/ipu3/algorithms/agc.cpp > >>> index 0d0584a8..ee3d3bc2 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,7 +50,7 @@ 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), > >>> @@ -58,7 +58,7 @@ IPU3Agc::IPU3Agc() > >>> { > >>> } > >>> > >>> -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >>> +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >>> { > >>> aeGrid_ = context.configuration.grid.bdsGrid; > >>> > >>> @@ -68,7 +68,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, > >>> @@ -109,7 +109,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) { > >>> @@ -143,7 +143,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 ? > >>> @@ -186,7 +186,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; > >>> @@ -195,6 +195,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 9ef2dd41..e75d92e0 100644 > >>> --- a/src/ipa/ipu3/algorithms/meson.build > >>> +++ b/src/ipa/ipu3/algorithms/meson.build > >>> @@ -1,6 +1,7 @@ > >>> # SPDX-License-Identifier: CC0-1.0 > >>> > >>> ipu3_ipa_algorithms = files([ > >>> + 'agc.cpp', > >>> 'awb.cpp', > >>> 'contrast.cpp', > >>> 'grid.cpp', > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >>> index bea2a372..a99fbcac 100644 > >>> --- a/src/ipa/ipu3/ipu3.cpp > >>> +++ b/src/ipa/ipu3/ipu3.cpp > >>> @@ -30,10 +30,10 @@ > >>> #include "libcamera/internal/mapped_framebuffer.h" > >>> > >>> #include "algorithms/algorithm.h" > >>> +#include "algorithms/agc.h" > >>> #include "algorithms/awb.h" > >>> #include "algorithms/contrast.h" > >>> #include "algorithms/grid.h" > >>> -#include "ipu3_agc.h" > >>> #include "libipa/camera_sensor_helper.h" > >>> > >>> namespace libcamera { > >>> @@ -84,8 +84,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_; > >>> > >>> @@ -167,6 +165,7 @@ int IPAIPU3::init(const IPASettings &settings, > >>> /* Construct our Algorithms */ > >>> algorithms_.emplace_back(new algorithms::Grid()); > >>> /* Grid needs to be prepared before AWB */ > >>> + algorithms_.emplace_back(new algorithms::Agc()); > >>> algorithms_.emplace_back(new algorithms::Awb()); > >>> algorithms_.emplace_back(new algorithms::Contrast()); > >> > >> Ultimately, I would expect this 'table' of algorithms to look like the > >> diagram in (or at least match the dependencies in): > >> > >> https://www.kernel.org/doc/html/latest/admin-guide/media/ipu3.html?highlight=ipu3#overview-of-ipu3-pipeline > >> > >> I wonder if it's worth referencing that link here. > >> > >> Probably when we add the first algorithms_.emplace_back(). > >> > >>> > >>> @@ -229,9 +228,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) > >>> return ret; > >>> } > >>> > >>> - agcAlgo_ = std::make_unique<IPU3Agc>(); > >>> - agcAlgo_->configure(context_, configInfo); > >>> - > >>> return 0; > >>> } > >>> > >>> @@ -320,17 +316,12 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >>> [[maybe_unused]] const ipu3_uapi_stats_3a *stats) > >>> { > >>> ControlList ctrls(controls::controls); > >> > >> At least a new line required here. > >> > >> And as commented before, this needs some explicit > >> documentation/reasoning/comments as to why the IPU3 layer is setting agc > >> specific parameters outside of the AGC module. > > > > I'm also interested in understanding the rationale for this, and the > > next steps. > > > >> Of course, if those are added in the earlier patch, they'll then move > >> here with the code move. > >> > >> Are these lines even actually required? > >> Are you expecting those values to have changed such that you need to > >> reset them? > >> > >>> + context_.frameContext.agc.gain = camHelper_->gain(gain_); > >>> + context_.frameContext.agc.exposure = exposure_; > >>> + > > > > If I understand correctly, this is related to providing historical data > > (as in the gain and exposure for the previous frame) to the AGC > > implementation. We'll need a more generic mechanism to access the > > history, and this can come in subsequent patches. Care should be taken, > > when designing that, to make it easy to use for algorithms. If an > > algorithm needs to access values it has computed for the last frame, > > that's fairly simple, it can always look at the last frame. Things get > > more complicated when an algorithm needs to access the latest values > > computed by another algorithm, as in that case where the latest value is > > stored will depend on the order in which algorithms are run. The > > implementation of the algorithms should as much as possible not depend > > on any particular order (any departure from this rule needs to be > > discussed), and they should also not need to manually look at values for > > the current frame and for the last frame to find the most recent one. We > > need either helper functions, or a data storage design that ensures that > > the latest value is always available from the same place. > > I foresee two types of data we might want from the history (perhaps). > Either ... what was the last value set, that could also be stored > privately to an algorithm. That only works if the data is consumed by the algorithm that produces it though. That's probably the majority of cases, but doesn't cover them all (for instance the colour temperature computed by the AWB algorithm could be consumed by the lens shading correction algorithm). > Or ... what was the value of the exposure that was set on the frame to > which we are now parsing the statistics (which if we have a pipeline of > 4 buffers, may be 4 runs ago, or such for example). > > So we either likely want 'the last value' or 'the frameContext that was > associated with the parameter buffer that was used when queuing the > Request/sequence number of which we are now processing the statistics for. > > I.e. that should be a map from either the sequence number or Request > back to a (now read-only) FrameContext. > > > But here, currently we have only one frameContext, and we are copying > the gain and exposure out of it - into the gain_ and exposure_, simply > to copy it back in again ... > > But that's still essentially what is already happening anyway, so this > is still part of the move to modularity, and any topics about dealing > with historical data, or reworking how we set and adapt gain and > exposure can be done on top IMO. Sure, there's no disagreement on that. Maybe I haven't expressed myself correctly, I raised these points to start discussions on the topic, not to request them to be addressed as part of this series. > >>> for (auto const &algo : algorithms_) > >>> algo->process(context_, stats); > >>> > >>> - double gain = camHelper_->gain(gain_); > >>> - context_.frameContext.agc.exposure = exposure_; > >>> - context_.frameContext.agc.gain = gain; > >>> - agcAlgo_->process(context_, stats); > >>> - exposure_ = context_.frameContext.agc.exposure; > >>> - gain = context_.frameContext.agc.gain; > >>> - gain_ = camHelper_->gainCode(gain); > >>> - > >>> setControls(frame); > >>> > >>> /* \todo Use VBlank value calculated from each frame exposure. */ > >>> @@ -350,6 +341,9 @@ void IPAIPU3::setControls(unsigned int frame) > >>> IPU3Action op; > >>> op.op = ActionSetSensorControls; > >>> > >>> + exposure_ = context_.frameContext.agc.exposure; > >>> + gain_ = camHelper_->gainCode(context_.frameContext.agc.gain); > >> > >> Perhaps these should be local variables, and the gain_ and exposure_ > >> variables in the IPU3 class can be removed? > >> > >> > >> There's plenty more we can clean up on the IPU3 now that the interfaces > >> are more modular, but I think this is all going in the right direction. > >> > >> With all relevant comments addressed, > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> + > >>> 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
diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp similarity index 94% rename from src/ipa/ipu3/ipu3_agc.cpp rename to src/ipa/ipu3/algorithms/agc.cpp index 0d0584a8..ee3d3bc2 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,7 +50,7 @@ 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), @@ -58,7 +58,7 @@ IPU3Agc::IPU3Agc() { } -int IPU3Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) +int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) { aeGrid_ = context.configuration.grid.bdsGrid; @@ -68,7 +68,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, @@ -109,7 +109,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) { @@ -143,7 +143,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 ? @@ -186,7 +186,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; @@ -195,6 +195,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 9ef2dd41..e75d92e0 100644 --- a/src/ipa/ipu3/algorithms/meson.build +++ b/src/ipa/ipu3/algorithms/meson.build @@ -1,6 +1,7 @@ # SPDX-License-Identifier: CC0-1.0 ipu3_ipa_algorithms = files([ + 'agc.cpp', 'awb.cpp', 'contrast.cpp', 'grid.cpp', diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index bea2a372..a99fbcac 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -30,10 +30,10 @@ #include "libcamera/internal/mapped_framebuffer.h" #include "algorithms/algorithm.h" +#include "algorithms/agc.h" #include "algorithms/awb.h" #include "algorithms/contrast.h" #include "algorithms/grid.h" -#include "ipu3_agc.h" #include "libipa/camera_sensor_helper.h" namespace libcamera { @@ -84,8 +84,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_; @@ -167,6 +165,7 @@ int IPAIPU3::init(const IPASettings &settings, /* Construct our Algorithms */ algorithms_.emplace_back(new algorithms::Grid()); /* Grid needs to be prepared before AWB */ + algorithms_.emplace_back(new algorithms::Agc()); algorithms_.emplace_back(new algorithms::Awb()); algorithms_.emplace_back(new algorithms::Contrast()); @@ -229,9 +228,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo) return ret; } - agcAlgo_ = std::make_unique<IPU3Agc>(); - agcAlgo_->configure(context_, configInfo); - return 0; } @@ -320,17 +316,12 @@ void IPAIPU3::parseStatistics(unsigned int frame, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) { ControlList ctrls(controls::controls); + context_.frameContext.agc.gain = camHelper_->gain(gain_); + context_.frameContext.agc.exposure = exposure_; + for (auto const &algo : algorithms_) algo->process(context_, stats); - double gain = camHelper_->gain(gain_); - context_.frameContext.agc.exposure = exposure_; - context_.frameContext.agc.gain = gain; - agcAlgo_->process(context_, stats); - exposure_ = context_.frameContext.agc.exposure; - gain = context_.frameContext.agc.gain; - gain_ = camHelper_->gainCode(gain); - setControls(frame); /* \todo Use VBlank value calculated from each frame exposure. */ @@ -350,6 +341,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
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@ideasonboard.com> --- .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 18 +++++++-------- src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 20 ++++++++--------- src/ipa/ipu3/algorithms/meson.build | 1 + src/ipa/ipu3/ipu3.cpp | 22 +++++++------------ src/ipa/ipu3/meson.build | 1 - 5 files changed, 28 insertions(+), 34 deletions(-) rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (94%) rename src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} (73%)