D3D9 leaks if any?

Question

I am writing a D3D9 hook plugin to give back to a community that helped me learn programming. It is a community based on programming and scripting for games.

The plugin is supposed to hook d3d9.dll and allow clients to read pixels from the backbuffer. That is the entire extent of the plugin.

Thus I have hooked only the EndScene function.

I have also looked into the following functions to find their release methods:

• IDirect3DDevice9::CreateOffscreenPlainSurface -> IDirect3DSurface9::Release
• IDirect3DDevice9::GetRenderTarget -> IDirect3DSurface9::Release
• IDirect3DDevice9::GetRenderTargetData -> No Release Method
• IDirect3DSurface9::GetDesc -> No destroy method
• IDirect3DSurface9::GetDC -> IDirect3DSurface9::ReleaseDC

So that ends my research of finding where a "possible" leak might be.

Now I have the following code that creates a texture for drawing, draws the texture and immediately frees it. I also have the following for reading the back-buffer into an array provided to my plugin via JNI. This array is guaranteed to be safe and released since it is created in a Java client.

All of my Direct-X code is as follows:

Definitions & Structures:

#define VERTEX_FVF (D3DFVF_XYZRHW | D3DFVF_DIFFUSE)
#define VERTEX_FVF_TEX (D3DFVF_XYZRHW | D3DFVF_DIFFUSE | D3DFVF_TEX1)

struct D3DVertex
{
    float X, Y, Z, RHW;
    unsigned int Colour;
    float U, V;
};


/**
    Calls the Release function of specified classes:
        Textures, Devices, etc..
**/
template<typename T>
void SafeRelease(T* &ptr)
{
    if (ptr)
    {
        ptr->Release();
        ptr = nullptr;
    }
}

LoadTexture:

/**
    Creates a texture from a given buffer.
    Texture must be freed using SafeRelease.
**/
void LoadTexture(IDirect3DDevice9* Device, std::uint8_t* buffer, int width, int height, IDirect3DTexture9* &Texture)
{
    Device->CreateTexture(width, height, 1, 0, D3DFMT_A8R8G8B8, D3DPOOL_MANAGED, &Texture, 0);

    D3DLOCKED_RECT rect;
    Texture->LockRect(0, &rect, nullptr, D3DLOCK_DISCARD);
    TexturePixels = static_cast<std::uint8_t*>(rect.pBits);
    Texture->UnlockRect(0);
    memcpy(TexturePixels, &buffer[0], width * height * 4);
}

DrawTexture:

/**
    Draws a texture on screen using DrawPrimitiveUP.
    No allocations done.
**/
void DrawTexture(IDirect3DDevice9* Device, IDirect3DTexture9* Texture, float X1, float Y1, float X2, float Y2)
{
    float UOffset = 0.5f / (float)(X2 - X1);
    float VOffset = 0.5f / (float)(Y2 - Y1);

    D3DVertex Vertices[] =
    {
        {X1, Y1, 1.0f, 1.0f, D3DCOLOR_RGBA(0xFF, 0xFF, 0xFF, 0xFF), 0.0f + UOffset, 0.0f + VOffset},
        {X2, Y1, 1.0f, 1.0f, D3DCOLOR_RGBA(0xFF, 0xFF, 0xFF, 0xFF), 1.0f + UOffset, 0.0f + VOffset},
        {X1, Y2, 1.0f, 1.0f, D3DCOLOR_RGBA(0xFF, 0xFF, 0xFF, 0xFF), 0.0f + UOffset, 1.0f + VOffset},
        {X2, Y2, 1.0f, 1.0f, D3DCOLOR_RGBA(0xFF, 0xFF, 0xFF, 0xFF), 1.0f + UOffset, 1.0f + VOffset}
    };

    Device->SetFVF(VERTEX_FVF_TEX);
    Device->SetTexture(0, Texture);
    Device->DrawPrimitiveUP(D3DPT_TRIANGLESTRIP, 2, Vertices, sizeof(D3DVertex));
    Device->SetTexture(0, nullptr);
}

DrawCircle:

/**
    Draws a circle on screen using DrawPrimitiveUP.
    No allocations done.
**/
void DrawCircle(IDirect3DDevice9* Device, float CX, float CY, float Radius, D3DCOLOR Colour)
{
    static const int Resolution = 10;
    D3DVertex Vertices[Resolution];

    for (int I = 0; I < Resolution; ++I)
    {
        Vertices[I].X = CX + Radius * std::cos(3.141592654f * (I / (Resolution / 2.0f)));
        Vertices[I].Y = CY + Radius * std::sin(3.141592654f * (I / (Resolution / 2.0f)));
        Vertices[I].Z = 0.0f;
        Vertices[I].RHW = 1.0f;
        Vertices[I].Colour = Colour;
        Vertices[I].U = 0.0f;
        Vertices[I].V = 0.0f;
    }

    Device->SetFVF(VERTEX_FVF_TEX);
    Device->DrawPrimitiveUP(D3DPT_TRIANGLEFAN, Resolution - 2, Vertices, sizeof(D3DVertex));
}

DrawJavaBuffer:

/**
    Takes a given pixel buffer (SmartGlobal->dbg)
    and creates a texture from its pixels and draws it on screen
    releases the texture immediately as specified by the LoadTexture function.
**/
void BltSmartBuffer(IDirect3DDevice9* Device)
{
    if (SmartGlobal != nullptr)
    {
        std::uint8_t* Ptr = reinterpret_cast<std::uint8_t*>(SmartGlobal->dbg);

        LoadTexture(Device, Ptr, SmartGlobal->width, SmartGlobal->height, Texture); //CreateTexture

        DrawTexture(Device, Texture, 0, 0, SmartGlobal->width, SmartGlobal->height);

        SafeRelease(Texture); //Release Texture
    }
}

ReadBackBuffer:

/**
    Reads pixels from the back-buffer into a buffer of size (Width * Height * 4)
**/
HRESULT dxReadPixels(IDirect3DDevice9* Device, void* Buffer, HDC& DC, int& Width, int& Height, D3DFORMAT Format)
{
    IDirect3DSurface9* RenderTarget = nullptr;
    IDirect3DSurface9* DestTarget = nullptr;
    HRESULT result = Device->GetRenderTarget(0, &RenderTarget); //Call SafeRelease when finished.

    if (result == S_OK)
    {
        if (Width == 0 || Height == 0 || Format == D3DFMT_UNKNOWN)
        {
            D3DSURFACE_DESC descriptor = {};
            RenderTarget->GetDesc(&descriptor);  //No release method.
            Width = descriptor.Width;
            Height = descriptor.Height;
            Format = descriptor.Format;
        }

        //RenderTarget->GetDC(&DC); //Call RenderTarget->ReleaseDC.
        result = Device->CreateOffscreenPlainSurface(Width, Height, Format, D3DPOOL_SYSTEMMEM, &DestTarget, nullptr); //No release method :l
        result = Device->GetRenderTargetData(RenderTarget, DestTarget); //Call SafeRelease when finished.

        D3DLOCKED_RECT rect;
        DestTarget->LockRect(&rect, 0, D3DLOCK_READONLY); //Locked..
        memcpy(Buffer, rect.pBits, Width * Height * 4);
        DestTarget->UnlockRect();  //Unlocked..
    }

    SafeRelease(RenderTarget); //Released as promised above
    SafeRelease(DestTarget);   //Released as promised above
    return result;
}

EndScene Hook:

/**
    Draws specified texture and reads back-buffer.
**/
HRESULT Direct3DDevice9Proxy::EndScene()
{
    HDC hdc = nullptr;
    if (SmartGlobal && SmartGlobal->version)
    {
        dxReadPixels(ptr_Direct3DDevice9, SmartGlobal->img, hdc, SmartGlobal->width, SmartGlobal->height); //Read backbuffer into Java buffer using the function above.

        IDirect3DStateBlock9* block;
        ptr_Direct3DDevice9->CreateStateBlock(D3DSBT_ALL, &block); //Release when finished.
        block->Capture();  //Reset when finished.

        if (SmartDebugEnabled)
        {
            BltSmartBuffer(ptr_Direct3DDevice9); //Draw the texture using the function above.
        }

        int X = -1, Y = -1;
        SmartGlobal->getMousePos(X, Y);

        if (X > -1 && Y > -1)
        {
            ptr_Direct3DDevice9->SetTexture(0, nullptr);
            ptr_Direct3DDevice9->SetPixelShader(nullptr);
            ptr_Direct3DDevice9->SetVertexShader(nullptr);
            DrawCircle(ptr_Direct3DDevice9, X, Y, 2.5f);  //Draw a circle using DrawPrimitiveUP using the function above.
        }

        block->Apply(); //Reset as promised.
        block->Release(); //Released as promised.
    }

    return ptr_Direct3DDevice9->EndScene();
}

I know it seems long but I really need your help tracking down any leaks if they exist. I've tried to speed up the review by adding comments I believe is crucial and I hope they do not distract anyone reviewing the code. I'm having a hard time convincing anyone that there are no leaks. I really hope there aren't any leaks in this code.

Answer 1

Coding style:

This is a list of improvements you can apply to make your code nicer in a C++ context:

• Prefer enums or numerical constants over macros. VERTEX_FVF and VERTEX_FVF_TEX should either be members of an enum or const unsigned int objects.

• You did well by using std::uint8_t but you forgot to do the same for memcpy in the LoadTexture() function. memcpy for C++ is defined in the header <cstring>. So it should be referred to as std::memcpy.

• Avoid C-style casts. In most places you've correctly used the C++ cast operators. There are still a few C casts that should be removed for full consistency. In DrawTexture() there is such an instance.

• Use more const. Const can also be applied to local variables that are meant to be assigned once. This is a conceptual kind of const, unlike the one in an explicit numerical constant, which is a compile time constant. In DrawTexture() for example, UOffset, VOffset and Vertices are init-once, never change variables. It is a good practice to mark those as const.

• Read-only pointers are const. In LoadTexture(), buffer is a pointer to read-only memory in that context. It should then be a const std::uint8_t* buffer.

• Use more assertions. Most of your functions take pointers that are not being validated before use. A good rule of thumb when dealing with pointer parameters is to at least validate them with an assert(p != nullptr) before using them. You should apply that to your function parameters of pointer type.

• In the DrawCircle() function, your value of π is used twice, so it should be made a constant. You can define a local static const float PI in function scope for that. Alternatively, you could use the non-standard M_PI constant that is likely to be available on most compilers under the <cmath> header.

• Most functions presented here are doing no error checking. Almost all D3D calls will return you some error code. You are ignoring them most of the time. A function like LoadTexture(), for instance, can easily fail if the system is low on memory. If you don't care about returning an error code to a higher level of the application, for whatever reasons, you should at least check the error code inside the functions and log a message if something failed. That will give you a hint of where to start looking for the problem at a latter time. You can use std::cerr for that.

• The last advice in this list that must be present for a C++ review is about the procedural style of your program. Ideally, you should try to model some aspects of your C++ program using Object Oriented Programming (OOP). You seem to have a few globals in there, plus all the functions are declared in the global scope. The global vars, in special, can be a source of bugs and headache. If you could put some effort into making some parts of this code more object-oriented, you could quite possibly get rid of the globals and see some increase in the overall code quality and readability.

About the memory leaks:

At a glance, there doesn't seem to be any memory leaks in your code. D3D objects don't throw exceptions, so you should be fine for that. If exceptions are introduced at a later time though, that might also introduce memory leaks.

So how would you go about that? With smart pointers, of course! Unfortunately, the smart pointers provided by the standard library are not customizable for used with COM objects.

I've never had such a need, but I imagine someone else must have had, since Microsoft makes heavy use of COM, so it is very likely that smart pointer libraries that support COM objects do exist. You should try searching to find out.

In its simples form, however, a basic COM smart pointer could be implemented as the following:

template<class T>
class COMRefPtr {

public:

    RefPtr(T * object = nullptr)
        : ptr(object) { }

    RefPtr(const RefPtr & rhs)
        : ptr(rhs.ptr)
    {
        if (ptr != nullptr)
        {
            ptr->AddRef();
        }
    }

    RefPtr & operator = (T * object)
    {
        if (ptr != object)
        {
            T * tmp = ptr;
            ptr = object;
            if (tmp)
            {
                tmp->Release();
            }
        }
        return (*this);
    }

    RefPtr & operator = (const RefPtr & rhs)
    {
        if (ptr != rhs.ptr)
        {
            T * tmp = ptr;
            ptr = rhs.ptr;
            if (ptr)
            {
                ptr->AddRef();
            }
            if (tmp)
            {
                tmp->Release();
            }
        }
        return (*this);
    }

    T & operator *()
    {
        assert(ptr != nullptr && "Null pointer dereference!");
        return *ptr;
    }

    const T & operator *() const
    {
        assert(ptr != nullptr && "Null pointer dereference!");
        return *ptr;
    }

    T * operator->()
    {
        assert(ptr != nullptr && "Null pointer access!");
        return ptr;
    }

    const T * operator->() const
    {
        assert(ptr != nullptr && "Null pointer access!");
        return ptr;
    }

    bool operator!() const
    {
        // Allows a check such as "if (!ptr) ..."
        return ptr == nullptr;
    }

    // Access the raw pointer without changing the ref count.
    // Use these to interface with functions that take a raw pointer.
    T * get() { return ptr; }
    const T * get() const { return ptr; }

    // You will probably also want to provide comparison operators.
    // At least `==` and `!=`.

    ~RefPtr()
    {
        // Automatic cleanup.
        if (ptr != nullptr)
        {
            ptr->Release();
        }
    }

private:

    T * ptr;
};

That was modified from an older smart pointer implementation I wrote in the past. It should be usable as-is. For more advanced uses however, there are some things that would have to be further added/adjusted. If you are into learning more about writing smart pointers and templates in general, Modern C++ Design by Andrei Alexandrescu is a great book.

Sorry for the late answer. Hope the project is still alive and the suggestions made can still be of use.