[libcamera-devel] [PATCH] libcamera: camera: Optimize camera deletion

[Umang Jain]

Hi Laurent,

On 9/8/20 7:41 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Tue, Sep 08, 2020 at 07:28:04PM +0530, Umang Jain wrote:
>> On 9/8/20 1:24 PM, Laurent Pinchart wrote:
>>> In most cases the last reference to a Camera instance will be the one
>>> held by the CameraManager. That reference gets released when the
>>> CameraManager thread cleans up, just before it stops. There's no need to
>>> delete the camera with deleteLater() in that case.
>>>
>>> To optimize this case, use deleteLater() only when the camera gets
>>> deleted from a different thread, and delete is synchronously otherwise.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>> Umang, would you be able to test this patch with the Android HAL ?
>> I agree for the optimization, but do you have any specific use case in
>> mind. I tested it briefly with cros_camera_service cli to test both `if`
>> branches on hot-unplug but as you know my android build/test setup is
>> fairly limited with UVC. deleteLater() was brought in, when a currently
>> streaming camera in an app, is hot-unplugged from the system. Although I
>> am confident that this patch will hold correctly in such a situation as
>> well.
> I expect deleteLater() to be called in the hot-unplug case, and the
> regular delete when the camera manager is stopped, without unplug.
Yes, regular delete when camera manager is stopped. It is working as 
expected.
>
>> Reviewed-by: Umang Jain <email at uajain.com>
>>
>>>    src/libcamera/camera.cpp | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 4a9c19c33cfb..ae16a64a3b44 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -16,6 +16,7 @@
>>>    
>>>    #include "libcamera/internal/log.h"
>>>    #include "libcamera/internal/pipeline_handler.h"
>>> +#include "libcamera/internal/thread.h"
>>>    #include "libcamera/internal/utils.h"
>>>    
>>>    /**
>>> @@ -470,7 +471,10 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
>>>    	struct Deleter : std::default_delete<Camera> {
>>>    		void operator()(Camera *camera)
>>>    		{
>>> -			camera->deleteLater();
>>> +			if (Thread::current() == camera->thread())
>>> +				delete camera;
>>> +			else
>>> +				camera->deleteLater();
>>>    		}
>>>    	};
>>>