Remove memory and resource leaks (#754)

* Remove circular reference to renderer on Vk & D3D12 DescriptorSetImpl

* Refactor Stbi image loading such that memory is correctly freed when goes out of scope.
Added Crt memory dump at termination.
Reduced erroneous reporting by scoping TestContext.

* Used capitalized acronym for STBImage to keep Tim happy.

4 files changed, 185 insertions, 105 deletions

diff --git a/tools/gfx/render-d3d11.cpp b/tools/gfx/render-d3d11.cpp
index b0b2b9c04..e049bad38 100644
--- a/tools/gfx/render-d3d11.cpp
+++ b/tools/gfx/render-d3d11.cpp
@@ -96,6 +96,8 @@ public:
     virtual void waitForGpu() override {}
     virtual RendererType getRendererType() const override { return RendererType::DirectX11; }
 
+    ~D3D11Renderer() {}
+
     protected:
 
 #if 0
diff --git a/tools/gfx/render-d3d12.cpp b/tools/gfx/render-d3d12.cpp
index 360242dab..e8af974fa 100644
--- a/tools/gfx/render-d3d12.cpp
+++ b/tools/gfx/render-d3d12.cpp
@@ -313,7 +313,7 @@ protected:
             ResourceView*   textureView,
             SamplerState*   sampler) override;
 
-        RefPtr<D3D12Renderer>           m_renderer;
+        D3D12Renderer*           m_renderer = nullptr;          ///< Weak pointer - must be because if set on Renderer, will have a circular reference
         RefPtr<DescriptorSetLayoutImpl> m_layout;
 
         D3D12DescriptorHeap*        m_resourceHeap = nullptr;
@@ -336,7 +336,7 @@ protected:
 
 
     // During command submission, we need all the descriptor tables that get
-    // used to come from a single heap (for each descritpor heap type).
+    // used to come from a single heap (for each descriptor heap type).
     //
     // We will thus keep a single heap of each type that we hope will hold
     // all the descriptors that actually get needed in a frame.
@@ -2838,8 +2838,7 @@ void D3D12Renderer::setBindingState(BindingState* state)
 
 void D3D12Renderer::DescriptorSetImpl::setConstantBuffer(UInt range, UInt index, BufferResource* buffer)
 {
-    auto dxDevice = m_renderer->m_device;
-
+    auto dxDevice = m_renderer->m_device.get();
 
     auto resourceImpl = (BufferResourceImpl*) buffer;
     auto resourceDesc = resourceImpl->getDesc();
@@ -3069,7 +3068,7 @@ Result D3D12Renderer::createDescriptorSetLayout(const DescriptorSetLayout::Desc&
     RefPtr<DescriptorSetLayoutImpl> descriptorSetLayoutImpl = new DescriptorSetLayoutImpl();
 
     // We know the total number of resource and sampler "slots" that an instance
-    // of this decriptor-set layout would need:
+    // of this descriptor-set layout would need:
     //
     descriptorSetLayoutImpl->m_resourceCount = combinedCount + dedicatedResourceCount;
     descriptorSetLayoutImpl->m_samplerCount = combinedCount + dedicatedSamplerCount;
@@ -3345,7 +3344,7 @@ Result D3D12Renderer::createPipelineLayout(const PipelineLayout::Desc& desc, Pip
         //
         UInt bindingSpace   = dd;
 
-        // Copy descriptor range infromation from the set layout into our
+        // Copy descriptor range information from the set layout into our
         // temporary copy (this is required because the same set layout
         // might be applied to different ranges).
         //
diff --git a/tools/gfx/render-vk.cpp b/tools/gfx/render-vk.cpp
index 45b92fe98..32bc91de4 100644
--- a/tools/gfx/render-vk.cpp
+++ b/tools/gfx/render-vk.cpp
@@ -348,7 +348,7 @@ public:
         static Binding::Type _getBindingType(RefObject* ptr);
         void _setBinding(Binding::Type type, UInt range, UInt index, RefObject* ptr);
 
-        RefPtr<VKRenderer>                  m_renderer;
+        VKRenderer*                         m_renderer = nullptr;   ///< Weak pointer, can't be strong, because if set will become circular reference
         RefPtr<DescriptorSetLayoutImpl>     m_layout;
         VkDescriptorSet                     m_descriptorSet = VK_NULL_HANDLE;
 
diff --git a/tools/slang-test/main.cpp b/tools/slang-test/main.cpp
index 77005d03b..8a03a6100 100644
--- a/tools/slang-test/main.cpp
+++ b/tools/slang-test/main.cpp
@@ -1430,6 +1430,69 @@ TestResult doRenderComparisonTestRun(TestContext* context, TestInput& input, cha
     return TestResult::Pass;
 }
 
+class STBImage
+{
+public:
+    typedef STBImage ThisType;
+
+        /// Reset back to default initialized state (frees any image set)
+    void reset();
+        /// True if rhs has same size and amount of channels
+    bool isComparable(const ThisType& rhs) const;
+
+        /// The width in pixels
+    int getWidth() const { return m_width; }
+        /// The height in pixels
+    int getHeight() const { return m_height; }
+        /// The number of channels (typically held as bytes in order)
+    int getNumChannels() const { return m_numChannels; }
+
+        /// Get the contained pixels, nullptr if nothing loaded
+    const unsigned char* getPixels() const { return m_pixels; }
+    unsigned char* getPixels() { return m_pixels; }
+
+        /// Read an image with filename. SLANG_OK on success
+    SlangResult read(const char* filename);
+
+    ~STBImage() { reset(); }
+
+    int m_width = 0;
+    int m_height = 0;
+    int m_numChannels = 0;
+    unsigned char* m_pixels = nullptr;
+};
+
+void STBImage::reset()
+{
+    if (m_pixels)
+    {
+        stbi_image_free(m_pixels);
+        m_pixels = nullptr;
+    }
+    m_width = 0;
+    m_height = 0;
+    m_numChannels = 0;
+}
+
+SlangResult STBImage::read(const char* filename)
+{
+    reset();
+
+    m_pixels = stbi_load(filename, &m_width, &m_height, &m_numChannels, 0);
+    if (!m_pixels)
+    {
+        return SLANG_FAIL;
+    }
+    return SLANG_OK;
+}
+
+bool STBImage::isComparable(const ThisType& rhs) const
+{
+    return (this == &rhs) ||
+        (m_width == rhs.m_width && m_height == rhs.m_height && m_numChannels == rhs.m_numChannels);
+}
+
+
 TestResult doImageComparison(TestContext* context, String const& filePath)
 {
     // Allow a difference in the low bits of the 8-bit result, just to play it safe
@@ -1441,78 +1504,85 @@ TestResult doImageComparison(TestContext* context, String const& filePath)
     String expectedPath = filePath + ".expected.png";
     String actualPath = filePath + ".actual.png";
 
-    int expectedX,  expectedY,  expectedN;
-    int actualX,    actualY,    actualN;
-
-    unsigned char* expectedData = stbi_load(expectedPath.begin(), &expectedX, &expectedY, &expectedN, 0);
-    unsigned char* actualData = stbi_load(actualPath.begin(), &actualX, &actualY, &actualN, 0);
-
-    if(!expectedData)   
+    STBImage expectedImage;
+    if (SLANG_FAILED(expectedImage.read(expectedPath.Buffer())))
     {
         context->messageFormat(TestMessageType::RunError, "Unable to load image ;%s'", expectedPath.Buffer());
         return TestResult::Fail;
     }
-    if(!actualData)
+
+    STBImage actualImage;
+    if (SLANG_FAILED(actualImage.read(actualPath.Buffer())))
     {
-        context->messageFormat(TestMessageType::RunError, "Unable to load image '%s'", actualPath.Buffer());
+        context->messageFormat(TestMessageType::RunError, "Unable to load image ;%s'", actualPath.Buffer());
         return TestResult::Fail;
     }
 
-    if(expectedX != actualX || expectedY != actualY || expectedN != actualN)    
+    if (!expectedImage.isComparable(actualImage))
     {
         context->messageFormat(TestMessageType::TestFailure, "Images are different sizes '%s' '%s'", actualPath.Buffer(), expectedPath.Buffer());
         return TestResult::Fail;
     }
 
-    unsigned char* expectedCursor = expectedData;
-    unsigned char* actualCursor = actualData;
+    {
+        const unsigned char* expectedPixels = expectedImage.getPixels();
+        const unsigned char* actualPixels = actualImage.getPixels();
 
-    for( int y = 0; y < actualY; ++y )
-	{
-		for( int x = 0; x < actualX; ++x )
-		{
-			for( int n = 0; n < actualN; ++n )
-			{
-				int expectedVal = *expectedCursor++;
-				int actualVal = *actualCursor++;
-
-				int absoluteDiff = actualVal - expectedVal;
-				if(absoluteDiff < 0) absoluteDiff = -absoluteDiff;
-
-				if( absoluteDiff < kAbsoluteDiffCutoff )
-				{
-					// There might be a difference, but we'll consider it to be inside tolerance
-					continue;
-				}
-
-				float relativeDiff = 0.0f;
-				if( expectedVal != 0 )
-				{
-					relativeDiff = fabsf(float(actualVal) - float(expectedVal)) / float(expectedVal);
-
-					if( relativeDiff < kRelativeDiffCutoff )
-					{
-						// relative difference was small enough
-						continue;
-					}
-				}
-
-				// TODO: may need to do some local search sorts of things, to deal with
-				// cases where vertex shader results lead to rendering that is off
-				// by one pixel...
+        const int height = actualImage.getHeight();
+        const int width = actualImage.getWidth();
+        const int numChannels = actualImage.getNumChannels();
+        const int rowSize = width * numChannels;
+
+        for (int y = 0; y < height; ++y)
+        {
+            for (int i = 0; i < rowSize; ++i)
+            {
+                int expectedVal = expectedPixels[i];
+                int actualVal = actualPixels[i];
+
+                int absoluteDiff = actualVal - expectedVal;
+                if (absoluteDiff < 0) absoluteDiff = -absoluteDiff;
+
+                if (absoluteDiff < kAbsoluteDiffCutoff)
+                {
+                    // There might be a difference, but we'll consider it to be inside tolerance
+                    continue;
+                }
+
+                float relativeDiff = 0.0f;
+                if (expectedVal != 0)
+                {
+                    relativeDiff = fabsf(float(actualVal) - float(expectedVal)) / float(expectedVal);
+
+                    if (relativeDiff < kRelativeDiffCutoff)
+                    {
+                        // relative difference was small enough
+                        continue;
+                    }
+                }
+
+                // TODO: may need to do some local search sorts of things, to deal with
+                // cases where vertex shader results lead to rendering that is off
+                // by one pixel...
+
+                const int x = i / numChannels;
+                const int channelIndex = i % numChannels;
 
                 context->messageFormat(TestMessageType::TestFailure, "image compare failure at (%d,%d) channel %d. expected %d got %d (absolute error: %d, relative error: %f)\n",
-                    x, y, n,
+                    x, y, channelIndex,
                     expectedVal,
                     actualVal,
                     absoluteDiff,
                     relativeDiff);
 
-			    // There was a difference we couldn't excuse!
-			    return TestResult::Fail;
-		    }
-		}
-	}
+                // There was a difference we couldn't excuse!
+                return TestResult::Fail;
+            }
+
+            expectedPixels += rowSize;
+            actualPixels += rowSize; 
+        }
+    }
 
     return TestResult::Pass;
 }
@@ -2015,66 +2085,75 @@ int main(
         g_options.excludeCategories.Add(vulkanTestCategory, vulkanTestCategory);
     }
 
-    // Setup the context 
-    TestContext context;
-    if (SLANG_FAILED(context.init(g_options.outputMode)))
+    int returnCode = 0;
     {
-        // Unable to initialize context
-        return 1;
-    }
-
-    context.m_dumpOutputOnFailure = g_options.dumpOutputOnFailure;
-    context.m_useExes = g_options.useExes;
-    context.m_isVerbose = g_options.shouldBeVerbose;
-
-    { 
-        TestContext::SuiteScope suiteScope(&context, "tests");
-        // Enumerate test files according to policy
-        // TODO: add more directories to this list
-        // TODO: allow for a command-line argument to select a particular directory
-        runTestsInDirectory(&context, "tests/");
-    }
+        // Setup the context 
+        TestContext context;
+        if (SLANG_FAILED(context.init(g_options.outputMode)))
+        {
+            // Unable to initialize context
+            return 1;
+        }
 
-    // Run the unit tests (these are internal C++ tests - not specified via files in a directory) 
-    // They are registered with SLANG_UNIT_TEST macro
-    {
-        TestContext::SuiteScope suiteScope(&context, "unit tests");
-        TestContext::set(&context);
+        context.m_dumpOutputOnFailure = g_options.dumpOutputOnFailure;
+        context.m_useExes = g_options.useExes;
+        context.m_isVerbose = g_options.shouldBeVerbose;
 
-        // Run the unit tests
-        TestRegister* cur = TestRegister::s_first;
-        while (cur)
         {
-            StringBuilder filePath;
-            filePath << "unit-tests/" << cur->m_name << ".internal";
+            TestContext::SuiteScope suiteScope(&context, "tests");
+            // Enumerate test files according to policy
+            // TODO: add more directories to this list
+            // TODO: allow for a command-line argument to select a particular directory
+            runTestsInDirectory(&context, "tests/");
+        }
 
-            TestOptions testOptions;
-            testOptions.categories.Add(unitTestCatagory);
-            testOptions.command = filePath;
+        // Run the unit tests (these are internal C++ tests - not specified via files in a directory) 
+        // They are registered with SLANG_UNIT_TEST macro
+        {
+            TestContext::SuiteScope suiteScope(&context, "unit tests");
+            TestContext::set(&context);
 
-            if (shouldRunTest(&context, testOptions.command))
+            // Run the unit tests
+            TestRegister* cur = TestRegister::s_first;
+            while (cur)
             {
-                if (testPassesCategoryMask(&context, testOptions))
-                {
-                    context.startTest(testOptions.command);
-                    // Run the test function
-                    cur->m_func();
-                    context.endTest();
-                }
-                else
+                StringBuilder filePath;
+                filePath << "unit-tests/" << cur->m_name << ".internal";
+
+                TestOptions testOptions;
+                testOptions.categories.Add(unitTestCatagory);
+                testOptions.command = filePath;
+
+                if (shouldRunTest(&context, testOptions.command))
                 {
-                    context.addTest(testOptions.command, TestResult::Ignored);
+                    if (testPassesCategoryMask(&context, testOptions))
+                    {
+                        context.startTest(testOptions.command);
+                        // Run the test function
+                        cur->m_func();
+                        context.endTest();
+                    }
+                    else
+                    {
+                        context.addTest(testOptions.command, TestResult::Ignored);
+                    }
                 }
+
+                // Next
+                cur = cur->m_next;
             }
-                
-            // Next
-            cur = cur->m_next;
+
+            TestContext::set(nullptr);
         }
 
-        TestContext::set(nullptr);
+        context.outputSummary();
+
+        returnCode = context.didAllSucceed() ? 0 : 1;
     }
 
-    context.outputSummary();
+#ifdef _MSC_VER
+    _CrtDumpMemoryLeaks();
+#endif
 
-    return context.didAllSucceed() ? 0 : 1; 
+    return returnCode; 
 }