[PATCH v3 12/23] libcamera: software_isp: Track and pass frame ids
Milan Zamazal
mzamazal at redhat.com
Mon Aug 12 18:28:06 CEST 2024
Hi Dan,
Dan Scally <dan.scally at 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 at 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()
More information about the libcamera-devel
mailing list