Skia Canvas plugin for hardware-accelerated 2D rendering in SWT#3231
Skia Canvas plugin for hardware-accelerated 2D rendering in SWT#3231DenisUngemach wants to merge 9 commits into
Conversation
Integrates an SWT’s canvas extension framework, which replacing the native backend for canvases with the SWT.SKIA style bit and leveraging OpenGL. - Includes core classes for rendering - resource management - a factory for SWT integration. - Implements caching for fonts, images, and text, supports DPI scaling, and manages Skija resources explicitly. - The plugin is modular, minimizes API changes, and is OSGi-activatable. - for more information read the skia.md in bundles/org.eclipse.swt.skia
- for adding the SWT.SKIA field, the version of SWT should be increased - there seams to be a bug on the api tools for sealed classes. Locally on my system, there are no api bugs for handle etc. But on github i get error messages, that handle etc are used. this only happens for sealed classes.
|
Referring to the issue: #2616 |
|
I think we need another PR to add the Skia libraries to the buiild. @HeikoKlare can you guide what needs to be done for that? |
In the current change the source jars will also be loaded to the lib folder. On my builds this was only a warning, it seems here it is an error. |
From my understanding, we need to add the library to the eclipse-sdk-prereqs target, so they become available in our target platform. Just like we did here for the JSVG library used in the SWT SVG fragment: I can provide a PR once I find the time to do so. But feel free to just do it inspired by the linked PR even earlier. |
Only causes warnings and errors. These are not necessary in the org.eclipse.swt.skia plugin. Only convenience.
Is this right: swt-initiative31/eclipse.platform.releng.aggregator#1 Of course i will open then the PR to the original releng repo, if this should do it. |
|
|
||
| @Override | ||
| public Rectangle getClipping() { | ||
| return currentClipBounds; |
There was a problem hiding this comment.
The clip returned by this method should never be null, even if no clipping is set. From the JavaDoc:
If no clipping region is set, the return value will be a rectangle which covers the entire bounds of the object the receiver is drawing on.
| return currentClipBounds; | |
| if (currentClipRegion != null) { | |
| return currentClipBounds; | |
| } | |
| return canvas.getBounds(); |
Here an example that throws an exception with Skia but not with GTK:
public class Test {
public static void main(String[] args) {
Shell shell = new Shell();
shell.setSize(200, 200);
shell.setLayout(new FillLayout());
Canvas canvas = new Canvas(shell, SWT.SKIA);
canvas.addPaintListener(event -> {
GC gc = event.gc;
Objects.requireNonNull(gc.getClipping());
});
shell.open();
Display display = shell.getDisplay();
while (!shell.isDisposed()) {
if (!display.readAndDispatch()) {
display.sleep();
}
}
}
}| private boolean xorModeActive; | ||
| private int lineWidth = 1; | ||
| private int lineCap = SWT.CAP_FLAT; | ||
| private int lineStyle; |
There was a problem hiding this comment.
If the line style is never set, 0 is returned, which is an illegal value.
| private int lineStyle; | |
| private int lineStyle = SWT.LINE_SOLID; |
This will throw an IllegalArgumentException with Skia, but not with GTK.
public class Test {
public static void main(String[] args) {
Shell shell = new Shell();
shell.setSize(200, 200);
shell.setLayout(new FillLayout());
Canvas canvas = new Canvas(shell, SWT.SKIA);
canvas.addPaintListener(event -> {
GC gc = event.gc;
gc.setLineStyle(gc.getLineStyle());
});
shell.open();
Display display = shell.getDisplay();
while (!shell.isDisposed()) {
if (!display.readAndDispatch()) {
display.sleep();
}
}
}
}
|
I'm currently getting a consistent crash with the Draw2D TreeExample when changing the minor/major spacing. I'm not quite sure yet what exactly is going wrong, but it seems like the Skia surface is being disposed. Immediately before the crash, the following returns Perhaps as a first step, the following check needs to be added to every method, similar to the other GC's? Here the crash log: |
|
I also had a quick look at how it looks with the GEF Logic Editor and most figures seem to be drawn as expected. There seems to be an issue when using 
|
|
@ptziegler Thanks a lot for the support. I have already some changes that must be added to the PR. I will also check your remarks. |
- GC.getClipping never returns null - standard line stype is solid - if fonts loading fails, it uses the standard SWT font. - if in Canvas scroll is used, the externalCanvas handler redraws. - image cache is by default off (image caching can cause bugs), text image works fine.
|
-> Rebase But this makes it much more difficult to load other targets and still use org.eclipse.swt.skia. I guess using the update site https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/milestone/S202604250819 in other targets should work. |
|
I see no reason not to commit the TP changes from. Oops, I hit some buttons thinking I was updating the branch and actually did merge it now. 😬 But I don't think that should be a problem. Those IUs will not be pulled into the final product or into the p2 repository until something bundle/fragment/feature actually requires it... I guess that solves the current problem though? 😀 |
@merks Of course i am completely fine with that 😀. I will then check the last bug from @ptziegler where the surface is closed and the SkiaGC still seems to be in use. |
|
Apologies for the slow response. I wanted to further test the extension but didn't really get the chance.
I don't have a simple reproducer yet but as far as I can tell, the problem is that a resize event may be fired within the paint event. Because the resize event invalidates the Skija surface, it also invalidates the GC of the paint event. Below a screenshot I got during debugging. As you can see the event is fired while painting, specifically when enabling/disabling the scrollbar. 
Either the local variable in the |
|
Just as an open question: How difficult would it be to hook the SkiaGC into the GC tests? |
This example should print public class Test {
public static void main(String[] args) {
Shell shell = new Shell();
shell.setLayout(new FillLayout());
shell.setSize(400, 400);
Canvas canvas = new Canvas(shell, SWT.SKIA);
canvas.addPaintListener(event -> {
GC gc = event.gc;
System.out.println(gc.isDisposed());
canvas.setSize(100, 100);
System.out.println(gc.isDisposed());
gc.fillRectangle(0, 0, 100, 100);
});
shell.open();
Display display = shell.getDisplay();
while(!shell.isDisposed()) {
while(!display.readAndDispatch()) {
display.sleep();
}
}
}
}If you set a breakpoint inside the |
Do not listen to resize actions and resize the skia surface there. It is possible that this call comes from not main thread and this causes crashes. Instead check in the paint workflow whether the canvas client area changed and recreate the surface if the canvas client area was modified there. There we are always in the right thread.
|
Hello, the resize bug should be fixed now. Now the surface will be created and recreate in the paint workflow, if it is necessary. But i think the plugin setup must be improved. Executing the tests of org.eclipse.swt.skia in the IDE fails now because the build path does not work properly. |
|
What's wrong with the build path? What can I do to help? Are you using an Oomph setup; if not then all bets are off because you'll need to do everything, whatever that may be, manually.... |
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
I've checked locally for what is still wrong with the project configurations. I have made some comments in this PR pointing to some of the issues, but the whole bunch of changes I tried is covered by this commit: HeikoKlare@b42bcb6
I propose to:
- Make
org.eclipse.swt.skiaa separate bundle instead of a fragment. We need to properly configure access to internals oforg.eclipse.swtfor that (I just silenced the errors in my commit), but that should not be a problem - Clean up the build path of
org.eclipse.swt.skia: there is a mixture of PDE dependencies, explicit libraries on the classpath and Maven dependencies; PDE dependencies are even exported; this can now all be reduced to simple PDE dependencies with the Skija libraries being available via target platform - Move the Skia tests into a separate test bundle. Than the additional test-specific dependencies can also be removed from the
org.eclipse.swt.skiabundle. Though this is not essentially necessary, it should make everything cleaner (but I would also be fine with leaving the tests in the production bundle as they are now).
What is not working for me is the DLL library loading mechanism of Skija when not explicitly including the win32 Skija fragment in the launch configuration. I would have expected it to be properly picked up via the p2.inf setting, but somehow it does not. However, manually adding it to the launch configuration makes the tests work for me:
| Bundle-Localization: fragment | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-21 | ||
| Eclipse-ExtensibleAPI: true | ||
| Fragment-Host: org.eclipse.swt;bundle-version="[3.134.0.qualifier,4.0.0)" |
There was a problem hiding this comment.
We should make a standalone bundle out of this.
| Bundle-ClassPath: lib/skija-linux-x64-0.143.11.jar, | ||
| lib/skija-shared-0.143.11.jar, | ||
| lib/skija-windows-x64-0.143.11.jar, | ||
| lib/types-0.1.1.jar, | ||
| lib/, | ||
| ./ |
There was a problem hiding this comment.
These can be removed as they are now provided via target platform.
| @@ -0,0 +1,18 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <classpath> | |||
| <classpathentry exported="true" kind="con" path="org.eclipse.pde.core.requiredPlugins"/> | |||
There was a problem hiding this comment.
The plug-in dependencies container must not be exported. This will lead to problems in projects depending on this (such as a test project).
| <attribute name="test" value="true"/> | ||
| </attributes> | ||
| </classpathentry> | ||
| <classpathentry kind="con" path="org.eclipse.jdt.junit.JUNIT_CONTAINER/5"/> |
There was a problem hiding this comment.
I propose to extract the tests into a separate test bundle. Then we don't need this dependency.
I just found what's missing for this (and what is different with SWT). Why it works for SWT: the fragments are automatically picked up in a launch because the SWT host declares the Possible solution for Skija: We want to express that when launching something with a dependency to skija-shared, a fitting native fragment must be present and loaded as well. The "fitting" aspect is already covered by the Platform-Filter. The "must be present" aspect is missing. This is something we can express with OSGi capabilities. I've locally added a If someone sees anything wrong in that approach, please let me know. Otherwise I can try to provide a PR to Orbit for adding that metadata to the wrapped bundles/fragments as soon as I find the time. |
|
See here for the idea of using capabilities to properly include the Skija fragments with native libraries: |
5 participants
Skia Canvas plugin for hardware-accelerated 2D rendering in SWT.
[Experimental Feature]
Integrates an SWT’s canvas extension framework, which replaces the
native backend for canvases with the Skia drawing framework which relies in this implementation on
OpenGL.
adds a new fragment of org.eclipse.swt: bundles/org.eclipse.swt.skia
Includes core classes for rendering
resource management
a factory for SWT integration with the ServiceLoader
Implements caching for fonts, images, and text, supports DPI scaling
The plugin is modular an minimizes API changes
there are some tests in the org.eclipse.swt.skia plugin
for more information read the skia.md file in bundles/org.eclipse.swt.skia
uses the service loader, if there are bugs at the loading, the skia plugin will be ignored.
Since the API was modified the SWT version should be increased (still to do)
there seems to be an api tools bug because of sealed classes. This happens only in the github PR checks.
The second commit contains only the ignores for the api tools. It can be reverted and then the versioning could be fixed properly.