Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I need some help #80

Closed
MarcellVokk opened this issue Dec 11, 2022 · 31 comments
Closed

I need some help #80

MarcellVokk opened this issue Dec 11, 2022 · 31 comments

Comments

@MarcellVokk
Copy link
Contributor

Hi! I stumbled upon this repo on while I was looking for a way to create a game overlay for my game launcher, kind of like what steam has... I tought of creating a system where I'd draw an image every frame, and kind of use the image as a drawing media as I found it easyer to just manipulate a bitmap instead of using a graphics library...

However, I ran into a pretty big issue: The target app keeps crashing, and I think it's because of a memory leak somewhere... I'm not realy an expert in debuging this kind of stuff as I've never realy cared about memory management, since I was always using C#, and that mostly takes care of everything... I noticed there were a few posts about this on the issues page, but none of them gave me a concrete answer on how to go about fixing this. I know this repo is quiete old, but I'd realy apreciate if someone could help me out, as this is the only reliable method I found so far on this topic!

Thanks for your help in advance!

Best regards,
M

@justinstenning
Copy link
Owner

Hi M,

Are you able to share anything of how you are working with your bitmap?

A common problem is not disposing of your bitmap resources.

@MarcellVokk
Copy link
Contributor Author

MarcellVokk commented Dec 11, 2022

Here's my code

    ```
    Process Process;
    CaptureProcess CaptureProcess;
    int FramesDrawn = 0;

    void Init()
    {
        Process = Process.GetProcessesByName("DiceClub").First();

        Direct3DVersion direct3DVersion = Direct3DVersion.AutoDetect;

        CaptureConfig cc = new CaptureConfig()
        {
            Direct3DVersion = direct3DVersion,
            ShowOverlay = true
        };

        var captureInterface = new CaptureInterface();
        captureInterface.RemoteMessage += new MessageReceivedEvent((msg) => Debug.WriteLine(msg));
        CaptureProcess = new CaptureProcess(Process, cc, captureInterface);
    }

    void Redraw()
    {
        FramesDrawn += 1;

        Bitmap i = new Bitmap(100, 100);
        Graphics g = Graphics.FromImage(i);

        g.FillRectangle(Brushes.Black, new RectangleF(0, 0, 100, 100));
        g.DrawLine(new Pen(Brushes.White, 10), 0, 0, 50, 50);

        CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
        {
            Elements = new List<Capture.Hook.Common.IOverlayElement>
            {
                new Capture.Hook.Common.ImageElement()
                {
                    Location = new Point(FramesDrawn, 0),
                    Image = i.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp)
                }
            }
        });

        g.Dispose();
        i.Dispose();

        Debug.WriteLine(FramesDrawn);
    }
    ```

I'm using the latest release of the library (v1.1-beta.1). The target application I'm using for testing is a simple Unity game...

Here are some memory usage metrics per redraws:

0 redraws: 175.3MB
10 redraws: 183MB
20 redraws: 186.1MB
30 redraws: 192.5MB
40 redraws: 196.8MB
50 redraws: 198.3MB
60 redraws: 201.4MB
70 redraws: 205.2MB
80 redraws: 207.3MB
90 redraws: 211.5MB
100 redraws: 214.8MB

NOTE: I don't think the issue is in the way I'm handling bitmaps in my app as it is the target process that is experiencing the memory leak, and not my app...

@justinstenning
Copy link
Owner

Thanks,

Doesn’t look like you are doing anything wrong. Hopefully will get a chance to look at this soonish.

@MarcellVokk
Copy link
Contributor Author

That's be great! Let me know if you find anything and thank you for your time!

@Lakritzator
Copy link

Hey, you should definitively dispose the Pen you new in the line with the g.DrawLine, this does cause a memory (and handle) leak. You could either dispose it, or just create it once in your Init.

I would also advise you to look at caching your bitmap, as this causes a lot of allocations & deallocations, which definitively cause some slowdown. If you have an Init method, you probably also have some teardown or shutdown where you can dispose the Bitmap and Pen (if you go for that route).

That should help, BUT I do not know the ToByteArray method called on the Bitmap, so I assume this is also not a default .NET System.Drawing method. Maybe there is also something going on in there which causes more leaking.

@MarcellVokk
Copy link
Contributor Author

The ToByteArray is from the ScreenshotExtensions class, here's the method:

        public static byte[] ToByteArray(this Image img, System.Drawing.Imaging.ImageFormat format)
        {
            using (MemoryStream stream = new MemoryStream())
            {
                img.Save(stream, format);
                stream.Close();
                return stream.ToArray();
            }
        }

Caching the bitmap is not realy an option, as I need to update it regularly... I also followed your suggestion and changed my setup so that the pen is created on Init, instead of being created every redraw.

I do think this helped a little as at 100 redraws the target apps memory usage is 190MB instead of the previous 214MB, but still there's definietly something weird going on... I also experienced some crashes if I tried to update the overlay too fast, I don't know what can be done about that...

And also, I don't want to turn this into a feature request thread, but I could realy use some guidance on how to go about making an overlay system... I've been looking to do this for a realy long time, but never found anything helpful (and this looks very promissing :)) (I'm making my own game launcher, and want to make an overlay for the games on it, something similar to what steam has)

@justinstenning
Copy link
Owner

Re Robin’s comment about reusing the bitmap instance, I do remember this helping a lot with load tests etc, you still overwrite the content with what you need each frame, but keep the bitmap alive (scoped at the class or something) between frames.

@Lakritzator
Copy link

With caching the bitmap I mean you only new it once, as you redraw all the pixels anyway. This saves a lot of allocating and deallocating, and should have no direct impact on the rest of your code.

@Lakritzator
Copy link

I'm not sure about the other objects, like Overlay and ImageElement, make sure they do not need to be disposed.
Also I would advise you to use "using" or a try finally, to dispose the disposables.

If your C# level is set high enough, you can just write "using Graphics g = Graphics.FromImage(i);" and you will not need to dispose it later, the compiler will take care of it.

What is the point of this: Location = new Point(FramesDrawn, 0),
As FramesDrawn is increased indefinitely, I'm not sure what is going to happen...

Btw. you are also not disposing the process, what is returned by Process.GetProcessesByName, if you exit the program.
I didn't mention it, as I assume the init is only called once, so it will not have a huge impact but still.

@MarcellVokk
Copy link
Contributor Author

Ah okay, I see... But sadly still with these changed too, I get 186MB at 100 redrwas, 196.7MB at 130 redraws... Here's my code:

        Process Process;
        CaptureProcess CaptureProcess;
        int FramesDrawn = 0;
        Pen DefaultPen = new Pen(Brushes.White, 10);
        Bitmap Frame = new Bitmap(100, 100);
        Graphics FrameGraphics = null;

        void Init()
        {
            FrameGraphics = Graphics.FromImage(Frame);

            Process = Process.GetProcessesByName("DiceClub").First();

            Direct3DVersion direct3DVersion = Direct3DVersion.AutoDetect;

            CaptureConfig cc = new CaptureConfig()
            {
                Direct3DVersion = direct3DVersion,
                ShowOverlay = true
            };

            var captureInterface = new CaptureInterface();
            captureInterface.RemoteMessage += new MessageReceivedEvent((msg) => Debug.WriteLine(msg));
            CaptureProcess = new CaptureProcess(Process, cc, captureInterface);
        }

        void Redraw()
        {
            FramesDrawn += 1;

            FrameGraphics.FillRectangle(Brushes.Black, new RectangleF(0, 0, 100, 100));
            FrameGraphics.DrawLine(DefaultPen, 0, 0, 50, 50);

            CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
            {
                Elements = new List<Capture.Hook.Common.IOverlayElement>
                {
                    new Capture.Hook.Common.ImageElement()
                    {
                        Location = new Point(FramesDrawn, 0),
                        Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp)
                    }
                }
            });

            Debug.WriteLine(FramesDrawn);
        }

        void Destroy()
        {
            HookManager.RemoveHookedProcess(Process.Id);
            CaptureProcess.Dispose();
            Process.Dispose();
            Frame.Dispose();
            FrameGraphics.Dispose();
        }

"What is the point of this: Location = new Point(FramesDrawn, 0)": This is just for me too see that the overlay actualy updates, nothing to worry about...

I also included my Destroy() method that I call when the app exits, or when the overlay needs to be turned off...

@Lakritzator
Copy link

I didn't check the code in this project, so I just did...
You also need to dispose the ImageElement

@MarcellVokk
Copy link
Contributor Author

I made some more adjustments, but the issue still persists... 192MB at 100 redraws, 220MB at 150 redraws.

Here is the new snippet:

        Process Process;
        CaptureProcess CaptureProcess;
        int FramesDrawn = 0;
        Pen DefaultPen = new Pen(Brushes.White, 10);
        Bitmap Frame = new Bitmap(100, 100);
        Graphics FrameGraphics = null;
        Capture.Hook.Common.ImageElement OverlayApi_ImageElement = new Capture.Hook.Common.ImageElement();
        new Capture.Hook.Common.Overlay OverlayApi_Overlay = new Capture.Hook.Common.Overlay();

        void Init()
        {
            FrameGraphics = Graphics.FromImage(Frame);

            Process = Process.GetProcessesByName("DiceClub").First();

            Direct3DVersion direct3DVersion = Direct3DVersion.AutoDetect;

            CaptureConfig cc = new CaptureConfig()
            {
                Direct3DVersion = direct3DVersion,
                ShowOverlay = true
            };

            var captureInterface = new CaptureInterface();
            captureInterface.RemoteMessage += new MessageReceivedEvent((msg) => Debug.WriteLine(msg));
            CaptureProcess = new CaptureProcess(Process, cc, captureInterface);

            OverlayApi_Overlay.Elements.Add(OverlayApi_ImageElement);
        }

        void Redraw()
        {
            FramesDrawn += 1;

            FrameGraphics.FillRectangle(Brushes.Black, new RectangleF(0, 0, 100, 100));
            FrameGraphics.DrawLine(DefaultPen, 0, 0, 50, 50);

            OverlayApi_ImageElement.Location = new Point(FramesDrawn, 0);
            OverlayApi_ImageElement.Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp);

            CaptureProcess.CaptureInterface.DrawOverlayInGame(OverlayApi_Overlay);

            Debug.WriteLine(FramesDrawn);
        }

        void Destroy()
        {
            HookManager.RemoveHookedProcess(Process.Id);
            CaptureProcess.Dispose();
            Process.Dispose();
            OverlayApi_ImageElement.Dispose();
            Frame.Dispose();
            FrameGraphics.Dispose();
        }

@MarcellVokk
Copy link
Contributor Author

I honestly don't think it's an issue with my code, as I only get the memory leak in the target app... I'm realy not an expert in this, but as I understand it, this portion of the code never gets executed "inside" the target app...

@Lakritzator
Copy link

I only had a quick glance, but I'm actually not sure how re-assigning to Image will help you.
There seems to be no logic to take up that byte array and make it into an image after the first one.

I didn't test it, but something like this

            using var imageElement = new Capture.Hook.Common.ImageElement()
                    {
                        Location = new Point(FramesDrawn, 0),
                        Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp)
                    };

            CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
            {
                Elements = new List<Capture.Hook.Common.IOverlayElement>
                {
                    imageElement 
                }
            });

Or a try finally...

@Lakritzator
Copy link

I honestly don't think it's an issue with my code, as I only get the memory leak in the target app... I'm realy not an expert in this, but as I understand it, this portion of the code never gets executed "inside" the target app...

I'm pretty sure there is still something wrong with the code, but I am not extremely familiar with the hook code. I looked at it ages ago to see if I can use it for Greenshot. But I do not like the idea of hooking into other applications, so I didn't use it.
I do have a very good knowledge of system.drawing, other win32 apis and most logic around bitmaps.

@MarcellVokk
Copy link
Contributor Author

I only had a quick glance, but I'm actually not sure how re-assigning to Image will help you. There seems to be no logic to take up that byte array and make it into an image after the first one.

I didn't test it, but something like this

            using var imageElement = new Capture.Hook.Common.ImageElement()
                    {
                        Location = new Point(FramesDrawn, 0),
                        Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp)
                    };

            CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
            {
                Elements = new List<Capture.Hook.Common.IOverlayElement>
                {
                    imageElement 
                }
            });

Or a try finally...

I handled this in my last snippet... I created both scene and image on init.

@Lakritzator
Copy link

Lakritzator commented Dec 11, 2022

No you are doing this: OverlayApi_ImageElement.Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp);
And that will only work once... as far I was able to deduct.

@MarcellVokk
Copy link
Contributor Author

What do you mean by only work once?

@Lakritzator
Copy link

The Image array is picked up here:

if (_bitmap == null && Image != null)

And the code will only do it the first time, when it's not null, the second time it will just skip and you will not see any updates on your image.

@MarcellVokk
Copy link
Contributor Author

Oh, yeah... I didn't even notice that... This works though:

        void Redraw()
        {
            FramesDrawn += 1;

            FrameGraphics.FillRectangle(Brushes.Black, new RectangleF(0, 0, 100, 100));
            FrameGraphics.DrawLine(DefaultPen, FramesDrawn * 5, 0, 50, 50);

            using (var imageElement = new Capture.Hook.Common.ImageElement()
            {
                Location = new Point(FramesDrawn, 0),
                Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp)
            })
            {
                CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
                {
                    Elements = new List<Capture.Hook.Common.IOverlayElement>
                    {
                        new Capture.Hook.Common.ImageElement()
                        {
                            Location = new Point(FramesDrawn, 0),
                            Image = Frame.ToByteArray(System.Drawing.Imaging.ImageFormat.Bmp)
                        }
                    }
                });
            }

            Debug.WriteLine(FramesDrawn);
        }

@justinstenning
Copy link
Owner

@Vadkarika2 what does the memory look like after 1000 frames? Does it continue to grow?

@Lakritzator
Copy link

Lakritzator commented Dec 11, 2022

You are not assigning the ImageElement you created in the using to the Elements list. You are now creating 2
Besides, you can just skip the whole ToByteArray, and just supply the bitmap. That will save SOOOO much copying.

            using (var imageElement = new Capture.Hook.Common.ImageElement(Frame)
            {
                Location = new Point(FramesDrawn, 0)
            })
            {
                CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
                {
                    Elements = new List<Capture.Hook.Common.IOverlayElement>
                    {
                        imageElement 
                    }
                });
            }

@Lakritzator
Copy link

I do not own the code in Direct3DHook, but I would assume we are slowly getting to a solution. And if you would measure the time and memory the old Redraw took, compared to the newer version, it's a lot of difference.

Anyway I need to go to bed, I hope the most recent fixes I suggested will solve the memory leak and crashes.

@MarcellVokk
Copy link
Contributor Author

Well I tried, but this does not seem to work... Here's my code:

        Process Process;
        CaptureProcess CaptureProcess;
        int FramesDrawn = 0;
        Pen DefaultPen = new Pen(Brushes.White, 10);

        void Init()
        {
            Process = Process.GetProcessesByName("DiceClub").First();

            Direct3DVersion direct3DVersion = Direct3DVersion.AutoDetect;

            CaptureConfig cc = new CaptureConfig()
            {
                Direct3DVersion = direct3DVersion,
                ShowOverlay = true
            };

            var captureInterface = new CaptureInterface();
            captureInterface.RemoteMessage += new MessageReceivedEvent((msg) => Debug.WriteLine(msg));
            CaptureProcess = new CaptureProcess(Process, cc, captureInterface);
        }

        void Redraw()
        {
            FramesDrawn += 1;

            using (Bitmap Frame = new Bitmap(100, 100))
            using (Graphics FrameGraphics = Graphics.FromImage(Frame))
            using (var imageElement = new Capture.Hook.Common.ImageElement(Frame)
            {
                Location = new Point(FramesDrawn, 0)
            })
            {
                FrameGraphics.FillRectangle(Brushes.Black, new RectangleF(0, 0, 100, 100));
                FrameGraphics.DrawLine(DefaultPen, FramesDrawn * 5, 0, 50, 50);

                CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
                {
                    Elements = new List<Capture.Hook.Common.IOverlayElement>
                    {
                        imageElement
                    }
                });

                Debug.WriteLine(FramesDrawn);
            }
        }

        void Destroy()
        {
            HookManager.RemoveHookedProcess(Process.Id);
            CaptureProcess.Dispose();
            Process.Dispose();
        }

@Lakritzator
Copy link

You now changed a lot back, I'd say a bit too much, you should still cache the Bitmap.
But I wouldn't know why it wouldn't work, what do you mean with doesn't work?

I'd do it something like this.

        void Redraw()
        {
            FramesDrawn += 1;

            using (Graphics FrameGraphics = Graphics.FromImage(Frame)) {
                FrameGraphics.FillRectangle(Brushes.Black, new RectangleF(0, 0, 100, 100));
                FrameGraphics.DrawLine(DefaultPen, FramesDrawn * 5, 0, 50, 50);
            }

            using (var imageElement = new Capture.Hook.Common.ImageElement(Frame)
            {
                Location = new Point(FramesDrawn, 0)
            })
            {
                CaptureProcess.CaptureInterface.DrawOverlayInGame(new Capture.Hook.Common.Overlay
                {
                    Elements = new List<Capture.Hook.Common.IOverlayElement>
                    {
                        imageElement
                    }
                });

                Debug.WriteLine(FramesDrawn);
            }
        }

@MarcellVokk
Copy link
Contributor Author

I'm away from my computer now, so I can't say for sure, but I don't think I can create a Graphics object from an image that already has one created from it... I'm not sure this is the issue though, I'll look into it more tomorrow...

Thanks for your time and help, I realy hope we can figure this out somehow!

@Lakritzator
Copy link

From the system.drawing point of view, my example should work. You can repeat creating a graphics object, as long as you dispose it.

With your previous code, it might never flush the pending operations: https://learn.microsoft.com/en-us/dotnet/api/system.drawing.graphics.flush?view=windowsdesktop-7.0

@justinstenning
Copy link
Owner

@Lakritzator @Vadkarika2 my guess is that although there was a leak also in the code above, the main problem is within the injected code (since the target is the one with the memory increase).

I will try to take a look after work

@MarcellVokk
Copy link
Contributor Author

Thank you for your help! Let me know if you find anything!

@justinstenning
Copy link
Owner

@Vadkarika2 yep there was a dumb mistake in the DXOverlayEngine.cs classes (D3D9 and D3D11) where it did not call base.Dispose(disposing) to free the resources.

This didn't pop up before because typically the overlay was only updated very infrequently in my original use cases.

@MarcellVokk
Copy link
Contributor Author

MarcellVokk commented Dec 13, 2022

Hi! Thanks for the update!

This did indeed fix my issue!

Thank you so much for both of your help and time,
M

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants