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

[Kieran Bingham]

Hi Laurent,

On 08/09/2020 13:30, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tue, Sep 08, 2020 at 01:28:07PM +0100, Kieran Bingham wrote:
>> On 08/09/2020 08:54, 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 ?
>>>
>>>  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();
>>
>> Shouldn't/Couldn't this logic be handled in Object::deleteLater() directly?
> 
> I've considered that, but there can be valid use cases for deleting an
> object from its thread when control returns to the event loop. For
> instance the decision to delete the object could be located at a point
> where functions up the call stack will still access the object. This
> could be a sign of bad software design overall, but I think there are
> valid use cases.

Well, this is the only current user - so it's hard to compare other
use-cases, or whether they are valid or not yet

But adding specific code to handle a case which is specifically only
used this way feels a bit odd.

Anyway, Didn't this fix some issue in particular (I can't see anything
mentioned in the commit message though)? so lets just get it in if you
need it.



>>>  		}
>>>  	};
>>>  
> 

-- 
Regards
--
Kieran