Recent

Author Topic: [SOLVED] Memory leak using TJPEGImage  (Read 439 times)

petevick

  • Sr. Member
  • ****
  • Posts: 395
[SOLVED] Memory leak using TJPEGImage
« on: September 13, 2024, 09:02:25 pm »
I have the following procedure which creates a screenshot of the app window, it does create the screenshot but then produces a memory leak in Windows (Linux as well with changes to SaveDir string)....

Code: Pascal  [Select][+][-]
  1. procedure TForm1.SaveBmpClick(Sender: TObject);
  2. var
  3.   imgWindow: TJPEGImage;
  4.   pngName: String;
  5.   SaveDir: String;
  6. begin
  7.   pngName:='screenshot-'+FormatDateTime('dd-mm-yyyy hh-mm-ss',Now)+'.jpg';
  8.   SaveDir:= GetWindowsSpecialDir(CSIDL_PERSONAL);
  9.   SaveDir:= Savedir+'Screenshots\';
  10.   try
  11.     imgWindow:= TJPEGImage.Create;
  12.     imgWindow.Assign(GetFormImage);
  13.     imgWindow.SaveToFile(SaveDir+pngName);
  14.     DispMsg('A screenshot has been saved to...'+
  15.         sLineBreak+SaveDir, 'Screenshot saved');
  16.   finally
  17.     imgWindow.Free;
  18.   end;
  19. end;
  20.  

I can't see anything wrong, but there obviously is. The heap results indicates it's at line 12.
« Last Edit: September 14, 2024, 09:00:13 am by petevick »
Pete Vickerstaff
Linux Mint 21.2 Cinnamon, Windows 10, Lazarus 3.2, FPC 3.2.2

wp

  • Hero Member
  • *****
  • Posts: 12353
Re: Memory leak using TJPEGImage
« Reply #1 on: September 13, 2024, 10:07:29 pm »
This is an ugly trap, just like FindInFile was: Hold the CTRL key down and click on the word "GetFormImage". This opens the unit with the sources of this method:
Code: Pascal  [Select][+][-]
  1. function TCustomForm.GetFormImage: TBitmap;
  2. var
  3.   ARect: TRect;
  4. begin
  5.   Result := TBitmap.Create;
  6.   try
  7.     Result.SetSize(ClientWidth, ClientHeight);
  8.     LCLIntf.GetWindowRect(Handle, ARect);
  9.     with GetClientOrigin do
  10.       PaintTo(Result.Canvas, ARect.Left - X, ARect.Top - Y);
  11.   except
  12.     Result.Free;
  13.     raise;
  14.   end;
  15. end;
As you can see this function creates a bitmap and returns it as its result. And when you Assign this bitmap to the jpegimage, you do not copy the pointer but create a copy of the binary bitmap content. Therefore, your code does not destroy the bitmap that was created by GetFormImage.

This fixes the memory leak:
Code: Pascal  [Select][+][-]
  1. procedure TForm1.SaveBmpClick(Sender: TObject);
  2. var
  3.   imgWindow: TJPEGImage;
  4.   bmp: TBitmap;
  5.   pngName: String;
  6.   SaveDir: String;
  7. begin
  8.   pngName:='screenshot-'+FormatDateTime('dd-mm-yyyy hh-mm-ss',Now)+'.jpg';
  9.   SaveDir:= GetWindowsSpecialDir(CSIDL_PERSONAL);
  10.   SaveDir:= Savedir+'Screenshots\';
  11.   try
  12.     imgWindow:= TJPEGImage.Create;
  13.     bmp := GetFormImage;
  14.     imgWindow.Assign(bmp);
  15.     bmp.Free;
  16.     imgWindow.SaveToFile(SaveDir+pngName);
  17.     DispMsg('A screenshot has been saved to...'+
  18.         sLineBreak+SaveDir, 'Screenshot saved');
  19.   finally
  20.     imgWindow.Free;
  21.   end;
  22. end;

But please test also the following code. It overloads the GetFormImage function with a procedure which gets the already existing image as an argument and thus no longer creates anything. This, in my eyes, is a clearer way of coding because the same procedure which creates the image, uses it and destroys it. Moreover, the image parameter can be declared as TCustomBitmap which is the ancestor of all the important LCL image types; therefore, you have more freedom in selecting the image type. For example, you can already pass a JpegImage to the procedure.

Code: Pascal  [Select][+][-]
  1. uses
  2.   LCLIntf;
  3.  
  4. procedure TForm1.GetFormImage(ABitmap: TCustomBitmap);
  5. var
  6.   ARect: TRect;
  7. begin
  8.   ABitmap.SetSize(ClientWidth, ClientHeight);
  9.   LCLIntf.GetWindowRect(Handle, ARect);
  10.   with GetClientOrigin do
  11.     PaintTo(ABitmap.Canvas, ARect.Left - X, ARect.Top - Y);
  12. end;
  13.  
  14. procedure TForm1.Button1Click(Sender: TObject);
  15. var
  16.   jpg: TJpegImage;
  17. begin
  18.   jpg := TJpegImage.Create;
  19.   try
  20.     GetFormImage(jpg);
  21.     jpg.SaveToFile('test.jpg');
  22.   finally
  23.     jpg.Free;
  24.   end;
  25. end;
Tell me whether this modification works for you (it does for me), then I'll add it to TCustomForm.
« Last Edit: September 13, 2024, 11:13:47 pm by wp »

petevick

  • Sr. Member
  • ****
  • Posts: 395
Re: Memory leak using TJPEGImage
« Reply #2 on: September 13, 2024, 10:27:35 pm »
Thanks for the very promising reply wp, I'll give it a try tomorrow as it's nearly sleeping time for us oldies here in the UK 😂
Pete Vickerstaff
Linux Mint 21.2 Cinnamon, Windows 10, Lazarus 3.2, FPC 3.2.2

petevick

  • Sr. Member
  • ****
  • Posts: 395
Re: Memory leak using TJPEGImage
« Reply #3 on: September 14, 2024, 08:59:52 am »
But please test also the following code. It overloads the GetFormImage function with a procedure which gets the already existing image as an argument and thus no longer creates anything. This, in my eyes, is a clearer way of coding because the same procedure which creates the image, uses it and destroys it. Moreover, the image parameter can be declared as TCustomBitmap which is the ancestor of all the important LCL image types; therefore, you have more freedom in selecting the image type. For example, you can already pass a JpegImage to the procedure.

Code: Pascal  [Select][+][-]
  1. uses
  2.   LCLIntf;
  3.  
  4. procedure TForm1.GetFormImage(ABitmap: TCustomBitmap);
  5. var
  6.   ARect: TRect;
  7. begin
  8.   ABitmap.SetSize(ClientWidth, ClientHeight);
  9.   LCLIntf.GetWindowRect(Handle, ARect);
  10.   with GetClientOrigin do
  11.     PaintTo(ABitmap.Canvas, ARect.Left - X, ARect.Top - Y);
  12. end;
  13.  
  14. procedure TForm1.Button1Click(Sender: TObject);
  15. var
  16.   jpg: TJpegImage;
  17. begin
  18.   jpg := TJpegImage.Create;
  19.   try
  20.     GetFormImage(jpg);
  21.     jpg.SaveToFile('test.jpg');
  22.   finally
  23.     jpg.Free;
  24.   end;
  25. end;
Tell me whether this modification works for you (it does for me), then I'll add it to TCustomForm.
Your modification to my code worked just fine, no memory leak, and your code above works as well, and as you say, it's clearer and will be easier to work with, it gets my vote!
Many thanks again wp for your help and advice  ;)
Pete Vickerstaff
Linux Mint 21.2 Cinnamon, Windows 10, Lazarus 3.2, FPC 3.2.2

petevick

  • Sr. Member
  • ****
  • Posts: 395
Re: [SOLVED] Memory leak using TJPEGImage
« Reply #4 on: September 14, 2024, 09:39:15 am »
@wp - I get the following compile message from the GetFormImage() procedure...

Quote
unit1.pas(2073,38) Hint: Local variable "ARect" does not seem to be initialized
I'm assuming I can ignore this  :-\
Pete Vickerstaff
Linux Mint 21.2 Cinnamon, Windows 10, Lazarus 3.2, FPC 3.2.2

wp

  • Hero Member
  • *****
  • Posts: 12353
Re: [SOLVED] Memory leak using TJPEGImage
« Reply #5 on: September 14, 2024, 12:56:43 pm »
Committed the overloaded procedure to Laz/main; it will be back-ported to the Fixes branch and contained in Laz 3.6.

@wp - I get the following compile message from the GetFormImage() procedure...

Quote
unit1.pas(2073,38) Hint: Local variable "ARect" does not seem to be initialized
I'm assuming I can ignore this  :-\
Yes. And in the committed version, the rect is initialized.

 

TinyPortal © 2005-2018