| Message ID | 20240717085444.289997-13-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan On 17/07/2024 09:54, 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? Your confusion is understandable, it's caused some confusion previously. My attempt to explain things (which draws from previous discussions we had around this): 1) A "request" is the "please capture an image" event queued to libcamera by the application - the sequence number increments by one for each request queued. 2) A "frame" in the context this patch was created for is an a confusing concept. When people say frame my understanding is probably something like "the buffer dequeued from the driver", and I think that naming variables "frame" in the IPA modules and pipeline handlers was intended to refer to the frame number reported by the drivers when a buffer is reported complete in VB2, but really what some of the IPA modules refer to as a frame is just a number with which to index the FCQueue and that VB2 number would not actually be appropriate. 3) A "buffer" here usually means one of the buffers that are allocated in the pipeline handler and mapped to the IPA module. In a hardware pipeline they're queued to the parameters or statistics video device and when they're dequeued the ID of the buffer is sent to the IPA module so that it knows which one of its mapped buffers to inspect for the new data. Using the request sequence number is right, but in my view, no reference should be made to "frame" in these functions. The variable should just be named "sequence" or "frame_sequence" or even "index" or something like that. > > 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 e7149909..2cb2fee8 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. > + */ I would just drop the comment to be honest; it's not unusual to use request->sequence() here so I don't think it needs justification. > + 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 4d101798..e2295f35 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 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 b48c9903..09e33361 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" > @@ -279,12 +280,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) > { > /* > @@ -302,7 +304,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; > } > @@ -334,13 +336,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, Dan Scally <dan.scally@ideasonboard.com> writes: > Hi Milan > > On 17/07/2024 09:54, 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? > > > Your confusion is understandable, it's caused some confusion previously. My attempt to explain things (which draws from > previous discussions we had around this): > > > 1) A "request" is the "please capture an image" event queued to libcamera by the application - the sequence number increments > by one for each request queued. > > 2) A "frame" in the context this patch was created for is an a confusing concept. When people say frame my understanding is > probably something like "the buffer dequeued from the driver", and I think that naming variables "frame" in the IPA modules and > pipeline handlers was intended to refer to the frame number reported by the drivers when a buffer is reported complete in VB2, > but really what some of the IPA modules refer to as a frame is just a number with which to index the FCQueue and that VB2 > number would not actually be appropriate. > > 3) A "buffer" here usually means one of the buffers that are allocated in the pipeline handler and mapped to the IPA module. In > a hardware pipeline they're queued to the parameters or statistics video device and when they're dequeued the ID of the buffer > is sent to the IPA module so that it knows which one of its mapped buffers to inspect for the new data. > > > Using the request sequence number is right, but in my view, no reference should be made to "frame" in these functions. The > variable should just be named "sequence" or "frame_sequence" or even "index" or something like that. Thank you for explanation, it makes sense to me now. >> 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 e7149909..2cb2fee8 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. >> + */ > I would just drop the comment to be honest; it's not unusual to use request->sequence() here so I don't think it needs > justification. I had added the comment because Umang had suggested to not pass request->sequence() when it can be retrieved from `buffer'. I had also thought it would be a good idea until I actually tried it and realized it segfaults. The fact that buffer->sequence() is not valid here anymore is not obvious and hence I added the comment to avoid repeating the confusion in future. >> + 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 4d101798..e2295f35 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 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 b48c9903..09e33361 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" >> @@ -279,12 +280,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) >> { >> /* >> @@ -302,7 +304,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; >> } >> @@ -334,13 +336,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 e7149909..2cb2fee8 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 4d101798..e2295f35 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 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 b48c9903..09e33361 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" @@ -279,12 +280,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) { /* @@ -302,7 +304,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; } @@ -334,13 +336,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> --- .../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(-)