don't decrement --loop-file=N and --ab-loop-count=N #14302
Conversation
|
Download the artifacts for this pull request: |
DOCS/interface-changes/loop.txt
Outdated
| numerical values of `--loop-file` no longer decrease on each iteration | ||
| add `remaining-file-loops` property | ||
| numerical values of `--ab-loop-count` no longer decrease on each iteration | ||
| add `remaining-ab-loops` property |
There was a problem hiding this comment.
Please suggest that clients which observe these options for remaining counts should use the new properties instead.
player/command.c
Outdated
| { | ||
| MPContext *mpctx = ctx; | ||
| if (mpctx->remaining_file_loops == -1) | ||
| return m_property_double_ro(action, arg, INFINITY); |
There was a problem hiding this comment.
I'd like not having the property changing type based on this. How about just return -1 and document that -1 means infinity instead?
There was a problem hiding this comment.
We could return INT_MAX instead. I think returning bigger number for infinity is easier for downstream users to avoid special cases where simple compare would be enough.
There was a problem hiding this comment.
Or 10000 (+1) which is currently the max value for loop-file.
There was a problem hiding this comment.
I think -1 being infinity is an internal detail that script writers shouldn't have to care about, and having it positive is convenient to simplify comparisons, e.g. mp.get_property_native('remaining-file-loops') > 0. And I think that having this be math.huge in Lua is convenient whereas INT_MAX is not available in Lua, or 10k is just an arbitrary number. Only with math.huge it is intuitive what to compare to to check for infinity in scripts. Also --loop-file is 10000 max but --ab-loop-count is INT_MAX max which is an inconsistency.
There was a problem hiding this comment.
I don't know what is the best approach here, but I agree with @na-na-hi that type changing is not great. It would be annoying to handle if used from C for example.
There was a problem hiding this comment.
Maybe always return a double?
There was a problem hiding this comment.
Most existing options use special values for infinity (e.g. target-contrast, and stats.lua term_width_limit), and there is currently no property documented to contain infinite values.
Maybe always return a double?
That's suboptimal for languages which support real integers like C, and I think looping count is best represented as an integer. The current infinity value means that getting the value as an integer with libmpv API will fail.
There was a problem hiding this comment.
Updated to -1 but for what it's worth --image-display-duration=inf does map to math.huge in Lua and I use this and find it convenient.
This stops decreasing numerical values of --loop-file on each iteration so that loop-file=N loops every playlist entry without having to add --loop-file to --reset-on-next-file. The current behavior confuses users as seen in: mpv-player#2481 mpv-player#5943 mpv-player#11291 mpv-player#13860 https://www.reddit.com/r/mpv/comments/rcwnrw/looping_each_file_n_times_in_a_playlist/ Also options are supposed to reflect the value configured by the user and not change on their own. A remaining-file-loops property is exposed as a replacement to check how many loops are left.
Follow up to the previous commit. Stop decreasing --ab-loop-count=N on each iteration so it is preserved across different loops. In particular it is preserved between different files without adding it to --reset-on-next-file. Add a property to expose the remaning A-B loop count instead. The current behavior of --ab-loop-count=N is even worse than --loop-file since it also doesn't reset when defining a new A-B loop in the same file. Defining it has no effect after --ab-loop-count has decreased to 0, and this can't be fixed by adding it to --reset-on-next-file. This commit also resets remaining-ab-loops every time --ab-loop-a and --ab-loop-b are set to fix this.
| @@ -2139,6 +2139,18 @@ Property list | |||
| ``playback-time/full`` | |||
| ``playback-time`` with milliseconds. | |||
|
|
|||
| ``remaining-file-loops`` | |||
There was a problem hiding this comment.
wouldn't loop-file-remaining be more in line with existing naming?
There was a problem hiding this comment.
That makes no grammatical sense though. And the options are already inconsistent, --loop-file-count and --loop-playlist-count would more logical and consistent with --ab-loop-count.
commit 1: player: don't decrement --loop-file=N and add remaining-file-loops
This stops decreasing numerical values of --loop-file on each iteration so that loop-file=N loops every playlist entry without having to add --loop-file to --reset-on-next-file.
The current behavior confuses users as seen in:
#2481
#5943
#11291
#13860
https://www.reddit.com/r/mpv/comments/rcwnrw/looping_each_file_n_times_in_a_playlist/
Also options are supposed to reflect the value configured by the user and not change on their own.
A remaining-file-loops property is exposed as a replacement to check how many loops are left.
commit 2: player: don't decrement --ab-loop-count=N and add remaining-ab-loops
Follow up to the previous commit. Stop decreasing --ab-loop-count=N on each iteration so it is preserved across different loops. In particular it is preserved between different files without adding it to --reset-on-next-file. Add a property to expose the remaning A-B loop count instead.
The current behavior of --ab-loop-count=N is even worse than --loop-file since it also doesn't reset when defining a new A-B loop in the same file. Defining it has no effect after --ab-loop-count has decreased to 0, and this can't be fixed by adding it to --reset-on-next-file. This commit also resets remaining-ab-loops every time --ab-loop-a and --ab-loop-b are set to fix this.
I didn't add
remaining-playlist-loopsbecause--playlist-loopalready applies to all files. We also avoid dealing with the inconsistency of--loop-playlist=1corresponding to 1 playthrough and--loop-file=1corresponding to 2.