[libcamera-devel] [PATCH 3/3] ipa: ipu3: Add a IPAFrameContext queue
Umang Jain
umang.jain at ideasonboard.com
Mon Apr 25 21:52:18 CEST 2022
Hi Kieran,
On 4/25/22 16:09, Kieran Bingham wrote:
> 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.
Ah yes, I agree with this explaination. This matches whats on master as
of now.
>
>
> (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.)
Yep.
>
>
>>> 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.
It's frustrating to me now as well :-/ I have been successfully confused
for quite a few hours.
>
> 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.
So yes, am convinced that the algo->process() part in 7. is about
calculating the parameters for the 'next' frame
In that case...
>
> 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?)
It makes sense to make a 'next'FrameContext and get rid of
'prev'FrameContext. And we'll have already have a current frameContext
which we are working upon.
Does that make the lifetime a bit clearer? With putting the focus on
'next'FrameContext.
>
>>>>
>>>> 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.
Agreed for now, I'll probably need to mend the previous patches first.
>
>
>
>>>> /*
>>>> * 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