screenshot: add screenshot-to-clipboard command #15568

rcombs · 2024-12-25T03:36:08Z

Successor to #13837, post-#15355 and #15564.

Currently implemented only on macOS.

github-actions · 2024-12-25T03:57:19Z

Download the artifacts for this pull request:

Windows

macOS

kasper93 · 2024-12-25T13:27:16Z

osdep/mac/clipboard.swift

+
+            dest.params.color = plcsp
+
+            if mp_image_swscale(&dest, &image, SWS_FULL_CHR_H_INT | SWS_FULL_CHR_H_INP | SWS_ACCURATE_RND) < 0 {


This should use the same pattern and options as other screenshot code. See 2c43d2b

I will remove mp_image_swscale to avoid confusion.

Why add more brittle boilerplate? Seems more reasonable to me to add global and log options to mp_image_swscale and then remove the duplication in screenshot.c, image_writer.c, etc.

Rebased with this change; the relevant changeset is net-red (+11 -20) so I think it's a reasonable reduction in duplication. Also improved one path through player/screenshot that wasn't using the global options (nor checking for errors).

Akemi

the whole setImage function is very convoluted. do you think we can split it into some logical parts/helper functions, format selection (format, colour space, bit depth, etc), compatibility test, fallback/conversion, etc? it's fine not doing it in this PR and we do it later.

osdep/mac/clipboard.swift

Akemi · 2024-12-26T15:17:44Z

osdep/mac/clipboard.swift

+            withUnsafeMutableBytes(of: &dest.stride) { (stridePtr) in
+                withUnsafeMutableBytes(of: &dest.planes) { (planesPtr) in
+                    let destStrides = stridePtr.baseAddress!.assumingMemoryBound(to: type(of: image.stride.0))
+                    let destPlanes = planesPtr.baseAddress!.assumingMemoryBound(to: type(of: image.planes.0))


would be nice to get rid of the forced unwrapping here. don't want mpv to completely fail because the clipboard code fails.

These pointers are the addresses of members of a stack object; they can't be null.

i don't want to compromise even more on the force unwrappings than i already did on the previous occurrences. i also don't want to discuss the sense of it in any case.

please handle every unwrapping in an appropriate way and get rid of the force unwrappings.

Switched to use guard lets here and added the extra handling for that, but also added an assert for debug builds.

osdep/mac/clipboard.swift

Akemi · 2024-12-27T14:13:34Z

DOCS/man/input.rst

@@ -447,6 +447,12 @@ Property Manipulation
 Playlist Manipulation
 ~~~~~~~~~~~~~~~~~~~~~

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


the documentation for this should be under Screenshot Commands not Playlist Manipulation.

also please correct me if i am wrong. this is raw bitmap data which we pass here or at least not compressed data (png, jpg, whatever is configured with our screenshot options)? so shouldn't this be screenshot-raw-to-clipboard?

eg i would expect screenshot-to-clipboard to write a screenshot to the clipboard in the same format screenshot writes a screenshot to the file system. so it should be affected by 'all' the screenshot-* options.

screenshot-to-file also ignores the format option, though?

i guess the documentation mentions the screenshot-to-file ignores at least the format option. my expectations were a bit betrayed then.

Take a screenshot and save it to a given file. The format of the file will be guessed by the extension (and --screenshot-format is ignored - the behaviour when the extension is missing or unknown is arbitrary).

though thinking a bit about the future. what if we want to actually add screenshot-to-clipboard command that wants to add the aforementioned case (eg adding a screenshot as png, jpeg, etc)? would we add a new flag to this command? i am just wondering and trying to prevent a possible 'mess'.

though if we do what @na-na-hi proposed we don't need to think further about this case.

There's a conceptual difference between an "image on the clipboard" and a "file on the clipboard", and generally copying ephemeral image data does not result in you having a file on the clipboard. On macOS, images on the clipboard are commonly represented as PNG or TIFF data (sometimes both), but there is no eg JPEG clipboard format. There's also no concept of arbitrarily ephemeral file contents on the clipboard (only a file path). The same is true on Windows afaik, except that the image format is BMP instead of PNG/TIFF. I have no idea how this works on linux, apart from that apparently on X it's common for apps to expose several different types for a copied image.

Images copied out of mpv on this branch are PNG data (but that's an implementation detail, and it basically never matters to a user).

rcombs · 2024-12-28T02:10:47Z

Rebased with suggestions applied; split the function up to the extent that was possible without adding substantial duplication or major restructuring.

na-na-hi · 2024-12-28T08:23:03Z

I think there is no need for this to be a command. I designed the clipboard API with the ability for format conversion in mind: when the data formats in clipboard_access_params and clipboard_data are different, the backend can perform automatic format conversion if possible. This makes it possible to use the operating system's built-in data format conversion abilities.

In this specific case, it is possible to make the clipboard/image property writable, and a client can just set the clipboard content by writing a special token of MPV_FORMAT_STRING type like @screenshot window to the property.

Alternatively, writing a MPV_FORMAT_NODE nodemap containing information of the raw image data to that property should write that image to the clipboard, so a client can also use screenshot-raw command to acquire the image data and write the data to clipboard.

These approaches allow for more flexibility of setting image clipboard contents while not introducing a new command.

rcombs · 2024-12-28T08:39:06Z

I think supporting setting an MPV_FORMAT_NODE makes sense in the long term, but designing an API for it that's actually useful across a variety of pixel formats, colorspaces, etc. is going to be tricky, so I'd prefer to leave it out of this PR for now (see also the screenshot-raw command, which returns a node, but in a single hardcoded pixel format with no colorspace information).

I wouldn't object to having this be set clipboard/image "@screenshot window" or the like, though I do think the symmetry the current approach has with screenshot-to-file is probably a bit more intuitive for users?

Also, it probably makes most sense for any magic-string @screenshot support to happen up at the command/property level, rather than within each individual backend.

Akemi · 2024-12-28T14:39:03Z

tested the the mac parts and works as expected in programs that can handle those pastes. besides that one open point i am fine with he swift parts of the code.

would be nice if we could resolve the rest of the open points in a timely manner:

the user facing API
implementation on the mpv core side

This reverts commit d9eb9ed

rcombs requested a review from Akemi December 25, 2024 03:36

rcombs force-pushed the screenshot-clipboard branch from 3d5afcb to 7caaf04 Compare December 25, 2024 03:43

rcombs force-pushed the screenshot-clipboard branch from 7caaf04 to d72d66b Compare December 25, 2024 04:37

kasper93 reviewed Dec 25, 2024

View reviewed changes

rcombs force-pushed the screenshot-clipboard branch 2 times, most recently from f0df0fa to aac01fb Compare December 25, 2024 21:48

Akemi requested changes Dec 27, 2024

View reviewed changes

Akemi reviewed Dec 27, 2024

View reviewed changes

rcombs force-pushed the screenshot-clipboard branch from aac01fb to 5e26514 Compare December 28, 2024 02:08

rcombs added 6 commits December 30, 2024 17:51

sws_utils: re-add mp_image_swscale, with opts/log handling

674219e

This reverts commit d9eb9ed

player/screenshot: use mp_image_swscale to reduce boilerplate

b50a12b

video/image_writer: use mp_image_swscale to reduce boilerplate

91caec1

clipboard-mac: pass clipboard_ctx into swift code

dba1467

clipboard-mac: support writing CLIPBOARD_DATA_IMAGE

37a2699

screenshot: add screenshot-to-clipboard command

7fde240

rcombs force-pushed the screenshot-clipboard branch from 5e26514 to 7fde240 Compare December 31, 2024 03:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

screenshot: add screenshot-to-clipboard command #15568

screenshot: add screenshot-to-clipboard command #15568

rcombs commented Dec 25, 2024

github-actions bot commented Dec 25, 2024 •

edited

Loading

kasper93 Dec 25, 2024

rcombs Dec 25, 2024

rcombs Dec 25, 2024

Akemi left a comment

Akemi Dec 26, 2024

rcombs Dec 28, 2024

Akemi Dec 28, 2024

rcombs Dec 31, 2024

Akemi Dec 27, 2024 •

edited

Loading

rcombs Dec 28, 2024

Akemi Dec 28, 2024

rcombs Dec 28, 2024

rcombs commented Dec 28, 2024

na-na-hi commented Dec 28, 2024

rcombs commented Dec 28, 2024 •

edited

Loading

Akemi commented Dec 28, 2024


		dest.params.color = plcsp

		if mp_image_swscale(&dest, &image, SWS_FULL_CHR_H_INT \| SWS_FULL_CHR_H_INP \| SWS_ACCURATE_RND) < 0 {

screenshot: add screenshot-to-clipboard command #15568

Are you sure you want to change the base?

screenshot: add screenshot-to-clipboard command #15568

Conversation

rcombs commented Dec 25, 2024

github-actions bot commented Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akemi left a 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

Akemi Dec 27, 2024 • edited Loading

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

rcombs commented Dec 28, 2024

na-na-hi commented Dec 28, 2024

rcombs commented Dec 28, 2024 • edited Loading

Akemi commented Dec 28, 2024

github-actions bot commented Dec 25, 2024 •

edited

Loading

Akemi Dec 27, 2024 •

edited

Loading

rcombs commented Dec 28, 2024 •

edited

Loading