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

[Laurent Pinchart]

Hi Kieran,

On Wed, Sep 16, 2020 at 12:11:58PM +0100, Kieran Bingham wrote:
> On 08/09/2020 13:30, Laurent Pinchart wrote:
> > 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

Sure. Regardless of the option we pick now, I'm sure we'll change it at
least twice ;-) As a default, I've thought that following the
deleteLater() API implemented by Qt wouldn't be a bad idea.

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

It's an optimization, it doesn't fix a functional issue.

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

-- 
Regards,

Laurent Pinchart