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.

Sign up for GitHub

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

Fix effect cycle check #971

Open
wants to merge 3 commits into
base: uwp/main
Choose a base branch
from
Open

Fix effect cycle check #971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb25314
fix effect cycle detection
benstevens48 Nov 22, 2024
15c2979
Merge branch 'master' into fix-effect-cycle-check
benstevens48 Nov 22, 2024
a40e5b5
Merge branch 'master' into fix-effect-cycle-check
benstevens48 Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 15 additions & 17 deletions winrt/lib/effects/CanvasEffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,17 +205,15 @@ 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);

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 & WIN2D_GET_D2D_IMAGE_FLAGS_READ_DPI_FROM_DEVICE_CONTEXT) != WIN2D_GET_D2D_IMAGE_FLAGS_NONE)
{
Expand Down Expand Up @@ -465,7 +463,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())
Expand All @@ -483,7 +481,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;

Expand All @@ -503,7 +501,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())
Expand All @@ -529,7 +527,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na
return ExceptionBoundary(
[&]
{
auto lock = Lock(m_mutex);
auto lock = RecursiveLock(m_mutex);

if (value)
{
Expand Down Expand Up @@ -894,7 +892,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();

Expand All @@ -915,7 +913,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na

ComPtr<IGraphicsEffectSource> CanvasEffect::GetSource(unsigned int index)
{
auto lock = Lock(m_mutex);
auto lock = RecursiveLock(m_mutex);

auto& d2dEffect = MaybeGetResource();

Expand Down Expand Up @@ -946,7 +944,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();

Expand All @@ -971,7 +969,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();

Expand Down Expand Up @@ -1011,7 +1009,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();

Expand Down Expand Up @@ -1058,7 +1056,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();

Expand All @@ -1081,7 +1079,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.
Expand Down Expand Up @@ -1403,7 +1401,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());

Expand Down Expand Up @@ -1495,7 +1493,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { na

ComPtr<IPropertyValue> CanvasEffect::GetProperty(unsigned int index)
{
auto lock = Lock(m_mutex);
auto lock = RecursiveLock(m_mutex);

assert(index < m_properties.size());

Expand Down
2 changes: 1 addition & 1 deletion winrt/lib/effects/CanvasEffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (in GetOrCreate and to register effect factories).
Expand Down
10 changes: 5 additions & 5 deletions winrt/lib/effects/shader/PixelShaderEffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down