WIP: async commands #5803
Closed
WIP: async commands #5803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I consider this ready for merge now. I can move out the unrelated commits to a separate PR if you want. |
ghost
mentioned this pull request
|
For the record, at the point of 0d502f1 I have built this branch with latest FFmpeg master on both mingw-w64 win64 and armv7 Android, and at least on Android it seemed to run. I will attempt to take a further look tomorrow. |
wiiaboo
reviewed
May 22, 2018
DOCS/man/options.rst
Outdated
| @@ -4006,6 +4006,13 @@ Network | |||
| Field2: value2 | |||
| Connection: close | |||
|
|
|||
| ``--http-proxy=<proxy>`` | |||
| URL of for HTTP/HTTPS proxy. If this is set, the ``http_proxy`` environment | |||
added 25 commits
May 24, 2018 19:56
Pretty annoying.
This was accidentally moved together with the cmd stuff.
This is almost like rendezvous(), except it allows async wakeup, and does not require global state. It will be used by a later commit. struct mp_waiter is intended to be allocated on the stack, and uses an initializer including PTHREAD_MUTEX_INITIALIZER. This is the first case in mpv that it uses PTHREAD_MUTEX_INITIALIZER for stack-allocated mutexes. It seems POSIX still does not allow this formally, but since POSIX is worth less than used toilet paper, I don't really care. Modern OSes use futexes, which means you can make _every_ memory location a lock, and this code tries to make use of it, without using OS specific code. The name of the source file is rather generic, because I intend to dump further small helpers there (or maybe move mp_rendezvous() to it).
This has some tricky interactions. In particular, it requires the core to be locked due to reading outstanding_async, which is documented on the only caller only. It's probably better to merge it with its only caller. The new code should be strictly equivalent, other than the fact that it doesn't temporarily unlock+lock when entering the loop for the first time (which doesn't matter here).
The existing thread pool code is the most primitive thread pool possible. That's fine, but one annoying thing was that it used a static number of threads. Make it dynamic, so we don't need to "waste" idle threads. This tries to add threads as needed. If threads are idle for some time, destroy them again until a minimum number of threads is reached. Also change the license to ISC.
This enables two types of command behavior: 1. Plain async behavior, like "loadfile" not completing until the file is fully loaded. 2. Running parts of the command on worker threads, e.g. for I/O, such as "sub-add" doing network accesses on a thread while the core continues. Both have no implementation yet, and most new code is actually inactive. The plan is to implement a number of useful cases in the following commits. The most tricky part is handling internal keybindings (input.conf) and the multi-command feature (concatenating commands with ";"). It requires a bunch of roundabout code to make it do the expected thing in combination with async commands. There is the question how commands should be handled that come in at a higher rate than what can be handled by the core. Currently, it will simply queue up input.conf commands as long as memory lasts. The client API is limited by the size of the reply queue per client. For commands which require a worker thread, the thread pool is limited to 30 threads, and then will queue up work in memory. The number is completely arbitrary.
Pretty trivial, since commands can be async now, and the common code even provides convenience like running commands on a worker thread. The only ugly thing is that mp_add_external_file() needs an extra flag for locking. This is because there's still some code which calls this synchronously from the main thread, and unlocking the core makes no sense there.
It seems the ytdl script like to continue loading external tracks even if loading was aborted. Trying to do so will still quickly fail, but not without a load of log noise. So check and error out early.
Basically, the ytdl_hook script will not terminate the script, even if you change to a new playlist entry. This happens because ytdl_hook keeps the player core in an early loading stage, and the forceful playback abort is done only in the ermination code. This does not handle the "stop" and "quit" commands, which can still take longer than expected, but on the other hand have some weird special handling (see below). I'm not doing this out of laziness. Playback stopping will have to be somewhat redone anyway. Basically we want to give everything a chance to terminate, and if it doesn't work, we want to stop loading or playback forcefully after a small timeout. We also want to remove the mess with input.c's special handling of "quit" and some other commands (see abort_playback_cb stuff).
Still missing: not freezing when removing a track (i.e. closing demuxer) with the sub-remove/audio-remove/rescan-external-files commands.
Commands are not a monolithic giant switch() statement anymore, but individual functions. There's no reason to have the command handlers themselves in command.c, with a weird under-defined API in between. (In the future, I'd like to split up command.c further, and when I do that, scrrenshot.c will probably gets its own mp_cmd_def[] array, and define the commands locally instead of exporting the raw handlers.)
Basically reimplement the async behavior on top of the async command code. With this, all screenshot commands are async, and the "async" prefix basically does nothing. The prefix now behaves exactly like with other commands that use spawn_thread. This also means using the prefix in the preset input.conf is pointless (without effect) and misleading, so remove that. The each_frame mode was actually particularly painful in making this change, since the player wants to block for it when writing a screenshot, and generally doesn't fit into the new infrastructure. It was still relatively easy to reimplement by copying the original command and then repeating it on each frame. The waiting is reentrant now, so move the call in video.c to a "safer" spot. One way to observe how the new semantics interact with everything is using the mpv repl script and sending a screenshot command through it. Without async flag, the script will freeze while writing the screenshot (while playback continues), while with async flag it continues.
This was forgotten in commit fb9bbf2.
Both asynchronous and synchronous calls used to be put into the core's dispatch queue. Also, asynchronous calls were actually synchronous, just without forcing a wait on the client's thread. This meant that both kinds of calls were always strictly ordered. A longer time ago, synchronous calls were changed to simply lock the core. This could possibly lead to reordering. Recently, some commands were changed to run on worker threads, which made the order even looser. Also remove another now incorrect doxygen comment regarding async commands.
This was not done sooner out of laziness.
All other callers had to be changed, so there's no point in keeping this helper function around. It's just another unnecessary indirection.
Matters only to API callers, but still nice to have.
Might be useful for some.
(Why the fuck are there up to 20 mouse buttons?)
If the container FPS is correct, this can help getting ideal mix factors for vo_gpu interpolation mode. Otherwise, it doesn't matter.
added 23 commits
May 24, 2018 19:56
Actually rewrite most of the option management code. This affects how options are allocated, and how thread-safe access to them is done. One thing that is nicer is that creating m_config_cache does not need to ridiculously recreate and store the entire option list again. Instead, option metadata and option storage are now separated. m_config contains the metadata, and m_config_data all or parts of the actual option values. (m_config_cache simply uses the metadata part of m_config, which is immutable after creation.) The mentioned hack was introduced in commit 1a2319f, and is the global state around g_group_mutex. Although it was "benign" global state, it's good that it's finally removed.
C99 still works, but in theory we're using C11 features already, such as stdatomic.h. gcc/clang let us use it in C99 mode too, but using C11 is at least more proper.
Just wastes memory (a few KB, because there are so many options).
Options with dynamic memory allocations (such as strings) require some care. They need to be fully copied on initialization, and if a static default value was declared, we must not free that value either. Instead of going through the entire thing even for simple types like integers, really run it only for options with dynamic allocations. To distinguish types which use dynamic allocations, we can use the fact that they require a free callback (otherwise they would leak). As a result initialization of simple types becomes chaper, and the init function does nothing at all if src==dst for a simple type. (It's funny how mplayer had M_OPT_TYPE_DYNAMIC since 2002, until we replaced it by the same heuristic as used here in commit 3bb1349. It's also funny how the new check was used only for some asserts, and finally removed in commit 7539928. I guess at this time I felt like having uniform code was more important than pointless micro-optimizations.) The src==NULL case is removed because it can't happen.
These were deprecated almost 2 years ago. Now they happen to be in the way.
Instead of accessing MPOpts.
Passing NULL to mp_get_config_group() returns the main option struct. This is just a dumb hack to deal with inconsistencies caused by legacy things (as I'll claim), and will probably be changed in the future. So before littering the whole code base with hard to find NULL parameters, require using callers an easy to find separate define.
The path functions need to access the option that forces non-default config directories. Just add it as a field to mpv_global - it seems justified. The accessed options were always enforced as immutable after init, so there's not much of a change.
The --hwdec* options are a good fit for the vd_lavc local option struct. This annoyingly requires manual prefixing of most of these options with --vd-lavc (could be avoided by using more sub-struct craziness, but let's not).
This was always a legacy thing. Remove it by applying an orgy of mp_get_config_group() calls, and sometimes m_config_cache_alloc() or mp_read_option_raw(). win32 changes untested.
Although the new code actually fires update notifications only when needed, m_config_cache_update() itself returned a rather coarse change value, which could indicate change even if none of the cached options were changed. On top of that, some code (like vo_gpu) calls the update function on every frame, which would reconfigure the renderer even on unrelated option changes.
C11 can access atomic variables normally (in which case they use the strictest memory access semantics). But the mpv stdatomic wrapper for C99 compilers does not allow it, because it couldn't give any guarantees. This means we always need to access them with atomic macros. While we're at, use relaxed semantics for the m_config_cache field, since because it's accessed from a single thread only (essentially used in a non-atomic way). Switch the comparison arguments to make the formatting look slightly less weird.
This is working towards a change intended in the future: nothing should write to the option struct directly, but use functions that raise proper notifications. Until this is complete it will take a while, and this commit does not change all cases of direct access, just some simple ones. In all of these 3 changes, the actual write access is done by the generic property-option bridge.
Often requested, trivial.
Avoids 100% CPU usage due to terminal code retrying read(). Seems like this was "forgotten" (or there was somehow the assumption poll() would not signal POLLIN anymore). Fixes #5842.
If anyone happened to build with GL disabled, this could lead to option changes not always refreshing the screen. Since vo_gpu is always enabled now (just not necessarily any backend for it), we can drop the #if completely. (The way this works is a bit idiotic - the option cache exists only to grab the change notification, which will trigger a redraw and make vo_gpu update its own second copy of them. But at least it avoids some layering issues for now.)
This code shouldn't even exist in libavformat. If you still need it, you can enable it via --demuxer-lavf-o.
Always true, because a few lines above it checks for the same thing.
I'm also not sure whether this condition doesn't subtly break a lot of things.
ghost
closed this
ghost
deleted the
|
Maybe if you actually gave a shit and made a PR per feature branch like normal people this wouldn't be sitting at 100+ commits. |
|
It's pretty unreasonable to expect people to handle or review PRs that are huge dumps of tenuously related commits... |
|
Please, no need to attack each other. No one made unreasonable assumptions, or proceeded in bad faith. This is just pretty hard stuff, you have to really understand that first. There's this old adage in open source, something about "thousand eyeballs" and so on.. But that is just a myth. Or, to be exact, it's not wrong, the principle actually is true. But there simply are never a thousand sets of eyeballs. In reality, it is a lot less. Just consider the prerequisites:
I mean, if you think about it.. |
|
Yes, there're very few eyeballs. This PR in a way is a testament to that :P . I was trying to find time to go through things, unfortunately often real life just doesn't give you that possibility for one reason or another (either you're just tired and want to watch dumb youtube shit after day job, or you would prefer to hack on something else as well every now and then). In the end since this PR got closed, I have before falling asleep here been able to pick out some small, seemingly clear improvements from this PR to #5854 . Hopefully I don't have to merge that one myself :P . During the week-end I might be able to go through the async stuff, since that seems generally useful (as well as the subprocess re-implementation stuff). We'll see. |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Still incomplete PR for doing async commands. See in particular the manpage changes in "command: add infrastructure for async commands".
This still requires a lot of work. All the commands that are async still need to be implemented this way, and some commands (at least the "screenshot" command) need to be fixed. The commits before the "infrastructure" commit can be merged though.