| Message ID | 20240906120927.4071508-8-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan - thanks for the patch On 06/09/2024 13:09, Milan Zamazal wrote: > A previous preparation patch implemented passing frame ids to stats > processing but without actual meaningful frame id value passed there. > This patch extends that by actually providing the frame id and passing > it through to the stats processor. > > The frame id is taken from the request sequence number, the same as in > hardware pipelines. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../libcamera/internal/software_isp/software_isp.h | 4 ++-- > src/libcamera/pipeline/simple/simple.cpp | 8 +++++++- > src/libcamera/software_isp/debayer.cpp | 3 ++- > src/libcamera/software_isp/debayer.h | 2 +- > src/libcamera/software_isp/debayer_cpu.cpp | 9 ++++----- > src/libcamera/software_isp/debayer_cpu.h | 2 +- > src/libcamera/software_isp/software_isp.cpp | 11 +++++++---- > 7 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index 3602bce8..3a84418e 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -73,10 +73,10 @@ public: > int start(); > void stop(); > > - int queueBuffers(FrameBuffer *input, > + int queueBuffers(uint32_t frame, FrameBuffer *input, > const std::map<const Stream *, FrameBuffer *> &outputs); > > - void process(FrameBuffer *input, FrameBuffer *output); > + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output); > > Signal<FrameBuffer *> inputBufferReady; > Signal<FrameBuffer *> outputBufferReady; > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 48a568da..ebec592a 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -865,7 +865,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > if (converter_) > converter_->queueBuffers(buffer, conversionQueue_.front()); > else > - swIsp_->queueBuffers(buffer, conversionQueue_.front()); > + /* > + * request->sequence() cannot be retrieved from `buffer' inside > + * queueBuffers because unique_ptr's make buffer->request() invalid > + * already here. > + */ > + swIsp_->queueBuffers(request->sequence(), buffer, > + conversionQueue_.front()); > > conversionQueue_.pop(); > return; > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index db26c380..f0b83261 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -94,8 +94,9 @@ Debayer::~Debayer() > */ > > /** > - * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > + * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > * \brief Process the bayer data into the requested format > + * \param[in] frame The frame number > * \param[in] input The input buffer > * \param[in] output The output buffer > * \param[in] params The parameters to be used in debayering > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index c151fe5d..d7ca060d 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -40,7 +40,7 @@ public: > virtual std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > - virtual void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 2a2e7edb..f7b3a7d1 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -724,7 +724,7 @@ static inline int64_t timeDiff(timespec &after, timespec &before) > (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > } > > -void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > { > timespec frameStartTime; > > @@ -778,12 +778,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > } > > /* > - * Frame and buffer ids are currently not used, so pass zeros as parameters. > + * Buffer ids are currently not used, so pass zeros as its parameter. > * > - * \todo Pass real values once frame is passed here and stats buffer passing > - * is changed. > + * \todo Pass real bufferId once stats buffer passing is changed. > */ > - stats_->finishFrame(0, 0); > + stats_->finishFrame(frame, 0); > outputBufferReady.emit(output); > inputBufferReady.emit(input); > } > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 8237a64b..2c47e7c6 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -36,7 +36,7 @@ public: > std::vector<PixelFormat> formats(PixelFormat input); > std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); > - void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params); > + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); > > /** > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index a3855568..c5db45ae 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -14,6 +14,7 @@ > #include <unistd.h> > > #include <libcamera/formats.h> > +#include <libcamera/request.h> Is this needed? I can't see that it is, at least in this patch? Otherwise; Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > #include <libcamera/stream.h> > > #include "libcamera/internal/ipa_manager.h" > @@ -278,12 +279,13 @@ int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count, > > /** > * \brief Queue buffers to Software ISP > + * \param[in] frame The frame number > * \param[in] input The input framebuffer > * \param[in] outputs The container holding the output stream pointers and > * their respective frame buffer outputs > * \return 0 on success, a negative errno on failure > */ > -int SoftwareIsp::queueBuffers(FrameBuffer *input, > +int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, > const std::map<const Stream *, FrameBuffer *> &outputs) > { > /* > @@ -301,7 +303,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, > } > > for (auto iter = outputs.begin(); iter != outputs.end(); iter++) > - process(input, iter->second); > + process(frame, input, iter->second); > > return 0; > } > @@ -333,13 +335,14 @@ void SoftwareIsp::stop() > > /** > * \brief Passes the input framebuffer to the ISP worker to process > + * \param[in] frame The frame number > * \param[in] input The input framebuffer > * \param[out] output The framebuffer to write the processed frame to > */ > -void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output) > +void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) > { > debayer_->invokeMethod(&DebayerCpu::process, > - ConnectionTypeQueued, input, output, debayerParams_); > + ConnectionTypeQueued, frame, input, output, debayerParams_); > } > > void SoftwareIsp::saveIspParams()
Quoting Dan Scally (2024-09-09 09:10:39) > Hi Milan - thanks for the patch > > On 06/09/2024 13:09, Milan Zamazal wrote: > > A previous preparation patch implemented passing frame ids to stats > > processing but without actual meaningful frame id value passed there. > > This patch extends that by actually providing the frame id and passing > > it through to the stats processor. > > > > The frame id is taken from the request sequence number, the same as in > > hardware pipelines. I think we'll change/fix this in the future to make everything more consistent for the different 'clock' domains in regards to PFC - but I think it's fine for now. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > .../libcamera/internal/software_isp/software_isp.h | 4 ++-- > > src/libcamera/pipeline/simple/simple.cpp | 8 +++++++- > > src/libcamera/software_isp/debayer.cpp | 3 ++- > > src/libcamera/software_isp/debayer.h | 2 +- > > src/libcamera/software_isp/debayer_cpu.cpp | 9 ++++----- > > src/libcamera/software_isp/debayer_cpu.h | 2 +- > > src/libcamera/software_isp/software_isp.cpp | 11 +++++++---- > > 7 files changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > > index 3602bce8..3a84418e 100644 > > --- a/include/libcamera/internal/software_isp/software_isp.h > > +++ b/include/libcamera/internal/software_isp/software_isp.h > > @@ -73,10 +73,10 @@ public: > > int start(); > > void stop(); > > > > - int queueBuffers(FrameBuffer *input, > > + int queueBuffers(uint32_t frame, FrameBuffer *input, > > const std::map<const Stream *, FrameBuffer *> &outputs); > > > > - void process(FrameBuffer *input, FrameBuffer *output); > > + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output); > > > > Signal<FrameBuffer *> inputBufferReady; > > Signal<FrameBuffer *> outputBufferReady; > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 48a568da..ebec592a 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -865,7 +865,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > > if (converter_) > > converter_->queueBuffers(buffer, conversionQueue_.front()); > > else > > - swIsp_->queueBuffers(buffer, conversionQueue_.front()); > > + /* > > + * request->sequence() cannot be retrieved from `buffer' inside > > + * queueBuffers because unique_ptr's make buffer->request() invalid > > + * already here. > > + */ > > + swIsp_->queueBuffers(request->sequence(), buffer, > > + conversionQueue_.front()); > > > > conversionQueue_.pop(); > > return; > > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > > index db26c380..f0b83261 100644 > > --- a/src/libcamera/software_isp/debayer.cpp > > +++ b/src/libcamera/software_isp/debayer.cpp > > @@ -94,8 +94,9 @@ Debayer::~Debayer() > > */ > > > > /** > > - * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > + * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > * \brief Process the bayer data into the requested format > > + * \param[in] frame The frame number > > * \param[in] input The input buffer > > * \param[in] output The output buffer > > * \param[in] params The parameters to be used in debayering > > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > > index c151fe5d..d7ca060d 100644 > > --- a/src/libcamera/software_isp/debayer.h > > +++ b/src/libcamera/software_isp/debayer.h > > @@ -40,7 +40,7 @@ public: > > virtual std::tuple<unsigned int, unsigned int> > > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > > > - virtual void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > > + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > > > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > index 2a2e7edb..f7b3a7d1 100644 > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > @@ -724,7 +724,7 @@ static inline int64_t timeDiff(timespec &after, timespec &before) > > (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > > } > > > > -void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > { > > timespec frameStartTime; > > > > @@ -778,12 +778,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > > } > > > > /* > > - * Frame and buffer ids are currently not used, so pass zeros as parameters. > > + * Buffer ids are currently not used, so pass zeros as its parameter. > > * > > - * \todo Pass real values once frame is passed here and stats buffer passing > > - * is changed. > > + * \todo Pass real bufferId once stats buffer passing is changed. > > */ > > - stats_->finishFrame(0, 0); > > + stats_->finishFrame(frame, 0); > > outputBufferReady.emit(output); > > inputBufferReady.emit(input); > > } > > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > > index 8237a64b..2c47e7c6 100644 > > --- a/src/libcamera/software_isp/debayer_cpu.h > > +++ b/src/libcamera/software_isp/debayer_cpu.h > > @@ -36,7 +36,7 @@ public: > > std::vector<PixelFormat> formats(PixelFormat input); > > std::tuple<unsigned int, unsigned int> > > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); > > - void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params); > > + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); > > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); > > > > /** > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > > index a3855568..c5db45ae 100644 > > --- a/src/libcamera/software_isp/software_isp.cpp > > +++ b/src/libcamera/software_isp/software_isp.cpp > > @@ -14,6 +14,7 @@ > > #include <unistd.h> > > > > #include <libcamera/formats.h> > > +#include <libcamera/request.h> > > > Is this needed? I can't see that it is, at least in this patch? Otherwise; > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > > > #include <libcamera/stream.h> > > > > #include "libcamera/internal/ipa_manager.h" > > @@ -278,12 +279,13 @@ int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count, > > > > /** > > * \brief Queue buffers to Software ISP > > + * \param[in] frame The frame number > > * \param[in] input The input framebuffer > > * \param[in] outputs The container holding the output stream pointers and > > * their respective frame buffer outputs > > * \return 0 on success, a negative errno on failure > > */ > > -int SoftwareIsp::queueBuffers(FrameBuffer *input, > > +int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, > > const std::map<const Stream *, FrameBuffer *> &outputs) > > { > > /* > > @@ -301,7 +303,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, > > } > > > > for (auto iter = outputs.begin(); iter != outputs.end(); iter++) > > - process(input, iter->second); > > + process(frame, input, iter->second); > > > > return 0; > > } > > @@ -333,13 +335,14 @@ void SoftwareIsp::stop() > > > > /** > > * \brief Passes the input framebuffer to the ISP worker to process > > + * \param[in] frame The frame number > > * \param[in] input The input framebuffer > > * \param[out] output The framebuffer to write the processed frame to > > */ > > -void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output) > > +void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) > > { > > debayer_->invokeMethod(&DebayerCpu::process, > > - ConnectionTypeQueued, input, output, debayerParams_); > > + ConnectionTypeQueued, frame, input, output, debayerParams_); > > } > > > > void SoftwareIsp::saveIspParams()
Hi Dan, thank you for review. Dan Scally <dan.scally@ideasonboard.com> writes: > Hi Milan - thanks for the patch > > On 06/09/2024 13:09, Milan Zamazal wrote: >> A previous preparation patch implemented passing frame ids to stats >> processing but without actual meaningful frame id value passed there. >> This patch extends that by actually providing the frame id and passing >> it through to the stats processor. >> >> The frame id is taken from the request sequence number, the same as in >> hardware pipelines. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../libcamera/internal/software_isp/software_isp.h | 4 ++-- >> src/libcamera/pipeline/simple/simple.cpp | 8 +++++++- >> src/libcamera/software_isp/debayer.cpp | 3 ++- >> src/libcamera/software_isp/debayer.h | 2 +- >> src/libcamera/software_isp/debayer_cpu.cpp | 9 ++++----- >> src/libcamera/software_isp/debayer_cpu.h | 2 +- >> src/libcamera/software_isp/software_isp.cpp | 11 +++++++---- >> 7 files changed, 24 insertions(+), 15 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index 3602bce8..3a84418e 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -73,10 +73,10 @@ public: >> int start(); >> void stop(); >> - int queueBuffers(FrameBuffer *input, >> + int queueBuffers(uint32_t frame, FrameBuffer *input, >> const std::map<const Stream *, FrameBuffer *> &outputs); >> - void process(FrameBuffer *input, FrameBuffer *output); >> + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output); >> Signal<FrameBuffer *> inputBufferReady; >> Signal<FrameBuffer *> outputBufferReady; >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 48a568da..ebec592a 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -865,7 +865,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) >> if (converter_) >> converter_->queueBuffers(buffer, conversionQueue_.front()); >> else >> - swIsp_->queueBuffers(buffer, conversionQueue_.front()); >> + /* >> + * request->sequence() cannot be retrieved from `buffer' inside >> + * queueBuffers because unique_ptr's make buffer->request() invalid >> + * already here. >> + */ >> + swIsp_->queueBuffers(request->sequence(), buffer, >> + conversionQueue_.front()); >> conversionQueue_.pop(); >> return; >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >> index db26c380..f0b83261 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -94,8 +94,9 @@ Debayer::~Debayer() >> */ >> /** >> - * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> + * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> * \brief Process the bayer data into the requested format >> + * \param[in] frame The frame number >> * \param[in] input The input buffer >> * \param[in] output The output buffer >> * \param[in] params The parameters to be used in debayering >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >> index c151fe5d..d7ca060d 100644 >> --- a/src/libcamera/software_isp/debayer.h >> +++ b/src/libcamera/software_isp/debayer.h >> @@ -40,7 +40,7 @@ public: >> virtual std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >> - virtual void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >> + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >> virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index 2a2e7edb..f7b3a7d1 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -724,7 +724,7 @@ static inline int64_t timeDiff(timespec &after, timespec &before) >> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; >> } >> -void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> { >> timespec frameStartTime; >> @@ -778,12 +778,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams >> } >> /* >> - * Frame and buffer ids are currently not used, so pass zeros as parameters. >> + * Buffer ids are currently not used, so pass zeros as its parameter. >> * >> - * \todo Pass real values once frame is passed here and stats buffer passing >> - * is changed. >> + * \todo Pass real bufferId once stats buffer passing is changed. >> */ >> - stats_->finishFrame(0, 0); >> + stats_->finishFrame(frame, 0); >> outputBufferReady.emit(output); >> inputBufferReady.emit(input); >> } >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index 8237a64b..2c47e7c6 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -36,7 +36,7 @@ public: >> std::vector<PixelFormat> formats(PixelFormat input); >> std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); >> - void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params); >> + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); >> SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); >> /** >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index a3855568..c5db45ae 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -14,6 +14,7 @@ >> #include <unistd.h> >> #include <libcamera/formats.h> >> +#include <libcamera/request.h> > > > Is this needed? I can't see that it is, at least in this patch? It doesn't seem to be needed at all, I'll remove it. > Otherwise; > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > >> #include <libcamera/stream.h> >> #include "libcamera/internal/ipa_manager.h" >> @@ -278,12 +279,13 @@ int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count, >> /** >> * \brief Queue buffers to Software ISP >> + * \param[in] frame The frame number >> * \param[in] input The input framebuffer >> * \param[in] outputs The container holding the output stream pointers and >> * their respective frame buffer outputs >> * \return 0 on success, a negative errno on failure >> */ >> -int SoftwareIsp::queueBuffers(FrameBuffer *input, >> +int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, >> const std::map<const Stream *, FrameBuffer *> &outputs) >> { >> /* >> @@ -301,7 +303,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, >> } >> for (auto iter = outputs.begin(); iter != outputs.end(); iter++) >> - process(input, iter->second); >> + process(frame, input, iter->second); >> return 0; >> } >> @@ -333,13 +335,14 @@ void SoftwareIsp::stop() >> /** >> * \brief Passes the input framebuffer to the ISP worker to process >> + * \param[in] frame The frame number >> * \param[in] input The input framebuffer >> * \param[out] output The framebuffer to write the processed frame to >> */ >> -void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output) >> +void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) >> { >> debayer_->invokeMethod(&DebayerCpu::process, >> - ConnectionTypeQueued, input, output, debayerParams_); >> + ConnectionTypeQueued, frame, input, output, debayerParams_); >> } >> void SoftwareIsp::saveIspParams()
diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index 3602bce8..3a84418e 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -73,10 +73,10 @@ public: int start(); void stop(); - int queueBuffers(FrameBuffer *input, + int queueBuffers(uint32_t frame, FrameBuffer *input, const std::map<const Stream *, FrameBuffer *> &outputs); - void process(FrameBuffer *input, FrameBuffer *output); + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output); Signal<FrameBuffer *> inputBufferReady; Signal<FrameBuffer *> outputBufferReady; diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 48a568da..ebec592a 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -865,7 +865,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) if (converter_) converter_->queueBuffers(buffer, conversionQueue_.front()); else - swIsp_->queueBuffers(buffer, conversionQueue_.front()); + /* + * request->sequence() cannot be retrieved from `buffer' inside + * queueBuffers because unique_ptr's make buffer->request() invalid + * already here. + */ + swIsp_->queueBuffers(request->sequence(), buffer, + conversionQueue_.front()); conversionQueue_.pop(); return; diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index db26c380..f0b83261 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -94,8 +94,9 @@ Debayer::~Debayer() */ /** - * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) + * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) * \brief Process the bayer data into the requested format + * \param[in] frame The frame number * \param[in] input The input buffer * \param[in] output The output buffer * \param[in] params The parameters to be used in debayering diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index c151fe5d..d7ca060d 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -40,7 +40,7 @@ public: virtual std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; - virtual void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; + virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 2a2e7edb..f7b3a7d1 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -724,7 +724,7 @@ static inline int64_t timeDiff(timespec &after, timespec &before) (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; } -void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) +void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) { timespec frameStartTime; @@ -778,12 +778,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams } /* - * Frame and buffer ids are currently not used, so pass zeros as parameters. + * Buffer ids are currently not used, so pass zeros as its parameter. * - * \todo Pass real values once frame is passed here and stats buffer passing - * is changed. + * \todo Pass real bufferId once stats buffer passing is changed. */ - stats_->finishFrame(0, 0); + stats_->finishFrame(frame, 0); outputBufferReady.emit(output); inputBufferReady.emit(input); } diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 8237a64b..2c47e7c6 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -36,7 +36,7 @@ public: std::vector<PixelFormat> formats(PixelFormat input); std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); - void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params); + void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); /** diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index a3855568..c5db45ae 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -14,6 +14,7 @@ #include <unistd.h> #include <libcamera/formats.h> +#include <libcamera/request.h> #include <libcamera/stream.h> #include "libcamera/internal/ipa_manager.h" @@ -278,12 +279,13 @@ int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count, /** * \brief Queue buffers to Software ISP + * \param[in] frame The frame number * \param[in] input The input framebuffer * \param[in] outputs The container holding the output stream pointers and * their respective frame buffer outputs * \return 0 on success, a negative errno on failure */ -int SoftwareIsp::queueBuffers(FrameBuffer *input, +int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, const std::map<const Stream *, FrameBuffer *> &outputs) { /* @@ -301,7 +303,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, } for (auto iter = outputs.begin(); iter != outputs.end(); iter++) - process(input, iter->second); + process(frame, input, iter->second); return 0; } @@ -333,13 +335,14 @@ void SoftwareIsp::stop() /** * \brief Passes the input framebuffer to the ISP worker to process + * \param[in] frame The frame number * \param[in] input The input framebuffer * \param[out] output The framebuffer to write the processed frame to */ -void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output) +void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) { debayer_->invokeMethod(&DebayerCpu::process, - ConnectionTypeQueued, input, output, debayerParams_); + ConnectionTypeQueued, frame, input, output, debayerParams_); } void SoftwareIsp::saveIspParams()
A previous preparation patch implemented passing frame ids to stats processing but without actual meaningful frame id value passed there. This patch extends that by actually providing the frame id and passing it through to the stats processor. The frame id is taken from the request sequence number, the same as in hardware pipelines. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../libcamera/internal/software_isp/software_isp.h | 4 ++-- src/libcamera/pipeline/simple/simple.cpp | 8 +++++++- src/libcamera/software_isp/debayer.cpp | 3 ++- src/libcamera/software_isp/debayer.h | 2 +- src/libcamera/software_isp/debayer_cpu.cpp | 9 ++++----- src/libcamera/software_isp/debayer_cpu.h | 2 +- src/libcamera/software_isp/software_isp.cpp | 11 +++++++---- 7 files changed, 24 insertions(+), 15 deletions(-)