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

[Barnabás Pőcze]

2025. 03. 04. 14:14 keltezéssel, Barnabás Pőcze írta:
> 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.

Done: https://gitlab.freedesktop.org/camera/libcamera/-/commit/633063e099ccb0618359f9ecde9b05852c34259d


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

Is the above fine or did you have something else in mind?


Regards,
Barnabás Pőcze

> 
> 
>> 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";
>>
>