From fb25314e398d6e21e7c7869f848e672575f90eb2 Mon Sep 17 00:00:00 2001 From: benstevens48 Date: Fri, 22 Nov 2024 10:15:15 +0000 Subject: [PATCH] fix effect cycle detection --- winrt/lib/effects/CanvasEffect.cpp | 32 +++++++++---------- winrt/lib/effects/CanvasEffect.h | 2 +- .../lib/effects/shader/PixelShaderEffect.cpp | 10 +++--- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/winrt/lib/effects/CanvasEffect.cpp b/winrt/lib/effects/CanvasEffect.cpp index 2f87825e0..232d75cff 100644 --- a/winrt/lib/effects/CanvasEffect.cpp +++ b/winrt/lib/effects/CanvasEffect.cpp @@ -115,6 +115,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na { ThrowIfClosed(); + auto lock = RecursiveLock(m_mutex); + // Check for graph cycles if (m_insideGetImage) ThrowHR(D2DERR_CYCLIC_GRAPH); @@ -122,10 +124,6 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na m_insideGetImage = true; auto clearFlagWarden = MakeScopeWarden([&] { m_insideGetImage = false; }); - // Lock after the cycle detection, because m_mutex is not recursive. - // Cycle checks don't need to be threadsafe because that's just a developer error. - auto lock = Lock(m_mutex); - // Process the ReadDpiFromDeviceContext flag. if ((flags & GetImageFlags::ReadDpiFromDeviceContext) != GetImageFlags::None) { @@ -375,7 +373,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na { CheckInPointer(value); - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // If we are realized, read the latest value from the underlying D2D resource. if (auto& d2dEffect = MaybeGetResource()) @@ -393,7 +391,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na return ExceptionBoundary( [&] { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); m_cacheOutput = value; @@ -413,7 +411,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na { CheckAndClearOutPointer(value); - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // If we are realized, read the latest value from the underlying D2D resource. if (auto& d2dEffect = MaybeGetResource()) @@ -439,7 +437,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na return ExceptionBoundary( [&] { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); if (value) { @@ -670,7 +668,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na unsigned int CanvasEffect::GetSourceCount() { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); auto& d2dEffect = MaybeGetResource(); @@ -691,7 +689,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na ComPtr CanvasEffect::GetSource(unsigned int index) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); auto& d2dEffect = MaybeGetResource(); @@ -722,7 +720,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void CanvasEffect::SetSource(unsigned int index, IGraphicsEffectSource* source) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); auto& d2dEffect = MaybeGetResource(); @@ -747,7 +745,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void CanvasEffect::InsertSource(unsigned int index, IGraphicsEffectSource* source) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); auto& d2dEffect = MaybeGetResource(); @@ -787,7 +785,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void CanvasEffect::RemoveSource(unsigned int index) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); auto& d2dEffect = MaybeGetResource(); @@ -834,7 +832,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void CanvasEffect::AppendSource(IGraphicsEffectSource* source) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); auto& d2dEffect = MaybeGetResource(); @@ -857,7 +855,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void CanvasEffect::ClearSources() { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // Effects with variable number of inputs don't allow zero of them, // so we must unrealize before we can clear the collection. @@ -1118,7 +1116,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void CanvasEffect::SetProperty(unsigned int index, IPropertyValue* propertyValue) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); assert(index < m_properties.size()); @@ -1210,7 +1208,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na ComPtr CanvasEffect::GetProperty(unsigned int index) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); assert(index < m_properties.size()); diff --git a/winrt/lib/effects/CanvasEffect.h b/winrt/lib/effects/CanvasEffect.h index fba73bd61..072664270 100644 --- a/winrt/lib/effects/CanvasEffect.h +++ b/winrt/lib/effects/CanvasEffect.h @@ -138,7 +138,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na ICanvasDevice* RealizationDevice() { return m_realizationDevice.GetWrapper(); } - std::mutex m_mutex; + std::recursive_mutex m_mutex; public: // Used by ResourceManager::GetOrCreate. diff --git a/winrt/lib/effects/shader/PixelShaderEffect.cpp b/winrt/lib/effects/shader/PixelShaderEffect.cpp index 485b99fe9..130edde0e 100644 --- a/winrt/lib/effects/shader/PixelShaderEffect.cpp +++ b/winrt/lib/effects/shader/PixelShaderEffect.cpp @@ -300,7 +300,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na void PixelShaderEffect::SetProperty(HSTRING name, IInspectable* boxedValue) { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // Store the new property value into our shared state object. m_sharedState->SetProperty(name, boxedValue); @@ -329,7 +329,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na return ExceptionBoundary([&] { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // Store the new value into our shared state object. m_sharedState->CoordinateMapping().Mapping[index] = value; @@ -359,7 +359,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na return ExceptionBoundary([&] { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // Store the new value into our shared state object. m_sharedState->CoordinateMapping().BorderMode[index] = value; @@ -385,7 +385,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na { return ExceptionBoundary([&] { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // Store the new value into our shared state object. m_sharedState->CoordinateMapping().MaxOffset = value; @@ -415,7 +415,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na return ExceptionBoundary([&] { - auto lock = Lock(m_mutex); + auto lock = RecursiveLock(m_mutex); // Convert the enum from Win2D -> D2D format. auto d2dFilter = ToD2DFilter(value);