From 661a5a29ca204fabdb205b9b51a7dece7df78086 Mon Sep 17 00:00:00 2001 From: Cedric Guillemet <1312968+CedricGuillemet@users.noreply.github.com> Date: Thu, 30 Nov 2023 09:51:04 +0100 Subject: [PATCH] Static engine allocator (#1327) When shutting down the engine while having some textures, it's possible for NativeEngine to be disposed before the textures. In this lambda: https://github.com/BabylonJS/BabylonNative/blob/95240a59f2e1483d4c4b92b329b84ff333f0de84/Plugins/NativeEngine/Source/NativeEngine.cpp#L216 The imageContainer has still a ref to NativeEngine::m_allocator that gets invalid https://github.com/bkaradzic/bimg/blob/5d61905eb1aa8074ee62553b4a7bd2dc16f4cf24/src/image.cpp#L3335 Setting m_allocator as static fixes the issue. It only contains a vtable so it's threadsafe. --- .../Babylon/Graphics/DeviceContext.h | 4 +++ Plugins/NativeEngine/Source/NativeEngine.cpp | 32 +++++++++---------- Plugins/NativeEngine/Source/NativeEngine.h | 1 - Plugins/TestUtils/Source/TestUtils.cpp | 5 +-- Plugins/TestUtils/Source/TestUtils.h | 1 - Polyfills/Canvas/Source/Image.cpp | 2 +- Polyfills/Canvas/Source/Image.h | 1 - Polyfills/Canvas/Source/nanovg_babylon.cpp | 3 +- 8 files changed, 25 insertions(+), 24 deletions(-) diff --git a/Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h b/Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h index 90c6c928e..904545a40 100644 --- a/Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h +++ b/Core/Graphics/InternalInclude/Babylon/Graphics/DeviceContext.h @@ -1,6 +1,7 @@ #pragma once #include "BgfxCallback.h" +#include #include "continuation_scheduler.h" #include "SafeTimespanGuarantor.h" @@ -114,6 +115,7 @@ namespace Babylon::Graphics void AddTexture(bgfx::TextureHandle handle, uint16_t width, uint16_t height, bool hasMips, uint16_t numLayers, bgfx::TextureFormat::Enum format); void RemoveTexture(bgfx::TextureHandle handle); TextureInfo GetTextureInfo(bgfx::TextureHandle handle); + static bx::AllocatorI& GetDefaultAllocator() { return m_allocator; } private: friend UpdateToken; @@ -122,5 +124,7 @@ namespace Babylon::Graphics std::unordered_map m_textureHandleToInfo{}; std::mutex m_textureHandleToInfoMutex{}; + + static inline bx::DefaultAllocator m_allocator{}; }; } diff --git a/Plugins/NativeEngine/Source/NativeEngine.cpp b/Plugins/NativeEngine/Source/NativeEngine.cpp index 3e8ba7806..24e56c025 100644 --- a/Plugins/NativeEngine/Source/NativeEngine.cpp +++ b/Plugins/NativeEngine/Source/NativeEngine.cpp @@ -1224,9 +1224,9 @@ namespace Babylon const auto dataSpan = gsl::make_span(static_cast(data.ArrayBuffer().Data()) + data.ByteOffset(), data.ByteLength()); arcana::make_task(arcana::threadpool_scheduler, *m_cancellationSource, - [this, dataSpan, generateMips, invertY, srgb, texture, cancellationSource{m_cancellationSource}]() { - bimg::ImageContainer* image{ParseImage(m_allocator, dataSpan)}; - image = PrepareImage(m_allocator, image, invertY, srgb, generateMips); + [dataSpan, generateMips, invertY, srgb, texture, cancellationSource{m_cancellationSource}]() { + bimg::ImageContainer* image{ParseImage(Graphics::DeviceContext::GetDefaultAllocator(), dataSpan)}; + image = PrepareImage(Graphics::DeviceContext::GetDefaultAllocator(), image, invertY, srgb, generateMips); LoadTextureFromImage(texture, image, srgb); }) .then(m_runtimeScheduler, *m_cancellationSource, [dataRef{Napi::Persistent(data)}, onSuccessRef{Napi::Persistent(onSuccess)}, onErrorRef{Napi::Persistent(onError)}, cancellationSource{m_cancellationSource}](arcana::expected result) { @@ -1275,8 +1275,8 @@ namespace Babylon throw Napi::Error::New(Env(), "The data size does not match width, height, and format"); } - bimg::ImageContainer* image{bimg::imageAlloc(&m_allocator, format, width, height, 1, 1, false, false, bytes)}; - image = PrepareImage(m_allocator, image, invertY, false, generateMips); + bimg::ImageContainer* image{bimg::imageAlloc(&Graphics::DeviceContext::GetDefaultAllocator(), format, width, height, 1, 1, false, false, bytes)}; + image = PrepareImage(Graphics::DeviceContext::GetDefaultAllocator(), image, invertY, false, generateMips); LoadTextureFromImage(texture, image, false); } @@ -1342,9 +1342,9 @@ namespace Babylon const auto typedArray{data[face].As()}; const auto dataSpan{gsl::make_span(static_cast(typedArray.ArrayBuffer().Data()) + typedArray.ByteOffset(), typedArray.ByteLength())}; dataRefs[face] = Napi::Persistent(typedArray); - tasks[face] = arcana::make_task(arcana::threadpool_scheduler, *m_cancellationSource, [this, dataSpan, invertY, generateMips, srgb]() { - bimg::ImageContainer* image{ParseImage(m_allocator, dataSpan)}; - image = PrepareImage(m_allocator, image, invertY, srgb, generateMips); + tasks[face] = arcana::make_task(arcana::threadpool_scheduler, *m_cancellationSource, [dataSpan, invertY, generateMips, srgb]() { + bimg::ImageContainer* image{ParseImage(Graphics::DeviceContext::GetDefaultAllocator(), dataSpan)}; + image = PrepareImage(Graphics::DeviceContext::GetDefaultAllocator(), image, invertY, srgb, generateMips); return image; }); } @@ -1385,9 +1385,9 @@ namespace Babylon const auto typedArray = faceData[face].As(); const auto dataSpan = gsl::make_span(static_cast(typedArray.ArrayBuffer().Data()) + typedArray.ByteOffset(), typedArray.ByteLength()); dataRefs[(face * numMips) + mip] = Napi::Persistent(typedArray); - tasks[(face * numMips) + mip] = arcana::make_task(arcana::threadpool_scheduler, *m_cancellationSource, [this, dataSpan, invertY, srgb]() { - bimg::ImageContainer* image{ParseImage(m_allocator, dataSpan)}; - image = PrepareImage(m_allocator, image, invertY, srgb, false); + tasks[(face * numMips) + mip] = arcana::make_task(arcana::threadpool_scheduler, *m_cancellationSource, [dataSpan, invertY, srgb]() { + bimg::ImageContainer* image{ParseImage(Graphics::DeviceContext::GetDefaultAllocator(), dataSpan)}; + image = PrepareImage(Graphics::DeviceContext::GetDefaultAllocator(), image, invertY, srgb, false); return image; }); } @@ -1563,12 +1563,12 @@ namespace Babylon // Read the source texture. m_graphicsContext.ReadTextureAsync(sourceTextureHandle, textureBuffer, mipLevel) - .then(arcana::inline_scheduler, *m_cancellationSource, [this, textureBuffer{std::move(textureBuffer)}, sourceTextureInfo, targetTextureInfo]() mutable { + .then(arcana::inline_scheduler, *m_cancellationSource, [textureBuffer{std::move(textureBuffer)}, sourceTextureInfo, targetTextureInfo]() mutable { // If the source texture format does not match the target texture format, convert it. if (targetTextureInfo.format != sourceTextureInfo.format) { std::vector convertedTextureBuffer(targetTextureInfo.storageSize); - if (!bimg::imageConvert(&m_allocator, convertedTextureBuffer.data(), bimg::TextureFormat::Enum(targetTextureInfo.format), textureBuffer.data(), bimg::TextureFormat::Enum(sourceTextureInfo.format), sourceTextureInfo.width, sourceTextureInfo.height, /*depth*/ 1)) + if (!bimg::imageConvert(&Graphics::DeviceContext::GetDefaultAllocator(), convertedTextureBuffer.data(), bimg::TextureFormat::Enum(targetTextureInfo.format), textureBuffer.data(), bimg::TextureFormat::Enum(sourceTextureInfo.format), sourceTextureInfo.width, sourceTextureInfo.height, /*depth*/ 1)) { throw std::runtime_error{"Texture conversion to RBGA8 failed."}; } @@ -1849,7 +1849,7 @@ namespace Babylon throw Napi::Error::New(env, "CreateImageBitmap array buffer is empty."); } - image = ParseImage(m_allocator, gsl::make_span(static_cast(data.Data()), data.ByteLength())); + image = ParseImage(Graphics::DeviceContext::GetDefaultAllocator(), gsl::make_span(static_cast(data.Data()), data.ByteLength())); allocatedImage = true; } else if (info[0].IsObject()) @@ -1905,7 +1905,7 @@ namespace Babylon const Napi::Env env{info.Env()}; - bimg::ImageContainer* image = bimg::imageAlloc(&m_allocator, format, static_cast(width), static_cast(height), 1, 1, false, false, data.Data()); + bimg::ImageContainer* image = bimg::imageAlloc(&Graphics::DeviceContext::GetDefaultAllocator(), format, static_cast(width), static_cast(height), 1, 1, false, false, data.Data()); if (image == nullptr) { throw Napi::Error::New(env, "Unable to allocate image for ResizeImageBitmap."); @@ -1917,7 +1917,7 @@ namespace Babylon { image->m_format = bimg::TextureFormat::A8; } - bimg::ImageContainer* rgba = bimg::imageConvert(&m_allocator, bimg::TextureFormat::RGBA8, *image, false); + bimg::ImageContainer* rgba = bimg::imageConvert(&Graphics::DeviceContext::GetDefaultAllocator(), bimg::TextureFormat::RGBA8, *image, false); if (rgba == nullptr) { throw Napi::Error::New(env, "Unable to convert image to RGBA pixel format for ResizeImageBitmap."); diff --git a/Plugins/NativeEngine/Source/NativeEngine.h b/Plugins/NativeEngine/Source/NativeEngine.h index 3641cf079..6ad1b0bcb 100644 --- a/Plugins/NativeEngine/Source/NativeEngine.h +++ b/Plugins/NativeEngine/Source/NativeEngine.h @@ -230,7 +230,6 @@ namespace Babylon void ScheduleRequestAnimationFrameCallbacks(); bool m_requestAnimationFrameCallbacksScheduled{}; - bx::DefaultAllocator m_allocator{}; uint64_t m_engineState{BGFX_STATE_DEFAULT}; uint32_t m_stencilState{BGFX_STENCIL_TEST_ALWAYS | BGFX_STENCIL_FUNC_REF(0) | BGFX_STENCIL_FUNC_RMASK(0xFF) | BGFX_STENCIL_OP_FAIL_S_KEEP | BGFX_STENCIL_OP_FAIL_Z_KEEP | BGFX_STENCIL_OP_PASS_Z_REPLACE}; diff --git a/Plugins/TestUtils/Source/TestUtils.cpp b/Plugins/TestUtils/Source/TestUtils.cpp index 12620705f..0dd6e4150 100644 --- a/Plugins/TestUtils/Source/TestUtils.cpp +++ b/Plugins/TestUtils/Source/TestUtils.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #define STRINGIZEX(x) #x #define STRINGIZE(x) STRINGIZEX(x) @@ -31,7 +32,7 @@ namespace Babylon::Plugins::Internal return; } - bx::MemoryBlock mb(&allocator); + bx::MemoryBlock mb(&Graphics::DeviceContext::GetDefaultAllocator()); bx::FileWriter writer; bx::FilePath filepath(filename.c_str()); bx::FilePath filedir(filepath.getPath()); @@ -49,7 +50,7 @@ namespace Babylon::Plugins::Internal Image* image = new Image; const auto buffer = info[0].As(); - image->m_Image = bimg::imageParse(&allocator, buffer.Data(), static_cast(buffer.ByteLength())); + image->m_Image = bimg::imageParse(&Graphics::DeviceContext::GetDefaultAllocator(), buffer.Data(), static_cast(buffer.ByteLength())); auto finalizer = [](Napi::Env, Image* image) { delete image; }; return Napi::External::New(info.Env(), image, std::move(finalizer)); diff --git a/Plugins/TestUtils/Source/TestUtils.h b/Plugins/TestUtils/Source/TestUtils.h index 97e036589..8b0eeb49e 100644 --- a/Plugins/TestUtils/Source/TestUtils.h +++ b/Plugins/TestUtils/Source/TestUtils.h @@ -49,7 +49,6 @@ namespace Babylon::Plugins::Internal static inline Napi::FunctionReference constructor{}; inline static std::shared_ptr m_implData; - inline static bx::DefaultAllocator allocator{}; void Exit(const Napi::CallbackInfo& info); void UpdateSize(const Napi::CallbackInfo& info); diff --git a/Polyfills/Canvas/Source/Image.cpp b/Polyfills/Canvas/Source/Image.cpp index 72858bfda..34299142f 100644 --- a/Polyfills/Canvas/Source/Image.cpp +++ b/Polyfills/Canvas/Source/Image.cpp @@ -119,7 +119,7 @@ namespace Babylon::Polyfills::Internal return; } - m_imageContainer = bimg::imageParse(&m_allocator, buffer.data(), static_cast(buffer.size_bytes()), bimg::TextureFormat::RGBA8); + m_imageContainer = bimg::imageParse(&Graphics::DeviceContext::GetDefaultAllocator(), buffer.data(), static_cast(buffer.size_bytes()), bimg::TextureFormat::RGBA8); if (m_imageContainer == nullptr) { diff --git a/Polyfills/Canvas/Source/Image.h b/Polyfills/Canvas/Source/Image.h index a4b41adb0..c4b1c0a57 100644 --- a/Polyfills/Canvas/Source/Image.h +++ b/Polyfills/Canvas/Source/Image.h @@ -47,7 +47,6 @@ namespace Babylon::Polyfills::Internal Napi::FunctionReference m_onloadHandlerRef; Napi::FunctionReference m_onerrorHandlerRef; std::shared_ptr m_cancellationSource{}; - bx::DefaultAllocator m_allocator{}; bimg::ImageContainer* m_imageContainer{}; }; } diff --git a/Polyfills/Canvas/Source/nanovg_babylon.cpp b/Polyfills/Canvas/Source/nanovg_babylon.cpp index 1cfff6dd1..71c275045 100644 --- a/Polyfills/Canvas/Source/nanovg_babylon.cpp +++ b/Polyfills/Canvas/Source/nanovg_babylon.cpp @@ -1106,8 +1106,7 @@ NVGcontext* nvgCreate(int32_t _edgeaa, bx::AllocatorI* _allocator) { if (NULL == _allocator) { - static bx::DefaultAllocator allocator; - _allocator = &allocator; + _allocator = &Babylon::Graphics::DeviceContext::GetDefaultAllocator(); } struct NVGparams params;