[PATCH 08/19] libcamera: software_isp: Track and pass frame ids

[Umang Jain]

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

>   {
>   	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()