Add SKGLElement to SkiaSharp.Views.WPF #2317
Conversation
|
@mattleibow I'm not really sure how the nuspecs are managed since they all have 1.0.0 for the version? I assume something is injecting the actual required version numbers during the build? The actual dependencies for net462 and netCoreApp3.1 are called out in the csproj. |
|
This mirrors as closely as possible what the GL control does for WF with the OpenTK stuff. Main divergences is there didn't seem to be a way to explicitly pass on the graphics mode, and the resetting of the GRContext on render |
|
One obnoxious thing with GLWPFControl is that they had a bug whereby if you try to use it over RDP it isn't configured with the software fallback for the 3.x line, but hopefully they will accept my PR: opentk/GLWpfControl#93 |
|
I updated the OpenTK dependencies to use exact versions based on: https://github.com/mono/SkiaSharp/wiki/Creating-New-Libraries Even though this wasn't what the nuspec for WF seemed to be doing. May need some guidance on what is expected there. |
|
I would really like to see this :) I'm implementing this code personally in the meantime. |
|
Was testing this out, and it seemed to be considerably slower than using SKGLControl natively in WinForms. I was getting around ~15-16 FPS, and if I got lucky/adjusted some settings, got up to 30 FPS. I'm drawing a fairly big bitmap with lots of shapes/lines, and trying to achieve 60/120 FPS. On WinForms could get to 300+ no problem. I'm trying to port some WinForms projects over to WPF, but may hold off on this. |
|
hm.... I wonder if there is a bunch of overhead in clearing the grcontext in some scenarios. None of the scenarios I was trying involved big bitmaps, though. |
|
There was another strategy in making sure that there were multiple contexts/created. Maybe that would be better for that scenario. |
|
I haven't don't much perf testing of this yet for my scenarios. It subjectively seemed ok. Mostly I was just happy that there is no longer bad crosstalk with multiple controls present. |
|
Yeah, functionally this is good (no crosstalk). There's a bit of latency in the drawing from what I could tell too, when I would measure the time between changing a series of coordinates/images programatically, and when it would update on screen in the next render cycle. Using the FormsHost interop is slightly better (but has the airspace issue), but also severely degraded from native SKGLControl in winforms. Wish there was a way to get high performing GL Backed Skia in WPF easily :( |
|
@imerzan if (grContext != null)
{
grContext.ResetContext();
}and see if the FPS is higher. Commenting this out will bring the crosstalk issues back, but maybe you can see if it performs more like the crossbow WF approach. If so, the approach I was originally taking was to try to make sure each control created it's on graphics context/gl context rather than sharing them. Was advised that it would be better to share the context and just reset it, but maybe that is introducing too much overhead when swapping between surfaces. I'm guessing it may be just too much overhead if there might be a bunch of state/resources associated with the context. Again. I didn't really see any FPS issues in the minimal testing I've done. But while I was doing heavy drawing ops, I don't think I was doing anything that required allocation of heavy resources, so maybe that's the difference. |
|
I will see about trying that. Sorry when you say crosstalk is this another name for the 'airspace' issues where the GL Control is stuck on top of other controls? And yes, when I tested this drawing some simple lines/circles (many thousands), it seemed to work fine. But for realtime rendering with bigger resources (Bitmaps,etc.) it stuttered quite a bit. If I programmatically turned/rotated objects in the view, there was significant ghosting. |
|
Digging into the docs, it doesn't really seem like a great idea to call resetContext often. So I'm probably better off with my earlier approach of making sure multiple contexts are created. However, that was a little bit painful with OpenTK, and I'm not positive of all the implications to having multiple GL and skia contexts associated with the same thread. Still, will see if I can rearrange this when I get a chance. Do let me know if dropping resetContext makes the perf more equivalent with WF, though. |
|
no, for crosstalk I mean, if you have multiple SK GL controls on the same UI thread without that logic, it will be bad news. It seems like they tread on each other's GL context state and you just get a lot of graphical corruption. The approach with resetting the GRContext is that it will have skia resend the required state to the GL context on paint so that it doesn't matter that multiple GRContexts are playing with the same GL context. But it seems like that state sync is too expensive to be practical from what you are seeing. |
|
if you are only using one SK GL control, you probably won't notice any problem, even after commenting that logic out. |
|
My guess would be that resetting the context might be requiring that bitmap memory is reallocated on the GPU or something which might account for the more negative performance impact in your scenario. |
|
So I tested the snippet you suggested. It actually doesn't help at all, and introduces a lot of interference with the WPF Gui (weird anomalies). Must be the crosstalk you mentioned. I am using this snippet for my render loop: CompositionTarget.Rendering += CompositionTarget_Rendering;
private void CompositionTarget_Rendering(object sender, EventArgs e)
{
_gl?.InvalidateVisual(); ; // draw next frame
}
// Draw Func
private void GL_PaintSurface(object sender, SKPaintGLSurfaceEventArgs e)
{
var canvas = e.Surface.Canvas;
canvas.Clear();
// Draw bitmaps/lines/etc.
canvas.Flush();
}Is this not ideal? I tried the ContinuouslyRender property in the element and that didn't seem to help either. |
|
Huh... if yanking that logic didn't help then there could be something else making the WPF scenario slower for you... Wonder if there is something inefficient at the OpenTK level... |
|
Hmm.... My recollection might not be ideal with some of this stuff. But It's possible that it's not great to invalidate visual from compositionTarget.Rendering. IIRC I think that might fire at a capped rate based on your monitors refresh rate, and it might fire AS the layout is being resolved. So its possible it's too late for the component to update on that layout pass by that point? So its conceivable you are locking the max refresh rate to monitor hz / 2? But I can't recall the exact timings of when those things fire so this is guesswork. You might want to test out how often you paint with nothing else in the paint callback when using that as a render loop first. |
|
I removed the CompositionTarget event, and modified this property in my startup _gl.RenderContinuously = true;Still same frame rate. My canvas code is 1:1 copied from my Winforms app that gets 300+ FPS with Vsync Off. I really only need about 30 fps (getting 15-20) but need to figure out why there is so much input delay also. On Winforms I usually enable VSync in the app and the render feels pretty fluid at 30. |
|
I might be wrong though, since looking at the OpenTK GL control it uses a pretty similar render loop, so I'm guessing the timing is such that you aren't cutting the refresh rate in half. Still might be worth validating how often your paint fires without the skia stuff going on as a control. |
|
OK that was the issue. The internal render was stuck around 15-25 FPS. I did something similar to what I do in WinForms and now getting 80-110 FPS (still lower than Winforms 300+, but much better) private async void MainWindow_Loaded(object sender, RoutedEventArgs e)
{
while (true)
{
await Task.Run(() => Thread.SpinWait(50000));
_gl?.InvalidateVisual();
}
}However, there is a lot of input delay. If I move something, there is a ghosting effect where I moved my mouse, but the GUI takes a split second to catch up. In my WinForms app it responds in realtime, even when capped at 30fps. EDIT: I removed all my bitmaps from my draw loop, and there is still input delay/stuttering. |
|
It's possible that you are keeping the main thread busy enough that you are preventing WPF from processing it's input queues effectively. I feel like I've run into this before... It can get you the point where there is actually backlog of queued input to process, and it processes delayed. I think you'll want to avoid the spinwait. Try simply invalidating every time you are done painting, and see how that goes. If it doesn't like that you could task delay 0 to make sure the invalidate gets processed after the current layout. To cap the FPS you could keep track of when you had processed the prior frame. When I have a chance I can dig up what I've done in similar circumstances in the past. |
|
Yeah, I tried a few other things, and still get slight input delay that's not there in WinForms / SKGLControl. Using the spinwait loop, I was actually able to get really solid (300+ fps) performance on WPF using the WindowsFormsHost, without any noticeable input lag. But the airspace issues make that nonviable since I need to overlay WPF controls. I also use this exact loop mechanism in my WinForms version of the app, and have no issues. Something on SKGLElement is hampering performance. |
|
Looking at SKGLElement closely, it looks pretty similar to SKGLControl. I wonder if there are some differences in how OpenTK implements GLWPFControl vs GLControl, going to look into it... |
|
@gmurray81 @mattleibow Is this pull request stuck "only" on accepting the CLA, or are there any other problems? As it seems to be working, and it would be very beneficial to have SKGLElement for WPF - one can use it directly from source, but still ... |
There was a problem hiding this comment.
This is all working well and the implementation is nice and simple. Once we get this out and about, people can give feedback on any rough edges.
Thanks for doing this and the updates.
All I have are a few nits because this OpenTK WPF GL control is not something I am familiar with. But if those are non issues, I think this is mergy!
| private bool disposed = false; | ||
|
|
||
|
|
||
| protected virtual void Dispose(bool disposing) |
There was a problem hiding this comment.
WPF UI worlds don't really use Dispose as a mechanism for doing things. Unload is resetting already? I am not 100% confident of how things work in the worlds of heavy graphics objects.
There was a problem hiding this comment.
My recollection is that I was having an issue if the window was closed before the OpenTK stuff was disposed, which is why I was trying to hook the window close... But I haven't been able to repro revisiting it here. Maybe it had to do with me forcing OpenTK to create more than one glcontext associated with the window.
There was a problem hiding this comment.
its conceivable that opentk manages any resources associated with the shared context of the shared window well enough that this code isn't necessary if I'm not spinning up other contexts.... although, now that I'm typing this, I wonder if I was prompted by any multi window wpf scenarios. Been a while since I was first putting this together at this point.
|
I think I just have the last 2 questions:
|
Yes it is going to reset each frame as it is now. As I said in other replies, this is because OpenTK shares a gl context between the WPF controls and we need to clear enough state to avoid crosstalk. I'm almost positive there is a better thing to do here, but I don't know what it is, and don't know how much digging it would take to find out. We could try to force the creation of multiple gl contexts and then make them current before rendering each frame. But it's unclear if we are paying more in context switching than context clearing at that point. I'm not sure I know the right torture tests to evaluate between the two options. For IDisposable we'd have to try without and see if we can repro the problem I was having. Perhaps it was related to when I had multiple GL contexts and there was one not being managed by OpenTK |
|
OK, all makes sense. I will merge this for now. I would like to remove the IDisposable as that is not how the UI elements really work in WPF and other XAML worlds. However, there is no actual harm in it. Just waiting on CI to be a bit happier. I am running a build on main so we shall see if main is even green. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Thank you. Addition, I am rather hopeful for the ability to introduce IDisposable in WPF, as it could alleviate some of the memory pressure on WPF. Therefore, I believe it is reasonable to have IDisposable here, even though I am uncertain if my proposal will ultimately be accepted. See dotnet/wpf#7177 |
| { | ||
| if (grContext != null) | ||
| { | ||
| grContext.ResetContext(); |
There was a problem hiding this comment.
I am thinking about this one... GL has a "make current" concept... I wonder if the draw method is supposed to make the context current before drawing.
I see this which looks familiar, but I have no idea how to get access to this one: https://github.com/opentk/GLWpfControl/blob/516bf697e9c786ab3432156d75e1dfca67d3a18c/src/GLWpfControl/DXGLContext.cs#L127
In the FAQ:
How do I control OpenGL contexts?
On many OSes, there is the notion of an OpenGL context, a container that holds all of the allocated OpenGL resources like textures and shaders and buffers. There is also typically the concept of a current context — there’s usually only one context “active” at a time, and you have to switch between them if you have more than one.
OpenTK exposes this through NativeWindow in the form of its Context property and its MakeCurrent() method. Context is an IGraphicsContext, and includes low-level OpenGL operations like MakeCurrent() and MakeNoneCurrent() and SwapBuffers().
The graphics context is created automatically for you when a NativeWindow is created, so the only thing you may need to do is occasionally invoke MakeCurrent() or MakeNoneCurrent().
(If you only have one OpenGL context, you can ignore all of this.)
There was a problem hiding this comment.
What I tried before was to take a variant of that code you linked from inside OpenTK (that creates the shared context) to create additional contexts, and one per SKGLElement, Then each would make current before rendering the frame. IIRC that worked. But it required essentially recreating the context creation outside of OpenTK. I can't remember if I hit another limitation with it. This would probably work, I'm guessing, but would mean we are creating one GL context per control, and I'm not sure if that is desirable? This may be what the WF is doing under the covers though due to each being a separate HWND presumably? Not sure.
Anyhow, using the shared gl context from each SKGLElement was not working though without resetting the graphics context, since there were left over bits of state (maybe clipping stages?) that were crosstalking between the SKElements.
I'll see if I can find the old code that I was trying with the multiple gl contexts. I believe I posted it to discord in the past when I was asking for advice. The GL context creation was some variant of what is happening in that code you linked though.
There was a problem hiding this comment.
We can merge this with the release for now, and then in a separate PR benchmark and see if switching context is better. I don't think we need to make a new context, but somehow we need to make sure the one for the control is make current before drawing.
When you create a new GRContext, it calls make current internally, so this is what is probably making it work.
There was a problem hiding this comment.
Maybe there is a bug in the GL WPF Control, so we can also investigate that path. But, that can be done later.
|
I just noticed this:
This is going to be a pain now because I will have to remove the strong name from the WPF views. This is a breaking change, but I am hoping we can get away with it. It is annoying that the string name was dropped in 4.0, but it is here to stay. |
Sorry @mattleibow I just realized a literal ton of my comments were sitting in pending because I hadn't submitted the review. Whoops!! |
|
For the strong name removal, just add |
I turned off the signing for dotnet core, but left it on for DNF WPF, since that's targeting 3.x which I assume is still strong named? If that isn't the case, I could turn it off for both. |
|
I'll try yet again to get main green. :) |
|
Test failures are unrelated so this PR should not affect anything. |
|
Thanks for this PR and all the effort to keep it going. |
|
Nice, thanks for merging! |
Description of Change
This adds a GL version of SKElement to SkiaSharp.Views.WPF utilizing OpenTK.GLWPFControl
API Changes
Added:
class SKGLElementNew Dependencies for SkiaSharp.Views.WPF: OpenTK, OpenTK.GLWPFControl
PR Checklist
Fixes