| Message ID | 20220927023642.12341-21-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Sep 27, 2022 at 05:36:29AM +0300, Laurent Pinchart via libcamera-devel wrote: > Rework the algorithm's usage of the active state to store the value of > controls for the last queued request in the queueRequest() function, and > store a copy of the values in the corresponding frame context. > > The frame context is used in the prepare() function to populate the ISP > parameters with values corresponding to the right frame. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changes since v4: > > - Add todo comments related to sensor controls Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++++++------ > src/ipa/rkisp1/algorithms/agc.h | 3 ++- > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++----------- > src/ipa/rkisp1/ipa_context.h | 14 ++++++---- > src/ipa/rkisp1/rkisp1.cpp | 14 +++++++--- > 5 files changed, 68 insertions(+), 33 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 87fc5d1ffec7..4540bde66db4 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) > /** > * \brief Estimate the new exposure and gain values > * \param[inout] context The shared IPA Context > + * \param[in] frameContext The FrameContext for this frame > * \param[in] yGain The gain calculated on the current brightness level > * \param[in] iqMeanGain The gain calculated based on the relative luminance target > */ > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain) > +void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext, > + double yGain, double iqMeanGain) > { > IPASessionConfiguration &configuration = context.configuration; > IPAActiveState &activeState = context.activeState; > > /* Get the effective exposure and gain applied on the sensor. */ > - uint32_t exposure = activeState.sensor.exposure; > - double analogueGain = activeState.sensor.gain; > + uint32_t exposure = frameContext.sensor.exposure; > + double analogueGain = frameContext.sensor.gain; > > /* Use the highest of the two gain estimates. */ > double evGain = std::max(yGain, iqMeanGain); > @@ -286,9 +288,16 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const > * new exposure and gain for the scene. > */ > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > - [[maybe_unused]] IPAFrameContext &frameContext, > - const rkisp1_stat_buffer *stats) > + IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats) > { > + /* > + * \todo Verify that the exposure and gain applied by the sensor for > + * this frame match what has been requested. This isn't a hard > + * requirement for stability of the AGC (the guarantee we need in > + * automatic mode is a perfect match between the frame and the values > + * we receive), but is important in manual mode. > + */ > + > const rkisp1_cif_isp_stat *params = &stats->params; > ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP); > > @@ -320,7 +329,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > break; > } > > - computeExposure(context, yGain, iqMeanGain); > + computeExposure(context, frameContext, yGain, iqMeanGain); > frameCount_++; > } > > @@ -328,9 +337,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > * \copydoc libcamera::ipa::Algorithm::prepare > */ > void Agc::prepare(IPAContext &context, const uint32_t frame, > - [[maybe_unused]] IPAFrameContext &frameContext, > - rkisp1_params_cfg *params) > + IPAFrameContext &frameContext, rkisp1_params_cfg *params) > { > + frameContext.agc.exposure = context.activeState.agc.exposure; > + frameContext.agc.gain = context.activeState.agc.gain; > + > if (frame > 0) > return; > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index f115ba2ed85c..9ad5c32fd6f6 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -34,7 +34,8 @@ public: > const rkisp1_stat_buffer *stats) override; > > private: > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain); > + void computeExposure(IPAContext &Context, IPAFrameContext &frameContext, > + double yGain, double iqMeanGain); > utils::Duration filterExposure(utils::Duration exposureValue); > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain); > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const; > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 78a785f5f982..c7d5b1b6ec5a 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 { > * \var IPAActiveState::agc > * \brief State for the Automatic Gain Control algorithm > * > - * The exposure and gain determined are expected to be applied to the sensor > - * at the earliest opportunity. > + * The exposure and gain are the latest values computed by the AGC algorithm. > * > * \var IPAActiveState::agc.exposure > * \brief Exposure time expressed as a number of lines > * > * \var IPAActiveState::agc.gain > * \brief Analogue gain multiplier > - * > - * The gain should be adapted to the sensor specific gain code before applying. > */ > > /** > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 { > * \brief Indicates if ISP parameters need to be updated > */ > > -/** > - * \var IPAActiveState::sensor > - * \brief Effective sensor values > - * > - * \var IPAActiveState::sensor.exposure > - * \brief Exposure time expressed as a number of lines > - * > - * \var IPAActiveState::sensor.gain > - * \brief Analogue gain multiplier > - */ > - > /** > * \struct IPAFrameContext > * \brief Per-frame context for algorithms > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 { > * \todo Populate the frame context for all algorithms > */ > > +/** > + * \var IPAFrameContext::agc > + * \brief Automatic Gain Control parameters for this frame > + * > + * The exposure and gain are provided by the AGC algorithm, and are to be > + * applied to the sensor in order to take effect for this frame. > + * > + * \var IPAFrameContext::agc.exposure > + * \brief Exposure time expressed as a number of lines > + * > + * \var IPAFrameContext::agc.gain > + * \brief Analogue gain multiplier > + * > + * The gain should be adapted to the sensor specific gain code before applying. > + */ > + > +/** > + * \var IPAFrameContext::sensor > + * \brief Sensor configuration that used been used for this frame > + * > + * \var IPAFrameContext::sensor.exposure > + * \brief Exposure time expressed as a number of lines > + * > + * \var IPAFrameContext::sensor.gain > + * \brief Analogue gain multiplier > + */ > + > /** > * \struct IPAContext > * \brief Global IPA context data shared between all algorithms > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index c7041ea2a214..a4d134e700b5 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -83,14 +83,18 @@ struct IPAActiveState { > uint8_t sharpness; > bool updateParams; > } filter; > - > - struct { > - uint32_t exposure; > - double gain; > - } sensor; > }; > > struct IPAFrameContext : public FrameContext { > + struct { > + uint32_t exposure; > + double gain; > + } agc; > + > + struct { > + uint32_t exposure; > + double gain; > + } sensor; > }; > > static constexpr uint32_t kMaxFrameContexts = 16; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 5409c2d5219e..6297916cf86d 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -332,9 +332,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > reinterpret_cast<rkisp1_stat_buffer *>( > mappedBuffers_.at(bufferId).planes()[0].data()); > > - context_.activeState.sensor.exposure = > + frameContext.sensor.exposure = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > - context_.activeState.sensor.gain = > + frameContext.sensor.gain = > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > unsigned int aeState = 0; > @@ -349,8 +349,14 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > void IPARkISP1::setControls(unsigned int frame) > { > - uint32_t exposure = context_.activeState.agc.exposure; > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain); > + /* > + * \todo The frame number is most likely wrong here, we need to take > + * internal sensor delays and other timing parameters into account. > + */ > + > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > + uint32_t exposure = frameContext.agc.exposure; > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain); > > ControlList ctrls(ctrls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 87fc5d1ffec7..4540bde66db4 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) /** * \brief Estimate the new exposure and gain values * \param[inout] context The shared IPA Context + * \param[in] frameContext The FrameContext for this frame * \param[in] yGain The gain calculated on the current brightness level * \param[in] iqMeanGain The gain calculated based on the relative luminance target */ -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain) +void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext, + double yGain, double iqMeanGain) { IPASessionConfiguration &configuration = context.configuration; IPAActiveState &activeState = context.activeState; /* Get the effective exposure and gain applied on the sensor. */ - uint32_t exposure = activeState.sensor.exposure; - double analogueGain = activeState.sensor.gain; + uint32_t exposure = frameContext.sensor.exposure; + double analogueGain = frameContext.sensor.gain; /* Use the highest of the two gain estimates. */ double evGain = std::max(yGain, iqMeanGain); @@ -286,9 +288,16 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const * new exposure and gain for the scene. */ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, - [[maybe_unused]] IPAFrameContext &frameContext, - const rkisp1_stat_buffer *stats) + IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats) { + /* + * \todo Verify that the exposure and gain applied by the sensor for + * this frame match what has been requested. This isn't a hard + * requirement for stability of the AGC (the guarantee we need in + * automatic mode is a perfect match between the frame and the values + * we receive), but is important in manual mode. + */ + const rkisp1_cif_isp_stat *params = &stats->params; ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP); @@ -320,7 +329,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, break; } - computeExposure(context, yGain, iqMeanGain); + computeExposure(context, frameContext, yGain, iqMeanGain); frameCount_++; } @@ -328,9 +337,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, * \copydoc libcamera::ipa::Algorithm::prepare */ void Agc::prepare(IPAContext &context, const uint32_t frame, - [[maybe_unused]] IPAFrameContext &frameContext, - rkisp1_params_cfg *params) + IPAFrameContext &frameContext, rkisp1_params_cfg *params) { + frameContext.agc.exposure = context.activeState.agc.exposure; + frameContext.agc.gain = context.activeState.agc.gain; + if (frame > 0) return; diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index f115ba2ed85c..9ad5c32fd6f6 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -34,7 +34,8 @@ public: const rkisp1_stat_buffer *stats) override; private: - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain); + void computeExposure(IPAContext &Context, IPAFrameContext &frameContext, + double yGain, double iqMeanGain); utils::Duration filterExposure(utils::Duration exposureValue); double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain); double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const; diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp index 78a785f5f982..c7d5b1b6ec5a 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 { * \var IPAActiveState::agc * \brief State for the Automatic Gain Control algorithm * - * The exposure and gain determined are expected to be applied to the sensor - * at the earliest opportunity. + * The exposure and gain are the latest values computed by the AGC algorithm. * * \var IPAActiveState::agc.exposure * \brief Exposure time expressed as a number of lines * * \var IPAActiveState::agc.gain * \brief Analogue gain multiplier - * - * The gain should be adapted to the sensor specific gain code before applying. */ /** @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 { * \brief Indicates if ISP parameters need to be updated */ -/** - * \var IPAActiveState::sensor - * \brief Effective sensor values - * - * \var IPAActiveState::sensor.exposure - * \brief Exposure time expressed as a number of lines - * - * \var IPAActiveState::sensor.gain - * \brief Analogue gain multiplier - */ - /** * \struct IPAFrameContext * \brief Per-frame context for algorithms @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 { * \todo Populate the frame context for all algorithms */ +/** + * \var IPAFrameContext::agc + * \brief Automatic Gain Control parameters for this frame + * + * The exposure and gain are provided by the AGC algorithm, and are to be + * applied to the sensor in order to take effect for this frame. + * + * \var IPAFrameContext::agc.exposure + * \brief Exposure time expressed as a number of lines + * + * \var IPAFrameContext::agc.gain + * \brief Analogue gain multiplier + * + * The gain should be adapted to the sensor specific gain code before applying. + */ + +/** + * \var IPAFrameContext::sensor + * \brief Sensor configuration that used been used for this frame + * + * \var IPAFrameContext::sensor.exposure + * \brief Exposure time expressed as a number of lines + * + * \var IPAFrameContext::sensor.gain + * \brief Analogue gain multiplier + */ + /** * \struct IPAContext * \brief Global IPA context data shared between all algorithms diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index c7041ea2a214..a4d134e700b5 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -83,14 +83,18 @@ struct IPAActiveState { uint8_t sharpness; bool updateParams; } filter; - - struct { - uint32_t exposure; - double gain; - } sensor; }; struct IPAFrameContext : public FrameContext { + struct { + uint32_t exposure; + double gain; + } agc; + + struct { + uint32_t exposure; + double gain; + } sensor; }; static constexpr uint32_t kMaxFrameContexts = 16; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 5409c2d5219e..6297916cf86d 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -332,9 +332,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId reinterpret_cast<rkisp1_stat_buffer *>( mappedBuffers_.at(bufferId).planes()[0].data()); - context_.activeState.sensor.exposure = + frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); - context_.activeState.sensor.gain = + frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); unsigned int aeState = 0; @@ -349,8 +349,14 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId void IPARkISP1::setControls(unsigned int frame) { - uint32_t exposure = context_.activeState.agc.exposure; - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain); + /* + * \todo The frame number is most likely wrong here, we need to take + * internal sensor delays and other timing parameters into account. + */ + + IPAFrameContext &frameContext = context_.frameContexts.get(frame); + uint32_t exposure = frameContext.agc.exposure; + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain); ControlList ctrls(ctrls_); ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));