[libcamera-devel] [PATCH 3/3] ipa: ipu3: Add a IPAFrameContext queue
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Apr 25 12:39:49 CEST 2022
Quoting Umang Jain (2022-04-25 10:08:56)
> Hi Kieran,
>
> On 4/21/22 16:11, Kieran Bingham wrote:
> > Hi Umang,
> >
> > Quoting Umang Jain via libcamera-devel (2022-03-10 20:51:30)
> >> Having a single IPAFrameContext queue is limiting especially when
> >> we need to preserve per-frame controls coming in through
> >> libcamera::Request. Right now, we are not processing any controls
> >> on the IPA side (processControls()) but sooner or later we need to
> >> preserve the controls setting for the frames in the context in a
> >> retrievable fashion. Hence a std::deque is introduced to preserve
> >> the frame context of the incoming request's settings as soon as it is
> >> queued.
> >>
> >> Since IPAIPU3::processControls() is executed on
> >> IPU3CameraData::queuePendingRequests() code path, we need to store the
> >> incoming control setting in a separate IPAFrameContext and push that
> >> into the queue. The IPAFrameContext is then dropped when processing for
> >> that frame has been finished.
> > Great, having an identifiable (by frame number) context is going to make
> > things a lot easier I think.
> >
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> src/ipa/ipu3/algorithms/agc.cpp | 16 ++++-----
> >> src/ipa/ipu3/algorithms/agc.h | 4 +--
> >> src/ipa/ipu3/algorithms/awb.cpp | 17 +++++-----
> >> src/ipa/ipu3/algorithms/tone_mapping.cpp | 14 ++++----
> >> src/ipa/ipu3/ipa_context.cpp | 42 ++++++++++++++++++++++++
> >> src/ipa/ipu3/ipa_context.h | 12 +++++++
> >> src/ipa/ipu3/ipu3.cpp | 41 +++++++++++++++++++----
> >> 7 files changed, 115 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index cdc1327a..b24744f2 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -87,7 +87,7 @@ int Agc::configure(IPAContext &context,
> >> [[maybe_unused]] const IPAConfigInfo &configInfo)
> >> {
> >> IPASessionConfiguration &configuration = context.configuration;
> >> - IPAFrameContext &frameContext = context.frameContext;
> >> + IPAFrameContext &frameContext = context.getFrameContext(0);
> > I 'almost' want to call this context.frameContext(0); as an implicit
> > getter. But the 'get' does make it explicit.
>
>
> Yeah, I like being explicit as in - get frame context of <frame number>
>
> >>
> >> stride_ = configuration.grid.stride;
> >>
> >> @@ -182,14 +182,14 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
> >> * \param[in] yGain The gain calculated based on the relative luminance target
> >> * \param[in] iqMeanGain The gain calculated based on the relative luminance target
> >> */
> >> -void Agc::computeExposure(IPAContext &context, double yGain,
> >> - double iqMeanGain)
> >> +void Agc::computeExposure(const uint32_t frame, IPAContext &context,
> >> + double yGain, double iqMeanGain)
> >> {
> >> const IPASessionConfiguration &configuration = context.configuration;
> >> - IPAFrameContext &frameContext = context.frameContext;
> >> /* Get the effective exposure and gain applied on the sensor. */
> >> - uint32_t exposure = frameContext.sensor.exposure;
> >> - double analogueGain = frameContext.sensor.gain;
> >> + uint32_t exposure = context.prevFrameContext.sensor.exposure;
> >> + double analogueGain = context.prevFrameContext.sensor.gain;
> >> + IPAFrameContext &frameContext = context.getFrameContext(frame);
> > I would have almost expected to see
> > IPAFrameContext &previous = context.getFrameContext(frame - 1);
>
>
> Why?
>
> As far as I understand, the frameContext here is the current frame's
> context and we want to compute the exposure of current...
I think I was referring to keeping analogueGain and exposure as mapped
from within the 'previous' context.
But I don't think getFrameContext( n - 1 ) is possible/correct.
Perhaps all I really mean here was to make sure that exposure and
analogueGain here are referred to as
'previous.exposure'
and
'previous.analogueGain' ?
In fact *no*
exposure and analogueGain referenced here should be the values that were
*set* at the time this frame was captured.
So these shouldn't reference 'previous' at all, they should be about the
*current* frame. (in this perspective). It will be the 'oldest'
FrameContext in the queue ... because once we finish processing this
frame with the algorithms, we will update the associated request
metadata, and return that frame to the application as completed.
(We might have to ensure that the exposure and analogueGain are
correctly stored in this context still though, either at the point the
frame starts, or at the point that it completes.)
>
> >
> > But we might have occasions where that wasn't processed or not
> > available, so I guess having the most recent parameters referenced
> > directly probably makes sense, and also reduces having to look them up
> > each algorithm...
> >
> > That makes me think a name such as 'mostRecent' might be more
> > informative? I'm not sure... (There are many previous frames, but only
> > one mostRecent...?)
> >
> >
> > I was also going to argue/wonder why it wasn't a pointer to the existing
> > queue entry - but I suspect we might be freeing it when the frame is
> > completed, so a copy or move is likely easiest ... maybe that's an
> > optimisation later, and depends on how big it is if we are copying the
> > data.
>
>
> Yeah, optimization. I, for now, work with frame context's reference
> throughout.
>
> >
> >>
> >> /* Use the highest of the two gain estimates. */
> >> double evGain = std::max(yGain, iqMeanGain);
> >> @@ -344,7 +344,7 @@ void Agc::process(const uint32_t frame, IPAContext &context, const ipu3_uapi_sta
> >> double yTarget = kRelativeLuminanceTarget;
> >>
> >> for (unsigned int i = 0; i < 8; i++) {
> >> - double yValue = estimateLuminance(context.frameContext,
> >> + double yValue = estimateLuminance(context.prevFrameContext,
Likewise, here we are not calculating the lumincance of the previous
frame, it's the current (completed) frame ...
> >> context.configuration.grid.bdsGrid,
> >> stats, yGain);
> >> double extraGain = std::min(10.0, yTarget / (yValue + .001));
> >> @@ -357,7 +357,7 @@ void Agc::process(const uint32_t frame, IPAContext &context, const ipu3_uapi_sta
> >> break;
> >> }
> >>
> >> - computeExposure(context, yGain, iqMeanGain);
> >> + computeExposure(frame, context, yGain, iqMeanGain);
> >> frameCount_++;
> >> }
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> >> index e48506e5..f35de05f 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.h
> >> +++ b/src/ipa/ipu3/algorithms/agc.h
> >> @@ -34,8 +34,8 @@ private:
> >> double measureBrightness(const ipu3_uapi_stats_3a *stats,
> >> const ipu3_uapi_grid_config &grid) const;
> >> utils::Duration filterExposure(utils::Duration currentExposure);
> >> - void computeExposure(IPAContext &context, double yGain,
> >> - double iqMeanGain);
> >> + void computeExposure(const uint32_t frame, IPAContext &context,
> >> + double yGain, double iqMeanGain);
> >> double estimateLuminance(IPAFrameContext &frameContext,
> >> const ipu3_uapi_grid_config &grid,
> >> const ipu3_uapi_stats_3a *stats,
> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> >> index f1e753e6..00ac2984 100644
> >> --- a/src/ipa/ipu3/algorithms/awb.cpp
> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> >> @@ -390,16 +390,17 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> >> void Awb::process(const uint32_t frame, IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >> {
> >> calculateWBGains(stats);
> >> + IPAFrameContext &frameContext = context.getFrameContext(frame);
> > This code's reason enough here to keep the explicit 'get' in
> > getFrameContext for me I think.
> >
> >
> >> /*
> >> * Gains are only recalculated if enough zones were detected.
> >> * The results are cached, so if no results were calculated, we set the
> >> * cached values from asyncResults_ here.
> >> */
> >> - context.frameContext.awb.gains.blue = asyncResults_.blueGain;
> >> - context.frameContext.awb.gains.green = asyncResults_.greenGain;
> >> - context.frameContext.awb.gains.red = asyncResults_.redGain;
> >> - context.frameContext.awb.temperatureK = asyncResults_.temperatureK;
> >> + frameContext.awb.gains.blue = asyncResults_.blueGain;
> >> + frameContext.awb.gains.green = asyncResults_.greenGain;
> >> + frameContext.awb.gains.red = asyncResults_.redGain;
> >> + frameContext.awb.temperatureK = asyncResults_.temperatureK;
> >> }
> >>
> >> constexpr uint16_t Awb::threshold(float value)
> >> @@ -450,10 +451,10 @@ void Awb::prepare([[maybe_unused]] const uint32_t frame, IPAContext &context, ip
> >> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset
> >> * params->acc_param.bnr.opt_center.y_reset;
> >> /* Convert to u3.13 fixed point values */
> >> - params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;
> >> - params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red;
> >> - params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue;
> >> - params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;
> >> + params->acc_param.bnr.wb_gains.gr = 8192 * context.prevFrameContext.awb.gains.green;
> >> + params->acc_param.bnr.wb_gains.r = 8192 * context.prevFrameContext.awb.gains.red;
> >> + params->acc_param.bnr.wb_gains.b = 8192 * context.prevFrameContext.awb.gains.blue;
> >> + params->acc_param.bnr.wb_gains.gb = 8192 * context.prevFrameContext.awb.gains.green;
The 'prev' (/previous) naming here is frustrating me a little.
The sequence diagram we have now nicely shows that we do:
1. init()
2. configure
3. queueRequest(0)
4. start()
5. cio2BufferReady(0) (frame 0 is fully captured)
6. fillParamsBuffer(0) (Calls prepare, configures the ISP)
7. processStatsBuffer(0) (calls process, determines results, and
settings for frame (1))
The calculations done at 7 are a combination of identifying metadata for
the current frame (0) to return in the metadata of the completing
request, but also calculating the parameters that should be set for the
'next' frame(1) during it's call to fillParamsBuffer at 6.
Somehow, I wonder if we should make the lifetime of that more clear - so
that we are correctly filling in a 'next' structure in the context,
which is what the prepare calls fill from (rather than calling it
previous?)
> >>
> >> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> >> index bba5bc9a..f0fbaaf7 100644
> >> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> >> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> >> @@ -42,7 +42,7 @@ int ToneMapping::configure(IPAContext &context,
> >> [[maybe_unused]] const IPAConfigInfo &configInfo)
> >> {
> >> /* Initialise tone mapping gamma value. */
> >> - context.frameContext.toneMapping.gamma = 0.0;
> >> + context.frameContextQueue.front().toneMapping.gamma = 0.0;
> > In another call you used getFrameContext(0). Which one is more
> > appropriate? Will it always be the same requirement? (to get the first,
> > or 0th?)
> >
> > Does the front of the queue represent frame 0, or frame 10 if there are
> > ten frames on the queue?
>
>
> It should be the 0th. FrameContexts queue should be cleared up once
> stream has stopped.
Yes, I think the 'zero' indexed frame context in the queue is the right
one to use as it's the 'oldest'
>
> >
> > Perhaps a helper wrapper around the queue would let us declare methods
> > that are more descriptive? (Though I don't know which names are helpful
> > yet, so maybe it's overkill)
> >
> > frameContextQueue.first() // Oldest?
> > frameContextQueue.last() // Newest?
> > frameContextQueue.mostRecent()
> > frameContextQueue.mostRecentWithFilledStatistics() ?
> > frameContextQueue.get(n)
> > frameContextQueue.start(n)
> > frameContextQueue.finish(n)
> >
> > (Or such, if they are useful)
>
>
> I'll look into it.
>
> >
> >> return 0;
> >> }
> >> @@ -62,7 +62,7 @@ void ToneMapping::prepare([[maybe_unused]] const uint32_t frame,
> >> {
> >> /* Copy the calculated LUT into the parameters buffer. */
> >> memcpy(params->acc_param.gamma.gc_lut.lut,
> >> - context.frameContext.toneMapping.gammaCorrection.lut,
> >> + context.prevFrameContext.toneMapping.gammaCorrection.lut,
> >> IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
> >> sizeof(params->acc_param.gamma.gc_lut.lut[0]));
> >>
> >> @@ -73,7 +73,7 @@ void ToneMapping::prepare([[maybe_unused]] const uint32_t frame,
> >>
> >> /**
> >> * \brief Calculate the tone mapping look up table
> >> - * \param frame The frame number
> >> + te* \param frame The frame number
> > Oops?
> >
>
> :-/ Wondered why it didn't break the compilation and re-looked that it's
> technically comment section
>
> >> * \param context The shared IPA context
> >> * \param stats The IPU3 statistics and ISP results
> >> *
> >> @@ -83,6 +83,8 @@ void ToneMapping::prepare([[maybe_unused]] const uint32_t frame,
> >> void ToneMapping::process(const uint32_t frame, IPAContext &context,
> >> [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> >> {
> >> + IPAFrameContext &frameContext = context.getFrameContext(frame);
> >> +
> > It does seem like we should do this lookup once before calling process()
> > on all algorithms perhaps.
>
>
> Ack!
>
If the FrameContext has a frame number in it- then it could simply
replace the passing of the uint32_t, so that each call gets a
FrameContext, and a full IPAContext.
> >
> >> /*
> >> * Hardcode gamma to 1.1 as a default for now.
> >> *
> >> @@ -90,11 +92,11 @@ void ToneMapping::process(const uint32_t frame, IPAContext &context,
> >> */
> >> gamma_ = 1.1;
> >>
> >> - if (context.frameContext.toneMapping.gamma == gamma_)
> >> + if (frameContext.toneMapping.gamma == gamma_)
> >> return;
> >>
> >> struct ipu3_uapi_gamma_corr_lut &lut =
> >> - context.frameContext.toneMapping.gammaCorrection;
> >> + frameContext.toneMapping.gammaCorrection;
> >>
> >> for (uint32_t i = 0; i < std::size(lut.lut); i++) {
> >> double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> >> @@ -104,7 +106,7 @@ void ToneMapping::process(const uint32_t frame, IPAContext &context,
> >> lut.lut[i] = gamma * 8191;
> >> }
> >>
> >> - context.frameContext.toneMapping.gamma = gamma_;
> >> + frameContext.toneMapping.gamma = gamma_;
> >> }
> >>
> >> } /* namespace ipa::ipu3::algorithms */
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 9c4ec936..41cbf717 100644
> >> --- a/src/ipa/ipu3/ipa_context.cpp
> >> +++ b/src/ipa/ipu3/ipa_context.cpp
> >> @@ -24,6 +24,48 @@ namespace libcamera::ipa::ipu3 {
> >> * may also be updated in the start() operation.
> >> */
> >>
> >> +/**
> >> + * \brief Retrieve the context of a particular frame
> >> + * \param[in] frame Frame number
> >> + *
> >> + * Retrieve the frame context of the \a frame.
> >> + *
> >> + * \return The frame context of the given frame number or nullptr, if not found
> > This doesn't match the implementation below.
> >
> >> + */
> >> +IPAFrameContext &IPAContext::getFrameContext(const uint32_t frame)
> >> +{
> >> + auto iter = frameContextQueue.begin();
> >> + while (iter != frameContextQueue.end()) {
> >> + if (iter->frame == frame)
> >> + return *iter;
> >> +
> >> + iter++;
> >> + }
> >> +
> >> + /*
> >> + * \todo Handle the case where frame-context is not found here.
> >> + * Should we be FATAL ?
> > We're in IPA here - so fatal could kill the IPA process without killing
> > or signalling the libcamera process. That's something we need to figure
> > out separately...
>
>
> Yes, hence seaparted out as \todo
>
> >
> > We might end up hitting this if we try to do something like
> >
> > getFrameContext( frame - 1 );
> >
> > just after a frame was missed or not processed for some reason (like a
> > buffer underflow) which would mean we wouldn't have had stats on that
> > frame. So we need to work out how to handle that gracefully, and as
> > correctly as possible.
>
>
> and list down the cases in which this situation can occur. One is frame
> drop, other ones that the frames cannot be processed are...?
>
> (will look into this as well)
>
> >
> >
> >> + */
> >> + return *iter; /* returns frameContextQueue.end() */
> >> +}
> >> +
> >> +/**
> >> + * \brief Construct a IPAFrameContext instance
> >> + */
> >> +IPAFrameContext::IPAFrameContext() = default;
> >> +
> >> +/**
> >> + * \brief Move constructor for IPAFrameContext
> >> + * \param[in] other The other IPAFrameContext
> >> + */
> >> +IPAFrameContext::IPAFrameContext(IPAFrameContext &&other) = default;
> >> +
> >> +/**
> >> + * \brief Move assignment operator for IPAFrameContext
> >> + * \param[in] other The other IPAFrameContext
> >> + */
> >> +IPAFrameContext &IPAFrameContext::operator=(IPAFrameContext &&other) = default;
> >> +
> >> /**
> >> * \struct IPAFrameContext
> >> * \brief Per-frame context for algorithms
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index d993359a..eca346d6 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -8,6 +8,8 @@
> >>
> >> #pragma once
> >>
> >> +#include <deque>
> >> +
> >> #include <linux/intel-ipu3.h>
> >>
> >> #include <libcamera/base/utils.h>
> >> @@ -39,6 +41,12 @@ struct IPASessionConfiguration {
> >> };
> >>
> >> struct IPAFrameContext {
> >> + uint32_t frame;
> >> +
> >> + IPAFrameContext();
> >> + IPAFrameContext(IPAFrameContext &&other);
> >> + IPAFrameContext &operator=(IPAFrameContext &&other);
> >> +
> >> struct {
> >> uint32_t exposure;
> >> double gain;
> >> @@ -66,8 +74,12 @@ struct IPAFrameContext {
> >> };
> >>
> >> struct IPAContext {
> >> + IPAFrameContext &getFrameContext(const uint32_t frame);
> >> IPASessionConfiguration configuration;
> >> IPAFrameContext frameContext;
> >> +
> >> + std::deque<IPAFrameContext> frameContextQueue;
> >> + IPAFrameContext prevFrameContext;
> >> };
> >>
> >> } /* namespace ipa::ipu3 */
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 3d5c5706..f3128b0a 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -349,6 +349,8 @@ int IPAIPU3::start()
> >> */
> >> void IPAIPU3::stop()
> >> {
> >> + while (!context_.frameContextQueue.empty())
> >> + context_.frameContextQueue.pop_front();
> > would deque::clear() be more distinct here?
> >
>
> Ah yeah
>
> >> }
> >>
> >> /**
> >> @@ -451,6 +453,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >> */
> >> ctrls_ = configInfo.sensorControls;
> >>
> >> + /*
> >> + * Insert a initial context into the queue to faciliate
> >> + * algo->configure() below.
> >> + */
> >> + IPAFrameContext initContext;
> >> + initContext.frame = 0;
> > I ... am not 100% sure about how to handle this.
> >
> > Shouldn't this be frameStarted(0). ... and what happens if you've added
> > this frameContext(0) here, and then the PH delivers a frameStarted(0)
> > event?
> >
> >> + context_.frameContextQueue.push_back(std::move(initContext));
> >> +
> >> calculateBdsGrid(configInfo.bdsOutputSize);
> >>
> >> /* Clean frameContext at each reconfiguration. */
> >> @@ -501,10 +511,25 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> >>
> >> void IPAIPU3::frameStarted([[maybe_unused]] const uint32_t frame)
> >> {
> >> + IPAFrameContext newContext;
> >> + newContext.frame = frame;
> >> +
> >> + context_.frameContextQueue.push_back(std::move(newContext));
> >> }
> >>
> >> void IPAIPU3::frameCompleted([[maybe_unused]] const uint32_t frame)
> >> {
> >> + while (!context_.frameContextQueue.empty()) {
> >> + auto &fc = context_.frameContextQueue.front();
> >> + if (fc.frame < frame)
> >> + context_.frameContextQueue.pop_front();
> >> +
> >> + /* Keep newer frames */
> >> + if (fc.frame >= frame) {
> >> + context_.prevFrameContext = std::move(fc);
> >> + break;
> >> + }
> >> + }
> > I could see us making a dedicated templated queue class specialised to
> > the needs of keeping contexts around and remembering the most recent
> > completed data, which could then be used by other IPAs... But lets get
> > this working and right first and then we can refactor to something
> > generically re-usable I think.
> >
> >> }
> >>
> >> /**
> >> @@ -547,8 +572,9 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, const int64_t frameTimest
> >> const ipu3_uapi_stats_3a *stats =
> >> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> >>
> >> - context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> - context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >> + IPAFrameContext &curFrameContext = context_.getFrameContext(frame);
> >> + curFrameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> + curFrameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >>
> >> parseStatistics(frame, frameTimestamp, stats);
> >> }
> >> @@ -623,11 +649,11 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >> int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
> >> ctrls.set(controls::FrameDuration, frameDuration);
> >>
> >> - ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> >> + ctrls.set(controls::AnalogueGain, context_.prevFrameContext.sensor.gain);
> > This doesn't sound right - when we set this metadata, it should be
> > reflecting the gain that was set for that frame.
>
>
> err, I can't remember the reason why I needed the prevFrameContext here.
> I'll investigate it again.
>
> >
> > We really need a full sequence diagram of the operations from the app
> > (queueing a request) to the PH - to the IPA and when frames are
> > received over CSI2 and handled by the ISP...
> >
> > https://sequencediagram.org/ looks helpful for us to make that.
>
>
> Agreed, you already have an version, I will fork it to create the
> interaction with frame context queue.
Yup, great. Doing that so far has already helped me better understand
when the process is actually happening, and that it's happening on the
'current' frame (in regards to the most recently completed context),
while prepare needs to work on the results of the previous call to
process().
> >> - ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);
> >> + ctrls.set(controls::ColourTemperature, context_.prevFrameContext.awb.temperatureK);
> >>
> >> - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> >> + ctrls.set(controls::ExposureTime, context_.prevFrameContext.sensor.exposure * lineDuration);
> >>
> >> /*
> >> * \todo The Metadata provides a path to getting extended data
> >> @@ -651,8 +677,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >> */
> >> void IPAIPU3::setControls(unsigned int frame)
> >> {
> >> - int32_t exposure = context_.frameContext.agc.exposure;
> >> - int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
> >> + IPAFrameContext &context = context_.getFrameContext(frame);
> >> + int32_t exposure = context.agc.exposure;
> >> + int32_t gain = camHelper_->gainCode(context.agc.gain);
> >
> > We need to check and think about when these controls get set too. But -
> > I also think that's going to be part of the reworkign delayed
> > controls/perframe control support ... so maybe it's something to
> > consider on top anyway.
> >
> >
> >>
> >> ControlList ctrls(ctrls_);
> >> ctrls.set(V4L2_CID_EXPOSURE, exposure);
> >> --
> >> 2.31.0
> >>
More information about the libcamera-devel
mailing list