[libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise status

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 24, 2021 at 04:25:07PM +0000, Kieran Bingham wrote:
> On 24/03/2021 15:54, Kieran Bingham wrote:
> > On 24/03/2021 15:38, Laurent Pinchart wrote:
> >> On Wed, Mar 24, 2021 at 03:01:22PM +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 Cancelled
> >>> as the metadata associated with that frame will not be valid.
> >>>
> >>> Initialise the FrameMetadata to FrameCancelled to prevent uninitialised access.
> >>> Furthermore, re-initialise the metadata to this state when the buffers
> >>> are re-used, or added to a Request to ensure that they are in the
> >>> correct state in the event that they do not get used at all.
> >>
> >> I wonder if this scheme could be a bit fragile. We're now essentially
> >> telling pipeline handlers that they don't need to take care of marking
> >> the buffer status for cancelled requests if the buffers haven't been
> >> queued to the device. This means they could possibly overlook some other
> >> important processing needed on buffer. There's no such thing at the
> >> moment, time will tell if there will be in the future, in which case we
> >> may want to reconsider this.
> 
> Indeed, I was trying to ensure that /all/ unused buffers were
> initialised to an invalid state so that they weren't incorrectly
> identified as valid (or that the unintialised status flag may be read by
> an applciation, which could be set to 0 meaning it's a good buffer).
> 
> Perhaps we should also do this:
> 
> > From d7e9bba52e6395c4669a69bd09450f99c842c078 Mon Sep 17 00:00:00 2001
> > From: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Date: Wed, 24 Mar 2021 16:21:46 +0000
> > Subject: [PATCH] libcamera: pipeline: ipu3: Cancel unused buffers
> > 
> > When the CIO2 returns a cancelled buffer, we will not queue buffers
> > to the IMGU.
> > 
> > These buffers should be explicitly marked as cancelled to ensure
> > the application knows there is no valid metadata or frame data
> > provided in the buffer.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index af05cb2940c8..b495ea0a5b98 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1265,8 +1265,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> >  
> >  	/* If the buffer is cancelled force a complete of the whole request. */
> >  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > -		for (auto it : request->buffers())
> > -			pipe_->completeBuffer(request, it.second);
> > +		for (auto it : request->buffers()) {
> > +			FrameBufffer *b = it.second;
> > +			b->metadata_.status = FrameMetadata::Status::FrameCancelled;
> > +			pipe_->completeBuffer(request, b);
> > +		}
> >  
> >  		frameInfos_.remove(info);
> >  		pipe_->completeRequest(request);

This looks good to me too. It has the advantage of not making us feel
safe erronously that the libcamera core will always provide a solution
for us.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

But I'm also OK with the original patch for the time being, you can pick
the one you like best (or the one selected by your favourite random
number generator :-)).

> But I still think the state should be initialised to something to make
> sure we don't find ourselves making choices on an unitialised state
> variable.

Fully agreed.

> >>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>
> >>> ---
> >>> v3:
> >>>  - Initialise as FrameCancelled
> >>>  - Re-initialise to FrameCancelled when buffers are re-used
> >>>  - Do not re-order the enum
> >>>
> >>>  include/libcamera/buffer.h | 2 +-
> >>>  src/libcamera/request.cpp  | 9 +++++++--
> >>>  2 files changed, 8 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> >>> index 302fe3d3e86b..f6673e2f6eda 100644
> >>> --- a/include/libcamera/buffer.h
> >>> +++ b/include/libcamera/buffer.h
> >>> @@ -28,7 +28,7 @@ struct FrameMetadata {
> >>>  		unsigned int bytesused;
> >>>  	};
> >>>  
> >>> -	Status status;
> >>> +	Status status = FrameCancelled;
> >>
> >> This isn't strictly needed given the code below, but it's nice to be
> >> consistent. Maybe we should rename it to FrameInvalid or something along
> >> those lines later, to reflect the fact that it's not just about
> >> cancelled requests now.
> > 
> > Indeed, now that it's set during addBuffer() this becomes at least a
> > little redundant.
> > 
> > I agree on the naming doesn't feel right any more either.
> > FrameInvalid might be appropriate, and suitable for both an unused, and
> > cancelled frame...
> > 
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>
> >>>  	unsigned int sequence;
> >>>  	uint64_t timestamp;
> >>>  	std::vector<Plane> planes;
> >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> >>> index 0071667e4a2c..9ea6ed09446b 100644
> >>> --- a/src/libcamera/request.cpp
> >>> +++ b/src/libcamera/request.cpp
> >>> @@ -118,8 +118,12 @@ void Request::reuse(ReuseFlag flags)
> >>>  	pending_.clear();
> >>>  	if (flags & ReuseBuffers) {
> >>>  		for (auto pair : bufferMap_) {
> >>> -			pair.second->request_ = this;
> >>> -			pending_.insert(pair.second);
> >>> +			FrameBuffer *buffer = pair.second;
> >>> +
> >>> +			buffer->metadata_.status = FrameMetadata::Status::FrameCancelled;
> >>> +			buffer->request_ = this;
> >>> +
> >>> +			pending_.insert(buffer);
> >>>  		}
> >>>  	} else {
> >>>  		bufferMap_.clear();
> >>> @@ -188,6 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
> >>>  	}
> >>>  
> >>>  	buffer->request_ = this;
> >>> +	buffer->metadata_.status = FrameMetadata::Status::FrameCancelled;
> >>>  	pending_.insert(buffer);
> >>>  	bufferMap_[stream] = buffer;
> >>>  

-- 
Regards,

Laurent Pinchart