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

[Kieran Bingham]

Hi Laurent,

On 13/03/2021 22:47, Laurent Pinchart wrote:
> 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.

Maybe not, but my aim was to ensure/convey that buffers which are
freshly allocated are not considered as Valid.


>> 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 ?

Yes, I like the sound of that better. It ensures they always get set.
(If it's never queued, well then we don't care about it).



> 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.

Interesting, that might indeed be worthwhile.

I was also contemplating extending the Request states too.

We use Pending from 'new' and 'queued into the pipeline handler'... I
wonder if that should be:

RequestNew (freshly allocated, owned by the application)
RequestPending (queued into the PH)
RequestComplete
RequestCancelled


So there might be the same to apply to buffer states too.


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

-- 
Regards
--
Kieran