[PATCH v1] libcamera: request: addBuffer(): Do not destroy fence on failure

[Barnabás Pőcze]

Hi


2025. 03. 03. 23:26 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Mon, Mar 03, 2025 at 04:16:34PM +0100, Barnabás Pőcze wrote:
>> Take the unique pointer to the `Fence` object by rvalue reference
>> so that it is not destroyed if the function returns an error code
>> and does not take ownership of the unique pointer.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
>> ---
>>   include/libcamera/request.h | 2 +-
>>   src/libcamera/request.cpp   | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>> index e214a9d13..0c5939f7b 100644
>> --- a/include/libcamera/request.h
>> +++ b/include/libcamera/request.h
>> @@ -53,7 +53,7 @@ public:
>>   	ControlList &metadata() { return *metadata_; }
>>   	const BufferMap &buffers() const { return bufferMap_; }
>>   	int addBuffer(const Stream *stream, FrameBuffer *buffer,
>> -		      std::unique_ptr<Fence> fence = nullptr);
>> +		      std::unique_ptr<Fence> &&fence = {});
> 
> There's one caller in src/android/camera_device.cpp that passes nullptr
> as the third argument. Could you fix that ?

It does not cause any issues, but I'll add a separate commit to change it.


> 
> The other caller in the same file creates the unique_ptr right before
> calling the function, so the fence will be destroyed anyway. I suppose
> that's fine for now.

There is no change in behaviour in case of success, and in case of failure
there is minimal difference in most cases. No in-tree users are affected
as far as I could tell.


> 
> Should the unit test also be extended to validate the new behaviour ?

I can add some extra checks to `test/fence.cpp` if that's fine?


> The function documentation should also be updated.

ACK


Regards,
Barnabás Pőcze


> 
>>   	FrameBuffer *findBuffer(const Stream *stream) const;
>>   
>>   	uint32_t sequence() const;
>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>> index b206ac132..7d02f09fd 100644
>> --- a/src/libcamera/request.cpp
>> +++ b/src/libcamera/request.cpp
>> @@ -468,7 +468,7 @@ void Request::reuse(ReuseFlag flags)
>>    * \retval -EINVAL The buffer does not reference a valid Stream
>>    */
>>   int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,
>> -		       std::unique_ptr<Fence> fence)
>> +		       std::unique_ptr<Fence> &&fence)
>>   {
>>   	if (!stream) {
>>   		LOG(Request, Error) << "Invalid stream reference";
>