| Message ID | 20261005131700.97438-2-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2026-10-05 14:16:56) > The SwStatsCpu instance created in SoftwareIsp is currently moved to the > corresponding debayering object. This binds the stats to debayering, > but we'd like to be able to use stats with raw output as well. > > Let's keep the created stats in SoftwareIsp as a shared pointer that is > still passed to the debayering objects but remains accessible outside > them. This also allows us to get rid of Debayer::getStatsFD method. Keeping statistics separate from debayering sounds good to me! Was there a specific reason to keep them associated before? I guess I'll find out when I read more about that getStatsFD method update. > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../libcamera/internal/software_isp/software_isp.h | 2 ++ > src/libcamera/software_isp/debayer.cpp | 10 ---------- > src/libcamera/software_isp/debayer.h | 2 -- > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > src/libcamera/software_isp/debayer_cpu.h | 5 ++--- > src/libcamera/software_isp/debayer_egl.cpp | 2 +- > src/libcamera/software_isp/debayer_egl.h | 6 ++---- > src/libcamera/software_isp/software_isp.cpp | 12 ++++++------ > 8 files changed, 14 insertions(+), 27 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index b8b24bb1b..7c41bc7ad 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -33,6 +33,7 @@ > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/shared_mem_object.h" > #include "libcamera/internal/software_isp/debayer_params.h" > +#include "libcamera/internal/software_isp/swstats_cpu.h" > > namespace libcamera { > > @@ -101,6 +102,7 @@ private: > DebayerParams debayerParams_; > DmaBufAllocator dmaHeap_; > bool ccmEnabled_; > + std::shared_ptr<SwStatsCpu> stats_; > > std::unique_ptr<ipa::softisp::IPAProxySoftIsp> ipa_; > std::deque<FrameBuffer *> queuedInputBuffers_; > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index a4854e51b..7a151fa14 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -132,16 +132,6 @@ Debayer::~Debayer() > * \return The valid size ranges or an empty range if there are none > */ > > -/** > - * \fn const SharedFD &Debayer::getStatsFD() > - * \brief Get the file descriptor for the statistics > - * > - * This file descriptor provides access to the output statistics buffer > - * associated with the current debayering process. Aha, perhaps we did stats while doing debayering originally? > - * > - * \return The file descriptor pointing to the statistics data > - */ > - > /** > * \fn unsigned int Debayer::frameSize() > * \brief Get the output frame size > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index 556852260..2287ce995 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -54,8 +54,6 @@ public: > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > - virtual const SharedFD &getStatsFD() = 0; > - > unsigned int frameSize() { return outputConfig_.frameSize; } > > Signal<FrameBuffer *> inputBufferReady; > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index ce8b3c647..022e56a3d 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -93,7 +93,7 @@ DebayerCpuThread::DebayerCpuThread(DebayerCpu *debayer, unsigned int threadIndex > * \param[in] stats Pointer to the stats object to use > * \param[in] cm The camera manager > */ > -DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm) > +DebayerCpu::DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm) > : Debayer(cm), stats_(std::move(stats)) > { > /* > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 2c88c9e1a..1190216fb 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -32,7 +32,7 @@ class DebayerCpuThread; > class DebayerCpu : public Debayer > { > public: > - DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm); > + DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm); > ~DebayerCpu(); > > int configure(const StreamConfiguration &inputCfg, > @@ -46,7 +46,6 @@ public: > int start() override; > void stop() override; > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; > - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } > > private: > friend class DebayerCpuThread; > @@ -153,7 +152,7 @@ private: > debayerFn debayer2_; > debayerFn debayer3_; > Rectangle window_; > - std::unique_ptr<SwStatsCpu> stats_; > + std::shared_ptr<SwStatsCpu> stats_; > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > > static constexpr unsigned int kMinThreads = 1; > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 300822f9d..df294d6d6 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -42,7 +42,7 @@ namespace libcamera { > * \param[in] cm The camera manager > * \param[in] display The EGL display to use > */ > -DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) > +DebayerEGL::DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) > : Debayer(cm), stats_(std::move(stats)), egl_(display) > { > } > diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h > index 30e51a477..ffa4f61a4 100644 > --- a/src/libcamera/software_isp/debayer_egl.h > +++ b/src/libcamera/software_isp/debayer_egl.h > @@ -40,7 +40,7 @@ class CameraManager; > class DebayerEGL : public Debayer > { > public: > - DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); > + DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); > ~DebayerEGL(); > > int configure(const StreamConfiguration &inputCfg, > @@ -57,8 +57,6 @@ public: > int start() override; > void stop() override; > > - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } > - > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; > > private: > @@ -112,7 +110,7 @@ private: > > Size nativeOutputSize_; > Rectangle window_; > - std::unique_ptr<SwStatsCpu> stats_; > + std::shared_ptr<SwStatsCpu> stats_; > eGL egl_; > uint32_t width_; > uint32_t height_; > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index a40976055..46a7cb5c1 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -98,12 +98,12 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > > const CameraManager &cm = *pipe->cameraManager(); > > - auto stats = std::make_unique<SwStatsCpu>(cm); > - if (!stats->isValid()) { > + stats_ = std::make_shared<SwStatsCpu>(cm); > + if (!stats_->isValid()) { > LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object"; > return; > } > - stats->statsReady.connect(this, &SoftwareIsp::statsReady); > + stats_->statsReady.connect(this, &SoftwareIsp::statsReady); > > #if HAVE_DEBAYER_EGL > const GlobalConfiguration &configuration = cm._d()->configuration(); > @@ -121,7 +121,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > if (!softISPMode || softISPMode == "gpu") { > auto display = eGL::probeDisplay(); > if (display != EGL_NO_DISPLAY) { > - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm, display); > + debayer_ = std::make_unique<DebayerEGL>(stats_, cm, display); > } else { > LOG(SoftwareIsp, Info) > << "EGL not available, falling back to CPU debayer"; > @@ -130,7 +130,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > > #endif > if (!debayer_) > - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), cm); > + debayer_ = std::make_unique<DebayerCpu>(stats_, cm); > > debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); > debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); > @@ -158,7 +158,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > } > > ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, > - debayer_->getStatsFD(), > + stats_->getStatsFD(), Nothing jumps out at me, and now I've run out of review time (kids!), just as I was getting into this. I've tested this series by the way - and it looks like it's doing the right thing, and I can set controls and see metadata change on the output. I need a bright sunshine to test full AGC on the raw tomorrow though ... if someone could improve the weather for me ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > sharedParams_.fd(), > sensorInfo, > sensor->controls(), > -- > 2.55.0 >
Hi Milan On Mon, Oct 05, 2026 at 03:16:56PM +0200, Milan Zamazal wrote: > The SwStatsCpu instance created in SoftwareIsp is currently moved to the > corresponding debayering object. This binds the stats to debayering, > but we'd like to be able to use stats with raw output as well. > > Let's keep the created stats in SoftwareIsp as a shared pointer that is > still passed to the debayering objects but remains accessible outside > them. This also allows us to get rid of Debayer::getStatsFD method. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../libcamera/internal/software_isp/software_isp.h | 2 ++ > src/libcamera/software_isp/debayer.cpp | 10 ---------- > src/libcamera/software_isp/debayer.h | 2 -- > src/libcamera/software_isp/debayer_cpu.cpp | 2 +- > src/libcamera/software_isp/debayer_cpu.h | 5 ++--- > src/libcamera/software_isp/debayer_egl.cpp | 2 +- > src/libcamera/software_isp/debayer_egl.h | 6 ++---- > src/libcamera/software_isp/software_isp.cpp | 12 ++++++------ > 8 files changed, 14 insertions(+), 27 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index b8b24bb1b..7c41bc7ad 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -33,6 +33,7 @@ > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/shared_mem_object.h" > #include "libcamera/internal/software_isp/debayer_params.h" > +#include "libcamera/internal/software_isp/swstats_cpu.h" > > namespace libcamera { > > @@ -101,6 +102,7 @@ private: > DebayerParams debayerParams_; > DmaBufAllocator dmaHeap_; > bool ccmEnabled_; > + std::shared_ptr<SwStatsCpu> stats_; > > std::unique_ptr<ipa::softisp::IPAProxySoftIsp> ipa_; > std::deque<FrameBuffer *> queuedInputBuffers_; > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index a4854e51b..7a151fa14 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -132,16 +132,6 @@ Debayer::~Debayer() > * \return The valid size ranges or an empty range if there are none > */ > > -/** > - * \fn const SharedFD &Debayer::getStatsFD() > - * \brief Get the file descriptor for the statistics > - * > - * This file descriptor provides access to the output statistics buffer > - * associated with the current debayering process. > - * > - * \return The file descriptor pointing to the statistics data > - */ > - > /** > * \fn unsigned int Debayer::frameSize() > * \brief Get the output frame size > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index 556852260..2287ce995 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -54,8 +54,6 @@ public: > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > - virtual const SharedFD &getStatsFD() = 0; > - > unsigned int frameSize() { return outputConfig_.frameSize; } > > Signal<FrameBuffer *> inputBufferReady; > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index ce8b3c647..022e56a3d 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -93,7 +93,7 @@ DebayerCpuThread::DebayerCpuThread(DebayerCpu *debayer, unsigned int threadIndex > * \param[in] stats Pointer to the stats object to use > * \param[in] cm The camera manager > */ > -DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm) > +DebayerCpu::DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm) > : Debayer(cm), stats_(std::move(stats)) nit: this should make no practical difference, as 'stat' is destroyed after this call. I'm not too familiar with this part of the code base, in particular the SwStatsCpu class, but the change seems ok to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > { > /* > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 2c88c9e1a..1190216fb 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -32,7 +32,7 @@ class DebayerCpuThread; > class DebayerCpu : public Debayer > { > public: > - DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm); > + DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm); > ~DebayerCpu(); > > int configure(const StreamConfiguration &inputCfg, > @@ -46,7 +46,6 @@ public: > int start() override; > void stop() override; > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; > - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } > > private: > friend class DebayerCpuThread; > @@ -153,7 +152,7 @@ private: > debayerFn debayer2_; > debayerFn debayer3_; > Rectangle window_; > - std::unique_ptr<SwStatsCpu> stats_; > + std::shared_ptr<SwStatsCpu> stats_; > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > > static constexpr unsigned int kMinThreads = 1; > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 300822f9d..df294d6d6 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -42,7 +42,7 @@ namespace libcamera { > * \param[in] cm The camera manager > * \param[in] display The EGL display to use > */ > -DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) > +DebayerEGL::DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) > : Debayer(cm), stats_(std::move(stats)), egl_(display) > { > } > diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h > index 30e51a477..ffa4f61a4 100644 > --- a/src/libcamera/software_isp/debayer_egl.h > +++ b/src/libcamera/software_isp/debayer_egl.h > @@ -40,7 +40,7 @@ class CameraManager; > class DebayerEGL : public Debayer > { > public: > - DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); > + DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); > ~DebayerEGL(); > > int configure(const StreamConfiguration &inputCfg, > @@ -57,8 +57,6 @@ public: > int start() override; > void stop() override; > > - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } > - > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; > > private: > @@ -112,7 +110,7 @@ private: > > Size nativeOutputSize_; > Rectangle window_; > - std::unique_ptr<SwStatsCpu> stats_; > + std::shared_ptr<SwStatsCpu> stats_; > eGL egl_; > uint32_t width_; > uint32_t height_; > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index a40976055..46a7cb5c1 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -98,12 +98,12 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > > const CameraManager &cm = *pipe->cameraManager(); > > - auto stats = std::make_unique<SwStatsCpu>(cm); > - if (!stats->isValid()) { > + stats_ = std::make_shared<SwStatsCpu>(cm); > + if (!stats_->isValid()) { > LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object"; > return; > } > - stats->statsReady.connect(this, &SoftwareIsp::statsReady); > + stats_->statsReady.connect(this, &SoftwareIsp::statsReady); > > #if HAVE_DEBAYER_EGL > const GlobalConfiguration &configuration = cm._d()->configuration(); > @@ -121,7 +121,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > if (!softISPMode || softISPMode == "gpu") { > auto display = eGL::probeDisplay(); > if (display != EGL_NO_DISPLAY) { > - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm, display); > + debayer_ = std::make_unique<DebayerEGL>(stats_, cm, display); > } else { > LOG(SoftwareIsp, Info) > << "EGL not available, falling back to CPU debayer"; > @@ -130,7 +130,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > > #endif > if (!debayer_) > - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), cm); > + debayer_ = std::make_unique<DebayerCpu>(stats_, cm); > > debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); > debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); > @@ -158,7 +158,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > } > > ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, > - debayer_->getStatsFD(), > + stats_->getStatsFD(), > sharedParams_.fd(), > sensorInfo, > sensor->controls(), > -- > 2.55.0 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2026-10-05 14:16:56) >> The SwStatsCpu instance created in SoftwareIsp is currently moved to the >> corresponding debayering object. This binds the stats to debayering, > >> but we'd like to be able to use stats with raw output as well. >> >> Let's keep the created stats in SoftwareIsp as a shared pointer that is >> still passed to the debayering objects but remains accessible outside >> them. This also allows us to get rid of Debayer::getStatsFD method. > > Keeping statistics separate from debayering sounds good to me! > Was there a specific reason to keep them associated before? I guess I'll > find out when I read more about that getStatsFD method update. > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../libcamera/internal/software_isp/software_isp.h | 2 ++ >> src/libcamera/software_isp/debayer.cpp | 10 ---------- >> src/libcamera/software_isp/debayer.h | 2 -- >> src/libcamera/software_isp/debayer_cpu.cpp | 2 +- >> src/libcamera/software_isp/debayer_cpu.h | 5 ++--- >> src/libcamera/software_isp/debayer_egl.cpp | 2 +- >> src/libcamera/software_isp/debayer_egl.h | 6 ++---- >> src/libcamera/software_isp/software_isp.cpp | 12 ++++++------ >> 8 files changed, 14 insertions(+), 27 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index b8b24bb1b..7c41bc7ad 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -33,6 +33,7 @@ >> #include "libcamera/internal/pipeline_handler.h" >> #include "libcamera/internal/shared_mem_object.h" >> #include "libcamera/internal/software_isp/debayer_params.h" >> +#include "libcamera/internal/software_isp/swstats_cpu.h" >> >> namespace libcamera { >> >> @@ -101,6 +102,7 @@ private: >> DebayerParams debayerParams_; >> DmaBufAllocator dmaHeap_; >> bool ccmEnabled_; >> + std::shared_ptr<SwStatsCpu> stats_; >> >> std::unique_ptr<ipa::softisp::IPAProxySoftIsp> ipa_; >> std::deque<FrameBuffer *> queuedInputBuffers_; >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >> index a4854e51b..7a151fa14 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -132,16 +132,6 @@ Debayer::~Debayer() >> * \return The valid size ranges or an empty range if there are none >> */ >> >> -/** >> - * \fn const SharedFD &Debayer::getStatsFD() >> - * \brief Get the file descriptor for the statistics >> - * >> - * This file descriptor provides access to the output statistics buffer >> - * associated with the current debayering process. > > Aha, perhaps we did stats while doing debayering originally? We still do stats in CPU debayering, in the sense that we perform stats and debayering on each line next to each other, while the line is still in the CPU cache. > >> - * >> - * \return The file descriptor pointing to the statistics data >> - */ >> - >> /** >> * \fn unsigned int Debayer::frameSize() >> * \brief Get the output frame size >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >> index 556852260..2287ce995 100644 >> --- a/src/libcamera/software_isp/debayer.h >> +++ b/src/libcamera/software_isp/debayer.h >> @@ -54,8 +54,6 @@ public: >> >> virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; >> >> - virtual const SharedFD &getStatsFD() = 0; >> - >> unsigned int frameSize() { return outputConfig_.frameSize; } >> >> Signal<FrameBuffer *> inputBufferReady; >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index ce8b3c647..022e56a3d 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -93,7 +93,7 @@ DebayerCpuThread::DebayerCpuThread(DebayerCpu *debayer, unsigned int threadIndex >> * \param[in] stats Pointer to the stats object to use >> * \param[in] cm The camera manager >> */ >> -DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm) >> +DebayerCpu::DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm) >> : Debayer(cm), stats_(std::move(stats)) >> { >> /* >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index 2c88c9e1a..1190216fb 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -32,7 +32,7 @@ class DebayerCpuThread; >> class DebayerCpu : public Debayer >> { >> public: >> - DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm); >> + DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm); >> ~DebayerCpu(); >> >> int configure(const StreamConfiguration &inputCfg, >> @@ -46,7 +46,6 @@ public: >> int start() override; >> void stop() override; >> SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; >> - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } >> >> private: >> friend class DebayerCpuThread; >> @@ -153,7 +152,7 @@ private: >> debayerFn debayer2_; >> debayerFn debayer3_; >> Rectangle window_; >> - std::unique_ptr<SwStatsCpu> stats_; >> + std::shared_ptr<SwStatsCpu> stats_; >> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ >> >> static constexpr unsigned int kMinThreads = 1; >> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >> index 300822f9d..df294d6d6 100644 >> --- a/src/libcamera/software_isp/debayer_egl.cpp >> +++ b/src/libcamera/software_isp/debayer_egl.cpp >> @@ -42,7 +42,7 @@ namespace libcamera { >> * \param[in] cm The camera manager >> * \param[in] display The EGL display to use >> */ >> -DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) >> +DebayerEGL::DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) >> : Debayer(cm), stats_(std::move(stats)), egl_(display) >> { >> } >> diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h >> index 30e51a477..ffa4f61a4 100644 >> --- a/src/libcamera/software_isp/debayer_egl.h >> +++ b/src/libcamera/software_isp/debayer_egl.h >> @@ -40,7 +40,7 @@ class CameraManager; >> class DebayerEGL : public Debayer >> { >> public: >> - DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); >> + DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); >> ~DebayerEGL(); >> >> int configure(const StreamConfiguration &inputCfg, >> @@ -57,8 +57,6 @@ public: >> int start() override; >> void stop() override; >> >> - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } >> - >> SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; >> >> private: >> @@ -112,7 +110,7 @@ private: >> >> Size nativeOutputSize_; >> Rectangle window_; >> - std::unique_ptr<SwStatsCpu> stats_; >> + std::shared_ptr<SwStatsCpu> stats_; >> eGL egl_; >> uint32_t width_; >> uint32_t height_; >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index a40976055..46a7cb5c1 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -98,12 +98,12 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> >> const CameraManager &cm = *pipe->cameraManager(); >> >> - auto stats = std::make_unique<SwStatsCpu>(cm); >> - if (!stats->isValid()) { >> + stats_ = std::make_shared<SwStatsCpu>(cm); >> + if (!stats_->isValid()) { >> LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object"; >> return; >> } >> - stats->statsReady.connect(this, &SoftwareIsp::statsReady); >> + stats_->statsReady.connect(this, &SoftwareIsp::statsReady); >> >> #if HAVE_DEBAYER_EGL >> const GlobalConfiguration &configuration = cm._d()->configuration(); >> @@ -121,7 +121,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> if (!softISPMode || softISPMode == "gpu") { >> auto display = eGL::probeDisplay(); >> if (display != EGL_NO_DISPLAY) { >> - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm, display); >> + debayer_ = std::make_unique<DebayerEGL>(stats_, cm, display); >> } else { >> LOG(SoftwareIsp, Info) >> << "EGL not available, falling back to CPU debayer"; >> @@ -130,7 +130,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> >> #endif >> if (!debayer_) >> - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), cm); >> + debayer_ = std::make_unique<DebayerCpu>(stats_, cm); >> >> debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); >> debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); >> @@ -158,7 +158,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> } >> >> ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, >> - debayer_->getStatsFD(), >> + stats_->getStatsFD(), > > Nothing jumps out at me, and now I've run out of review time (kids!), > just as I was getting into this. > > I've tested this series by the way - and it looks like it's doing the > right thing, and I can set controls and see metadata change on the > output. I need a bright sunshine to test full AGC on the raw tomorrow > though ... if someone could improve the weather for me ;-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > >> sharedParams_.fd(), >> sensorInfo, >> sensor->controls(), >> -- >> 2.55.0 >>
diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index b8b24bb1b..7c41bc7ad 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -33,6 +33,7 @@ #include "libcamera/internal/pipeline_handler.h" #include "libcamera/internal/shared_mem_object.h" #include "libcamera/internal/software_isp/debayer_params.h" +#include "libcamera/internal/software_isp/swstats_cpu.h" namespace libcamera { @@ -101,6 +102,7 @@ private: DebayerParams debayerParams_; DmaBufAllocator dmaHeap_; bool ccmEnabled_; + std::shared_ptr<SwStatsCpu> stats_; std::unique_ptr<ipa::softisp::IPAProxySoftIsp> ipa_; std::deque<FrameBuffer *> queuedInputBuffers_; diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index a4854e51b..7a151fa14 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -132,16 +132,6 @@ Debayer::~Debayer() * \return The valid size ranges or an empty range if there are none */ -/** - * \fn const SharedFD &Debayer::getStatsFD() - * \brief Get the file descriptor for the statistics - * - * This file descriptor provides access to the output statistics buffer - * associated with the current debayering process. - * - * \return The file descriptor pointing to the statistics data - */ - /** * \fn unsigned int Debayer::frameSize() * \brief Get the output frame size diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index 556852260..2287ce995 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -54,8 +54,6 @@ public: virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; - virtual const SharedFD &getStatsFD() = 0; - unsigned int frameSize() { return outputConfig_.frameSize; } Signal<FrameBuffer *> inputBufferReady; diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index ce8b3c647..022e56a3d 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -93,7 +93,7 @@ DebayerCpuThread::DebayerCpuThread(DebayerCpu *debayer, unsigned int threadIndex * \param[in] stats Pointer to the stats object to use * \param[in] cm The camera manager */ -DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm) +DebayerCpu::DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm) : Debayer(cm), stats_(std::move(stats)) { /* diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 2c88c9e1a..1190216fb 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -32,7 +32,7 @@ class DebayerCpuThread; class DebayerCpu : public Debayer { public: - DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm); + DebayerCpu(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm); ~DebayerCpu(); int configure(const StreamConfiguration &inputCfg, @@ -46,7 +46,6 @@ public: int start() override; void stop() override; SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } private: friend class DebayerCpuThread; @@ -153,7 +152,7 @@ private: debayerFn debayer2_; debayerFn debayer3_; Rectangle window_; - std::unique_ptr<SwStatsCpu> stats_; + std::shared_ptr<SwStatsCpu> stats_; unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ static constexpr unsigned int kMinThreads = 1; diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 300822f9d..df294d6d6 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -42,7 +42,7 @@ namespace libcamera { * \param[in] cm The camera manager * \param[in] display The EGL display to use */ -DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) +DebayerEGL::DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) : Debayer(cm), stats_(std::move(stats)), egl_(display) { } diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h index 30e51a477..ffa4f61a4 100644 --- a/src/libcamera/software_isp/debayer_egl.h +++ b/src/libcamera/software_isp/debayer_egl.h @@ -40,7 +40,7 @@ class CameraManager; class DebayerEGL : public Debayer { public: - DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); + DebayerEGL(std::shared_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); ~DebayerEGL(); int configure(const StreamConfiguration &inputCfg, @@ -57,8 +57,6 @@ public: int start() override; void stop() override; - const SharedFD &getStatsFD() override { return stats_->getStatsFD(); } - SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; private: @@ -112,7 +110,7 @@ private: Size nativeOutputSize_; Rectangle window_; - std::unique_ptr<SwStatsCpu> stats_; + std::shared_ptr<SwStatsCpu> stats_; eGL egl_; uint32_t width_; uint32_t height_; diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index a40976055..46a7cb5c1 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -98,12 +98,12 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, const CameraManager &cm = *pipe->cameraManager(); - auto stats = std::make_unique<SwStatsCpu>(cm); - if (!stats->isValid()) { + stats_ = std::make_shared<SwStatsCpu>(cm); + if (!stats_->isValid()) { LOG(SoftwareIsp, Error) << "Failed to create SwStatsCpu object"; return; } - stats->statsReady.connect(this, &SoftwareIsp::statsReady); + stats_->statsReady.connect(this, &SoftwareIsp::statsReady); #if HAVE_DEBAYER_EGL const GlobalConfiguration &configuration = cm._d()->configuration(); @@ -121,7 +121,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, if (!softISPMode || softISPMode == "gpu") { auto display = eGL::probeDisplay(); if (display != EGL_NO_DISPLAY) { - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm, display); + debayer_ = std::make_unique<DebayerEGL>(stats_, cm, display); } else { LOG(SoftwareIsp, Info) << "EGL not available, falling back to CPU debayer"; @@ -130,7 +130,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, #endif if (!debayer_) - debayer_ = std::make_unique<DebayerCpu>(std::move(stats), cm); + debayer_ = std::make_unique<DebayerCpu>(stats_, cm); debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); @@ -158,7 +158,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, } ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, - debayer_->getStatsFD(), + stats_->getStatsFD(), sharedParams_.fd(), sensorInfo, sensor->controls(),
The SwStatsCpu instance created in SoftwareIsp is currently moved to the corresponding debayering object. This binds the stats to debayering, but we'd like to be able to use stats with raw output as well. Let's keep the created stats in SoftwareIsp as a shared pointer that is still passed to the debayering objects but remains accessible outside them. This also allows us to get rid of Debayer::getStatsFD method. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../libcamera/internal/software_isp/software_isp.h | 2 ++ src/libcamera/software_isp/debayer.cpp | 10 ---------- src/libcamera/software_isp/debayer.h | 2 -- src/libcamera/software_isp/debayer_cpu.cpp | 2 +- src/libcamera/software_isp/debayer_cpu.h | 5 ++--- src/libcamera/software_isp/debayer_egl.cpp | 2 +- src/libcamera/software_isp/debayer_egl.h | 6 ++---- src/libcamera/software_isp/software_isp.cpp | 12 ++++++------ 8 files changed, 14 insertions(+), 27 deletions(-)