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

[Kieran Bingham]

Hi Laurent,

On 24/03/2021 15:38, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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.
> 
>> 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
--
Kieran