| Message ID | 20240626072100.55497-9-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, On 26/06/24 12:50 pm, 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. > Dear reviewers: I'm confused even after looking at commit > 6084217cd3b52ba5677e3ca2de0e21008fdaa735. What's the relationship > between requests, buffers and frames? It's not 1:1:1 or is it? Could > you please provide some explanation that could be put here? > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/internal/software_isp/software_isp.h | 4 ++-- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 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 | 10 ++++++---- > 6 files changed, 15 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index 6a03b17f..7365b49a 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -72,10 +72,10 @@ public: > int start(); > void stop(); > > - int queueBuffers(FrameBuffer *input, > + int queueBuffers(uint32_t frame, FrameBuffer *input, > const std::map<unsigned int, 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 b1bf0d16..5cca94c3 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -864,7 +864,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > if (converter_) > converter_->queueBuffers(buffer, conversionQueue_.front()); > else > - swIsp_->queueBuffers(buffer, conversionQueue_.front()); > + swIsp_->queueBuffers(request->sequence(), buffer, conversionQueue_.front()); > > conversionQueue_.pop(); > return; > 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 1575cedb..c75b8967 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -731,7 +731,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; > > @@ -785,12 +785,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 1dac6435..6a9cb4c7 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 3fc4a64b..aa60fb5f 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -279,12 +279,13 @@ int SoftwareIsp::exportBuffers(unsigned int output, 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 indexes 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<unsigned int, FrameBuffer *> &outputs) I am not sure passing the frame here explicitly is the best idea. Instead one can get the sequence number Via `input` framebuffer in the function itself ? See FrameBuffer::request() API > { > unsigned int mask = 0; > @@ -308,7 +309,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, > mask |= 1 << index; > } > > - process(input, outputs.at(0)); > + process(frame, input, outputs.at(0)); > > return 0; > } > @@ -340,13 +341,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 Umang, thank you for review. Umang Jain <umang.jain@ideasonboard.com> writes: > Hi Milan, > > On 26/06/24 12:50 pm, 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. >> Dear reviewers: I'm confused even after looking at commit >> 6084217cd3b52ba5677e3ca2de0e21008fdaa735. What's the relationship >> between requests, buffers and frames? It's not 1:1:1 or is it? Could >> you please provide some explanation that could be put here? >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> include/libcamera/internal/software_isp/software_isp.h | 4 ++-- >> src/libcamera/pipeline/simple/simple.cpp | 2 +- >> 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 | 10 ++++++---- >> 6 files changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index 6a03b17f..7365b49a 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -72,10 +72,10 @@ public: >> int start(); >> void stop(); >> - int queueBuffers(FrameBuffer *input, >> + int queueBuffers(uint32_t frame, FrameBuffer *input, >> const std::map<unsigned int, 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 b1bf0d16..5cca94c3 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -864,7 +864,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) >> if (converter_) >> converter_->queueBuffers(buffer, conversionQueue_.front()); >> else >> - swIsp_->queueBuffers(buffer, conversionQueue_.front()); >> + swIsp_->queueBuffers(request->sequence(), buffer, conversionQueue_.front()); >> conversionQueue_.pop(); >> return; >> 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 1575cedb..c75b8967 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -731,7 +731,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; >> @@ -785,12 +785,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 1dac6435..6a9cb4c7 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 3fc4a64b..aa60fb5f 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -279,12 +279,13 @@ int SoftwareIsp::exportBuffers(unsigned int output, 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 indexes 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<unsigned int, FrameBuffer *> &outputs) > > I am not sure passing the frame here explicitly is the best idea. > > Instead one can get the sequence number Via `input` framebuffer in the function itself ? > > See FrameBuffer::request() API I tried it but it appeared it doesn't work. The problem is that the caller assigns buffer->request() to a local variable and some not very transparent unique_ptr machinery around invalidates the request in the FrameBuffer. Then trying to call input->request()->sequence() here hits a null pointer and segfaults. Maybe the calling function could be rearranged to avoid this but I think it's better not to walk on a thin ice here and to keep the things separated. >> { >> unsigned int mask = 0; >> @@ -308,7 +309,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, >> mask |= 1 << index; >> } >> - process(input, outputs.at(0)); >> + process(frame, input, outputs.at(0)); >> return 0; >> } >> @@ -340,13 +341,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 6a03b17f..7365b49a 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -72,10 +72,10 @@ public: int start(); void stop(); - int queueBuffers(FrameBuffer *input, + int queueBuffers(uint32_t frame, FrameBuffer *input, const std::map<unsigned int, 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 b1bf0d16..5cca94c3 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -864,7 +864,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) if (converter_) converter_->queueBuffers(buffer, conversionQueue_.front()); else - swIsp_->queueBuffers(buffer, conversionQueue_.front()); + swIsp_->queueBuffers(request->sequence(), buffer, conversionQueue_.front()); conversionQueue_.pop(); return; 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 1575cedb..c75b8967 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -731,7 +731,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; @@ -785,12 +785,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 1dac6435..6a9cb4c7 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 3fc4a64b..aa60fb5f 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -279,12 +279,13 @@ int SoftwareIsp::exportBuffers(unsigned int output, 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 indexes 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<unsigned int, FrameBuffer *> &outputs) { unsigned int mask = 0; @@ -308,7 +309,7 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input, mask |= 1 << index; } - process(input, outputs.at(0)); + process(frame, input, outputs.at(0)); return 0; } @@ -340,13 +341,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. Dear reviewers: I'm confused even after looking at commit 6084217cd3b52ba5677e3ca2de0e21008fdaa735. What's the relationship between requests, buffers and frames? It's not 1:1:1 or is it? Could you please provide some explanation that could be put here? Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- include/libcamera/internal/software_isp/software_isp.h | 4 ++-- src/libcamera/pipeline/simple/simple.cpp | 2 +- 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 | 10 ++++++---- 6 files changed, 15 insertions(+), 14 deletions(-)