amerkoleci / Vortice.Windows

.NET bindings for Direct3D12, Direct3D11, WIC, Direct2D1, XInput, XAudio, X3DAudio, DXC, Direct3D9 and DirectInput.
MIT License
1.01k stars 73 forks source link

Direct 2D Custom Effects bug. #315

Closed Steph55 closed 1 year ago

Steph55 commented 2 years ago

Good day!

I have made a software which uses Direct 2D Customs Effects. It was working properly with Vortice 2.1.18 beta, but it is no longer either with 2.1.19 or 2.1.20 beta. It bugs at the ID2D1DeviceContext.EndDraw() command. Here is the stack trace:

System.Exception HResult=0x80131500 Message=Shadow Vortice.Direct2D1.ID2D1Transform is dead Source=SharpGen.Runtime Arborescence des appels de procédure : à SharpGen.Runtime.CppObjectShadow.ToCallback[T](IntPtr thisPtr)

or from the Stack Window:

SharpGen.Runtime.dll!SharpGen.Runtime.CppObjectShadow.ToCallback<Vortice.Direct2D1.ID2D1Transform>(System.IntPtr thisPtr) Inconnu Vortice.Direct2D1.dll!Vortice.Direct2D1.ID2D1TransformVtbl.MapInputRectsToOutputRectImpl_(System.IntPtr thisObject, void* _inputRects, void* _inputOpaqueSubRects, int _inputRectCount, void* _outputRect, void* _outputOpaqueSubRect) Inconnu

Do I have to change something in my code or is it a bug in Vortice?

Thanks!

amerkoleci commented 2 years ago

@andrew-boyarshin any thoughts? I'll debug it tomorrow morning, can you share small repro case?

andrew-boyarshin commented 2 years ago

I can help you debug it if you provide the repro. Without it I can only recommend checking that there are references to the instance of the class that implements ID2D1Transform while that instance can be used by native code. E.g. don't allocate temporary object and pass it to native API as a callback without first storing it somewhere. I also find it curious that 2.1.18 works, that means it's not due to migration to SharpGen 2.0.0-beta.11.

Steph55 commented 2 years ago

Thank you Andrew. You're right, I made an error: it was working with 2.1.8-beta, not 2.1.18-beta.

I have been actively checking this problem, and it indeed seems to be related to migration to SharpGen 2.0.0-beta.11.

I will think of making a repro, but the code to achieve custom effects is quite complicated...

Le jeu. 9 juin 2022 à 17:13, Andrew Boyarshin @.***> a écrit :

I can help you debug it if you provide the repro. Without it I can only recommend checking that there are references to the instance of the class that implements ID2D1Transform while that instance can be used by native code. E.g. don't allocate temporary object and pass it to native API as a callback without first storing it somewhere. I also find it curious that 2.1.18 works, that means it's not due to migration to SharpGen 2.0.0-beta.11.

— Reply to this email directly, view it on GitHub https://github.com/amerkoleci/Vortice.Windows/issues/315#issuecomment-1151630226, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJJUEH65RO26DP2WC7ZNMDVOJM7LANCNFSM5YLKAA5Q . You are receiving this because you authored the thread.Message ID: @.***>

andrew-boyarshin commented 2 years ago

AFAIR, before beta.11 SharpGen was storing a strong reference to the implementation instance in the CppObjectShadow instance (Callback property) while creating a strong GCHandle to itself. Now it doesn't (it now uses weak references in CallbackBase.ThisHandle that is used for all non-shadowed interfaces).

Probably a compat switch can be added to SharpGen if this is the case and it's not feasible to just fix your code, although I'm not a big fan of something that keeps in memory essentially forever with no chance of dying except when explicitly disposed of.

Steph55 commented 2 years ago

Thank you very much Andrew, what you wrote allowed me to correct the problem. However, I would suggest to put the fix directly in Vortice, since it is far from obvious and anyone wishing to use direct2D custom effects will stumble on this. Here is the problem and my suggestion:

To create a custom effect, we need a class like this one:

public class MyCustomEffect : CustomEffectBase, ID2D1DrawTransform
{

...

    public virtual void MapInputRectsToOutputRect(RawRect[] inputRects, RawRect[] inputOpaqueSubRects, out RawRect outputRect, out RawRect outputOpaqueSubRect)
    { }
    public virtual void MapOutputRectToInputRects(RawRect outputRect, RawRect[] inputRects)
    { }
}

The problem comes from the fact that MyCustomEffect is only instantiated by Direct 2D in the following command:

var nv12ToBgraEffect = new ID2D1Effect(deviceContext.CreateEffect(typeof(MyCustomEffect)));

NB: We suppy the type of MyCustomEffect, not an instance of it.

Later, when the MapInputRectsToOutputRect() callback is called by Direct2D, it happens that the MyCustomEffect instance is dead...

So what I did is the following:

public class MyCustomEffect : CustomEffectBase, ID2D1DrawTransform
{

    static List<object> instances = new();
    public MyCustomEffect()
    {
        instances.Add(this);
    }
    public new virtual void Dispose()
    {
        base.Dispose();
        if (instances.Contains(this)) instances.Remove(this);
        GC.SuppressFinalize(this);
    }

...

    public virtual void MapInputRectsToOutputRect(RawRect[] inputRects, RawRect[] inputOpaqueSubRects, out RawRect outputRect, out RawRect outputOpaqueSubRect)
    { }
    public virtual void MapOutputRectToInputRects(RawRect outputRect, RawRect[] inputRects)
    { }
}

Couldn't Vortice do this, only for the CustomEffectBase class?

andrew-boyarshin commented 2 years ago

@Steph55 I'm not sure if moving the reference storage semantics to Vortice is the right way forward. If done for D2D effects, it can be surprising that some other callback interface type is implemented differently. It has to be either all or none. It also appears that only you as the user of Vortice library have a complete picture of the expected lifetime of each of your callback-implementing types. If Vortice starts doing this (or if CallbackBase.ThisHandle implementation is changed in SharpGen), we basically revert to pre-beta.11 behavior of callbacks dying only when explicitly Disposed of. I understand the compat concern and invite @amerkoleci to share his thoughts on the matter.

amerkoleci commented 2 years ago

Probably when creating custom effects with types I can keep a Dictionary with Type and Instance and make sure it doesn't get destroyed by native code.

Let me see what I can do

andrew-boyarshin commented 2 years ago

@amerkoleci what about the lifecycle concerns? Isn't this exactly what I said is a bad idea? Okay, imagine you did it for CustomEffect, what about other callback interface implementations? Why should it be the concern of Vortice? It's got to be either the end-developer (user of Vortice) or SharpGen. I believe that only the end-developer really knows the expected lifetime of his callbacks, so it's their responsibility to store the reference or at least a GCHandle, and then free it when no longer needed. But I can also see the desire to make it so that the end-developer doesn't concern himself with that, like it always was with SharpGen before beta 11. Yeah, their callbacks never die, that's unfortunate. It can also be catastrophic for RAM usage if they hold references inside his callback to some large object graph (so that they prevent not just one object from deletion by GC, but many more). But hey, it's simple. Finally, something smart can be done in the case of IUnknown-implementing callbacks: actually use reference counting, so that the callback (strongly self-referenced at creation) is demoted to being weakly-referenced when reference count reaches 0 (by native-side calls to Release, like it should be).

amerkoleci commented 2 years ago

Maybe I can keep track of custom effects being created, similar like IDirectWrite: https://github.com/amerkoleci/Vortice.Windows/blob/main/src/Vortice.Direct2D1/DirectWrite/IDWriteFactory.cs#L8

Just need to find some time and setup example where I can test this behavior.

Steph55 commented 2 years ago

Good day Amer,

Making a working example of custom effects with Direct2D is not easy! I have extracted a little piece of my code that will allow you to do your tests. The patch I made for the custom effects to work with the most recent versions of Vortice can be found in the file CustomEffects/Shaders/PVShaderBase. Look for the field s_instances. If you remove this patch, you will get the bug...

I have included the code with this email...

Le lun. 4 juil. 2022 à 09:50, Amer Koleci @.***> a écrit :

Maybe I can keep track of custom effects being created, similar like IDirectWrite: https://github.com/amerkoleci/Vortice.Windows/blob/main/src/Vortice.Direct2D1/DirectWrite/IDWriteFactory.cs#L8

Just need to find some time and setup example where I can test this behavior.

— Reply to this email directly, view it on GitHub https://github.com/amerkoleci/Vortice.Windows/issues/315#issuecomment-1173845598, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJJUEC4433XDPQJZS6NVWTVSLTYXANCNFSM5YLKAA5Q . You are receiving this because you were mentioned.Message ID: @.***>

Steph55 commented 1 year ago

I finally took some time to check the change you made concerning this issue on custom D2D effects. I wasn't in a hurry since the patch I did was working correctly.

Unfortunately, the changes you made don't seem to fix the problem. As soon as I remove my patch, I get the problem again...

Thanks a lot for everything, for your very good library!

Le mar. 1 nov. 2022 à 15:02, Amer Koleci @.***> a écrit :

Closed #315 https://github.com/amerkoleci/Vortice.Windows/issues/315 as completed.

— Reply to this email directly, view it on GitHub https://github.com/amerkoleci/Vortice.Windows/issues/315#event-7716331764, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADJJUED7HUIFXEJOCHH7JDDWGFSNLANCNFSM5YLKAA5Q . You are receiving this because you were mentioned.Message ID: @.***>