Lazarus

Programming => Widgetset => QT => Topic started by: mm7 on December 24, 2015, 08:15:38 pm

Title: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 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
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko 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.
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko on December 25, 2015, 12:44:14 pm
ok, QPainter_destroy() is missing there ...
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko 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.
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 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.  
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 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.
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: Septe 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. ;)
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko 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.
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko 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.
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 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.  




Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko 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) ...
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 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...
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 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. ;)
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko 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).
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 on December 27, 2015, 04:17:44 pm
where I can open issue?
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko on December 27, 2015, 04:56:08 pm
At lazarus bug tracker http://bugs.freepascal.org/view_all_bug_page.php?project_id=1
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: mm7 on December 27, 2015, 08:46:32 pm
Should I open an issue for memory leak in DCSetPixel?
Or for new feature in DCGetPixel?
Or for both?
Title: Re: Severe memory leak from Canvas.Pixels[X,Y]:=aColor;
Post by: zeljko on December 28, 2015, 08:52:46 am
Memory leak is fixed in trunk, so no need to open issue. Open it for improved DCSetPixel and DCGetPixel