Recent

Author Topic: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;  (Read 10573 times)

mm7

  • Full Member
  • ***
  • Posts: 119
Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« on: December 24, 2015, 08:15:38 pm »
Graphical application uses lots of calls Canvas.Pixels[X,Y]:=aColor;
Memory grows constantly. Valgrind leak analysis shows that leaks originated from
procedure TQtWidgetSet.DCSetPixel [qtobject.inc]
Pen := QPen_create(QPainter_pen(Painter));

I suspect lots of pens are created and not released.

Lazarus 1.4.4
Linux Ubuntu 14.04
« Last Edit: December 24, 2015, 08:21:24 pm by mm7 »

zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #1 on: December 25, 2015, 12:17:28 pm »
Please open an issue about it and I'll fix it, also do not forget to attach example project.

zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #2 on: December 25, 2015, 12:44:14 pm »
ok, QPainter_destroy() is missing there ...

zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #3 on: December 25, 2015, 12:55:15 pm »
I've fixed that in trunk. Will be merged to 1.6.
If you want it fixed on your 1.4.XX then you must add QPen_destroy(Pen); after
finally
   QPainter_setPen(Painter, Pen);
   QPen_destroy(Pen); <---- add this
end;
in TQtWidgetSet.DCSetPixel() and rebuild lazarus.

mm7

  • Full Member
  • ***
  • Posts: 119
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #4 on: December 25, 2015, 08:45:12 pm »
Thank you zeljko!
I'll do this.

PS/ I am not familiar with QT graphics, but it seems to me it is not very efficient to create pen - destroy pen each call.
How about to define pen field on class level and then use in SetPixel?
Something like below.
Code: Pascal  [Select]
  1.  
  2. TQtWidgetSet = Class(TWidgetSet)          
  3. private
  4. ...
  5.     FPenForSetPixel: QPenH;
  6. ...
  7.  
  8. procedure TQtWidgetSet.DCSetPixel(CanvasHandle: HDC; X, Y: integer; AColor: TGraphicsColor);
  9. var
  10.   Color: TQColor;
  11.   ColorRef: TColorRef;
  12.   Painter: QPainterH;
  13.   Pen:QPenH;
  14. begin
  15.   if IsValidDC(CanvasHandle) then
  16.   begin
  17.     // WriteLn('TQtWidgetSet.DCSetPixel X=',X,' Y=',Y, ' AColor=',dbghex(AColor),' rgb ? ',dbgHex(ColorToRGB(AColor)));
  18.     Painter := TQtDeviceContext(CanvasHandle).Widget;
  19.     {Save current pen.Better save copy of pen instead of
  20.      using painter save/restore, or saved Pen in devicecontext which
  21.      may be null. Issue #27620}
  22.  
  23.     {Use global pen for SetPixel operation to avoid creation/destroying on each call.
  24.      Create if not exusts}
  25.     if FPenForSetPixel = nil then
  26.       begin
  27.       FPenForSetPixel := QPen_create();
  28.       QPen_setWidth(FPenForSetPixel,1);
  29.       end;
  30.     Pen:=QPainter_pen(Painter); //save current Pen
  31.     try
  32.       ColorRef := TColorRef(ColorToRGB(AColor));
  33.       QColor_fromRgb(@Color, Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  34.       QPen_setColor(FPenForSetPixel, @Color);
  35.       QPainter_setPen(Painter, FPenForSetPixel);
  36.       QPainter_drawPoint(Painter, X,Y);
  37.     finally
  38.       QPainter_setPen(Painter, Pen); //restore current pen
  39.     end;
  40.   end;
  41. end;  
  42.  

mm7

  • Full Member
  • ***
  • Posts: 119
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #5 on: December 25, 2015, 09:36:44 pm »
Another idea, why to use Pen?
Why not just simly use QImage_setPixel, similarly to DCGetPixel?

Code: Pascal  [Select]
  1. procedure TQtWidgetSet.DCSetPixel(CanvasHandle: HDC; X, Y: integer; AColor: TGraphicsColor);
  2. var
  3.   ColorRef: TColorRef;
  4.   Rgb:QRgb;
  5.   ColorH: QColorH;
  6. begin
  7.   if IsValidDC(CanvasHandle) then
  8.   begin
  9.     // WriteLn('TQtWidgetSet.DCSetPixel X=',X,' Y=',Y, ' AColor=',dbghex(AColor),' rgb ? ',dbgHex(ColorToRGB(AColor)));
  10.     ColorRef := TColorRef(ColorToRGB(AColor));
  11.     try
  12.       ColorH:=QColor_create(Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  13.       Rgb:=QColor_rgba(ColorH);
  14.       QImage_setPixel(TQtDeviceContext(CanvasHandle).vImage.Handle, X,Y, Rgb);
  15.     finally
  16.       QColor_destroy(ColorH);
  17.     end;
  18.   end;
  19. end;  
  20.  

it works 4 times faster.
« Last Edit: December 25, 2015, 09:38:38 pm by mm7 »

Septe

  • Jr. Member
  • **
  • Posts: 68
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #6 on: December 26, 2015, 03:07:22 am »
While that might be true that method works 4 times faster, it's a good thing he's bringing this up because bugs need to be squashed. ;)

zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #7 on: December 26, 2015, 09:42:06 am »
Another idea, why to use Pen?
Why not just simly use QImage_setPixel, similarly to DCGetPixel?


it works 4 times faster.

Because vImage can be nil.

zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #8 on: December 26, 2015, 09:43:10 am »
Thank you zeljko!
I'll do this.

PS/ I am not familiar with QT graphics, but it seems to me it is not very efficient to create pen - destroy pen each call.
How about to define pen field on class level and then use in SetPixel?
Something like below.
Code: Pascal  [Select]
  1.  
  2. TQtWidgetSet = Class(TWidgetSet)          
  3. private
  4. ...
  5.     FPenForSetPixel: QPenH;
  6. ...
  7.  
  8. procedure TQtWidgetSet.DCSetPixel(CanvasHandle: HDC; X, Y: integer; AColor: TGraphicsColor);
  9. var
  10.   Color: TQColor;
  11.   ColorRef: TColorRef;
  12.   Painter: QPainterH;
  13.   Pen:QPenH;
  14. begin
  15.   if IsValidDC(CanvasHandle) then
  16.   begin
  17.     // WriteLn('TQtWidgetSet.DCSetPixel X=',X,' Y=',Y, ' AColor=',dbghex(AColor),' rgb ? ',dbgHex(ColorToRGB(AColor)));
  18.     Painter := TQtDeviceContext(CanvasHandle).Widget;
  19.     {Save current pen.Better save copy of pen instead of
  20.      using painter save/restore, or saved Pen in devicecontext which
  21.      may be null. Issue #27620}
  22.  
  23.     {Use global pen for SetPixel operation to avoid creation/destroying on each call.
  24.      Create if not exusts}
  25.     if FPenForSetPixel = nil then
  26.       begin
  27.       FPenForSetPixel := QPen_create();
  28.       QPen_setWidth(FPenForSetPixel,1);
  29.       end;
  30.     Pen:=QPainter_pen(Painter); //save current Pen
  31.     try
  32.       ColorRef := TColorRef(ColorToRGB(AColor));
  33.       QColor_fromRgb(@Color, Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  34.       QPen_setColor(FPenForSetPixel, @Color);
  35.       QPainter_setPen(Painter, FPenForSetPixel);
  36.       QPainter_drawPoint(Painter, X,Y);
  37.     finally
  38.       QPainter_setPen(Painter, Pen); //restore current pen
  39.     end;
  40.   end;
  41. end;  
  42.  

Yes, it's possible to have cached pen object.

mm7

  • Full Member
  • ***
  • Posts: 119
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #9 on: December 26, 2015, 04:38:16 pm »
Another idea, why to use Pen?
Why not just simly use QImage_setPixel, similarly to DCGetPixel?
it works 4 times faster.
Because vImage can be nil.
It does not sound convincing. TQtWidgetSet.DCGetPixel uses QImage_pixel. It just checks if vImage is not nil.

Yes, I've forgot to check if vImage is not nil there in my variant of DCSetPixel.
Here it is now.
Code: Pascal  [Select]
  1. procedure TQtWidgetSet.DCSetPixel(CanvasHandle: HDC; X, Y: integer; AColor: TGraphicsColor);
  2. var
  3.   Color: TQColor;
  4.   ColorRef: TColorRef;
  5.   Rgb:QRgb;
  6.   ColorH: QColorH;
  7. begin
  8.   if IsValidDC(CanvasHandle) then
  9.   begin
  10.     // WriteLn('TQtWidgetSet.DCSetPixel X=',X,' Y=',Y, ' AColor=',dbghex(AColor),' rgb ? ',dbgHex(ColorToRGB(AColor)));
  11.     if (TQtDeviceContext(CanvasHandle).vImage = nil) then exit;
  12.     ColorRef := TColorRef(ColorToRGB(AColor));
  13.     try
  14.       ColorH:=QColor_create(Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  15.       Rgb:=QColor_rgba(ColorH);
  16.       QImage_setPixel(TQtDeviceContext(CanvasHandle).vImage.Handle, X,Y, Rgb);
  17.     finally
  18.       QColor_destroy(ColorH);
  19.     end;
  20.   end;
  21. end;
  22.  





zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #10 on: December 26, 2015, 05:35:29 pm »
Another idea, why to use Pen?
Why not just simly use QImage_setPixel, similarly to DCGetPixel?
it works 4 times faster.
Because vImage can be nil.
It does not sound convincing. TQtWidgetSet.DCGetPixel uses QImage_pixel. It just checks if vImage is not nil.

Yes, I've forgot to check if vImage is not nil there in my variant of DCSetPixel.
Here it is now.
Code: Pascal  [Select]
  1. procedure TQtWidgetSet.DCSetPixel(CanvasHandle: HDC; X, Y: integer; AColor: TGraphicsColor);
  2. var
  3.   Color: TQColor;
  4.   ColorRef: TColorRef;
  5.   Rgb:QRgb;
  6.   ColorH: QColorH;
  7. begin
  8.   if IsValidDC(CanvasHandle) then
  9.   begin
  10.     // WriteLn('TQtWidgetSet.DCSetPixel X=',X,' Y=',Y, ' AColor=',dbghex(AColor),' rgb ? ',dbgHex(ColorToRGB(AColor)));
  11.     if (TQtDeviceContext(CanvasHandle).vImage = nil) then exit;
  12.     ColorRef := TColorRef(ColorToRGB(AColor));
  13.     try
  14.       ColorH:=QColor_create(Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  15.       Rgb:=QColor_rgba(ColorH);
  16.       QImage_setPixel(TQtDeviceContext(CanvasHandle).vImage.Handle, X,Y, Rgb);
  17.     finally
  18.       QColor_destroy(ColorH);
  19.     end;
  20.   end;
  21. end;
  22.  

I've already commited that (see r51040 in trunk). There's also difference between device type (QWidget,QPixmap, QImage) ...

mm7

  • Full Member
  • ***
  • Posts: 119
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #11 on: December 26, 2015, 09:56:21 pm »
I've already commited that (see r51040 in trunk). There's also difference between device type (QWidget,QPixmap, QImage) ...
Thank you zeljko!

Following should be removed from var section.
Color: TQColor;

For QWidget and QPixmap I will add Painter code in SetPixel.
BTW, hashing of Pen does not work. Calling QPainter_setPen overrides previously "saved" pen taken by QPainter_pen.
Unfortunately I do not know a standard way to save/restore just Pen.
I am looking into documented QPainter_save / QPainter_restore. Or will return to create /destroy pen each time.
Whatever is less expensive...

mm7

  • Full Member
  • ***
  • Posts: 119
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #12 on: December 27, 2015, 01:25:41 am »
Finally I've managed GetPixel to work with other Canvas, like TForm.Canvas.
Code: Pascal  [Select]
  1. function TQtWidgetSet.DCGetPixel(CanvasHandle: HDC; X, Y: integer): TGraphicsColor;
  2. var
  3.   Color: QColorH;
  4.   Widget: QWidgetH;
  5.   Pixmap: QPixmapH;
  6.   Image: QImageH;
  7.   Painter: QPainterH;
  8.   Rgb: QRgb;
  9. begin
  10.   Result := clNone;
  11.  
  12.   if not IsValidDC(CanvasHandle) then Exit;
  13.  
  14.   if (TQtDeviceContext(CanvasHandle).vImage <> nil) then
  15.     begin
  16.       Color := QColor_create(QImage_pixel(TQtDeviceContext(CanvasHandle).vImage.Handle, X, Y));
  17.       Result := RGBToColor(QColor_red(Color), QColor_green(Color), QColor_blue(Color));
  18.       QColor_destroy(Color);
  19.     end
  20.   else if (TQtDeviceContext(CanvasHandle).Widget <> nil) then
  21.     begin
  22.       Widget := TQtDeviceContext(CanvasHandle).Parent; //Parent is actually QWidgetH, and Widget is QPainterH.
  23.       if (Widget = nil) then
  24.         raise Exception.CreateFmt('TQtDeviceContext(CanvasHandle) %u : does not have Widget',[PtrUInt(CanvasHandle)]);
  25.       Image := QImage_create(1,1, QImageFormat_ARGB32);
  26.       Pixmap:= QPixmap_create(1,1);
  27.       try
  28.         QPixmap_grabWidget(Pixmap, Widget, X,Y, 1,1);
  29.         //QPixmap_grabWindow(Pixmap, QWidget_winId(Widget) ,X,Y,1,1); //this may cause hangs in linux
  30.         QPixmap_toImage(PixMap,Image);
  31.         Rgb := QImage_Pixel(Image, 0,0);
  32.         Color := QColor_create(Rgb);
  33.         Result := RGBToColor(QColor_red(Color), QColor_green(Color), QColor_blue(Color));
  34.       finally
  35.         if Pixmap <> nil then QPixmap_destroy(Pixmap);
  36.         if Image <> nil then QImage_destroy(Image);
  37.         if Color <> nil then QColor_destroy(Color);
  38.       end;
  39.     end
  40.     else
  41.      begin
  42.        raise Exception.CreateFmt('TQtDeviceContext(CanvasHandle) %u : does not have vImage or Painter',[PtrUInt(CanvasHandle)]);
  43.      end;
  44. end;
  45.  

And here is setPixel that works with Canvas of a bitmap and other canvas like TForm.Canvas.
Code: Pascal  [Select]
  1. procedure TQtWidgetSet.DCSetPixel(CanvasHandle: HDC; X, Y: integer; AColor: TGraphicsColor);
  2. var
  3.   ColorRef: TColorRef;
  4.   Painter: QPainterH;
  5.   Pen:QPenH;
  6.   Rgb:QRgb;
  7.   Color: QColorH;
  8. begin
  9.   if IsValidDC(CanvasHandle) then
  10.   begin
  11.     // WriteLn('TQtWidgetSet.DCSetPixel X=',X,' Y=',Y, ' AColor=',dbghex(AColor),' rgb ? ',dbgHex(ColorToRGB(AColor)));
  12.  
  13.     if (TQtDeviceContext(CanvasHandle).vImage <> nil) then
  14.       begin
  15.         ColorRef := TColorRef(ColorToRGB(AColor));
  16.         try
  17.           Color:=QColor_create(Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  18.           Rgb:=QColor_rgba(Color);
  19.           QImage_setPixel(TQtDeviceContext(CanvasHandle).vImage.Handle, X,Y, Rgb);
  20.         finally
  21.           QColor_destroy(Color);
  22.         end;
  23.       end
  24.     else if (TQtDeviceContext(CanvasHandle).Widget <> nil) then
  25.       // It is not bitmap. Fallback to Painter drawPoint.
  26.       begin
  27.         Painter := TQtDeviceContext(CanvasHandle).Widget;
  28.         {Save current pen.Better save copy of pen instead of
  29.          using painter save/restore, or saved Pen in devicecontext which
  30.          may be null. Issue #27620}
  31.         Pen := QPen_create(QPainter_pen(Painter)); //save current pen
  32.  
  33.         {Create pen for SetPixel from scratch to prevent inheriting Width
  34.          and other Pen properties from Painter current Pen.}
  35.         if FPenForSetPixel = nil then
  36.           FPenForSetPixel := QPen_create;
  37.  
  38.         try
  39.           ColorRef := TColorRef(ColorToRGB(AColor));
  40.           QColor_fromRgb(@Color, Red(ColorRef), Green(ColorRef), Blue(ColorRef));
  41.           QPen_setColor(FPenForSetPixel, @Color);
  42.           QPainter_setPen(Painter, FPenForSetPixel);
  43.           QPainter_drawPoint(Painter, X,Y);
  44.         finally
  45.           QPainter_setPen(Painter, Pen); //restore saved pen
  46.           QPen_destroy(Pen);  //destroy the saved pen.
  47.         end;
  48.       end
  49.      else
  50.       begin
  51.         raise Exception.CreateFmt('TQtDeviceContext(CanvasHandle) %u : does not have vImage or Widget',[PtrUInt(CanvasHandle)]);
  52.       end;
  53.   end;
  54. end;
  55.  

Of course with bitmap canvas set/getPixel work much faster. ;)

zeljko

  • Hero Member
  • *****
  • Posts: 1081
    • http://wiki.lazarus.freepascal.org/User:Zeljan
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #13 on: December 27, 2015, 09:27:19 am »
Please open an issue if you feel that it should be opened and attach there example projects and patches (like DCGetPixel from QWidget).

mm7

  • Full Member
  • ***
  • Posts: 119
Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
« Reply #14 on: December 27, 2015, 04:17:44 pm »
where I can open issue?