[libcamera-devel] [PATCH] pipeline: rpi: always initialize the embedded buffer in tryRunPipeline

[Laurent Pinchart]

Hi Elias,

Thank you for the patch.

On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:
> Vc4CameraData::findMatchBuffers may return successfully without setting
> the embedded buffer. Make sure to initialize the buffer and id to avoid
> accessing garbage data.

How so ? The function starts with

	if (bayerQueue_.empty())
	        return false;

	/*
	 * Find the embedded data buffer with a matching timestamp to pass to
	 * the IPA. Any embedded buffers with a timestamp lower than the
	 * current bayer buffer will be removed and re-queued to the driver.
	 */
	uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;
	embeddedBuffer = nullptr;

so I don't see how it can leave embeddedBuffer unset if it returns
successfully.

> Without this change, libcamera v0.2.0 usually crashes with an assertion
> error:
> 
>  ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp()

I've seen this indeed, and will test your patch. Maybe it's due to the
second change, where you set params.buffers.embedded to 0 ?

> Signed-off-by: Elias Naur <mail at eliasnaur.com>
> ---
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 26102ea7..d76389f3 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)
>  
>  void Vc4CameraData::tryRunPipeline()
>  {
> -	FrameBuffer *embeddedBuffer;
> +	FrameBuffer *embeddedBuffer = nullptr;
>  	BayerFrame bayerFrame;
>  
>  	/* If any of our request or buffer queues are empty, we cannot proceed. */
> @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()
>  	params.requestControls = request->controls();
>  	params.ipaContext = request->sequence();
>  	params.delayContext = bayerFrame.delayContext;
> +	params.buffers.embedded = 0;
>  
>  	if (embeddedBuffer) {
>  		unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);

-- 
Regards,

Laurent Pinchart