diff options
authorwm4 <wm4@nowhere>2018-04-15 10:14:00 +0200
committerJan Ekström <>2018-04-18 01:17:41 +0300
commit11c573fda0b96a55ae1b76436b9425bc5ca5a9b2 (patch)
parent41d559c4a9b8311ff18803ad795b211d80f04cc5 (diff)
scripting: change when/how player waits for scripts being loaded
Fundamentally, scripts are loaded asynchronously, but as a feature, there was code to wait until a script is loaded (for a certain arbitrary definition of "loaded"). This was done in scripting.c with the wait_loaded() function. This called mp_idle(), and since there are commands to load/unload scripts, it meant the player core loop could be entered recursively. I think this is a major complication and has some problems. For example, if you had a script that does 'os.execute("sleep inf")', then every time you ran a command to load an instance of the script would add a new stack frame of mp_idle(). This would lead to some sort of reentrancy horror that is hard to debug. Also misc/dispatch.c contains a somewhat tricky mess to support such recursive invocations. There were also some bugs due to this and due to unforeseen interactions with other messes. This scripting stuff was the only thing making use of that reentrancy, and future commands that have "logical" waiting for something should be implemented differently. So get rid of it. Change the code to wait only in the player initialization phase: the only place where it really has to wait is before playback is started, because scripts might want to set options or hooks that interact with playback initialization. Unloading of builtin scripts (can happen with e.g. "set osc no") is left asynchronous; the unloading wasn't too robust anyway, and this change won't make a difference if someone is trying to break it intentionally. Note that this is not in mp_initialize(), because mpv_initialize() uses this by locking the core, which would have the same problem. In the future, commands which logically wait should use different mechanisms. Originally I thought the current approach (that is removed with this commit) should be used, but it's too much of a mess and can't even be used in some cases. Examples are: - "loadfile" should be made blocking (needs to run the normal player code and manually unblock the thread issuing the command) - "add-sub" should not freeze the player until the URL is opened (needs to run opening on a separate thread) Possibly the current scripting behavior could be restored once new mechanisms exist, and if it turns out that anyone needs it. With this commit there should be no further instances of recursive playloop invocations (other than the case in the following commit), since all mp_idle()/mp_wait_events() calls are done strictly from the main thread (and not commands/properties or libmpv client API that "lock" the main thread).
5 files changed, 27 insertions, 22 deletions
diff --git a/DOCS/interface-changes.rst b/DOCS/interface-changes.rst
index 576a472cd0..a39043dc9b 100644
--- a/DOCS/interface-changes.rst
+++ b/DOCS/interface-changes.rst
@@ -88,6 +88,11 @@ Interface changes
- deprecate the old command based hook API, and introduce a proper C API
(the high level Lua API for this does not change)
- rename the the lua-settings/ config directory to script-opts/
+ - the way the player waits for scripts getting loaded changes slightly. Now
+ scripts are loaded in parallel, and block the player from continuing
+ playback only in the player initialization phase. It could change again in
+ the future. (This kind of waiting was always a feature to prevent that
+ playback is started while scripts are only half-loaded.)
--- mpv 0.28.0 ---
- rename --hwdec=mediacodec option to mediacodec-copy, to reflect
conventions followed by other hardware video decoding APIs
diff --git a/DOCS/man/input.rst b/DOCS/man/input.rst
index 7efc700550..4a59a919f0 100644
--- a/DOCS/man/input.rst
+++ b/DOCS/man/input.rst
@@ -730,7 +730,9 @@ Input Commands that are Possibly Subject to Change
merely sets all option values listed within the profile.
``load-script "<path>"``
- Load a script, similar to the ``--script`` option.
+ Load a script, similar to the ``--script`` option. Whether this waits for
+ the script to finish initialization or not changed multiple times, and the
+ future behavior is left undefined.
``change-list "<option>" "<operation>" "<value>"``
This command changes list options as described in `List Options`_. The
diff --git a/DOCS/man/lua.rst b/DOCS/man/lua.rst
index f943f3421d..c0c4f9b00a 100644
--- a/DOCS/man/lua.rst
+++ b/DOCS/man/lua.rst
@@ -63,7 +63,10 @@ the player will more or less hang until the script returns from the main chunk
(and ``mp_event_loop`` is called), or the script calls ``mp_event_loop`` or
``mp.dispatch_events`` directly. This is done to make it possible for a script
to fully setup event handlers etc. before playback actually starts. In older
-mpv versions, this happened asynchronously.
+mpv versions, this happened asynchronously. With mpv 0.29.0, this changes
+slightly, and it merely waits for scripts to be loaded in this manner before
+starting playback as part of the player initialization phase. Scripts run though
+initialization in parallel. This might change again.
mp functions
diff --git a/player/loadfile.c b/player/loadfile.c
index 673ede1a71..8065b6f007 100644
--- a/player/loadfile.c
+++ b/player/loadfile.c
@@ -31,6 +31,7 @@
#include "osdep/threads.h"
#include "osdep/timer.h"
+#include "client.h"
#include "common/msg.h"
#include "common/global.h"
#include "options/path.h"
@@ -1519,6 +1520,15 @@ struct playlist_entry *mp_next_file(struct MPContext *mpctx, int direction,
// Return if all done.
void mp_play_files(struct MPContext *mpctx)
+ // Wait for all scripts to load before possibly starting playback.
+ if (!mp_clients_all_initialized(mpctx)) {
+ MP_VERBOSE(mpctx, "Waiting for scripts...\n");
+ while (!mp_clients_all_initialized(mpctx))
+ mp_idle(mpctx);
+ mp_wakeup_core(mpctx); // avoid lost wakeups during waiting
+ MP_VERBOSE(mpctx, "Done loading scripts.\n");
+ }
prepare_playlist(mpctx, mpctx->playlist);
for (;;) {
diff --git a/player/scripting.c b/player/scripting.c
index b98588d948..bcb5ea65ef 100644
--- a/player/scripting.c
+++ b/player/scripting.c
@@ -100,13 +100,6 @@ static void *script_thread(void *p)
return NULL;
-static void wait_loaded(struct MPContext *mpctx)
- while (!mp_clients_all_initialized(mpctx))
- mp_idle(mpctx);
- mp_wakeup_core(mpctx); // avoid lost wakeups during waiting
static int mp_load_script(struct MPContext *mpctx, const char *fname)
char *ext = mp_splitext(fname, NULL);
@@ -150,9 +143,6 @@ static int mp_load_script(struct MPContext *mpctx, const char *fname)
return -1;
- wait_loaded(mpctx);
- MP_DBG(mpctx, "Done loading %s.\n", fname);
return 0;
@@ -203,16 +193,11 @@ static void load_builtin_script(struct MPContext *mpctx, bool enable,
if (enable) {
mp_load_script(mpctx, fname);
} else {
- // Try to unload it by sending a shutdown event. Wait until it has
- // terminated, or re-enabling the script could be racy (because it'd
- // recognize a still-terminating script as "loaded").
- while (mp_client_exists(mpctx, name)) {
- if (mp_client_send_event(mpctx, name, 0, MPV_EVENT_SHUTDOWN,
- NULL) < 0)
- break;
- mp_idle(mpctx);
- }
- mp_wakeup_core(mpctx); // avoid lost wakeups during waiting
+ // Try to unload it by sending a shutdown event. This can be
+ // unreliable, because user scripts could have clashing names, or
+ // disabling and then quickly re-enabling a builtin script might
+ // detect the still-terminating script as loaded.
+ mp_client_send_event(mpctx, name, 0, MPV_EVENT_SHUTDOWN, NULL);