Skip to content

Commit

Permalink
input/player: add loadfile/loadlist insert-at command
Browse files Browse the repository at this point in the history
  • Loading branch information
@DavidRV00 @Dudemanguy
DavidRV00 authored and Dudemanguy committed Feb 26, 2024
1 parent da75319 commit c678033
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 29 deletions.
36 changes: 31 additions & 5 deletions DOCS/man/input.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,9 @@ Remember to quote string arguments in input.conf (see `Flat command syntax`_).
restarted if for example the new playlist entry is the same as the previous
one.

``loadfile <url> [<flags> [<options>]]``
``loadfile <url> [<flags> [<index> [<options>]]]``
Load the given file or URL and play it. Technically, this is just a playlist
manipulation command (which either replaces the playlist or appends an entry
manipulation command (which either replaces the playlist or adds an entry
to it). Actual file loading happens independently. For example, a
``loadfile`` command that replaces the current file with a new one returns
before the current file is stopped, and the new file even begins loading.
Expand All @@ -475,15 +475,28 @@ Remember to quote string arguments in input.conf (see `Flat command syntax`_).
Insert the file next, and if nothing is currently playing, start playback.
(Always starts with the added file, even if the playlist was not empty
before running this command.)

The third argument is a list of options and values which should be set
<insert-at>
Insert the file into the playlist, at the index given in the third
argument.
<insert-at-play>
Insert the file at the index given in the third argument, and if nothing
is currently playing, start playback. (Always starts with the added
file, even if the playlist was not empty before running this command.)

The third argument is an insertion index, used only by the `insert-at` and
`insert-at-play` actions. When used with those actions, the new item will be
insert at the <index> position in the playlist, or appended to the end if
<index> is less than 0 or greater than the size of the playlist. This
argument will be ignored for all other actions.

The fourth argument is a list of options and values which should be set
while the file is playing. It is of the form ``opt1=value1,opt2=value2,..``.
When using the client API, this can be a ``MPV_FORMAT_NODE_MAP`` (or a Lua
table), however the values themselves must be strings currently. These
options are set during playback, and restored to the previous value at end
of playback (see `Per-File Options`_).

``loadlist <url> [<flags>]``
``loadlist <url> [<flags> [<index>]]``
Load the given playlist file or URL (like ``--playlist``).

Second argument:
Expand All @@ -503,6 +516,19 @@ Remember to quote string arguments in input.conf (see `Flat command syntax`_).
Insert the new playlist, and if nothing is currently playing, start
playback. (Always starts with the new playlist, even if the internal
playlist was not empty before running this command.)
<insert-at>
Insert the new playlist at the index given in the third argument.
<insert-at-play>
Insert the new playlist at the index given in the third argument, and if
nothing is currently playing, start playback. (Always starts with the
new playlist, even if the internal playlist was not empty before running
this command.)

The third argument is an insertion index, used only by the `insert-at` and
`insert-at-play` actions. When used with those actions, the new playlist
will be insert at the <index> position in the internal playlist, or appended
to the end if <index> is less than 0 or greater than the size of the
internal playlist. This argument will be ignored for all other actions.

``playlist-clear``
Clear the playlist, except the currently played file.
Expand Down
4 changes: 2 additions & 2 deletions common/playlist.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,8 @@ void playlist_set_stream_flags(struct playlist *pl, int flags)
pl->entries[n]->stream_flags = flags;
}

static int64_t playlist_transfer_entries_to(struct playlist *pl, int dst_index,
struct playlist *source_pl)
int64_t playlist_transfer_entries_to(struct playlist *pl, int dst_index,
struct playlist *source_pl)
{
assert(pl != source_pl);
struct playlist_entry *first = playlist_get_first(source_pl);
Expand Down
2 changes: 2 additions & 0 deletions common/playlist.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ struct playlist_entry *playlist_get_first_in_same_playlist(struct playlist_entry
char *current_playlist_path);
void playlist_add_base_path(struct playlist *pl, bstr base_path);
void playlist_set_stream_flags(struct playlist *pl, int flags);
int64_t playlist_transfer_entries_to(struct playlist *pl, int dst_index,
struct playlist *source_pl);
int64_t playlist_transfer_entries(struct playlist *pl, struct playlist *source_pl);
int64_t playlist_append_entries(struct playlist *pl, struct playlist *source_pl);

Expand Down
100 changes: 78 additions & 22 deletions player/command.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,18 @@ struct hook_handler {
bool active; // hook is currently in progress (only 1 at a time for now)
};

enum load_action_type {
LOAD_TYPE_REPLACE,
LOAD_TYPE_INSERT_AT,
LOAD_TYPE_INSERT_NEXT,
LOAD_TYPE_APPEND,
};

struct load_action {
enum load_action_type type;
bool play;
};

// U+279C HEAVY ROUND-TIPPED RIGHTWARDS ARROW
// U+00A0 NO-BREAK SPACE
#define ARROW_SP "\342\236\234\302\240"
Expand Down Expand Up @@ -5522,37 +5534,71 @@ static void cmd_expand_path(void *p)
};
}

static struct load_action get_load_action(struct MPContext *mpctx, int action_flag)
{
switch (action_flag) {
case 0: // replace
return (struct load_action){LOAD_TYPE_REPLACE, .play = true};
case 1: // append
return (struct load_action){LOAD_TYPE_APPEND, .play = false};
case 2: // append-play
return (struct load_action){LOAD_TYPE_APPEND, .play = true};
case 3: // insert-next
return (struct load_action){LOAD_TYPE_INSERT_NEXT, .play = false};
case 4: // insert-next-play
return (struct load_action){LOAD_TYPE_INSERT_NEXT, .play = true};
case 5: // insert-at
return (struct load_action){LOAD_TYPE_INSERT_AT, .play = false};
case 6: // insert-at-play
return (struct load_action){LOAD_TYPE_INSERT_AT, .play = true};
default: // default: replace
return (struct load_action){LOAD_TYPE_REPLACE, .play = true};
}
}

static struct playlist_entry *get_insert_entry(struct MPContext *mpctx, struct load_action *action,
int insert_at_idx)
{
switch (action->type) {
case LOAD_TYPE_INSERT_NEXT:
return playlist_get_next(mpctx->playlist, +1);
case LOAD_TYPE_INSERT_AT:
return playlist_entry_from_index(mpctx->playlist, insert_at_idx);
case LOAD_TYPE_REPLACE:
case LOAD_TYPE_APPEND:
default:
return NULL;
}
}

static void cmd_loadfile(void *p)
{
struct mp_cmd_ctx *cmd = p;
struct MPContext *mpctx = cmd->mpctx;
char *filename = cmd->args[0].v.s;
int action = cmd->args[1].v.i;
int action_flag = cmd->args[1].v.i;
int insert_at_idx = cmd->args[2].v.i;

bool replace = (action == 0);
bool insert_next = (action == 3 || action == 4);
bool play = (action == 2 || action == 4);
struct load_action action = get_load_action(mpctx, action_flag);

if (replace)
if (action.type == LOAD_TYPE_REPLACE)
playlist_clear(mpctx->playlist);

struct playlist_entry *entry = playlist_entry_new(filename);
if (cmd->args[2].v.str_list) {
char **pairs = cmd->args[2].v.str_list;
if (cmd->args[3].v.str_list) {
char **pairs = cmd->args[3].v.str_list;
for (int i = 0; pairs[i] && pairs[i + 1]; i += 2)
playlist_entry_add_param(entry, bstr0(pairs[i]), bstr0(pairs[i + 1]));
}

struct playlist_entry *at = insert_next ?
playlist_get_next(mpctx->playlist, +1) : NULL;

struct playlist_entry *at = get_insert_entry(mpctx, &action, insert_at_idx);
playlist_insert_at(mpctx->playlist, entry, at);

struct mpv_node *res = &cmd->result;
node_init(res, MPV_FORMAT_NODE_MAP, NULL);
node_map_add_int64(res, "playlist_entry_id", entry->id);

if (replace || (play && !mpctx->playlist->current)) {
if (action.type == LOAD_TYPE_REPLACE || (action.play && !mpctx->playlist->current)) {
if (mpctx->opts->position_save_on_quit) // requested in issue #1148
mp_write_watch_later_conf(mpctx);
mp_set_playlist_entry(mpctx, entry);
Expand All @@ -5566,33 +5612,37 @@ static void cmd_loadlist(void *p)
struct mp_cmd_ctx *cmd = p;
struct MPContext *mpctx = cmd->mpctx;
char *filename = cmd->args[0].v.s;
int flag = cmd->args[1].v.i;
int action_flag = cmd->args[1].v.i;
int insert_at_idx = cmd->args[2].v.i;

bool replace = (flag == 0);
bool insert_next = (flag == 3 || flag == 4);
bool play = (flag == 2 || flag == 4);
struct load_action action = get_load_action(mpctx, action_flag);

struct playlist *pl = playlist_parse_file(filename, cmd->abort->cancel,
mpctx->global);
if (pl) {
prepare_playlist(mpctx, pl);
struct playlist_entry *new = pl->current;
if (replace)
if (action.type == LOAD_TYPE_REPLACE)
playlist_clear(mpctx->playlist);
struct playlist_entry *first = playlist_entry_from_index(pl, 0);
int num_entries = pl->num_entries;
if (insert_next) {
playlist_transfer_entries(mpctx->playlist, pl);
} else {

struct playlist_entry *at = get_insert_entry(mpctx, &action, insert_at_idx);
if (at == NULL) {
playlist_append_entries(mpctx->playlist, pl);
} else {
int at_index = playlist_entry_to_index(mpctx->playlist, at);
playlist_transfer_entries_to(mpctx->playlist, at_index, pl);
}
talloc_free(pl);

if (!new)
new = playlist_get_first(mpctx->playlist);

if ((replace || (play && !mpctx->playlist->current)) && new)
if ((action.type == LOAD_TYPE_REPLACE ||
(action.play && !mpctx->playlist->current)) && new) {
mp_set_playlist_entry(mpctx, new);
}

struct mpv_node *res = &cmd->result;
node_init(res, MPV_FORMAT_NODE_MAP, NULL);
Expand Down Expand Up @@ -6699,8 +6749,11 @@ const struct mp_cmd_def mp_cmds[] = {
{"append", 1},
{"append-play", 2},
{"insert-next", 3},
{"insert-next-play", 4}),
{"insert-next-play", 4},
{"insert-at", 5},
{"insert-at-play", 6}),
.flags = MP_CMD_OPT_ARG},
{"index", OPT_INT(v.i), OPTDEF_INT(-1)},
{"options", OPT_KEYVALUELIST(v.str_list), .flags = MP_CMD_OPT_ARG},
},
},
Expand All @@ -6712,8 +6765,11 @@ const struct mp_cmd_def mp_cmds[] = {
{"append", 1},
{"append-play", 2},
{"insert-next", 3},
{"insert-next-play", 4}),
{"insert-next-play", 4},
{"insert-at", 5},
{"insert-at-play", 6}),
.flags = MP_CMD_OPT_ARG},
{"index", OPT_INT(v.i), OPTDEF_INT(-1)},
},
.spawn_thread = true,
.can_abort = true,
Expand Down

20 comments on commit c678033

@bitingsock
Copy link
Contributor

@bitingsock bitingsock commented on c678033 Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the third argument be made to only be taken for these two flags and otherwise keep options as the third argument, so that loadfile retains backward compatability?

@mitzsch
Copy link
Contributor

@mitzsch mitzsch commented on c678033 Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, would be awesome if some sort of backward compatibility could be brought back. Now with every new release mpv is unusable in apps relying on the "old" and established syntax. I guess I will open a bug report for this...

@Dudemanguy
Copy link
Member

@Dudemanguy Dudemanguy commented on c678033 Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original implementation attempted backwards compatibility, but I deemed it far too complex and we went with this instead. I can think of a hack to keep backwards compatibility but not sure it should really be done. Is the options argument really that commonly used for this to be a big problem? I would not think so. It is not like properties/commands are never supposed to change.

@bitingsock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not that rare, but another issue is clarity. What is suppose to go in the index argument for the other flags? There is no Default construct, and including a random value on the other flags is somewhat confusing.

@Dudemanguy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it says "This argument will be ignored for all other actions." You can put whatever number you want. It's just not used.

@bitingsock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that it's easy to work around. IMO this functionality already existed, albeit with multiple steps, so is not worth breaking compatibility and readability.

@mitzsch
Copy link
Contributor

@mitzsch mitzsch commented on c678033 Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the options argument really that commonly used for this to be a big problem?

At least Plex HTPC (haven´t tested Plex for Desktop) uses the loadfile command and will not play any file with the new commit. The log states that the syntax is wrong.
I assume other apps are broken too now.

I will now maintain a fork with this commit reverted. With this one reverted Plex HTPC works as expected... Having a fork just for the purpose of having one commit reverted is probably not ideal, however...

@Dudemanguy
Copy link
Member

@Dudemanguy Dudemanguy commented on c678033 Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I don't know the inner workings of Plex but it's not like it's hard to update syntax (and not sure why they would unconditionally pass the options argument in the first place). It's not like this is a C API that never has incompatible changes.

However, see #13624 which will make it backwards compatible again.

@mitzsch
Copy link
Contributor

@mitzsch mitzsch commented on c678033 Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it's not like it's hard to update syntax

Thats true, but Plex is probably not able to update the syntax. It would probably break their (outdated) stripped-down correctly licensed version of mpv.
Thanks for pointing out the new PR! Awesome! Will try this! Thanks!

@llyyr
Copy link
Contributor

@llyyr llyyr commented on c678033 Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats true, but Plex is probably not able to update the syntax. It would probably break their (outdated) stripped-down correctly licensed version of mpv.

Why does Plex care about a new upstream change if they're using an outdated stripped down version of mpv?

@mitzsch
Copy link
Contributor

@mitzsch mitzsch commented on c678033 Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don´t know if Plex cares about upstream changes but the users do - like me. The mpv.dll can be swapped on their apps to get some nice new features. Thats why we care about upstream compatibility. ;)

@llyyr
Copy link
Contributor

@llyyr llyyr commented on c678033 Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see why mpv should try to not break command syntax or IPC to maintain compatibility with Plex Media Player's outdated mpv version

@mitzsch
Copy link
Contributor

@mitzsch mitzsch commented on c678033 Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plex Media Player is dead for years. ^^ The current app is called Plex HTPC. It was just an example mentioned by me.
There are probably other non-plex apps that are broken due to this change. @bitingsock what app is affected on your side?

@Dudemanguy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see why mpv should try to not break command syntax or IPC to maintain compatibility with Plex Media Player's outdated mpv version

I mean yeah that's not really my main concern. However, extending generally useful functionality to commands is nice and can save you from typing -1 so Plex people benefiting is just a side effect.

@bitingsock
Copy link
Contributor

@bitingsock bitingsock commented on c678033 Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less of an issue because I actively maintain it, but my youtube playlist preloading script requires the old syntax. Easy fix there.

@yuv420p10le
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don´t know if Plex cares about upstream changes but the users do - like me. The mpv.dll can be swapped on their apps to get some nice new features. Thats why we care about upstream compatibility. ;)

I wrote a hack that simplifies the loadfile command that Plex calls; you don't need a separate maintained fork with one commit excluded. https://github.com/yuv420p10le/plex_loadfile_hook

Hopefully Plex will just fix it on their end, and update libmpv; as needing to do this at all is pretty inconvenient.

@trip54654
Copy link

@trip54654 trip54654 commented on c678033 May 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a breaking API change. If commands weren't part of the API, you couldn't use libmpv for anything. Shame on @Dudemanguy for trying to defend his failure as maintainer.

@Dudemanguy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a documented breaking change. Sometimes commands have breaking changes between versions. I'm sorry that you might have to spend a couple of seconds to change 1 or 2 lines in your code. If Plex didn't apparently unconditionally use the options argument or used named arguments instead, I doubt half the complaints on this commit would even exist.

@trip54654
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will fix the bugs for free and you will be happy.

Anyway, it's an unnecessary breaking change in a command that needs to be used by every single API user. Plex was brought up only later.

@PCigales
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt half the complaints on this commit would even exist.

I am inclined to complain on this commit... (url PCigales/DLNAmpvRenderer@7f9c08d), but thank you anyway for all the work on this media player.

Please sign in to comment.