Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mpv freezes when taking screenshots (screenshot command is not asynchronous) #4250

Closed
UserNaem opened this issue Mar 17, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@UserNaem
Copy link

commented Mar 17, 2017

mpv version and platform

mpv 0.24.0-git-0f1afc6ba, Windows 7 build 7601, SP1

Reproduction steps

Add these lines to mpv.conf:
screenshot-format=png
screenshot-png-compression=9

Open any video file (the higher resolution the better) and take a screenshot with hotkey (default: s)

Expected behavior

I'd like mpv to pass the screenshot to another process for compression to not interfere with playback.

Actual behavior

mpv freezes for a few seconds to compress the screenshot, disrupting playback.

Also, is it possible to create a screenshot with 0 compression and then run some other PNG compression tool on the screenshot?

@lachs0r

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

Also, is it possible to create a screenshot with 0 compression and then run some other PNG compression tool on the screenshot?

That’s possible by adding a run command after the screenshot command in input.conf (you can execute multiple commands for a keybinding; see manual).

@lachs0r lachs0r changed the title mpv freezes for 1-2 seconds when taking screenshots mpv freezes when taking screenshots (screenshot command is not asynchronous) Mar 17, 2017

@UserNaem

This comment has been minimized.

Copy link
Author

commented Mar 18, 2017

I tried running PngOptimizer but I have no idea how to pass the screenshot's name to it as a parameter, how to use screenshot wildcards as parameters?

@wm4 wm4 added the feature request label Mar 18, 2017

@wm4

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

Scripts and other use-cases might rely on screenshots being fully written once the command completes (even lachs0r's suggestion just now assumes that). So it can't be async by default, which means it essentially freezes up. So we'd explicitly need an async variant of the command, which would be a mess.

@UserNaem

This comment has been minimized.

Copy link
Author

commented Mar 18, 2017

How about take a raw bitmap screenshot and then pass it to another thread for compression, then?

@wm4

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

That's not really the problem. The problem is the user-interface. If the screenshot command returns before the file is fully written, tons of user scripts etc. could break.

@UserNaem

This comment has been minimized.

Copy link
Author

commented Mar 18, 2017

So the only option is to set PNG compression to 0 and use some other tool to watch for new files in mpv's screenshots folder? That sounds rather inconvenient.
I hope some day there will be a way to make well compressed screenshots without disrupting playback or having to use other tools.

@wm4

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

There's also screenshot-to-file and screenshot-raw. The latter would allow you to implement async screenshots - if you managed to write the data to disk using a Lua script of a cplugin.

@Hrxn

This comment has been minimized.

Copy link

commented Mar 18, 2017

If I may ask: what are you even trying to do?

@UserNaem

This comment has been minimized.

Copy link
Author

commented Mar 18, 2017

I take screenshots. When I take screenshots, the video hangs, unless I disable PNG compression, in which case I would have to compress it later using some other tools. I tried to make mpv run PngOptimizer on a freshly created screenshot but I have no idea how to pass the resulting screenshot's filename to it.

@Hrxn

This comment has been minimized.

Copy link

commented Mar 18, 2017

Just a few random screenshots? And what is the problem with compressing them later? It seems to me like you're trying to automate something, but I don't see the point here.

@wm4

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2017

I'll probably implement an async mode. But first I got distracted and then bored, so not today.

@tmpsa

This comment has been minimized.

Copy link

commented Mar 28, 2017

Perhaps related (or should this be made a separate issue?):
Taking a screenshot using the command "screenshot-to-file" result in an image that appears to be taken from the video input. But if the image on the screen is rotated with the command "video-rotate" this does not affect the screenshot. So the screenshot is not really what is shown on the real screen. It would be very useful if that was the case.

@wm4 wm4 closed this in 9bcb9fc Apr 1, 2017

@wm4

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2017

Adding theasync flag to your screenshot key binding with git master should fix it.

@tmpsa

This comment has been minimized.

Copy link

commented Apr 3, 2017

My program takes screenshots on a regular basis, using the libmpv API function mpv_command(ctx, "screenshot-to-file", "mydir/myfile.png"). Not when the user presses a key. So there's no key binding involved. How can my program get a correctly rotated screenshot?

atomnuker added a commit to atomnuker/mpv that referenced this issue Jun 4, 2017

player: make screenshot commands honor the async flag
And also change input.conf to make all screenshots async. (Except the
every-frame mode, which always uses synchronous mode and ignores the
flag.) By default, the "screenshot" command is still asynchronous,
because scripts etc. might depend on this behavior.

This is only partially async. The code for determining the filename is
still always run synchronously. Only encoding the screenshot and writing
it to disk is asynchronous. We explicitly document the exact behavior as
undefined, so it can be changed any time.

Some of this is a bit messy, because I wanted to avoid duplicating the
message display code between sync and async mode. In async mode, this is
called from a worker thread, which is not safe because showing a message
accesses the thread-unsafe OSD code. So the core has to be locked during
this, which implies accessing the core and all that. So the code has
weird locking calls, and we need to do core destruction in a more
"controlled" manner (thus the outstanding_async field).

(What I'd really want would be the OSD simply showing log messages
instead.)

This is pretty untested, so expect bugs.

Fixes mpv-player#4250.
@deterenkelt

This comment has been minimized.

Copy link

commented Mar 24, 2018

watch.sh can, among other things, run pngcrush with GNU parallel, so you could take PNG screenshots without compression and once mpv quits, parallel and pngcrush start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.