player: only coalesce callbacks from parsed config files

[Dudemanguy]

cc @christoph-heinrich: I think with this you just need to do this (potential backwards compatibility handling ignored):

diff --git a/subtitle-lines.lua b/subtitle-lines.lua
index d836b83..556796f 100644
--- a/subtitle-lines.lua
+++ b/subtitle-lines.lua
@@ -130,7 +130,7 @@ local function acquire_subtitles()
     mp.set_property_bool(sub_strings.visibility, false)
 
     -- go to the first subtitle line
-    mp.commandv('set', sub_strings.delay, mp.get_property_number('duration', 0) + 365 * 24 * 60 * 60)
+    mp.commandv('immediate', 'set', sub_strings.delay, mp.get_property_number('duration', 0) + 365 * 24 * 60 * 60)
     mp.commandv('sub-step', 1, sub_strings.step)
 
     -- this shouldn't be necessary, but it's kept just in case there actually
@@ -207,7 +207,7 @@ local function acquire_subtitles()
         mp.commandv('sub-step', 1, sub_strings.step)
     end
 
-    mp.set_property_number(sub_strings.delay, sub_delay)
+    mp.commandv('immediate', 'set', sub_strings.delay, sub_delay)
     mp.set_property_bool(sub_strings.visibility, sub_visibility)
 
     for _, subtitle in ipairs(subtitles) do

And you'll get the old behavior back.

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32
• mpv-x86_64-windows-msvc

macOS

• mpv-macos-13-intel
• mpv-macos-14-arm
• mpv-macos-15-arm

[verygoodlee]

New command prefix need to be added to these places as well.

mpv/player/lua/console.lua

Lines 1669 to 1677 in fc2f176

	-- Skip command prefixes because it is not worth lumping them together with
	-- command completions when they are useless for interactive usage.
	local command_prefixes = {
	['osd-auto'] = true, ['no-osd'] = true, ['osd-bar'] = true,
	['osd-msg'] = true, ['osd-msg-bar'] = true, ['raw'] = true,
	['expand-properties'] = true, ['repeatable'] = true,
	['nonrepeatable'] = true, ['nonscalable'] = true,
	['async'] = true, ['sync'] = true
	}

mpv/player/lua/stats.lua

Lines 414 to 420 in fc2f176

	-- command prefix tokens to strip - includes generic property commands
	local cmd_prefixes = {
	osd_auto=1, no_osd=1, osd_bar=1, osd_msg=1, osd_msg_bar=1, raw=1, sync=1,
	async=1, expand_properties=1, repeatable=1, nonrepeatable=1, nonscalable=1,
	set=1, add=1, multiply=1, toggle=1, cycle=1, cycle_values=1, ["!reverse"]=1,
	change_list=1,
	}

[kasper93]

Shouldn't the whole feature go through a deprecation period?

We added a new behavior that breaks multiple use-cases (this subtitle thing and hwdec thing), now we add a command prefix to revert to old behavior. This is breaking change and we need to give users time to migrate their scripts to immediate command.

This defered options callback invites all sort of race conditions like shown by subtitle-lines.lua. Not all setters/getter return the opts/property value, some read real state of mpv, so it may be unexpected that the state doesn't change.

[Dudemanguy]

hwdec thing

To be fair, that didn't really work before. You would need some kind of queueing approach to wait until the hwdec is actually set for real before printing it no matter what.

This defered options callback invites all sort of race conditions like shown by subtitle-lines.lua

It's always been a bit racy. The earlier hwdec is a good example since it may take a few frames before it gets applied. So if you do a get right after setting it, it could be wrong. This specific script has failures because it sets the sub-delay and then immediately performs a sub-step which is directly reliant on sub-delay. And the timing requirements are small enough to where the next playloop isn't sufficient. I'm not saying there aren't people out there that will be affected, but I still think it's a low number of people.

Deprecating is easy enough and I'm not like opposed to it or anything. All I'd have to do is make immediate always true, but I'm not sure how we would actually communicate that to users. I mean yeah we can put it in the release notes/docs but is anyone going to really read it? I don't think printing a warning makes much sense either.

[kasper93]

Deprecating is easy enough and I'm not like opposed to it or anything. All I'd have to do is make immediate always true, but I'm not sure how we would actually communicate that to users. I mean yeah we can put it in the release notes/docs but is anyone going to really read it? I don't think printing a warning makes much sense either.

I don't mind either way. We need to be careful not to release next stable version without making sure it is safe. I'm fine with having this change, but we need to monitor the impact. It's basically Hyrum's law, it really doesn't matter how we define this, what matters how it works and if it start breaking scripts even in subtle way, because they depended on some strange behavior it's a problem.

[na-na-hi]

I think it's clear now that the impact of this change was very underestimated. sub-delay has the UPDATE_OSD flag which means it has a high potential of breakage because it involves callbacks.

Instead of this command prefix, the option change coalescing should be disabled for UPDATE_OSD flag, and potentially some other flags of high risk, and then gradually enable this handling for options of a particular UPDATE_* flag once it's known to not break.

[Dudemanguy]

I think it's clear now that the impact of this change was very underestimated.

I'd appreciate it if we could at least wait until there's a sample size greater than 1 before we start making melodramatic statements.

[christoph-heinrich]

This doesn't work for me. I end up with messed up timestamps. I tried adding immediate before the sub-step commands as well, but that didn't help.

[Dudemanguy]

Sorry implementation flaw on my part. Does it work correctly now?

[christoph-heinrich]

It works now, even without adding the immediate to sub-step.

To make the script backwards compatible I'll need some way to check if immediate is available.
I can run immediate ignore and then check the return value, but that logs an error. Any other ideas?
After the release I can simply check the version.

[Dudemanguy]

It's silly but something like this would work.

local function check_immediate()
    have_immediate = mp.commandv('immediate', 'ignore') or false
    mp.commandv('set', 'msg-level', 'subtitle_lines=info')
    mp.unobserve_property(check_immediate)
end
mp.observe_property('msg-level', 'string', check_immediate)
mp.commandv('set', 'msg-level', 'subtitle_lines=no')

[christoph-heinrich]

It's silly but something like this would work.

local function check_immediate()
    have_immediate = mp.commandv('immediate', 'ignore') or false
    mp.commandv('set', 'msg-level', 'subtitle_lines=info')
    mp.unobserve_property(check_immediate)
end
mp.observe_property('msg-level', 'string', check_immediate)
mp.commandv('set', 'msg-level', 'subtitle_lines=no')

Considering that this is a temporary solution, I guess that would work.
Alternatively we could create a prefix-list like the already existing command-list, which could then also be used in console.lua instead of having an array with prefixes. But that's not that important, I'd be fine with leaving things as they already are.

[Dudemanguy]

@kasper93: Do you think I should also forbid --input-commands from using this? Ideally only scripts/api users should ever have access to this in my mind. It's still technically possible to make a really dumb config file if we allow the immediate prefix to have meaning there.

[kasper93]

@kasper93: Do you think I should also forbid --input-commands from using this? Ideally only scripts/api users should ever have access to this in my mind. It's still technically possible to make a really dumb config file if we allow the immediate prefix to have meaning there.

I thinking, maybe we should split callbacks between config files and normal commands. Say, we use "delayed" callbacks when parsing config file, but use "immediate" in all other cases to preserve compatibility. And we don't need this prefix, because we assume that single commands are fine to process immediately. While big config files makes more sense to coalesce.

Does it make sense? Maybe it will be best of both worlds.

It should be easy to detect, there are similar flags for opts like M_SETOPT_FROM_CONFIG_FILE

[Dudemanguy]

Say, we use "delayed" callbacks when parsing config file, but use "immediate" in all other cases to preserve compatibility.

I implemented this now. The two cases that use the delayed callbacks are config file parsing and the --input-commands option. Couldn't think of anything else that really needs this. With the latest push, @christoph-heinrich's script should not need any code changes.

[kasper93]

I think I like this. And if we want in the future extend it somehow, it is also possible.