[libcamera-devel] [PATCH v2 3/8] libcamera: buffer: Initialise status

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 12, 2021 at 05:47:22AM +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 Error as
> the metadata associated with that frame will not be valid.

Shouldn't it be Cancelled then ?

 * \var FrameMetadata::FrameError
 * An error occurred during capture of the frame. The frame data may be partly
 * or fully invalid. The sequence and timestamp fields of the FrameMetadata
 * structure is valid, the other fields may be invalid.
 * \var FrameMetadata::FrameCancelled
 * Capture stopped before the frame completed. The frame data is not valid. All
 * fields of the FrameMetadata structure but the status field are invalid.

> Initialise the FrameMetadata to FrameError to prevent uninitialised access.
> Furthermore, swap the Enum values of the Status such that the first
> state represents the initial Error state.

Now that the status is initialized by default, is this needed ? I'm not
opposed to the change, just trying to see if it has any functional
impact.

> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/buffer.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 302fe3d3e86b..3f5d0f1b6363 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -19,8 +19,8 @@ class Request;
>  
>  struct FrameMetadata {
>  	enum Status {
> -		FrameSuccess,
>  		FrameError,
> +		FrameSuccess,
>  		FrameCancelled,
>  	};
>  
> @@ -28,7 +28,7 @@ struct FrameMetadata {
>  		unsigned int bytesused;
>  	};
>  
> -	Status status;
> +	Status status = FrameError;

This will achieve the stated goal when a buffer is first allocated and
queued. After the buffer completes, its status will be FrameSuccess
(assuming capture completed successfully). When the buffer is then
requeued as part of a new request, it will reach the pipeline handler
with state set to FrameSuccess. Wouldn't it be better to set the state
explicitly in Camera::queueRequest() instead ?

Given that the pipeline handlers are supposed to set the buffer state
before completing it, I wonder if this change could hide other pipeline
handler issues. Another option would be to add a fourth state
(FramePending for instance, to mimic RequestPending), set the buffer
state to FramePending in Camera::queueRequest(), and add an
ASSERT(status != FramePending) in PipelineHandler::completeBuffer().
That way we would catch incorrect pipeline handler behaviour right away.

>  	unsigned int sequence;
>  	uint64_t timestamp;
>  	std::vector<Plane> planes;

-- 
Regards,

Laurent Pinchart