Why is this code not thread safe (Delphi 7)

[Yaron 53]

I wonder if anything in my code is not thread safe as I get the occasional random freezes in the copy-background code (bitblt functions) when using multiple threads.

 

As you can see, all the bitmaps are created within the thread's execute function and their source data is coming from bitmaps in the main thread that is copied over within a critical section.

 

This is using Delphi 7.

procedure TThumbRendererThread.Execute;
var
  thumbBitmapDarkL : TBitmap;
  backgroundBitmap : TBitmap;
  WorkBM           : TBitmap;
begin
  thumbBitmapDarkL   := TBitmap.Create;
  backgroundBitmap   := TBitmap.Create;
  WorkBM             := TBitmap.Create;
  WorkBM.Width       := thumbWidth;
  WorkBM.Height      := thumbHeightTotal;
  BackdropChanged    := True;

  While (Terminated = False)  do
  Begin
      If BackdropChanged = True then
      Begin
        csThumbRenderer.Enter;
        Try
          // Copy bitmaps from main thread (within Critical Section)
          thumbBitmapDarkL.Assign(LibNavForm.thumbBitmapDark);
          backgroundBitmap.Assign(s_mlBackground);
        Finally
          csThumbRenderer.Leave;
        End;
        BackdropChanged := False;
      End;

      DebugLog('Copy backgrounds (before)');
      BitBlt(WorkBM.Canvas.Handle,0,0,thumbWidth,thumbHeight,thumbBitmapDarkL.Canvas.Handle,xOfs,yOfs,SRCCOPY);
      BitBlt(WorkBM.Canvas.Handle,0,thumbHeight,thumbWidth,thumbHeightTotal-thumbHeight,BackgroundBitmap.Canvas.Handle,xOfs,yOfs+thumbHeight,SRCCOPY);
      DebugLog('Copy backgrounds (after)');

      // Do other things
  End;
  WorkBM.Free;
  backgroundBitmap.Free;
  thumbBitmapDarkL.Free;
end;

 

[Der schöne Günther 316]

Querying a random internet search engine with "Delphi TBitmap Thread", it appears to me as TCanvas is not meant to be used in threads. There are countless threads on this subject.

Edited July 15, 2019 by Der schöne Günther

[John Kouraklis 94]

Check what @Der schöne Günther says about TCanvas. This operation should ALWAYS take place in the main thread

 

Also, what is happenng in LibNavForm? When you assign thumbBitmapDark to thumbBitmapDarkL, how do you enter a critical section? The code implies that you perform a normal assignment. 

 

I don't know how you store (if you do) bitmaps in LibNavForm. What I found helpful is to store bitmaps in TThreadList and access them from there.

[PeterBelow 238]

If you work with a TCanvas in background threads always wrap your code in

canvas.lock;
try
  ...operate on canvas here
finally
  canvas.unlock;
end;

The VCL has a build-in mechanism to optimize use of GDI objects, which is shared between all TCanvases used, and his mechanism is not thread-safe. Locking the canvas makes sure it does not use this mechanism while it is locked.

[Der schöne Günther 316]

But doesn't that defeat the purpose of using a thread in the first place if there can only be one canvas operation at a time?

[Sherlock 663]

18 hours ago, Yaron said:

While (Terminated = False) do

This is more of a style thing, but please consider human readability and write

 

while not Terminated do

 

[PeterBelow 238]

5 hours ago, Der schöne Günther said:

But doesn't that defeat the purpose of using a thread in the first place if there can only be one canvas operation at a time?

No, you can operate on several canvases (of offline bitmaps or metafiles) in parallel, its is just a little bit less efficient since each canvas has to create its own font, pen, brush instead of using an existing one from the VCL sharing pool.

[Steve Maughan 26]

In my app (AlignMix - a mapping package) we draw the map in layers (e.g. one layer may be road, another towns). Each layer is rendered by its own thread. We then "smush" all the layers together to get the final image. It works well, so clearly it is possible to draw on a TCanvas in a thread as long as no other thread is drawing on the same canvas.

 

Steve

[Remy Lebeau 1394]

In VCL, the TBitmap.Canvas must be locked while using the TBitmap in a worker thread.  The RTL keeps track of all GDI resources it allocates, and the main UI thread runs a routine periodically that frees GDI resources that are not locked.  Which is very bad for TBitmap used in a worker thread.  The alternative is to not use TBitmap at all, just use the Win32 API directly for all GDI operations in a worker thread.

[Yaron 53]

Thank you Peter & Remy,

I wasn't aware that TCanvas was sharing resources, the D7 documentation on lock/unlock refers to use lock to prevent other threads from writing to the canvas, but since my code wasn't accessing the canvas from outside the thread, I didn't think locking was mandatory.

I have now changed the code to lock all 3 canvases outside the thread loop.

 

Sherlock:

It's funny how one person's confusing syntax is exactly the opposite for another person, I hate using "not" in any boolean check because for me it's confusing with logical (bitwise) operators.

 

John Kouraklis

I'm not sure what you mean. I use "csThumbRenderer" TCriticalSection in a try/finally block to copy over the contents of TBitmaps from the main thread.

I use the same critical section in the main thread as well when accessing these bitmaps, so there should be no case where the bitmaps are used concurrently by more than one thread.

 

[Steve Maughan 26]

19 hours ago, Remy Lebeau said:

In VCL, the TBitmap.Canvas must be locked while using the TBitmap in a worker thread.  The RTL keeps track of all GDI resources it allocates, and the main UI thread runs a routine periodically that frees GDI resources that are not locked.  Which is very bad for TBitmap used in a worker thread.  The alternative is to not use TBitmap at all, just use the Win32 API directly for all GDI operations in a worker thread.

Are you sure?

 

As I said above, in AlignMix I render a TBitmap.Canvas for each layer of a map using one worker thread per layer. It's rock solid, no crashes or resource issues when rendering.

 

Steve

[Remy Lebeau 1394]

57 minutes ago, Steve Maughan said:

Are you sure?

Yes, very sure.  This is a very well-known issue with TBitmap's implementation.

57 minutes ago, Steve Maughan said:

As I said above, in AlignMix I render a TBitmap.Canvas for each layer of a map using one worker thread per layer. It's rock solid, no crashes or resource issues when rendering.

If your threads are not calling Canvas.Lock() on their TBitmap objects then your rendering code is not thread-safe, and you have just been lucky so far that it hasn't failed on you yet.  For instance, if your main UI thread is not processing any messages while your threads are rendering, then the main thread's GDI garbage collection will not occur to trash your TBitmap resources.  But that is not a guarantee you should rely on, unless you explicitly code for that situation, ie by blocking the main UI thread while your rendering threads are running, which is probably not what you want to do.  Otherwise, don't use TBitmap in threads, use a different implementation that doesn't suffer from threading issues, such as TBitmap32 from http://www.graphics32.org.  Or, simply don't manipulate TBitmap objects in worker threads.  Prepare the pixel data as needed, and then sync with the main UI thread to transfer the data into TBitmap.

[Remy Lebeau 1394]

1 hour ago, Yaron said:

I'm not sure what you mean. I use "csThumbRenderer" TCriticalSection in a try/finally block to copy over the contents of TBitmaps from the main thread.

I use the same critical section in the main thread as well when accessing these bitmaps, so there should be no case where the bitmaps are used concurrently by more than one thread.

Actually, a manual critical section is not good enough, because behind the scenes the main UI thread will periodically reach inside any TCanvas objects that are not locked via their Lock() method.  A manual critical section will not prevent that, you have to use the critical section that TCanvas.Lock() uses internally.

[Yaron 53]

Looks like I have to raise this thread from the dead.

 

Even though I'm calling Bitmap.Canvas.lock and no other thread is writing to the canvas, I rarely get a weird fill color issue.

 

This is a subset of the code:
 

WorkBM.Canvas.Lock;

Try

   WorkBM.Canvas.Brush.Color := clRed;
   WorkBM.Canvas.FillRect(barRect);

Finally

  WorkBM.Canvas.Unlock; 

End;

 

Somehow, the fillrect colors occasionally (very rarely) swaps to a different color.

Do brushes require a separate type of lock?

 

[David Heffernan 2345]

20 minutes ago, Yaron said:

Looks like I have to raise this thread from the dead.

 

Even though I'm calling Bitmap.Canvas.lock and no other thread is writing to the canvas, I rarely get a weird fill color issue.

 

This is a subset of the code:
 

WorkBM.Canvas.Lock;

Try

   WorkBM.Canvas.Brush.Color := clRed;
   WorkBM.Canvas.FillRect(barRect);

Finally

  WorkBM.Canvas.Unlock; 

End;

 

Somehow, the fillrect colors occasionally (very rarely) swaps to a different color.

Do brushes require a separate type of lock?

 

Don't you need to do this for all canvas operations because it's an issue with pooled GDI objects. 

[Yaron 53]

@David Heffernan

 

The entire threaded code is written within the canvas's lock section and works without any issues (font drawing and bitmap operations), except for the brush color which occasionally seems to use the wrong color.

 

I didn't paste the entire code for simplicity.

[Dalija Prasnikar 1396]

23 minutes ago, Yaron said:

The entire threaded code is written within the canvas's lock section and works without any issues (font drawing and bitmap operations), except for the brush color which occasionally seems to use the wrong color.

Brushes like other GDI objects need to be protected when shared between threads https://docs.microsoft.com/en-us/windows/win32/procthread/multiple-threads-and-gdi-objects

 

I never worked with brushes in the background, so  don't know exactly how you should protect it. Answer is somewhere in Vcl.Graphics unit.

[Lars Fosdal 1792]

I always capture the current brush/pen handles, set my own, use them, then restore them to what they were.  Oldschool, I guess.

[Yaron 53]

I ended up writing my own bitmap based fill function, since it has a lot less overhead (my code needs less sanity checks as the input values are always in valid ranges), it's about 40% faster than Delphi's implementation, so win-win.

 

Although I resolved the issue, if anyone knows the "recommended" method of using brushes with threads, I'd be interested.

Edited March 1, 2021 by Yaron

[Guest]

40 minutes ago, Yaron said:

...Although I resolved the issue, if anyone knows the "recommended" method of using brushes with threads, I'd be interested.

hi @Yaron

 

If I'm not mistaken, at the time of the book "The Bible - Delphi 3" by Marcos Cantu, there was a sample showing how to fill a "canvas" using "threads" to fill each pixel, to demonstrate how it could be done in a multi- processing. But unfortunately, I have no information about the project name in the examples in the book.

 

hug

[AlexBelo 12]

22 hours ago, Yaron said:

Even though I'm calling Bitmap.Canvas.lock and no other thread is writing to the canvas, I rarely get a weird fill color issue.

 

21 hours ago, Yaron said:

I didn't paste the entire code for simplicity.

If problem is not in skipped code then this is a bug.

 

I suspect that you do something wrong with canvas(es) somewhere in your program. Locking canvas should work.

 

BTW, Do you use  TJPEGImage.Draw in your threads? This method had a bug (no canvas lock) in old versions.

 

Also, do you copy bmps in thread(s)? I know for sure (after a lot of experiments) that both canvases must be locked in this case. Moreover, to be sure that bmp is really copied in this "safe locked" state you need to modify target bmp (assign a pixel to itself for example); this creates real copy of data in target bmp (instead of reference on source data).

Edited March 2, 2021 by AlexBelo

[Yaron 53]

No, I don't use TJPEGImage at all, I'm basically compositing several tbitmaps, writing a bit of text on an image and using 'fill' to draw a small box (for Zoom Player's media library thumbnails),

 

The canvas are all locked and the issue only seems related to brush colors.

 

 

 

[AlexBelo 12]

On 3/2/2021 at 6:53 PM, Yaron said:

The canvas are all locked and the issue only seems related to brush colors.

GDI objects like brushes are in global list. RTL browses this list in main thread on every idle event and deletes all unused objects; locking any canvas simply disables updating of the list. If you (or some 3-d party library) somewhere do not lock a canvas in backgrownd thread it is possible that some object (brush in your case) is already deleted or/and overwritten but your code is still using old element in list. As a result you can see erratic behaviour of any kind.

Edited March 4, 2021 by AlexBelo

[Fr0sT.Brutal 900]

4 hours ago, AlexBelo said:

GDI objects like brushes are in global list. RTL browses this list in main thread on every idle event and deletes all unused objects; locking any canvas simply disables updating of the list. If you (or some 3-d party library) somewhere do not lock a canvas in backgrownd thread it is possible that some object (brush in your case) is already deleted or/and overwritten but your code is still using old element in list. As a result you can see erratic behaviour of any kind.

Are you sure that's RTL that does these things? In which method?

[AlexBelo 12]

1 hour ago, Fr0sT.Brutal said:

Are you sure

No. 🙂

My allegation about lists of objects is not right. (I investigated this problem too long ago; memory fail me.)

 

More accurate info:

 

unit Graphics;

{ FreeMemoryContexts is called by the VCL main winproc to release
  memory DCs after every message is processed (garbage collection).
  Only memory DCs not locked by other threads will be freed.
}
procedure FreeMemoryContexts;
var
  I: Integer;
begin
  with BitmapCanvasList.LockList do
  try
    for I := Count-1 downto 0 do
    with TBitmapCanvas(Items[I]) do
      if TryLock then
      try
        FreeContext;
      finally
        Unlock;
      end;
  finally
    BitmapCanvasList.UnlockList;
  end;
end;



unit Controls;

procedure TWinControl.MainWndProc(var Message: TMessage);
begin
  try
    try
      WindowProc(Message);
    finally
      FreeDeviceContexts;
      FreeMemoryContexts;
    end;
  except
    Application.HandleException(Self);
  end;
end;

 

So main thread (where MainWndProc works) fairly often deselects unlocked memory contexts. It causes all kinds of problems in background threads which use bitmaps without locking.