[PATCH 08/19] libcamera: software_isp: Track and pass frame ids
Milan Zamazal
mzamazal at redhat.com
Wed Jul 3 19:03:10 CEST 2024
Hi Umang,
thank you for review.
Umang Jain <umang.jain at 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 at 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()
More information about the libcamera-devel
mailing list