[libcamera-devel] qcam: Fix ViewFinder memory leak
diff mbox series

Message ID 20190923132229.17027-1-kieran.bingham@ideasonboard.com
State Accepted
Commit f0b2582c4961ce9e5b637a2a326aa59ebc905e52
Headers show
Series
  • [libcamera-devel] qcam: Fix ViewFinder memory leak
Related show

Commit Message

Kieran Bingham Sept. 23, 2019, 1:22 p.m. UTC 
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.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/qcam/viewfinder.cpp | 5 +++++
 src/qcam/viewfinder.h   | 1 +
 2 files changed, 6 insertions(+)

Comments

Kieran Bingham Sept. 23, 2019, 1:24 p.m. UTC | #1 
On 23/09/2019 14:22, 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.

Minor addition to the patch:

Fixes: 97e8b3a2eb32 ("qcam: Add Qt-based GUI application")

> Signed-off-by: Kieran Bingham <kieran.bingham@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);
>
Jacopo Mondi Sept. 24, 2019, 8:16 a.m. UTC | #2 
Hi Kieran,

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?

Thanks
   j

>
> Signed-off-by: Kieran Bingham <kieran.bingham@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);
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Sept. 24, 2019, 11:51 a.m. UTC | #3 
Hi Jacopo,

On 24/09/2019 09:16, Jacopo Mondi wrote:
> Hi Kieran,
> 
> 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.

--
Thanks

Kieran


> 
> Thanks
>    j
> 
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@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);
>> --
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Sept. 24, 2019, 11:54 a.m. UTC | #4 
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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >> Signed-off-by: Kieran Bingham <kieran.bingham@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);
Kieran Bingham Sept. 24, 2019, noon UTC | #5 
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


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@ideasonboard.com>
> 
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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);
>
Laurent Pinchart Sept. 24, 2019, 12:13 p.m. UTC | #6 
Hi Kieran,

On Tue, Sep 24, 2019 at 01:00:58PM +0100, Kieran Bingham wrote:
> On 24/09/2019 12:54, Laurent Pinchart wrote:
> > 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
> 
> 
> Aha - ok so because it's constructed with 'this' passed in the
> destruction is linked:
>         viewfinder_ = new ViewFinder(this);

I've thought about implementing similar for our Object class, but have
refrained to do so so far. I may revisit the topic later, depending on
if it would make the code more robust. Inputs are welcome :-)

> That explains why just cleaning up the QImage leak was enough.
> 
> I'll stick with V1 then.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@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);
Jacopo Mondi Sept. 24, 2019, 3:41 p.m. UTC | #7 
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@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@ideasonboard.com>
> >
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@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
Kieran Bingham Sept. 24, 2019, 4:36 p.m. UTC | #8 
Hi Jacopo, Laurent,

Pushed with your RB tags.

--
Kieran


On 24/09/2019 16:41, Jacopo Mondi wrote:
> 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@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@ideasonboard.com>
>>>
>>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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

Patch
diff mbox series 

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);