Make SDL3Clipboard support images #6241

Speykious · 2024-04-13T12:25:18Z

This PR supersedes #6151 which aimed at adding support for copying and pasting images from the clipboard on Linux, but had the wrong approach of calling into an external executable (xclip for X11).

In the mean time I had actually started a native clipboard library in Rust for applications that would support any mime types and be given arbitrary data (the X11 implementation was somewhat finished, but not Wayland), but since osu!framework now uses SDL3, I can just use SDL_GetClipboardData and SDL_SetClipboardData as suggested back then by @Susko3.

This is a first attempt at implementing this, and on my Linux machine, the visual tests for copying images are all green! :D
~~Edit: actually doesn't work every time! D: I'll have to really refactor this code and make sure I understand what's going on.~~
Edit 2: bug identified, it seems to come from the given image itself...

~~That said, I really don't know what to think of this code, [...]~~ I think the code looks ok now :)

Speykious · 2024-04-13T12:31:07Z

There's also the caveat that I only implemented a way to copy PNGs. Wouldn't it also need to be able to copy JPGs depending on some setting? I know osu! lets you choose the image format for screenshot savings.

smoogipoo · 2024-04-13T12:50:33Z

What you have looks pretty standard for the new age of unmanaged->managed callbacks. That being said, this PR now competes with #6223 and we should probably make sure that this implementation is going to easily lend itself to other mime types in the future.

FreezyLemon · 2024-04-13T13:10:21Z

osu.Framework/Platform/SDL/SDL3Clipboard.cs

+            unsafe
+            {
+                nuint size = 0;
+                byte* data = (byte*)SDL3.SDL_GetClipboardData(Encoding.UTF8.GetBytes("image/png"), &size);


Maybe SDL3-CS handles this automagically somehow, but I think you're missing a null terminator here (Pretty sure the marshaller only does this for string-typed pinvoke-parameters). The byte[] returned by Encoding.GetBytes does not include a null byte at the end.

Would "image/png\0"u8 work better then?

Yeah, that should work

Speykious · 2024-04-13T15:37:49Z

I debugged a lot today (both with a debugger and with prints) and concluded that pngData was fine as a static due to how the API works. It only calls for a cleanup once if we're requesting for an image again.

Pass unmanaged struct to callbacks and free it on cleanup

Speykious · 2024-04-14T11:17:22Z

It seems that sometimes the test gives a completely empty screenshot. I was worried that my implementation had some weird undefined behavior thing, but it seems like that bug is happening elsewhere...?

Speykious · 2024-04-14T11:28:06Z

~~It seems I'm missing something related to the unmanaged call convention~~
Solved, it was because I was using C#'s collection expression syntax... oh well :/

less finnicky than creating a managed string every time

Apparently I can't use C#'s collection expression syntax here :(

Susko3 · 2024-04-15T13:30:34Z

I'm testing this on windows (by commenting out the WindowsClipboard override) and I can't get images from osu!framework into external programs (and vice versa). The clipboard tests pass fine, but applications complain about there being no image in the clipboard. And trying to paste images into framework doesn't work: either I get the stale image (last one copied from framework) or no image at all (even when copying a sample PNG from the internet).

Does copying images to/from framework into external programs work on Linux?

Speykious · 2024-04-15T13:33:12Z

It does work on Linux (at least on my machine on X11), as long as there is a clipboard manager present. If there is no clipboard manager, the selection will disappear once the application closes, which is expected behavior.

I don't know why it doesn't work on Windows though. :/

Susko3 · 2024-04-15T14:15:06Z

Works perfectly fine on windows if the mime type is changed to image/bmp and the image is saved with .SaveAsBmp(). Do bitmaps work on Linux too?

I wonder if it works on macOS (which currently uses MacOSClipboard) and iOS (which uses SDL3Clipboard).

FreezyLemon · 2024-04-15T14:32:47Z

Works perfectly fine on windows if the mime type is changed to image/bmp and the image is saved with .SaveAsBmp(). Do bitmaps work on Linux too?

I wonder if it works on macOS (which currently uses MacOSClipboard) and iOS (which uses SDL3Clipboard).

At least on Wayland, the mime type also has to match. If I e.g. copy a PNG from my local storage, the current code works fine. If I use a Paint-like program to scribble around, Ctrl-A and Ctrl-C, then it's also saved as an image/bmp and needs that exact mime type to be retrieved from SDL.

The obvious solution would be to just have a list of image mimetypes that are tried one after another. FWIW, SDL itself also does this for the built-in GetClipboardText function.

Speykious · 2024-04-15T15:00:15Z

I'll try to implement a multi-mime-types solution.

Susko3 · 2024-04-15T15:08:24Z

We should probably ask ImageSharp about its supported decoder formats, see SixLabors/ImageSharp#1353 (but with IImageFormat.MimeTypes).

This is what I get on windows:

image/png
image/apng
image/jpeg
image/pjpeg
image/gif
image/bmp
image/x-windows-bmp
image/x-portable-pixmap
image/x-portable-anymap
image/x-tga
image/x-targa
image/tiff
image/tiff-fx
image/webp
image/qoi
image/x-qoi
image/vnd.qoi

Susko3

I'd rather rewrite this PR then to try and fix it.

Susko3 · 2024-04-15T15:13:04Z

osu.Framework/Platform/SDL/SDL3Clipboard.cs

+                nuint size = 0;
+                byte* data = (byte*)SDL3.SDL_GetClipboardData("image/png\0"u8, &size);
+                if (data == null)
+                    return null;
+
+                buffer = new byte[size];
+                for (nuint i = 0; i < size; i++)
+                    buffer[i] = data[i];


This should use Marshal.Copy.

Susko3 · 2024-04-15T15:15:44Z

osu.Framework/Platform/SDL/SDL3Clipboard.cs

+        private unsafe struct ClipboardImageData
+        {
+            public byte* RawBuffer;
+            public nuint Length;
        }


Can probably be done much cleaner with a GCHandleType.Pinned ObjectHandle<byte[]>.

Similar to:

osu-framework/osu.Framework/Audio/Sample/SampleBassFactory.cs

Lines 84 to 85 in 32e7571

using (var handle = new ObjectHandle<byte[]>(data, GCHandleType.Pinned))

SampleId = Bass.SampleLoad(handle.Address, 0, dataLength, PlaybackConcurrency.Value, flags);

Passing around a byte[] means the copying will happen in the callback and not before.
But I might end up using an ObjectHandle anyways to support multiple mime types. Only this time it'll need to be the Image itself.

This is trickier than I expected.
In the SampleBassFactory code, the handle is created with using, so it gets freed immediately after SampleLoad gets called. This is fine because SampleLoad makes a copy of the data immediately - from its documentation:

There is no need to pin the memory buffer for this method, since after loading a sample from memory, the memory can safely be discarded, as a copy is made.

But I can't just free the ObjectHandle immediately after SDL_SetClipboardData gets called. Instead I have to free it inside of the cleanup callback, which will be called with the old userdata before each new call to SDL_SetClipboardData.
And unfortunately, I can't just do it manually with .Dispose() either, because disposing an object handle obtained from a nint won't actually free the object handle, we can only free it on the original one.

I feel like now I need to have two swapping global variables (due to how the cleanup logic works), which would make an object handle unnecessary...

I feel like now I need to have two swapping global variables

Welp, this is what I have currently

Susko3 · 2024-04-15T15:17:33Z

osu.Framework/Platform/SDL/SDL3Clipboard.cs

+            fixed (byte* pngMimetypeStr = "image/png\0"u8)
+            {
+                if (SDL3.SDL_strcmp(mimeType, pngMimetypeStr) == 0)


There is no reason to delegate string comparison to SDL. Better to decode the pointer with SDL3.PtrToStringUTF8 and use == on C# strings.

By using SDL_strcmp, I don't need to copy any data over before doing the comparison.

I ended up doing it anyway to be able to use ImageFormatManager.TryFindFormatByMimeType :)

Speykious · 2024-04-18T10:40:29Z

Works perfectly fine on windows if the mime type is changed to image/bmp and the image is saved with .SaveAsBmp().

@Susko3 Can you tell me if this works on Windows now?

Speykious · 2024-04-18T11:01:38Z

In any case, I can confirm that it works on MacOS.

Susko3 · 2024-04-19T19:30:27Z

Thanks for the PR, I've improved on your idea and made it into #6254.

Please give it a test and see if the BMP format works for you. If not, I can change it to PNG.

Implement SDL3Clipboard.GetImage and SDL3Clipboard.SetImage

dcd9cb9

pull-request-size bot added the size/M label Apr 13, 2024

Speykious mentioned this pull request Apr 13, 2024

Implement Linux clipboard with support for images #6151

Closed

Fix styling

44c0319

FreezyLemon reviewed Apr 13, 2024

View reviewed changes

zero-terminate mime type string

ad5a938

Speykious added 2 commits April 14, 2024 13:13

Hack: do not get clipboard data if it is text

e69d13f

Better SetImage implementation

a7fc6ca

Pass unmanaged struct to callbacks and free it on cleanup

Speykious marked this pull request as ready for review April 14, 2024 11:19

Speykious added 4 commits April 14, 2024 13:41

Let strings stay unmanaged in the callback

9a5ee5b

less finnicky than creating a managed string every time

Forgot zero termination again...

ab6c051

EntryPoint specification is not needed actually

6118317

Eliminate calling convention compatibility errors

40f9383

Apparently I can't use C#'s collection expression syntax here :(

Susko3 self-requested a review April 15, 2024 08:13

Susko3 suggested changes Apr 15, 2024

View reviewed changes

Speykious added 3 commits April 15, 2024 17:37

Use Mashal.Copy to copy data buffer

d649b97

Get and set multiple image formats

fe89a69

Merge branch 'master' into sdl3-clipboard-data

81754af

Susko3 mentioned this pull request Apr 19, 2024

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

Merged

4 tasks

Susko3 closed this Apr 19, 2024

	using (var handle = new ObjectHandle<byte[]>(data, GCHandleType.Pinned))
	SampleId = Bass.SampleLoad(handle.Address, 0, dataLength, PlaybackConcurrency.Value, flags);

Make SDL3Clipboard support images #6241

Make SDL3Clipboard support images #6241

Uh oh!

Conversation

Speykious commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Speykious commented Apr 13, 2024

Uh oh!

smoogipoo commented Apr 13, 2024

Uh oh!

FreezyLemon Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Speykious commented Apr 13, 2024

Uh oh!

Speykious commented Apr 14, 2024

Uh oh!

Speykious commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susko3 commented Apr 15, 2024

Uh oh!

Speykious commented Apr 15, 2024

Uh oh!

Susko3 commented Apr 15, 2024

Uh oh!

FreezyLemon commented Apr 15, 2024

Uh oh!

Speykious commented Apr 15, 2024

Uh oh!

Susko3 commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Susko3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Speykious Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Speykious commented Apr 18, 2024

Uh oh!

Speykious commented Apr 18, 2024

Uh oh!

Susko3 commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Speykious commented Apr 13, 2024 •

edited

Loading

FreezyLemon Apr 13, 2024 •

edited

Loading

Speykious commented Apr 14, 2024 •

edited

Loading

Susko3 commented Apr 15, 2024 •

edited

Loading

Speykious Apr 15, 2024 •

edited

Loading

Susko3 commented Apr 19, 2024 •

edited

Loading