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

[Laurent Pinchart]

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.

> >  		}
> >  	};
> >  

-- 
Regards,

Laurent Pinchart