osu-framework icon indicating copy to clipboard operation
osu-framework copied to clipboard

Add support for deferred SDL3 clipboard callbacks (and use it for images)

Open Susko3 opened this issue 1 year ago • 10 comments

  • Alternative to / closes https://github.com/ppy/osu-framework/pull/6241

Improvements on previous PR

  • made in a generic way to support future expansions
    • e.g. for https://github.com/ppy/osu-framework/pull/6223
  • data is not copied when crossing the unmanaged boundary
  • SDL_HasClipboardData is checked before getting the data

How it works

All the required data for the clipboard callback is saved to a ClipboardCallbackContext object. An ObjectHandle is allocated for that object to keep it alive when it's owned by SDL (ownership is transferred upon successfully calling SDL_SetClipboardData).

When another application (or we) request clipboard data, the clipboard content is generated and the resulting ReadOnlyMemory<byte> is pinned (so the GC doesn't move it) and its pointer is passed to SDL.

When new clipboard data is set, our cleanup callback gets called so we unpin the ReadOnlyMemory and free the ObjectHandle.

Tested on

  • [x] Windows (with BMP format, still uses WindowsClipbard)
  • [ ] macOS (still uses MacOSClipboard)
  • [x] X
  • [ ] Wayland

Please make sure that TestSceneClipboard passes and that copying and pasting images to/from external apps also works.

iOS and Android are not supported (clipboard text only)

Susko3 avatar Apr 19 '24 19:04 Susko3

The Debug.Assert fails on my Linux machine. image

This might not be due to not supporting the image format though. Here, dataCallback wrongly assumes that it'll only get called once with the requested mimetype, but on Linux it gets called multiple times with a different mimetype each time, even though we pass an array of mimetypes with only one element in it as an argument to SDL_SetClipboardData.

I'd recommend integrating the logic I wrote in my latest commit to solve this issue. Apparently the image could be dropped at any time since we don't own it, so the only solution I see for this is to copy the Image itself in SetImage and pass that to the callback context.

Speykious avatar Apr 19 '24 19:04 Speykious

Seems to fail for BMP files on Wayland (PNG, JPEG work fine):

[runtime] 2024-04-19 20:01:01 [verbose]: SDL error log [debug]: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: Failed to get SDL clipboard data for image/png. SDL error: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: SDL error log [debug]: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: Failed to get SDL clipboard data for image/jpeg. SDL error: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: SDL error log [debug]: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: Failed to get SDL clipboard data for image/bmp. SDL error: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: SDL error log [debug]: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: Failed to get SDL clipboard data for image/tiff. SDL error: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: SDL error log [debug]: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: Failed to get SDL clipboard data for image/webp. SDL error: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: SDL error log [debug]: Pipe timeout
[runtime] 2024-04-19 20:01:01 [verbose]: Failed to get SDL clipboard data for image/qoi. SDL error: Pipe timeout
[runtime] 2024-04-19 20:01:01 [error]: Step "retrieve image from clipboard" SingleStepButton triggered an error
[runtime] 2024-04-19 20:01:01 [error]: System.NullReferenceException: Object reference not set to an instance of an object.
[runtime] 2024-04-19 20:01:01 [error]: at osu.Framework.Tests.Visual.Platform.TestSceneClipboard.<TestImage>b__15_5() in /home/jannik/Development/osu-framework/osu.Framework.Tests/Visual/Platform/TestSceneClipboard.cs:line 73
[runtime] 2024-04-19 20:01:01 [error]: at osu.Framework.Testing.Drawables.Steps.SingleStepButton.<.ctor>b__1_0() in /home/jannik/Development/osu-framework/osu.Framework/Testing/Drawables/Steps/SingleStepButton.cs:line 19
[runtime] 2024-04-19 20:01:01 [error]: at osu.Framework.Testing.Drawables.Steps.StepButton.PerformStep(Boolean userTriggered) in /home/jannik/Development/osu-framework/osu.Framework/Testing/Drawables/Steps/StepButton.cs:line 124
[runtime] 2024-04-19 20:01:01 [error]: at osu.Framework.Testing.Drawables.Steps.StepButton.OnClick(ClickEvent e) in /home/jannik/Development/osu-framework/osu.Framework/Testing/Drawables/Steps/StepButton.cs:line 98
[runtime] 2024-04-19 20:01:01 [debug]: ClickEvent(Left) handled by "retrieve image from clipboard" SingleStepButton.
[runtime] 2024-04-19 20:01:01 [debug]: MouseClick handled by "retrieve image from clipboard" SingleStepButton.

FreezyLemon avatar Apr 19 '24 20:04 FreezyLemon

Apparently the image could be dropped at any time since we don't own it, so the only solution I see for this is to copy the Image itself in SetImage and pass that to the callback context.

If we're copying the image, we're going to allocate memory (and might do some processing to change the pixel format) -- might as well save it to the clipboard directly.

Susko3 avatar Apr 19 '24 20:04 Susko3

If we're copying the image, we're going to allocate memory (and might do some processing to change the pixel format) -- might as well save it to the clipboard directly.

I think I was for whatever reason thinking about returning the image in multiple different formats at the same time, but I guess it's unnecessary...

Speykious avatar Apr 19 '24 20:04 Speykious

That being said, it still fails for me. :( image

Speykious avatar Apr 19 '24 20:04 Speykious

@FreezyLemon your logs are a bit concerning. SDL_HasClipboardData is returning true, but then getting the data fails. Of course, it's not guaranteed that if SDL_HasClipboardData == true then SDL_GetClipboardData will always have some data (TOCTOU, etc.), but it failing in such a simple case warrants a bug report upstream.

Susko3 avatar Apr 19 '24 20:04 Susko3

That being said, it still fails for me. :(

Right, the callback can get called multiple times. SDL internally copies the BMP data on windows.

Susko3 avatar Apr 19 '24 20:04 Susko3

@FreezyLemon your logs are a bit concerning. SDL_HasClipboardData is returning true, but then getting the data fails. Of course, it's not guaranteed that if SDL_HasClipboardData == true then SDL_GetClipboardData will always have some data (TOCTOU, etc.), but it failing in such a simple case warrants a bug report upstream.

The pipe timeout does feel like an upstream bug, yeah. I'll try and create a repro and then report it.

FreezyLemon avatar Apr 19 '24 22:04 FreezyLemon

X11

Seems to work fine on X.

bdach avatar Apr 22 '24 09:04 bdach

macOS should have full support for all clipboard data, but I'm mostly wondering which image formats are supported and compatible with other apps. Looking online, I see tiff, png and jpg mentioned.

We can always put multiple formats on the clipboard 😉

diff --git a/osu.Framework/Platform/MacOS/MacOSGameHost.cs b/osu.Framework/Platform/MacOS/MacOSGameHost.cs
index 0cb6f2362..45657c24a 100644
--- a/osu.Framework/Platform/MacOS/MacOSGameHost.cs
+++ b/osu.Framework/Platform/MacOS/MacOSGameHost.cs
@@ -11,6 +11,10 @@
 using osu.Framework.Input.Bindings;
 using osu.Framework.Input.Handlers;
 using osu.Framework.Input.Handlers.Mouse;
+using osu.Framework.Platform.SDL;
+using SixLabors.ImageSharp.Formats.Jpeg;
+using SixLabors.ImageSharp.Formats.Png;
+using SixLabors.ImageSharp.Formats.Tiff;
 
 namespace osu.Framework.Platform.MacOS
 {
@@ -35,7 +39,7 @@ public override IEnumerable<string> UserStoragePaths
             }
         }
 
-        protected override Clipboard CreateClipboard() => new MacOSClipboard();
+        protected override Clipboard CreateClipboard() => new SDL3Clipboard(TiffFormat.Instance /* or PngFormat, JpegFormat */);
 
         protected override ReadableKeyCombinationProvider CreateReadableKeyCombinationProvider() => new MacOSReadableKeyCombinationProvider();
 

Susko3 avatar Apr 23 '24 11:04 Susko3

I've also tested macOS with SDL3Clipboard + PngFormat.Instance. Not sure what else to test here.

smoogipoo avatar May 08 '24 12:05 smoogipoo

On both Wayland and macOS, if I have something already in the clipboard, SetClipboardText("") doesn't clear it. The return code is 0 and there's no error in SDL_GetError().

SDL_ClearClipboardData() also seems to do nothing in this case, I assume that's because it's clearing data and not text.

Looks like an SDL issue.

smoogipoo avatar May 08 '24 13:05 smoogipoo

On both Wayland and macOS, if I have something already in the clipboard, SetClipboardText("") doesn't clear it. The return code is 0 and there's no error in SDL_GetError().

SDL_ClearClipboardData() also seems to do nothing in this case, I assume that's because it's clearing data and not text.

Looks like an SDL issue.

Definitely an SDL issue as SetClipboardText() is just shorthand for SDL_SetClipboardData with text/plain mime type. Plus, calling SetClipboardText(NULL) or SetClipboardText("") actually calls SDL_ClearClipboardData()

Susko3 avatar May 09 '24 19:05 Susko3