player: coalesce option updates and drop redundant ones

[Dudemanguy]

Could use some more testing for sure, but it seems like the dumb way of just processing these in the playloop does the trick.

Stuff like input-commands or hwdec are good to test since it's obvious if those do repeated executions. e.g. consider these config files.

hwdec=auto
hwdec=auto-safe
hwdec=auto
hwdec=auto-safe
hwdec=auto
hwdec=auto-safe
hwdec=auto
hwdec=auto-safe

input-commands=loadfile ${options}${playlist/0}${playlist/0}${playlist/0}
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a
input-commands-pre=a

Load them at runtime with load-config-file. On master, the first one spams the terminal with garbage because of constant probing. The second one freezes the player and probably will OOM. With this PR, only the last hwdec is processed on the first config file and the second one just quits the player since you're giving loadfile garbage.

Bonus:

input-commands=show-text b
input-commands-append=show-text a
input-commands-append=show-text a
input-commands-append=show-text a

Results in this on master:

[   4.947][v][cplayer] Setting option 'input-commands' = 'show-text b' (flags = 4)
[   4.947][v][cplayer] Setting option 'input-commands-append' = 'show-text a' (flags = 4)
[   4.966][v][cplayer] Setting option 'input-commands-append' = 'show-text a' (flags = 4)
[   4.967][v][cplayer] Setting option 'input-commands-append' = 'show-text a' (flags = 4)
[   4.968][t][cplayer] Run command: show-text, flags=73, args=[text="b", duration="-1", level="0"]
[   4.968][t][cplayer] Run command: show-text, flags=73, args=[text="b", duration="-1", level="0"]
[   4.969][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   4.969][t][cplayer] Run command: show-text, flags=73, args=[text="b", duration="-1", level="0"]
[   4.970][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   4.971][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   4.971][t][cplayer] Run command: show-text, flags=73, args=[text="b", duration="-1", level="0"]
[   4.972][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   4.972][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   4.973][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]

With this PR:

[   1.360][v][cplayer] Setting option 'input-commands' = 'show-text b' (flags = 4)
[   1.361][v][cplayer] Setting option 'input-commands-append' = 'show-text a' (flags = 4)
[   1.361][v][cplayer] Setting option 'input-commands-append' = 'show-text a' (flags = 4)
[   1.362][v][cplayer] Setting option 'input-commands-append' = 'show-text a' (flags = 4)
[   1.363][t][cplayer] video_output_image: r=2/eof=0/st=playing
[   1.363][t][cplayer] s=1.000950 vsyncs=3 dur=0.041708 ratio=2.500000 err=-0.00833375002083428656 (-0.500000/-0.200000)
[   1.363][t][cplayer] frametime=0.041
[   1.366][t][cplayer] Run command: show-text, flags=73, args=[text="b", duration="-1", level="0"]
[   1.366][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   1.366][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]
[   1.366][t][cplayer] Run command: show-text, flags=73, args=[text="a", duration="-1", level="0"]

[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

[kasper93]

In principle it make sense. I didn't think hard about corner cases that it might produce, but the change is not invasive, so we can monitor the impact.

And overall this is nice optimization if for whatever reason some option is spammed to be updated.

[kasper93]

Should we document it? Not sure where. But option callback won't be immediate now. So I wonder if it is not a problem. Say someone do enable option x, run command, disable option x. What option state the command will see?

[Dudemanguy]

I did a few crude tests fully expecting it to break something but it didn't. Probably still needs some more testing to see if it does something unexpected.

[na-na-hi]

One issue is that many of the functions called in the option change callback do nothing when the option value is not changed. Example:

mpv/player/scripting.c

Line 247 in 5809d4d

if ((*pid > 0) != enable) {

Which means if the option is changed to another value before changing it back to the original value, it would do nothing, instead of reloading the script.

There are many other examples of this value change detection:

mpv/player/playloop.c

Line 161 in 5809d4d

if (internal_paused != mpctx->paused) {

I don't know how this will affect usage in practice, but there are many things handled inside this callback so the potential change in behavior is high.

[kasper93]

I don't know how this will affect usage in practice, but there are many things handled inside this callback so the potential change in behavior is high.

Yes, this is pretty clear that some sequence of events may not occur. This is what this change suppose to fix. Like we can read in PR description.

hwdec=auto
hwdec=auto-safe
hwdec=auto
hwdec=auto-safe
hwdec=auto
hwdec=auto-safe
hwdec=auto
hwdec=auto-safe

will no longer rapidly change hwdec, possibly reinitializing whole VO. Same with your pause example.

The change has impact for sure, but like I said previously I'm not sure what corner cases it can trigger in real usage. I mean it is hard to imagine someone depends on behavior of executing commands in real time manner.

[Dudemanguy]

Internally, the option values do still change with this. So for a command that is like set property to a; do something; set property to b, the do something will operate as if the value of property is a. What will not happen, of course, is the callback. I don't know if something would specifically be dependent something in the callback part running.

As for clients, we already actually coalesce notification for property/option changes. If something changes the value of a property a bunch of times in a row, the observer will only see the latest change. This PR does not change the behavior there. So I think the potential impact is probably pretty low but again hard to predict since we have so many options that interact in so many ways.

[verygoodlee]

Windows

• mpv-x86_64-w64-mingw32

This breaks --force-window on Windows 10/11.

• run mpv --force-window=immediate --idle.
  there's a black window appears and immediately disappears, and then the mpv window appears, but not focused.

[Dudemanguy]

Well that was fast. Neither of those options even run callbacks so I'm not sure how it's even possible. Investigating...

Edit: Turned out to be a small miss.