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

[Laurent Pinchart]

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.

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

Laurent Pinchart