[PATCH 2/5] pipeline_handler: Call releaseDevice() before unlocking media devices

[Hans de Goede]

Hi,

On 8/25/24 2:27 AM, Laurent Pinchart wrote:

> The commit message doesn't really explain why the change is needed.
> Could you elaborate, maybe providing an example of incorrect behaviour
> of a pipeline handler implementation with the existing calling
> convention ?

This change is mostly to make this function do the releaseDevice() +
unlockMediaDevices() in opposite order of acquire() once acquireDevice()
is added.

For acquireDevice() we clearly only want to call that after having
successfully locked all media devices.

And then the logical (mirrored) approach on release() is to first
undo the acquireDevice() and then unlock the media devices.

I'll move it to be after the patch adding acquireDevice() and modify
the commit message.

Regards,

Hans



> 
>> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:
>>
>>> It is better / more logical to call releaseDevice() before unlocking
>>> the devices. ATM the only pipeline handler implementing releaseDevice()
> 
> s/ATM/At the moment/
> 
>>> is the rpi pipeline handler which releases buffers from its releaseDevice()
>>> implementation.
>>>
>>> Releasing buffers before unlocking the media devices is ok to do
>>> and arguably it is better to release the buffers before unlocking.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>>> ---
>>>  src/libcamera/pipeline_handler.cpp | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline_handler.cpp
>>> b/src/libcamera/pipeline_handler.cpp
>>> index a20cd27d..1fc22d6a 100644
>>> --- a/src/libcamera/pipeline_handler.cpp
>>> +++ b/src/libcamera/pipeline_handler.cpp
>>> @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)
>>>
>>>         ASSERT(useCount_);
>>>
>>> +       releaseDevice(camera);
>>> +
>>>         if (useCount_ == 1)
>>>                 unlockMediaDevices();
>>>
>>> -       releaseDevice(camera);
>>> -
>>>         --useCount_;
>>>  }
>>>
>