Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support next-archive or next-nonrecursive playlist controls (and/or next-directory) #12495

Closed
pcordes opened this issue Sep 26, 2023 · 9 comments · Fixed by #12526 or #12626
Closed

Support next-archive or next-nonrecursive playlist controls (and/or next-directory) #12495

pcordes opened this issue Sep 26, 2023 · 9 comments · Fixed by #12526 or #12626

Comments

@pcordes
Copy link

pcordes commented Sep 26, 2023

After running mpv gallery1.zip gallery2.zip
no key I can press will seek to the first file in gallery2.zip, skipping the rest of gallery1.zip


It's fairly common to have some video files and some image sets, with each image set in its own .zip or directory.
mpv is almost the perfect tool to play a collection of such files, like mpv foo*. (libarchive is inefficient especially when first opening an archive, but with a fast enough computer and non-huge archives, it's still faster than running gwenview or other full GUIs, and I often want MPV's video-playing features.)

But if you get to a big image gallery / photoset you don't want to see all of right now, it's painful to get past it in the playlist.

e.g. if I want to look at recently-downloaded (and still-downloading) videos and image-sets in a directory, I use this bash function mpvcrt() { local args=($MPV); local IFS=$'\n'; mpv "${args[@]}" $(ls -Hct "$@"); } to play files and directories in order of newest ctime, for filenames that don't contain spaces. (Or something else if I pass ls options mixed with paths, like -d if I specify a directory.)

One current workaround is to alt-tab back to the shell so you can at least use keyboard-repeat, since MPV over the past few years has changed so right-arrow doesn't go to the next JPEG, ] speed multiplier doesn't scale the --image-display-duration=, and keyboard-repeat doesn't happen for enter/return (in MPV's window; it can't do anything about the terminal emulator), or for multimedia keyboard keys like XF86AudioNext. (This is arguably a good choice by default, although some way to get auto-repeat for playlist-next might make sense.)

Expected behavior of the wanted feature

On pressing a key bound to playlist-next-archive, scan forward in the playlist until reaching an entry whose top-level archive:// is different from the current one. (Or isn't an archive at all.) If current isn't an archive:// entry, precisely the same as playlist-next, whether or not the next entry is an archive or not.

On pressing playlist-next-directory, search for an entry whose dirname (stuff before the final /) is different from the current entry. Maybe unless this entry is already top level like foo.mp4? e.g. from foo/set42.jpg, skip until getting to bar/set1.jpg. But there are probably lots of cases where this algorithm doesn't give the behaviour we actually want.

Alternative behavior of the wanted feature (I like this idea better)

A playlist-next-nonrecursive input command could cover both archives and directories the way I'm looking for, seeking to the next playlist entry that was given explicitly rather than via recursive expansion of a .zip into multiple archive:// entries, or a directory into its contents. (Or to an entry that came from expansion of a different top-level entry, if you have two back-to-back archives.)

Ideally with an exception for the case where the initial playlist had one entry, like mpv /foo/bar or mpv foo.playlist. Otherwise you'd have to write mpv /foo/bar/* if you want to be able to playlist-next-nonrecursive across archives and subdirectories

That would also handle the case of a .zip or .rar containing .zip files: if you specified just that on the command line, the next/prev nonrecursive commands would skip between the zip files it contained, since the top-level playlist was a single entry. (Making next-nonrecursive equivalent to quit is less useful than allowing seeking by groups within the expansion results.)

This is hopefully simple (a playlist entry can be marked as being generated by recursive expansion vs. supplied explicitly), and only needs 2 new commands. It wouldn't be turning mpv into a general file-browser or anything. (GUIs like gwenview exist for that, or for only still images not in archives, geeqie is nice and lightweight). And it wouldn't need any complicated heuristics for directories vs. top-level to get the behaviour I want, I think

If people want to skip between directories or archives 2 levels deep, they could do something like mpv /foo/*/*.zip so the things they want to skip between are top-level playlist entries.

So for example, if the initial playlist (from the command line) contains

foo.mp4
foo.zip
bar.mp4
bar.zip
some_dir/
some.zip
xyz.mp4

(The top-level entries can of course be in different directories; we don't care about paths, just which original entries they were expanded from, so I kept it flat.)

playlist-next-nonrecursive from archive://foo.zip/42.jpg would get to bar.mp4. And playlist-prev-nonrecursive would go to foo.mp4. playlist-next-nonrecursive from foo.mp4 would go to archive://foo.zip/01.jpg, same as a normal playlist-next because foo.mp4 wasn't the result of recursive expansion.

The same for some_dir/42.jpg going forward or back. Going forward into archive://some.zip/01.jpg or whatever it contains, we're going from one recursively-expanded entry to another, but they came from different top-level entries so we do stop there. So I guess we'd need a unique ID number to record the entry number they were expanded from, instead of just a boolean recursive-or-not.

Perhaps auto-number the initial playlist entries, and when recursively expanding give each entry the same ID as its parent.

Or generate a new ID for each level of recursive expansion, so foo.zip/set1/*.jpg and foo.zip/set2/*.jpg end up as separate entries. That would be different behaviour that some other users might want; it could be worth having an option for that. It still just needs one unique ID per entry, like a uint32_t or u64 counter, not recording the tree structure.

(Although if that's easy, maybe a playlist-up-and-next command could work, with up from the first entry of an expanded group counting as the parent from whence you can go farther up? IDK if that makes any sense in UI design / usability even if the playlist is a tree data structure.)

@Dudemanguy
Copy link
Member

We internally save the original playlist paths/urls now for every entry, so simply going up/down playlists is perfectly doable. You could script it right now but it makes sense to have playlist-prev-playlist/playlist-next-playlist (or I dunno what you want to call it) commands. Does that cover what you want?

@pcordes
Copy link
Author

pcordes commented Sep 26, 2023

Does that cover what you want?

I think so, if mpv foo.zip bar.mp4 some_dir treats foo.zip and some_dir as "playlists" for that purpose. Then yes, it's precisely what I came up with for playlist-next-nonrecursive.

For zip-within-rar (or zip within some_dir/), that's rare enough that I'm not super picky about skipping to the next top-level playlist vs. just skipping out of the inner-most zip to another entry within the rar or directory. In many cases, just skipping to the next zip in the same rar is what I'd prefer, but whichever one is most natural to implement would work.

( zip within zip or rar within rar would be the same, but just for the sake of example.)

@pcordes
Copy link
Author

pcordes commented Sep 26, 2023

Possible enhancement to this, perhaps not super useful:

  • Take an optional step=n arg for playlist-next-playlist or prev to go farther than 1 step counting whole playlists as 1 entry.

For normal playlist-next, we can get that functionality with

alt+PGUP   add playlist-pos-1  10
alt+PGDWN  add playlist-pos-1 -10

And apparently it's possible to script things so you can overload the same keybind (like plain PGUP) with different bindings for images vs. non-image (https://github.com/occivink/mpv-image-viewer#detect-imagelua), although that's even less simple.

(This makes flipping through image galleries less painful, and it doesn't disable auto-repeat. But playlist-next-playlist or equivalent would still be a lot nicer, since overshoot by 0 to 9 playlist entries past the end of a gallery is a lot less nice.)

@Hrxn
Copy link
Contributor

Hrxn commented Sep 27, 2023

We internally save the original playlist paths/urls now for every entry, so simply going up/down playlists is perfectly doable. You could script it right now but it makes sense to have playlist-prev-playlist/playlist-next-playlist (or I dunno what you want to call it) commands. Does that cover what you want?

I'd suggest to maybe call it something like playlist-next-sublist/playlist-prev-sublist?

@Dudemanguy
Copy link
Member

I'd suggest to maybe call it something like playlist-next-sublist/playlist-prev-sublist?

I have literally no idea what to call so I'm open to any suggestions. They're not really sublists though. You can have multiple playlists. However, I did just realize one pretty big flaw with this. You'd have to probe every file somehow to see if it's actually a playlist first if it wasn't opened already. Makes it not trivial.

@pcordes
Copy link
Author

pcordes commented Sep 27, 2023

You'd have to probe every file somehow to see if it's actually a playlist first if it wasn't opened already. Makes it not trivial.

Do you you mean for my proposed step=n suggestion? I don't mind skipping over archives or directories and not expanding them until later, exactly like happens with add playlist-pos-1 10. (Can also happen if you press enter fast enough that I/O latency prevents MPV from opening an archive and recursing before skipping away from it.)

Not knowing MPV internals, I'd have guessed that a working algorithm for playlist-next-group would be

  • If current has a playlist it was expanded from, or its actually URI isn't equal to the "original playlist paths/urls" associated with it, it's an entry in a playlist that was expanded after startup. Iterate forward or backward until you get to an entry with a different string.

    Else just go forward or back by 1 like normal playlist-next.

  • If that triggers lazy expansion of a filename into multiple playlist entries, current is now the first item in that expansion. (Do we need to check at this point if the "original playlist paths/urls" string is still the same as the one we're seeking away from? If expansion is all or nothing, all entries from the same parent would already be expanded.)


Or are you talking about a case like zip inside zip, deciding whether to just go to an other zip inside the outer zip vs. leaving the outer zip? If that decision depends on whether you've already visited a lazy-expanded playlist item or not, that seems ok. People will learn how the feature works for their use-case and adapt, maybe, or not use it if it doesn't suit.

A simple implementation would work a lot of the time for a lot of people, or at least for me, and be much better than nothing.

@Dudemanguy
Copy link
Member

No I meant in general. mpv doesn't know if something is a playlist until it gets opened and either expanded or not. So if you have a list of arguments, there's no way to go forward to the next playlist without probing every item and seeing if it is one. Obviously you could guess if the path is a directory or something, but that's ugly wouldn't cover everything (e.g. a url could be a playlist).

@pcordes
Copy link
Author

pcordes commented Sep 27, 2023

there's no way to go forward to the next playlist without probing every item and seeing if it is one.

But that's not the feature I'm requesting. I want to go forward to the first item that's not part of the current sub-playlist.

For mpv foo.zip bar.mp4 bar.zip, I want playlist-next-group to go from foo.zip/42.jpg to bar.mp4, not past it into bar.zip.

guidocella added a commit to guidocella/mpv that referenced this issue Oct 1, 2023
playlist-prev-playlist goes to the beginning of the previous playlist
because this seems more useful and symmetrical to
playlist-next-playlist. It does not go to the beginning when the current
playlist-path starts with the previous playlist-path, e.g. with mpv
--loop-playlist foo/, which expands to foo/{1..9}.zip, the current
playlist path foo/1.zip beings with the playlist-path foo/ of {2..9}.zip
and thus playlist-prev-playlist goes to 9.zip rather than to 2.zip.

Closes mpv-player#12495.
guidocella added a commit to guidocella/mpv that referenced this issue Oct 7, 2023
playlist-prev-playlist goes to the beginning of the previous playlist
because this seems more useful and symmetrical to
playlist-next-playlist. It does not go to the beginning when the current
playlist-path starts with the previous playlist-path, e.g. with mpv
--loop-playlist foo/, which expands to foo/{1..9}.zip, the current
playlist path foo/1.zip beings with the playlist-path foo/ of {2..9}.zip
and thus playlist-prev-playlist goes to 9.zip rather than to 2.zip.

Closes mpv-player#12495.
guidocella added a commit to guidocella/mpv that referenced this issue Oct 7, 2023
playlist-prev-playlist goes to the beginning of the previous playlist
because this seems more useful and symmetrical to
playlist-next-playlist. It does not go to the beginning when the current
playlist-path starts with the previous playlist-path, e.g. with mpv
--loop-playlist foo/, which expands to foo/{1..9}.zip, the current
playlist path foo/1.zip beings with the playlist-path foo/ of {2..9}.zip
and thus playlist-prev-playlist goes to 9.zip rather than to 2.zip.

Closes mpv-player#12495.
guidocella added a commit to guidocella/mpv that referenced this issue Oct 7, 2023
playlist-prev-playlist goes to the beginning of the previous playlist
because this seems more useful and symmetrical to
playlist-next-playlist. It does not go to the beginning when the current
playlist-path starts with the previous playlist-path, e.g. with mpv
--loop-playlist foo/, which expands to foo/{1..9}.zip, the current
playlist path foo/1.zip beings with the playlist-path foo/ of {2..9}.zip
and thus playlist-prev-playlist goes to 9.zip rather than to 2.zip.

Closes mpv-player#12495.
guidocella added a commit to guidocella/mpv that referenced this issue Oct 9, 2023
playlist-prev-playlist goes to the beginning of the previous playlist
because this seems more useful and symmetrical to
playlist-next-playlist. It does not go to the beginning when the current
playlist-path starts with the previous playlist-path, e.g. with mpv
--loop-playlist foo/, which expands to foo/{1..9}.zip, the current
playlist path foo/1.zip beings with the playlist-path foo/ of {2..9}.zip
and thus playlist-prev-playlist goes to 9.zip rather than to 2.zip.

Closes mpv-player#12495.
@pcordes
Copy link
Author

pcordes commented Oct 13, 2023

@Dudemanguy Thanks for adding this. I pulled the latest git and built it on x86-64 Arch GNU/Linux.
It is indeed exactly what I wanted. (Edit: I mean thanks Guidocella for actually writing the code. The issue reported below is fixed now.)

I'm getting a segfault with it in some cases at the start of the topmost playlist when going forward into a zip, then back, then forward again.

The test case where I happened to see this was mpv foo.mp4 bar.zip xyz* (video then zip of JPG then a bunch of videos and zips).

The minimal sequence of keystrokes is:

  • HOME (playlist-next-playlist, because it's next to page-up, and going up to higher numbers made sense to me for seeking forwards.) This goes from the video into the zip of over 200 JPEG images, specifically to the first one.
  • END (playlist-prev-playlist). This goes back to the start of the first video.
  • HOME (playlist-next-playlist). This segfaults instead of going forward into the first image of the zip.

This is 100% reproducible for me with this build and set of command line args. Another repro sequence is p-next-p / p-next-p (to a video that follows the zip) / p-prev-p (back into the zip segfaults; that seems to be the common factor.) It still happens even if I press return (playlist-next) a few times within the zip before leaving and trying to come back.

Let me know if it's not reproducible for you and I'll figure out how to get meson to compile with -g and run it under GDB to see where the bad pointer was and get a backtrace to make a proper bug report as a new issue.

guidocella added a commit to guidocella/mpv that referenced this issue Oct 13, 2023
This was wrongly assuming that playlist_path is always set for the
current playlist entry, but it's only set when a file was added by
expanding a playlist.

The crash in playlist_get_first_in_next_playlist can be reproduced with
mpv foo.mkv foo.zip, playlist-next, playlist-prev,
playlist-next-playlist. You need to run playlist-next, playlist-prev
first because foo.zip's playlist_path is NULL until you do that, which
makes playlist_get_first_in_next_playlist return immediately.

The crash in cmd_playlist_next_prev_playlist can be replicated with mpv
--loop-playlist foo.zip foo.mkv, running playlist-next until foo.mkv,
and playlist-play-next. Again, you need to open foo.zip first or its
playlist_path is NULL which skips running strcmp(entry->playlist_path,
mpctx->playlist->current->playlist_path).

Fixes mpv-player#12495 (comment)
Dudemanguy pushed a commit that referenced this issue Oct 13, 2023
This was wrongly assuming that playlist_path is always set for the
current playlist entry, but it's only set when a file was added by
expanding a playlist.

The crash in playlist_get_first_in_next_playlist can be reproduced with
mpv foo.mkv foo.zip, playlist-next, playlist-prev,
playlist-next-playlist. You need to run playlist-next, playlist-prev
first because foo.zip's playlist_path is NULL until you do that, which
makes playlist_get_first_in_next_playlist return immediately.

The crash in cmd_playlist_next_prev_playlist can be replicated with mpv
--loop-playlist foo.zip foo.mkv, running playlist-next until foo.mkv,
and playlist-play-next. Again, you need to open foo.zip first or its
playlist_path is NULL which skips running strcmp(entry->playlist_path,
mpctx->playlist->current->playlist_path).

Fixes #12495 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants