[libcamera-devel] [PATCH] qcam: Fix ViewFinder memory leak

[Jacopo Mondi]

Hi Kieran,

On Tue, Sep 24, 2019 at 01:00:58PM +0100, Kieran Bingham wrote:
> Hi Laurent,
>
> On 24/09/2019 12:54, Laurent Pinchart wrote:
> > Hello,
> >
> > On Tue, Sep 24, 2019 at 12:51:32PM +0100, Kieran Bingham wrote:
> >> On 24/09/2019 09:16, Jacopo Mondi wrote:
> >>> On Mon, Sep 23, 2019 at 02:22:29PM +0100, Kieran Bingham wrote:
> >>>> When setting the format on the ViewFinder, a new image_ is allocated.
> >>>> Any change in format deletes the existing allocation, but it is not
> >>>> cleaned up on shutdown:
> >>>>
> >>>> Direct leak of 32 byte(s) in 1 object(s) allocated from:
> >>>>     #0 0x7f0bf8a7e17f in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10e17f)
> >>>>     #1 0x564c7205f7b0 in ViewFinder::setFormat(unsigned int, unsigned int, unsigned int) ../src/qcam/viewfinder.cpp:43
> >>>>     #2 0x564c71fec467 in MainWindow::startCapture() ../src/qcam/main_window.cpp:152
> >>>>     #3 0x564c71fe6c1a in MainWindow::MainWindow(libcamera::CameraManager*, OptionsParser::Options const&) ../src/qcam/main_window.cpp:40
> >>>>     #4 0x564c71fdf133 in main ../src/qcam/main.cpp:76
> >>>>     #5 0x7f0bf5944b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a)
> >>>>
> >>>> Provide a ViewFinder destructor, and delete the allocation as
> >>>> appropriate.
> >>>
> >>> Good catch, but I don't see where the viewfinder gets destroyed in
> >>> main_window.cpp. How does the newly added destructor gets called?
> >>
> >> Intriguing, the addition of the destructor made the LeakSanitizer
> >> warning go away, however I believe you are right. There is no explicit
> >> call to delete the ViewFinder which is created as part of the MainWindow.
> >>
> >> I'll add this, and respin a v2.
> >
> > No need to, https://doc.qt.io/qt-5/qobject.html#dtor.QObject

With the trick pointed out by Laurent clarifying my concerns:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j
>
>
> Aha - ok so because it's constructed with 'this' passed in the
> destruction is linked:
>         viewfinder_ = new ViewFinder(this);
>
> That explains why just cleaning up the QImage leak was enough.
>
> I'll stick with V1 then.
>
> Regards
>
> Kieran
>
>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> ---
> >>>>  src/qcam/viewfinder.cpp | 5 +++++
> >>>>  src/qcam/viewfinder.h   | 1 +
> >>>>  2 files changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> >>>> index 224a227ddd5b..98a8ab68e5f6 100644
> >>>> --- a/src/qcam/viewfinder.cpp
> >>>> +++ b/src/qcam/viewfinder.cpp
> >>>> @@ -16,6 +16,11 @@ ViewFinder::ViewFinder(QWidget *parent)
> >>>>  {
> >>>>  }
> >>>>
> >>>> +ViewFinder::~ViewFinder()
> >>>> +{
> >>>> +	delete image_;
> >>>> +}
> >>>> +
> >>>>  void ViewFinder::display(const unsigned char *raw, size_t size)
> >>>>  {
> >>>>  	converter_.convert(raw, size, image_);
> >>>> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> >>>> index c9ca98913e05..33bdb1460f84 100644
> >>>> --- a/src/qcam/viewfinder.h
> >>>> +++ b/src/qcam/viewfinder.h
> >>>> @@ -17,6 +17,7 @@ class ViewFinder : public QLabel
> >>>>  {
> >>>>  public:
> >>>>  	ViewFinder(QWidget *parent);
> >>>> +	~ViewFinder();
> >>>>
> >>>>  	int setFormat(unsigned int format, unsigned int width,
> >>>>  		      unsigned int height);
> >
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190924/7c36649c/attachment.sig>