Add clipboard support #13837

rcombs · 2024-04-08T07:59:02Z

Add internal routines for clipboard access
Expose a screenshot-to-clipboard command
Expose a clipboard property

This is currently macOS-only; patches welcome for other platforms.

github-actions · 2024-04-08T08:52:56Z

Download the artifacts for this pull request:

Windows

macOS

na-na-hi · 2024-04-08T11:12:07Z

DOCS/man/input.rst

+
+ The top-level object itself can be written directly as a shortcut.
+
+ Converting this property to a string will give a JSON representation of all types.


How do you plan to represent image data with this? Base64?

I don't expect raw image data to appear in the string representation; it'd only be useful to API clients interacting with binary directly (a la screenshot-raw).

na-na-hi · 2024-04-08T11:17:51Z

The API in the current form is too simplistic: it will never work with X11 or Wayland.

On these platforms, there is no system storage for clipboard data: clipboard is simply an IPC mechanism where X11/Wayland clients listen to data request events, for which the data owning clients respond by sending the data directly to the requesting client via a pipe. Thus the data cannot be freed after a set clipboard call, and an active X11/Wayland connection must be kept open.

rcombs · 2024-04-08T11:21:41Z

Hmmm, either the clipboard code could statically spawn a thread that keeps running, or these routines could add an mpctx arg.

na-na-hi · 2024-04-08T11:34:23Z

I think a separate thread dedicated to clipboard event handling is the only realistic option if the clipboard doesn't live in the VO thread.

Getting the clipboard on these platforms has the same problem: X11/Wayland data requesting clients need to listen to data available events, and respond them by reading from a pipe, which can potentially block indefinitely.

rcombs · 2024-04-08T18:09:59Z

So, it sounds like these routines will want to take a currently-unused mpctx, and when adding linux support, someone will need to add functions that set up and tear down a thread? And the thread can read in clipboard data as it becomes available, and make it available for the get routine to return?

Also, does anyone care about firing events on clipboard updates? On macOS, this would require polling (though very low-cost polling; there's an integer changeCount value we could check on each tick or something).

rcombs · 2024-04-08T21:19:07Z

Rebased with some improvements:

All routines now take an MPContext* arg, to better support platforms like X or Wayland which might require more persistent state
The clipboard property is now observable
The text member of m_clipboard_item's union is now named string (for usage with types that aren't simple plain text)
New types CLIPBOARD_PATH and CLIPBOARD_URL are added
String types now have property type STRING, so they won't be quoted when used in raw mode

na-na-hi · 2024-04-09T01:45:43Z

Also, does anyone care about firing events on clipboard updates? On macOS, this would require polling

AFAIK none of the other platforms require polling: event-based clipboard monitoring is available on Windows, X11, and Wayland.

The problem is that on Wayland, this can only be done with the wlr data control extension, which requires KWin or wlroots-based compositors, and is unusable because I think the current policy is to avoid vendor-specific extensions.

Otherwise, if the clipboard needs to be usable without a VO, even polling for clipboard updates is practically impossible on Wayland, because only focused windows can get any clipboard contents on Wayland. The only way to do that "headless" is to actually create an invisible 1x1 px popup window just to grab focus and get access to the clipboard, and destroy it immediately after. On tiling WMs this is especially unacceptable. I know this sounds completely idiotic, but this is what wl-clipboard does, and what Wayland is.

@Dudemanguy
Any thoughts on clipboard handling on Wayland? Should the idea of "headless" clipboard handling be ditched altogether?

Dudemanguy · 2024-04-09T02:34:36Z

Don't let wayland sucking hold other platforms back. I have no idea how this should work there. Honestly if the only decent way is that specific extension (never looked at it), vendoring it for our purposes is probably fine.

na-na-hi · 2024-04-09T03:12:38Z

Honestly if the only decent way is that specific extension (never looked at it), vendoring it for our purposes is probably fine.

If this can be done, then it's good news since this means that Windows and Linux can use the same clipboard event monitoring thread approach. Just want to confirm. Although that extension is marked for use by "privileged clients" so I'm not sure if it's OK for mpv to continuously monitor the clipboard in the background.

na-na-hi · 2024-04-09T03:18:22Z

osdep/clipboard.h

+int m_clipboard_get(struct MPContext *ctx, struct m_clipboard_item *item);
+int m_clipboard_set(struct MPContext *ctx, const struct m_clipboard_item *item);
+bool m_clipboard_poll(struct MPContext *ctx);


For Windows and Linux, an event monitoring thread will be used, and data need to be kept alive for Linux, so there needs to be init/uninit functions for the clipboard stuffs so that mpv can clean up the mess when quitting. How about making them take a struct clipboard_ctx parameter with the init and uninit functions? They don't need to do anything on macOS.

Also on Linux, since this clipboard API is headless and has no VO dependence, it's possible that both X11 and Wayland servers are running, so it requires two separate monitoring threads to handle requests from both window systems. I'm not sure if this means that the API needs a mechanism for multiple clipboard sources.

Re: context, I currently have struct clipboard_state inited at startup and owned by MPContext, and it'd be easy enough to add an explicit uninit at shutdown. I don't think that needs to be added in this PR, though, since nothing in it precludes adding explicit shutdown function later.

Re: X/Wayland, I think we'd want to either have both run continuously (can they run in a single thread, select()ing both?) and return data from whichever was updated most recently, or have a user setting for which to use (defaulting to whichever we'd use for vo).

can they run in a single thread, select()ing both?

It's possible, but it will significantly complicate the implementation. This is especially true for large data transfers on X11, which manifest as a complicated multi-step process, for which the transfer state need to be saved and resumed between calls.

Personally I would like to limit clipboard to 1 backend at a time, to keep it sane. But dynamic switching would need to be implemented, like for VOs. That switching needs to take care of init/uninit on backend switches.

(Side note: not sure about the situation on macOS, but is XQuartz supported by mpv? That would mean that macOS has multiple clipboard backends too.)

Personally I would like to limit clipboard to 1 backend at a time, to keep it sane.

Agreed.

I think a reasonable way would be to delegate/marry this to a VO. Then the event loop can be reused to handle the necessary clipboard protocol.
Then add a way to have a global clipboard manager for the sake of win32 and mac (where this doesn't depend on a VO).

I think a reasonable way would be to delegate/marry this to a VO. Then the event loop can be reused to handle the necessary clipboard protocol.

Note that win32 also benefits from this: win32 clipboard notification requires a window, so without VO dependence, it is achieved by creating a dummy event-only window and wait for clipboard change messages. If it is delegated to the VO window instead, the window event loop can be reused.

This also provides a reasonable way out for Wayland compositors which don't support the wlr data control protocol.

Microsoft documentation explicitly discourages polling GetClipboardSequenceNumber():

Note that this is a not a notification method and should not be used in a polling loop. To be notified when clipboard contents change, use a clipboard format listener or a clipboard viewer.

Although this might not be a problem in practice if mpv does not call the function too frequently.

Also OpenClipboard() documentation says using NULL as window causes SetClipboardData to fail, although several unofficial sources report that it does not fail in practice on win32.

About X/Wayland, I think a vo-based API (using VOCTRLs) would be the best solution since it solves both the problem of persistent state and Wayland restriction. Although this may have some problems with frequent polling.

Ah, hmm, I'd missed those caveats; m_clipboard_poll is called on every render loop iteration, so that's probably not the best solution.

I think this could be worked around by letting the vo monitor clipboard through change events, and on clipboard changes, it runs a command to update a variable in player core, which m_clipboard_poll checks. This way m_clipboard_poll does not need to stop the vo thread.

Alternately, the vo could monitor clipboard events and fire mp_notify_property when applicable (might need a dispatch call?), and m_clipboard_poll could remain a no-op on windows. Not sure if any of this actually implies changes to the API as implemented here at all, then; the vo can just reach into mpctx->clipboard to hand it a HWND to pass to OpenClipboard when performing read/write calls.

The problem is that the current mpv architecture is not designed to make synchronous calls from somewhere down the call hierarchy (vo) to somewhere up (player core). Such usage is a recipe for deadlock: see tech-overview.txt, "Avoiding callback hell" section. Commands (through input_ctx) are safe to call from anywhere because they're asynchronous.

As mentioned before, the win32 concerns may be largely theoretical, so the API usage pattern can be the same as macOS. I see no large problem with the current API design for now if X/Wayland handle the clipboard in the vo thread, as there is no special init/uninit functions required.

rcombs · 2024-04-12T03:57:15Z

Rebased with minor changes:

Observing the clipboard now works when no file is loaded
Setting CLIPBOARD_PATH or CLIPBOARD_URL now also sets the clipboard's text value, rather than clearing it

I think there are only 2 meaningful known issues left, and I don't think either is a blocker:

Observing the clipboard only works when the playloop or idleloop is actually running; if we're paused/idle and waiting for events, clipboard observers won't fire. This could be addressed by setting the default sleep time to some non-infinite value on affected platforms, or adding a dedicated thread for polling the pasteboard change count on macOS, but I'm not sure if it's really important.
CLIPBOARD_IMAGE isn't exposed to API clients at all, so support for reading CLIPBOARD_IMAGE is untested; I could expose it as a node map similarly to how screenshot-raw works (but as a property, for both reading and writing), but it'd be kinda complex, and I'm inclined to just wait for someone to actually want it for something.

Once this merges, there's a decent chance we'll want to add something like this as a built-in script: https://gist.github.com/rcombs/321a3c6b42501290c75cfb3aadf49f63

This allows for bindings like this:

meta+c script-binding copypaste/copy
meta+v script-binding copypaste/paste

(Also, should we add a way to specify keybinds that use meta on macOS but ctrl elsewhere? Seems useful for default binds.)

rcombs · 2024-04-12T23:48:54Z

Rebased with changes:

Added CLIPBOARD_PATHS, a list of multiple path strings; this allows e.g. pasting in multiple files at once.
Fixed some typing errors in the property interface.

A couple notable caveats to the current API (probably largely macOS-specific):

On macOS, it's possible to have a "URL name" string ("public.url-name") on the clipboard along with a URL; this happens when eg copying a link in a web browser (the URL name is the link text). Do we want to return that for CLIPBOARD_TEXT rather than the URL itself, in those cases?
We don't have a way to write to multiple clipboard formats at once, other than implicitly (e.g. the write implementation for CLIPBOARD_URL also sets the text clipboard to the same value; CLIPBOARD_PATH[S] also sets the text clipboard to the last path component). Do we care?

sfan5 · 2024-04-17T11:13:14Z

DOCS/man/input.rst

@@ -402,6 +402,12 @@ Remember to quote string arguments in input.conf (see `Flat command syntax`_).
 Like all input command parameters, the filename is subject to property
 expansion as described in `Property Expansion`_.

+``screenshot-to-clipboard [<flags>]``


I definitely see the usefulness but maybe copying images is better left out of the initial implementation to keep it simple first.

Screenshot support is pretty core to this functionality (it's one of, like, 2 main use-cases I'm targeting), so I'm hesitant to remove it unless we have a specific reason.

My thought was that it might be better to have a fleshed-out API for the "copy text/URL(s) to the clipboard" first, merge that and then integrate image clipboards into that (in a different PR). Perhaps even in a way that can be used from the client API.

DOCS/man/input.rst

sfan5 · 2024-04-17T11:22:11Z

DOCS/man/input.rst

+ The top-level object itself can be written directly as a shortcut; the intended
+ type will be guessed.
+
+ Converting this property to a string will give a JSON representation of all types.


I don't think we have any other properties that do this? What's the intended use case for this behavor?

Everything that uses m_property_read_sub does this (this doesn't use that to avoid reading every clipboard whenever a single one is requested, though maybe I could add a callback feature to m_sub_property to address that).

The manual doesn't mention this for any other properties so that might be what tripped me up.

sfan5 · 2024-04-17T11:23:36Z

DOCS/man/input.rst

+
+ ``path``
+ A single absolute path to a file on disk.
+ Exposed for convenience and not included in the top-level map.


So get_property_native("clipboard") will include text, url and paths yet get_property_string("clipboard/path") will work?
It doesn't seem wise to introduce such behaviour into properties.

I could include it; it's just redundant with paths, which would make it a little more annoying to use for development/debugging.

sfan5 · 2024-04-17T11:25:21Z

meson.build

@@ -514,6 +515,10 @@ if not posix and not features['win32-desktop']
 'osdep/terminal-dummy.c')
 endif

+if not features['cocoa']
+ sources += files('osdep/clipboard-dummy.c')


The "one global handler" approach will not work on Linux. Both the X11 and Wayland code needs to be compiled in, but only one of them will be active depending on the VO.

I'd expect to have a single linux implementation of the m_clipboard_* functions that dispatches between X and Wayland as needed.

Requiring only one implementation per platform sounds unnecessarily inflexible to me and as you proposed also leads to weird patterns where you have code that only forwards calls. Plus not being able to handle edge cases like X11 on macOS (just an example, not suggesting we should have that).

There can be multiple implementations of the underlying functionality per platform, they just need a implementation of the cross-platform APIs; if e.g. clipboard-linux.c wants to call into specific routines in clipboard-wayland.c and clipboard-x11.c, that's fine.

osdep/clipboard.h

sfan5 · 2024-04-17T11:37:08Z

osdep/clipboard.h

+int m_clipboard_get(struct MPContext *ctx, struct m_clipboard_item *item);
+int m_clipboard_set(struct MPContext *ctx, const struct m_clipboard_item *item);
+bool m_clipboard_poll(struct MPContext *ctx);


Personally I would like to limit clipboard to 1 backend at a time, to keep it sane.

Agreed.

I think a reasonable way would be to delegate/marry this to a VO. Then the event loop can be reused to handle the necessary clipboard protocol.
Then add a way to have a global clipboard manager for the sake of win32 and mac (where this doesn't depend on a VO).

kasper93 · 2024-04-17T20:43:56Z

osdep/clipboard-dummy.c

+
+#include "clipboard.h"
+
+int m_clipboard_set(struct MPContext *ctx, const struct m_clipboard_item* item)


Also could we use mp_ prefix like with all other things?

I'm not entirely clear on what the style guide situation is for when we use m_ vs mp_; I see a lot of usage of both within the codebase.

I would prefer to use mp_ like we use for everything recently.

rcombs requested review from Akemi and Dudemanguy April 8, 2024 07:59

rcombs force-pushed the clipboard branch 2 times, most recently from 869d646 to 99e1f27 Compare April 8, 2024 08:46

na-na-hi reviewed Apr 8, 2024

View reviewed changes

rcombs force-pushed the clipboard branch from 99e1f27 to 535c1c8 Compare April 8, 2024 21:16

na-na-hi reviewed Apr 9, 2024

View reviewed changes

kasper93 added this to the Release v0.39.0 milestone Apr 9, 2024

rcombs force-pushed the clipboard branch 2 times, most recently from 57136ee to 1287e5b Compare April 12, 2024 03:50

rcombs force-pushed the clipboard branch from 1287e5b to ccaf6bd Compare April 12, 2024 23:44

rcombs force-pushed the clipboard branch from ccaf6bd to 3a85174 Compare April 17, 2024 08:12

sfan5 reviewed Apr 17, 2024

View reviewed changes

kasper93 reviewed Apr 17, 2024

View reviewed changes

rcombs added 3 commits May 3, 2024 16:33

osdep: add clipboard facilities (currently mac-only)

bfe5fd8

screenshot: add screenshot-to-clipboard command

0b5f32a

player/command: add clipboard property

a756dc6

rcombs force-pushed the clipboard branch from 3a85174 to a756dc6 Compare May 3, 2024 23:33

kasper93 added the os:mac label May 7, 2024


		The top-level object itself can be written directly as a shortcut.

		Converting this property to a string will give a JSON representation of all types.


		#include "clipboard.h"

		int m_clipboard_set(struct MPContext ctx, const struct m_clipboard_item item)

Add clipboard support #13837

Are you sure you want to change the base?

Add clipboard support #13837

Conversation

rcombs commented Apr 8, 2024

github-actions bot commented Apr 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-na-hi commented Apr 8, 2024

rcombs commented Apr 8, 2024

na-na-hi commented Apr 8, 2024

rcombs commented Apr 8, 2024

rcombs commented Apr 8, 2024 • edited

na-na-hi commented Apr 9, 2024

Dudemanguy commented Apr 9, 2024

na-na-hi commented Apr 9, 2024 • edited

na-na-hi Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-na-hi Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-na-hi Apr 17, 2024 • edited

Choose a reason for hiding this comment

na-na-hi Apr 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-na-hi Apr 17, 2024 • edited

Choose a reason for hiding this comment

rcombs commented Apr 12, 2024

rcombs commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 8, 2024 •

edited

rcombs commented Apr 8, 2024 •

edited

na-na-hi commented Apr 9, 2024 •

edited

na-na-hi Apr 9, 2024 •

edited

na-na-hi Apr 9, 2024 •

edited

na-na-hi Apr 17, 2024 •

edited

na-na-hi Apr 17, 2024 •

edited

na-na-hi Apr 17, 2024 •

edited