[libcamera-devel] [PATCH] qcam: Fix camera reference leak on hot-unplug

[Laurent Pinchart]

Hi Umang,

On Mon, Jul 27, 2020 at 03:38:00PM +0000, Umang Jain wrote:
> On 7/27/20 8:35 PM, Laurent Pinchart wrote:
> > On Mon, Jul 27, 2020 at 12:32:59PM +0000, Umang Jain wrote:
> >> If the currently streaming camera is hot-unplugged,
> >> a camera reference was still held by MainWindow::camera_,
> >> preventing it to be destructed, until qcam window is
> >> closed. Plug the leak in the hot-unplug handler itself.
> >>
> >> Signed-off-by: Umang Jain <email at uajain.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Does this fix all the shared pointer refcount issues you've been chasing
> > ?
>
> The refcount issues are sorted here. I spent extensive time on it (both for hotplug
> and un-plug refcount).
> 
> However, the other issue I am chasing (earlier believed was caused by refcount issue)
> is still present - which primarily deals with event loop and how event-notifiers are processed.
> 
> I can still trigger ASSERT(iter != notifiers_.end()); in EventDispatcherPoll::processNotifiers(),
> on hot-unplugging a currently streaming camera. This is probably due to the fact that, when
> destroying a Camera object, it always destroys V4L2VideoDevice() - which in turn, "delete"s
> two EventNotifiers in it's destructor() and then processNotifiers() isn't aware about the deletion
> and still trying to find a EventNotifier with concerned pollfd and fails  :-(

The issue is that the camera object is meant to be destroyed in the
thread it belongs to, as explained in a previous e-mail (see [1]). Is
that something you can try fixing, with the pointers in that e-mail, or
would you like to discuss it further first ?

[1] https://lists.libcamera.org/pipermail/libcamera-devel/2020-July/010951.html

> >> ---
> >>   src/qcam/main_window.cpp | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> >> index 7bc1360..c8298ec 100644
> >> --- a/src/qcam/main_window.cpp
> >> +++ b/src/qcam/main_window.cpp
> >> @@ -587,6 +587,8 @@ void MainWindow::processHotplug(HotplugEvent *e)
> >>   		/* Check if the currently-streaming camera is removed. */
> >>   		if (camera == camera_.get()) {
> >>   			toggleCapture(false);
> >> +			camera_->release();
> >> +			camera_.reset();
> >>   			cameraCombo_->setCurrentIndex(0);
> >>   		}
> >>   

-- 
Regards,

Laurent Pinchart