| Message ID | 20200727123247.20412-1-email@uajain.com |
|---|---|
| State | Accepted |
| Commit | c8edfe29c185dade3aa584363d91643755fe7e02 |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. 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@uajain.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Does this fix all the shared pointer refcount issues you've been chasing ? > --- > 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); > } >
Hi Laurent, Thanks for the review. On 7/27/20 8:35 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > 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@uajain.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 :-( > >> --- >> 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); >> } >>
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@uajain.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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); > >> } > >>
Hi Laurent, On 7/27/20 9:20 PM, Laurent Pinchart wrote: > 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@uajain.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 You were right all along. I will try to follow the pointers and come up with an implementation. > >>>> --- >>>> 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); >>>> } >>>>
Hi again, On 7/27/20 9:20 PM, Laurent Pinchart wrote: > 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@uajain.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 hmm, I figured that I need to call the destructor via Object::invokeMethod() with connection type ConnectionTypeQueued to ensure the destructor is called within it's own thread. Something on the lines: void Object::deleteLater() { this->invokeMethod(&(typeid(this).name()::customDeletorFunc), ConnectionTypeQueued); } and void className::customDeletorFunc() { ~className(); } which obviously does not compile for me. How would one go about calling a destructor(in this case Camera's) from it's one of the parent class(i.e. Object)? I am very cloudy around the syntax formation for this->invokeMethod() above. Any help is appreciated. > >>>> --- >>>> 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); >>>> } >>>>
Hi Umang, On Mon, Jul 27, 2020 at 08:20:40PM +0000, Umang Jain wrote: > On 7/27/20 9:20 PM, Laurent Pinchart wrote: > > 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@uajain.com> > >>> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 > > hmm, I figured that I need to call the destructor via > Object::invokeMethod() with > connection type ConnectionTypeQueued to ensure the destructor is called > within > it's own thread. Something on the lines: > > void Object::deleteLater() > { > this->invokeMethod(&(typeid(this).name()::customDeletorFunc), > ConnectionTypeQueued); You can drop the 'this->', calling a member function of an object from within the same object doesn't require explicitly accessing 'this'. > } > > and > > void className::customDeletorFunc() > { > ~className(); > } > > which obviously does not compile for me. > > How would one go about calling a destructor(in this case Camera's) from it's one > of the parent class(i.e. Object)? I am very cloudy around the syntax formation for > this->invokeMethod() above. Any help is appreciated. This indeed won't compile. The good news is that it can be simplified, fixing compilation at the same time :-) Your code, calling ~className(), would indeed call the destructor, but wouldn't free memory. Destruction of an object involves two steps, first the destructors are called (in order from the most derived class to the parent class) to perform cleanups specific to the object. Then, memory is freed. When you allocate an object on the stack with { Object o(...); ... } <-- 1 at point 1 the object becomes out of scope and is destroyed. The compiler generates code to call the destructors, and then simply updates the stack pointer to free the memory. When you allocate an object on the heap with Object *o = new Object(...); ... delete o; <-- 1 at point 1 the object is explicitly destroyed. The destructors are called, as in the stack case, but to free the memory, the compiler calls the delete operator (to be pedantic, a delete expression, formed by the 'delete' keyword followed by an expression, is compiled as an invocation of the destructor, followed by a call to the delete operator to free the memory - see [1] if you're interested in the details). Freeing memory is thus independent of the destructor, so calling the destructor manually isn't enough. The good news is that we don't need to do so manually. The Object class has a virtual destructor, so deleting an object using an Object pointer will call the destructor of the derived class. You don't need to get hold of a pointer to the derived class. You can thus simply implement void Object::customDeletorFunc() { delete this; } void Object::deleteLater() { invokeMethod(&Object::customDeletorFunc, ConnectionTypeQueued); } However, I think we can optimize the implementation a little bit to avoid going through the complex machinery of invokeMethod(). I would recommend adding a new DeferredDelete entry to the Message::Type enumeration, and simply posting the message with void Object::deleteLater() { postMessage(std::make_unique<Message>(Message::DeferredDelete)); } and to handle that, in Object::message(), add case Message::DeferredDelete: delete this; break; Finally, you'll have to wire that with a custom deleter when creating the camera shared pointer. Fortunately we have one already in Camera::create() :-) It should thus just be a matter of replacing struct Deleter : std::default_delete<Camera> { void operator()(Camera *camera) { delete camera; } }; with struct Deleter : std::default_delete<Camera> { void operator()(Camera *camera) { camera->deleteLater(); } }; and of course crossing fingers :-) [1] https://en.cppreference.com/w/cpp/language/delete > >>>> --- > >>>> 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); > >>>> } > >>>>
Hi Laurent, On 7/28/20 4:39 AM, Laurent Pinchart wrote: > Hi Umang, > > On Mon, Jul 27, 2020 at 08:20:40PM +0000, Umang Jain wrote: >> On 7/27/20 9:20 PM, Laurent Pinchart wrote: >>> 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@uajain.com> >>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 >> hmm, I figured that I need to call the destructor via >> Object::invokeMethod() with >> connection type ConnectionTypeQueued to ensure the destructor is called >> within >> it's own thread. Something on the lines: >> >> void Object::deleteLater() >> { >> this->invokeMethod(&(typeid(this).name()::customDeletorFunc), >> ConnectionTypeQueued); > You can drop the 'this->', calling a member function of an object from > within the same object doesn't require explicitly accessing 'this'. > >> } >> >> and >> >> void className::customDeletorFunc() >> { >> ~className(); >> } >> >> which obviously does not compile for me. >> >> How would one go about calling a destructor(in this case Camera's) from it's one >> of the parent class(i.e. Object)? I am very cloudy around the syntax formation for >> this->invokeMethod() above. Any help is appreciated. > This indeed won't compile. The good news is that it can be simplified, > fixing compilation at the same time :-) > > Your code, calling ~className(), would indeed call the destructor, but > wouldn't free memory. Destruction of an object involves two steps, first > the destructors are called (in order from the most derived class to the > parent class) to perform cleanups specific to the object. Then, memory > is freed. When you allocate an object on the stack with > > { > Object o(...); > ... > } <-- 1 > > at point 1 the object becomes out of scope and is destroyed. The > compiler generates code to call the destructors, and then simply updates > the stack pointer to free the memory. > > When you allocate an object on the heap with > > Object *o = new Object(...); > ... > delete o; <-- 1 > > at point 1 the object is explicitly destroyed. The destructors are > called, as in the stack case, but to free the memory, the compiler calls > the delete operator (to be pedantic, a delete expression, formed by the > 'delete' keyword followed by an expression, is compiled as an invocation > of the destructor, followed by a call to the delete operator to free the > memory - see [1] if you're interested in the details). Thanks for the explaination. I indeed was missing these basics concepts :( > > Freeing memory is thus independent of the destructor, so calling the > destructor manually isn't enough. The good news is that we don't need to > do so manually. The Object class has a virtual destructor, so deleting > an object using an Object pointer will call the destructor of the > derived class. You don't need to get hold of a pointer to the derived > class. You can thus simply implement > > void Object::customDeletorFunc() > { > delete this; > } > > void Object::deleteLater() > { > invokeMethod(&Object::customDeletorFunc, ConnectionTypeQueued); > } > > However, I think we can optimize the implementation a little bit to > avoid going through the complex machinery of invokeMethod(). I would > recommend adding a new DeferredDelete entry to the Message::Type > enumeration, and simply posting the message with > > void Object::deleteLater() > { > postMessage(std::make_unique<Message>(Message::DeferredDelete)); > } > > and to handle that, in Object::message(), add > > case Message::DeferredDelete: > delete this; > break; Yeah, this quite comes close to what Qt does for it's QObject::deleteLater(). It posts an event to defer delete. > > Finally, you'll have to wire that with a custom deleter when creating > the camera shared pointer. Fortunately we have one already in > Camera::create() :-) It should thus just be a matter of replacing > > struct Deleter : std::default_delete<Camera> { > void operator()(Camera *camera) > { > delete camera; > } > }; > > with > > struct Deleter : std::default_delete<Camera> { > void operator()(Camera *camera) > { > camera->deleteLater(); > } > }; > > and of course crossing fingers :-) Thanks for the input. This is really simplified now and does not look as scary as I thought initially :) I have pushed patches for review right now on the list. > > [1] https://en.cppreference.com/w/cpp/language/delete > >>>>>> --- >>>>>> 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); >>>>>> } >>>>>>
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); }
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@uajain.com> --- src/qcam/main_window.cpp | 2 ++ 1 file changed, 2 insertions(+)