mpv-player / mpv

πŸŽ₯ Command line video player
https://mpv.io
Other
28.72k stars 2.93k forks source link

Race condition in `end-file` event? #15307

Closed alterNERDtive closed 1 week ago

alterNERDtive commented 1 week ago

mpv Information

mpv 0.39.0 Copyright Β© 2000-2024 mpv/MPlayer/mplayer2 projects
libplacebo version: v7.349.0
FFmpeg version: 7.0.2
FFmpeg library versions:
   libavcodec      61.3.100
   libavdevice     61.1.100
   libavfilter     10.1.100
   libavformat     61.1.100
   libavutil       59.8.100
   libswresample   5.1.100
   libswscale      8.1.100

Other Information

- Linux version: Fedora 41
- Kernel Version: 6.11.7-300.fc41.x86_64
- GPU Model: Navi 32 [Radeon RX 7700 XT / 7800 XT]
- Mesa/GPU Driver Version: Mesa 24.2.6 (LLVM 19.1.0)
- Window Manager and Version: Sway 1.10
- Source of mpv: Fedora Repo
- Latest known working version: 0.37 (Fedora 40)
- Issue started after the following happened: upgrade Fedora 40 β†’ 41, mpv 0.37 β†’ 0.39

Reproduction Steps

I am using the following script to exit fullscreen after the last item in a playlist has ended (with --idle=yes):

mp.register_event("end-file", function()
    if mp.get_property_native("playlist-pos") == -1 then
        mp.set_property_native("fullscreen", false)
    end
end)

After updating to 0.39, this stopped working. Investigation has turned out that there seems to be a race condition now (or always has been and I had never run into it) that leads to playlist-pos not resetting to -1 at the end of the play list fast enough for this event handler to work.

If I add a couple of

    mp.msg.info(mp.get_property_native("playlist-pos"))

to the start of the handler, I get an output of e.g.

[exit_fullscreen] 0
[exit_fullscreen] 0
[exit_fullscreen] -1

(and then the script works, since it’s -1 when the condition is hit).

Expected Behavior

playlist-pos is -1 in the end-file event of the last entry in the playlist.

Actual Behavior

playlist-pos still holds the value of the last entry in the playlist for a little while before assuming the value -1.

Log File

output.txt

Sample Files

No response

I carefully read all instruction and confirm that I did the following:

CounterPillow commented 1 week ago

Pretty sure this was always racey, I think you'll want playlist-playing-pos:

Index of the "playing" item on playlist. A playlist item is "playing" if it's being loaded, actually playing, or being unloaded. This property is set during the MPV_EVENT_START_FILE (start-file) and the MPV_EVENT_START_END (end-file) events. Outside of that, it returns -1. If the playlist entry was somehow removed during playback, but playback hasn't stopped yet, or is in progress of being stopped, it also returns -1. (This can happen at least during state transitions.)

Though I don't know whether "or is in progress of being stopped" refers to reaching the end of playlist, worth looking into. Otherwise you might just want to observe the idle-active property.

kasper93 commented 1 week ago

During end-file event, the playlist id corresponding to the item that has been just unloaded is provided, see docs.

Processing events, doesn't stop the player from progressing, so any get_property_native inside event handler are "racey", scripts are run asynchronous.

Note that valid value for playlist-pos on end-file is 0 in your case. By repeatedly calling the get_property_native you basically forcing script to wait until this value is cleared and mpv switches to idle.

has ended (with --idle=yes)

Observe idle-active instead.

alterNERDtive commented 1 week ago

I think you'll want playlist-playing-pos

I thought I had double checked that, but … apparently not. Seems to work as expected, thanks!

Otherwise you might just want to observe the idle-active property.

That one exhibits the same behaviour as reported, just with false β†’ true.

kasper93 commented 1 week ago

That one exhibits the same behaviour as reported, just with false β†’ true.

What do you mean by false β†’ true?

It reports idle after finishing playback correctly for me.

mp.observe_property("idle-active", "native", function(_, val)
    if val then
        mp.set_property_native("fullscreen", false)
    end
end)
alterNERDtive commented 1 week ago

Oh. Yes. My bad.