| Message ID | 20250925221708.7471-6-hansg@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, thank you for the patch. Hans de Goede <hansg@kernel.org> writes: > Generating statistics for every single frame is not really necessary. > > However a roundtrip through ipa_->processStats() still need to be done > every frame, even if there are no stats to make the IPA generate metadata > for every frame. > > Add a valid flag to the statistics struct to let the IPA know when there > are no statistics for the frame being processed and modify the IPA to > only generate metadata for frames without valid statistics. > > This is a preparation patch for skipping statistics generation for some > frames. > > Signed-off-by: Hans de Goede <hansg@kernel.org> > --- > include/libcamera/internal/software_isp/swisp_stats.h | 4 ++++ > src/ipa/simple/algorithms/agc.cpp | 3 +++ > src/ipa/simple/algorithms/awb.cpp | 3 +++ > src/ipa/simple/algorithms/blc.cpp | 3 +++ > src/ipa/simple/soft_simple.cpp | 3 +++ > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > src/libcamera/software_isp/swstats_cpu.cpp | 4 +++- > src/libcamera/software_isp/swstats_cpu.h | 2 +- > 8 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h > index ae11f112..cbcfd351 100644 > --- a/include/libcamera/internal/software_isp/swisp_stats.h > +++ b/include/libcamera/internal/software_isp/swisp_stats.h > @@ -20,6 +20,10 @@ namespace libcamera { > * wrap around. > */ > struct SwIspStats { > + /** > + * \brief Indicates if the statistics buffer contains valid data > + */ > + bool valid; I'd add to the docstring what "valid" data is. > /** > * \brief Holds the sum of all sampled red pixels > */ > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 3471cc88..e4e24113 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -107,6 +107,9 @@ void Agc::process(IPAContext &context, > metadata.set(controls::ExposureTime, exposureTime.get<std::micro>()); > metadata.set(controls::AnalogueGain, frameContext.sensor.gain); > > + if (!stats->valid) > + return; > + > /* > * Calculate Mean Sample Value (MSV) according to formula from: > * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index cf567e89..ddd0b791 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -61,6 +61,9 @@ void Awb::process(IPAContext &context, > }; > metadata.set(controls::ColourGains, mdGains); > > + if (!stats->valid) > + return; > + > /* > * Black level must be subtracted to get the correct AWB ratios, they > * would be off if they were computed from the whole brightness range > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > index 8c1e9ed0..616da0ee 100644 > --- a/src/ipa/simple/algorithms/blc.cpp > +++ b/src/ipa/simple/algorithms/blc.cpp > @@ -60,6 +60,9 @@ void BlackLevel::process(IPAContext &context, > }; > metadata.set(controls::SensorBlackLevels, blackLevels); > > + if (!stats->valid) > + return; > + > if (context.configuration.black.level.has_value()) > return; > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index b147aca2..e1c8e21a 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -318,6 +318,9 @@ void IPASoftSimple::processStats(const uint32_t frame, > algo->process(context_, frame, frameContext, stats_, metadata); > metadataReady.emit(frame, metadata); > > + if (!stats_->valid) > + return; > + As discussed in the followup patch, this means the later setSensorControls.emit(ctrls); call gets skipped, which means SimpleCameraData::setSensorControls is skipped and delayedCtrls_ is not updated, resulting in a possibly invalid access in frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); above -- sensorControls is taken from delayedCtrls_ for the given frame. > /* Sanity check */ > if (!sensorControls.contains(V4L2_CID_EXPOSURE) || > !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 2dc85e5e..bfa60888 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -851,7 +851,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > * > * \todo Pass real bufferId once stats buffer passing is changed. > */ > - stats_->finishFrame(frame, 0); > + stats_->finishFrame(frame, 0, true); > outputBufferReady.emit(output); > inputBufferReady.emit(input); > } > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp > index 4b77b360..da91f912 100644 > --- a/src/libcamera/software_isp/swstats_cpu.cpp > +++ b/src/libcamera/software_isp/swstats_cpu.cpp > @@ -313,11 +313,13 @@ void SwStatsCpu::startFrame(void) > * \brief Finish statistics calculation for the current frame > * \param[in] frame The frame number > * \param[in] bufferId ID of the statistics buffer > + * \param[in] valid Indicates if the statistics for the current frame are valid > * > * This may only be called after a successful setWindow() call. > */ > -void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) > +void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId, bool valid) > { > + stats_.valid = valid; > *sharedStats_ = stats_; > statsReady.emit(frame, bufferId); > } > diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h > index 26a2f462..6ac3c4de 100644 > --- a/src/libcamera/software_isp/swstats_cpu.h > +++ b/src/libcamera/software_isp/swstats_cpu.h > @@ -41,7 +41,7 @@ public: > int configure(const StreamConfiguration &inputCfg); > void setWindow(const Rectangle &window); > void startFrame(); > - void finishFrame(uint32_t frame, uint32_t bufferId); > + void finishFrame(uint32_t frame, uint32_t bufferId, bool valid); > > void processLine0(unsigned int y, const uint8_t *src[]) > {
Hi Milan, On 26-Sep-25 9:32 PM, Milan Zamazal wrote: > Hi Hans, > > thank you for the patch. > > Hans de Goede <hansg@kernel.org> writes: > >> Generating statistics for every single frame is not really necessary. >> >> However a roundtrip through ipa_->processStats() still need to be done >> every frame, even if there are no stats to make the IPA generate metadata >> for every frame. >> >> Add a valid flag to the statistics struct to let the IPA know when there >> are no statistics for the frame being processed and modify the IPA to >> only generate metadata for frames without valid statistics. >> >> This is a preparation patch for skipping statistics generation for some >> frames. >> >> Signed-off-by: Hans de Goede <hansg@kernel.org> >> --- >> include/libcamera/internal/software_isp/swisp_stats.h | 4 ++++ >> src/ipa/simple/algorithms/agc.cpp | 3 +++ >> src/ipa/simple/algorithms/awb.cpp | 3 +++ >> src/ipa/simple/algorithms/blc.cpp | 3 +++ >> src/ipa/simple/soft_simple.cpp | 3 +++ >> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >> src/libcamera/software_isp/swstats_cpu.cpp | 4 +++- >> src/libcamera/software_isp/swstats_cpu.h | 2 +- >> 8 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h >> index ae11f112..cbcfd351 100644 >> --- a/include/libcamera/internal/software_isp/swisp_stats.h >> +++ b/include/libcamera/internal/software_isp/swisp_stats.h >> @@ -20,6 +20,10 @@ namespace libcamera { >> * wrap around. >> */ >> struct SwIspStats { >> + /** >> + * \brief Indicates if the statistics buffer contains valid data >> + */ >> + bool valid; > > I'd add to the docstring what "valid" data is. Ack will fix for v3. >> /** >> * \brief Holds the sum of all sampled red pixels >> */ >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index 3471cc88..e4e24113 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -107,6 +107,9 @@ void Agc::process(IPAContext &context, >> metadata.set(controls::ExposureTime, exposureTime.get<std::micro>()); >> metadata.set(controls::AnalogueGain, frameContext.sensor.gain); >> >> + if (!stats->valid) >> + return; >> + >> /* >> * Calculate Mean Sample Value (MSV) according to formula from: >> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index cf567e89..ddd0b791 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -61,6 +61,9 @@ void Awb::process(IPAContext &context, >> }; >> metadata.set(controls::ColourGains, mdGains); >> >> + if (!stats->valid) >> + return; >> + >> /* >> * Black level must be subtracted to get the correct AWB ratios, they >> * would be off if they were computed from the whole brightness range >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp >> index 8c1e9ed0..616da0ee 100644 >> --- a/src/ipa/simple/algorithms/blc.cpp >> +++ b/src/ipa/simple/algorithms/blc.cpp >> @@ -60,6 +60,9 @@ void BlackLevel::process(IPAContext &context, >> }; >> metadata.set(controls::SensorBlackLevels, blackLevels); >> >> + if (!stats->valid) >> + return; >> + >> if (context.configuration.black.level.has_value()) >> return; >> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index b147aca2..e1c8e21a 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -318,6 +318,9 @@ void IPASoftSimple::processStats(const uint32_t frame, >> algo->process(context_, frame, frameContext, stats_, metadata); >> metadataReady.emit(frame, metadata); >> >> + if (!stats_->valid) >> + return; >> + > > As discussed in the followup patch, this means the later > > setSensorControls.emit(ctrls); > > call gets skipped, which means SimpleCameraData::setSensorControls is > skipped and delayedCtrls_ is not updated, resulting in a possibly > invalid access in > > frameContext.sensor.exposure = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > above -- sensorControls is taken from delayedCtrls_ for the given frame. Ah, that is too bad I was sorta expecting that the delayedCtrls_ code would hang on to the last set of controls if it ran out of history because we were not feeding it any new values. Ah well. I'll drop this "if (!stats_->valid)" addition then. That will cause unchanged gain + expo to get pushed to the delayed controls for frames without statistics, which should fix the delayedCtrls underrun. Regards, Hans
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index ae11f112..cbcfd351 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -20,6 +20,10 @@ namespace libcamera { * wrap around. */ struct SwIspStats { + /** + * \brief Indicates if the statistics buffer contains valid data + */ + bool valid; /** * \brief Holds the sum of all sampled red pixels */ diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 3471cc88..e4e24113 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -107,6 +107,9 @@ void Agc::process(IPAContext &context, metadata.set(controls::ExposureTime, exposureTime.get<std::micro>()); metadata.set(controls::AnalogueGain, frameContext.sensor.gain); + if (!stats->valid) + return; + /* * Calculate Mean Sample Value (MSV) according to formula from: * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index cf567e89..ddd0b791 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -61,6 +61,9 @@ void Awb::process(IPAContext &context, }; metadata.set(controls::ColourGains, mdGains); + if (!stats->valid) + return; + /* * Black level must be subtracted to get the correct AWB ratios, they * would be off if they were computed from the whole brightness range diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp index 8c1e9ed0..616da0ee 100644 --- a/src/ipa/simple/algorithms/blc.cpp +++ b/src/ipa/simple/algorithms/blc.cpp @@ -60,6 +60,9 @@ void BlackLevel::process(IPAContext &context, }; metadata.set(controls::SensorBlackLevels, blackLevels); + if (!stats->valid) + return; + if (context.configuration.black.level.has_value()) return; diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index b147aca2..e1c8e21a 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -318,6 +318,9 @@ void IPASoftSimple::processStats(const uint32_t frame, algo->process(context_, frame, frameContext, stats_, metadata); metadataReady.emit(frame, metadata); + if (!stats_->valid) + return; + /* Sanity check */ if (!sensorControls.contains(V4L2_CID_EXPOSURE) || !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) { diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 2dc85e5e..bfa60888 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -851,7 +851,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output * * \todo Pass real bufferId once stats buffer passing is changed. */ - stats_->finishFrame(frame, 0); + stats_->finishFrame(frame, 0, true); outputBufferReady.emit(output); inputBufferReady.emit(input); } diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp index 4b77b360..da91f912 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -313,11 +313,13 @@ void SwStatsCpu::startFrame(void) * \brief Finish statistics calculation for the current frame * \param[in] frame The frame number * \param[in] bufferId ID of the statistics buffer + * \param[in] valid Indicates if the statistics for the current frame are valid * * This may only be called after a successful setWindow() call. */ -void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) +void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId, bool valid) { + stats_.valid = valid; *sharedStats_ = stats_; statsReady.emit(frame, bufferId); } diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h index 26a2f462..6ac3c4de 100644 --- a/src/libcamera/software_isp/swstats_cpu.h +++ b/src/libcamera/software_isp/swstats_cpu.h @@ -41,7 +41,7 @@ public: int configure(const StreamConfiguration &inputCfg); void setWindow(const Rectangle &window); void startFrame(); - void finishFrame(uint32_t frame, uint32_t bufferId); + void finishFrame(uint32_t frame, uint32_t bufferId, bool valid); void processLine0(unsigned int y, const uint8_t *src[]) {
Generating statistics for every single frame is not really necessary. However a roundtrip through ipa_->processStats() still need to be done every frame, even if there are no stats to make the IPA generate metadata for every frame. Add a valid flag to the statistics struct to let the IPA know when there are no statistics for the frame being processed and modify the IPA to only generate metadata for frames without valid statistics. This is a preparation patch for skipping statistics generation for some frames. Signed-off-by: Hans de Goede <hansg@kernel.org> --- include/libcamera/internal/software_isp/swisp_stats.h | 4 ++++ src/ipa/simple/algorithms/agc.cpp | 3 +++ src/ipa/simple/algorithms/awb.cpp | 3 +++ src/ipa/simple/algorithms/blc.cpp | 3 +++ src/ipa/simple/soft_simple.cpp | 3 +++ src/libcamera/software_isp/debayer_cpu.cpp | 2 +- src/libcamera/software_isp/swstats_cpu.cpp | 4 +++- src/libcamera/software_isp/swstats_cpu.h | 2 +- 8 files changed, 21 insertions(+), 3 deletions(-)