[libcamera-devel] [PATCH v2 3/8] libcamera: buffer: Initialise status
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Mar 13 23:47:13 CET 2021
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
More information about the libcamera-devel
mailing list