[libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise status

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Mar 24, 2021 at 03:01:22PM +0000, Kieran Bingham wrote:
> Buffers queued to a pipeline handler may not be yet queued to a device
> when the request is cancelled.
> 
> This can lead to the FrameMetadata having never been explicitly set by
> an underlying V4L2 device.
> 
> The status field on this is used to check the state of the buffer to
> determine if it was correctly filled, or if it was cancelled.
> 
> In the event that the buffer is not used, it must be marked as Cancelled
> as the metadata associated with that frame will not be valid.
> 
> Initialise the FrameMetadata to FrameCancelled to prevent uninitialised access.
> Furthermore, re-initialise the metadata to this state when the buffers
> are re-used, or added to a Request to ensure that they are in the
> correct state in the event that they do not get used at all.

I wonder if this scheme could be a bit fragile. We're now essentially
telling pipeline handlers that they don't need to take care of marking
the buffer status for cancelled requests if the buffers haven't been
queued to the device. This means they could possibly overlook some other
important processing needed on buffer. There's no such thing at the
moment, time will tell if there will be in the future, in which case we
may want to reconsider this.

> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> v3:
>  - Initialise as FrameCancelled
>  - Re-initialise to FrameCancelled when buffers are re-used
>  - Do not re-order the enum
> 
>  include/libcamera/buffer.h | 2 +-
>  src/libcamera/request.cpp  | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 302fe3d3e86b..f6673e2f6eda 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -28,7 +28,7 @@ struct FrameMetadata {
>  		unsigned int bytesused;
>  	};
>  
> -	Status status;
> +	Status status = FrameCancelled;

This isn't strictly needed given the code below, but it's nice to be
consistent. Maybe we should rename it to FrameInvalid or something along
those lines later, to reflect the fact that it's not just about
cancelled requests now.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	unsigned int sequence;
>  	uint64_t timestamp;
>  	std::vector<Plane> planes;
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 0071667e4a2c..9ea6ed09446b 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -118,8 +118,12 @@ void Request::reuse(ReuseFlag flags)
>  	pending_.clear();
>  	if (flags & ReuseBuffers) {
>  		for (auto pair : bufferMap_) {
> -			pair.second->request_ = this;
> -			pending_.insert(pair.second);
> +			FrameBuffer *buffer = pair.second;
> +
> +			buffer->metadata_.status = FrameMetadata::Status::FrameCancelled;
> +			buffer->request_ = this;
> +
> +			pending_.insert(buffer);
>  		}
>  	} else {
>  		bufferMap_.clear();
> @@ -188,6 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  	}
>  
>  	buffer->request_ = this;
> +	buffer->metadata_.status = FrameMetadata::Status::FrameCancelled;
>  	pending_.insert(buffer);
>  	bufferMap_[stream] = buffer;
>  

-- 
Regards,

Laurent Pinchart