| Message ID | 20200908075406.14039-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 54557e25f2d478623839fe17f82700bb97fff0b2 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, 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@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? > } > }; > >
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@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. > > } > > }; > >
Hi Laurent, 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@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. Reviewed-by: Umang Jain <email@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(); > } > }; >
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@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. > Reviewed-by: Umang Jain <email@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(); > > } > > }; > >
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@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@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(); >>> } >>> }; >>>
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@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. >>> } >>> }; >>> >
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@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. > >>> } > >>> }; > >>>
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(); } };
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@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(-)