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

[Kieran Bingham]

Hi Laurent,

On 15/03/2021 23:24, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Mar 15, 2021 at 12:47:26PM +0000, Kieran Bingham wrote:
>> On 13/03/2021 22:47, Laurent Pinchart wrote:
>>> 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.
> 
> Sorry, I wasn't very precise, my question was related to swapping the
> enum values only.


Yes, I realised that. There is no expected functional change in swapping
the order *as long as* the FrameMetadata is always correctly initialised.

My statement above was referring to the fact that by ordering the states
as FrameError first, it leads towards mirroring the expected states.

I.e. Frames start in FrameError, and move to FrameSuccess or FrameCancelled.

If FrameSuccess is the first state, it implies that frames start
successfully, and move to either error or cancelled...

That's the reason for reordering the states.

Maybe that is different if they should start in FrameCancelled. I'll
think about that more when I get back to reworking for v2.



--
Kieran


> 
>>>> 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.
> 
> We could do that. I'm not sure if telling "new" and "pending" apart
> would really be useful though, as the new state will never be seen by
> libcamera (the request state will be set to pending by
> Camera::queueRequest()). It would thus be mostly be a debugging aid for
> applications. That doesn't make it worthless, but I haven't thought
> enough about common bug patterns on the application side to tell if this
> is what applications will need.


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

-- 
Regards
--
Kieran