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

Opening folder spawns playlist that includes non-media files #4548

Closed
justinmayer opened this issue Jul 31, 2023 · 12 comments · Fixed by #4566
Closed

Opening folder spawns playlist that includes non-media files #4548

justinmayer opened this issue Jul 31, 2023 · 12 comments · Fixed by #4566

Comments

@justinmayer
Copy link

justinmayer commented Jul 31, 2023

Opening a folder creates a playlist that includes non-media files that do not belong in playlists.

System and IINA version:

  • macOS 12.6.7
  • IINA 1.3.3

Expected behavior:

Dragging a folder containing a mix of playable media files and non-playable files should result in a playlist that only contains the playable media files. This was the behavior in version 1.3.2 and all IINA versions I have used prior to version 1.3.3.

Actual behavior:

Resulting playlist contains spurious unplayable files. This behavior appears to have started with version 1.3.3.

Steps to reproduce:

Drag a folder containing the following files to the IINA icon in the MacOS dock:

  • 01 - Apple.flac
  • 02 - Peach.flac
  • 03 - Orange.flac
  • Melon.jpg
  • Melon.accurip
  • Melon.cue
  • Melon.log
  • Melon.m3u

In 1.3.2 and before, the resulting playlist would only include the first three items — the FLAC files.

In 1.3.3, the resulting playlist contains all eight items, including the last five which should not be included. Including the .m3u in the playlist is particularly problematic, as it causes the entire album to be played all over again when playback arrives at this item in the list.

How often does this happen?

Every time. I had no choice but to revert to version 1.3.2.

@svobs
Copy link
Contributor

svobs commented Jul 31, 2023

Looks like this was a consequence of the fix for #4434. The logic was changed so that supplying a single folder onto IINA resulted in sending it directly to mpv.

It looks like we are being squashed between contradictory requirements here. Doesn't look like mpv does any filtering when it is handed a directory. I did some tests and saw it opening PDFs, and even a .txt file made it into the playlist (although it was skipped over immediately when played).

What can be done? Off the top of my head:

  1. Reimplement all of mpv's features on a per-folder basis, but with IINA's filtering (no, won't do this).
  2. Create some kind of elaborate temporary/virtual folder scheme to populate with filtered files and fool mpv into accepting it instead of the original folder.
  3. Add a switch in the Settings to let the user decide whether to let mpv process a folder.

Or maybe there's a way to ask mpv to do the filtering? Will have to look deeper in the mpv manual but didn't find anything with a quick scan.

@justinmayer
Copy link
Author

Thank you for the detailed info, @svobs. My unfiltered thoughts include:

  • IINA’s directory loading feature is valuable and should been retained (even when passed a single directory).
  • I have doubts about relying on mpv to do the filtering. Relying on external tools to provide the directory loading functionality seems… unwise. What happens when that behavior changes underfoot? Seems better to provide a consistent playlist loading experience by controlling it “in-house”, as it were.
  • The stated goal in PR #4439 was to “provide a workable solution for most users … while not breaking IINA’s directory loading features.” I think we should still shoot for that.
  • I’m not sure if the problem is contradictory requirements so much as a situation in which a fix for one (possibly esoteric) use case accidentally broke core playlist functionality.
  • If the perspective is that these are indeed contradictory requirements, then supporting iina-cli --mpv-shuffle is not worth the regression.
  • In addition to proposed solutions 2 & 3 above, another could be to only defer directory loading to mpv specifically when the --mpv-shuffle option is passed. (I’m not saying it’s the best solution, but it is a solution.)

I say all this as someone who uses the GUI playlist daily and who has never used the CLI, so of course my perspective reflects that.

IINA is awesome, and I have great respect for y’all for all the work you put into making it great! 🌟

@svobs
Copy link
Contributor

svobs commented Aug 2, 2023

More than an external tool, mpv is more like the engine which IINA wraps around...or at least that's how some see it. Although there are now so many instances where some limitation of mpv (heh, changing key bindings in real time?) have resulted in IINA going its own way, It's difficult to say where the dividing line is. Obviously the best outcome would be to support as much of mpv's features as possible. But I agree with you it's not desirable to leave the current situation as-is.

After thinking about it a bit more, instead of a UI setting as in (3) above, we could add a new command line option to tell IINA send the directory straight to mpv. Something like --mpv-dir={some_dir}. And it could only be given at most once per run - i.e. for a single directory. I'd rather not generalize it more because at the moment IINA's architecture can't handle supplying more than one directory to mpv, so that would be a larger change. Then IINA could again default to its own filtering/handling.

@low-batt what do you think?

@low-batt
Copy link
Contributor

low-batt commented Aug 2, 2023

On my thoughts… First the one @justinmayer is most concerned with. I think this is a regression that must be fixed. I will label it so.

On IINA's relationship with mpv… From the main mpv site:

mpv is a free (as in freedom) media player for the command line.

And:

While mpv strives for minimalism and provides no real GUI, it has a small controller on top of the video for basic control.

And:

A straightforward C API was designed from the ground up to make mpv usable as a library and facilitate easy integration into other applications.

The mpv wiki lists projects providing GUI frontends among them:

  • IINA:
    A media player for macOS 10.10+ based on mpv.

(that wiki page will need to be updated with IINA's latest macOS requirements at some point)

In IINA's Advanced setting you are able to directly make use of mpv options and switch to mpv's OSD. So… yes, mpv should not be thought of as an external tool. Instead think of IINA as a more extensive GUI for mpv that adheres to macOS conventions.

Given this, IINA should strive to be as compatible with mpv as possible. IINA should be very cautious about adding or extending functionality. If IINA adds something on top of mpv then IINA may in the future have to deal with the situation of mpv having added the functionality in a different way.

Back to the problem at hand…

The mpv project's thoughts on this are expressed in issue mpv-player/mpv#9652. They want you to use a script to remove playlist entries after the fact.

As we only want filtering when loading a directory and not in other cases, using a script is not an acceptable solution.

Going back to IINA issue #4434, the alternative fix suggested was for IINA to continue to load files itself and support the --mpv-shuffle option by sending the playlist-shuffle command to mpv once the directory has been loaded. I'm thinking that is the correct fix.

@svobs
Copy link
Contributor

svobs commented Aug 3, 2023

Going back to IINA issue #4434, the alternative fix suggested was for IINA to continue to load files itself and support the --mpv-shuffle option by sending the playlist-shuffle command to mpv once the directory has been loaded. I'm thinking that is the correct fix.

I'm fine with doing that. Small disclaimer: it will have a slightly different outcome than --shuffle for the case of multiple directory args. It may actually work "better" since it seems mpv's --shuffle will first shuffle the directories, then shuffle the inside of each directory, but will not intermix them (like separately shuffled decks of cards which are then stacked on top of one another), whereas --playlist-shuffle will mix them all together.

Since I was the author of the original PR I can take ownership of this one.

@low-batt
Copy link
Contributor

low-batt commented Aug 3, 2023

Thanks for taking this one. The difference in shuffle behavior sounds fine. We will want to make sure the first video in the playlist is shuffled as well.

@low-batt
Copy link
Contributor

@svobs Where did we end up on this one? The PR is still a draft.

@svobs
Copy link
Contributor

svobs commented Dec 30, 2023

@svobs Where did we end up on this one? The PR is still a draft.

See my comment in this random place.

@justinmayer
Copy link
Author

From the above-linked comment:

There is no "proper" fix for this yet because it's non-trivial and I never got around to putting in the time for it. I recommend just reverting PR 4439 as a short-term fix, because more users seem to be impacted by the regression than are helped by the original fix.

As one of those affected users (I am still stuck on v1.3.2 as a result), I agree that reverting PR 4439 would be the best short-term solution to this issue. 👍

@low-batt
Copy link
Contributor

low-batt commented Jan 2, 2024

This issue was one of the high priority issues that were supposed to be included in IINA 1.3.4. Unfortunately the 1.3.4 effort crashed into the end of 2023 deadline imposed upon IINA by Open Subtitles to switch to their REST API that connects to their new .com site. There are quite a few issues that were planned for 1.3.4 that did not get finished in time. How to deal with this is currently under discussion. This issue is one of the ones that must be addressed.

low-batt pushed a commit that referenced this issue Feb 3, 2024
…arg instead (fixes regression: #4548) (#4566)

* Handle folder filtering in IINA instead of sending to mpv, and intercept mpv shuffle arg & convert it to playlist-shuffle command. Fixes regression where folder filtering was broken, while still supporting shuffle via cmd-line

* Change on_load hook to use main thread, based on feedback

* Move shuffle playlist CLI logic from on_load hook to on_before_start_file hook

* Refactor shuffle arg handling: add hook only if needed, and add earlier check whether it is still needed. Add some comments, cleanup
@low-batt
Copy link
Contributor

low-batt commented Feb 3, 2024

The fix for this issue has been merged into the IINA develop branch. GitHub automatically closed the linked issue in reaction to the merge. I am reopening this issue until the fix is available in an official release of IINA so that users intending to report this problem can easily locate this existing report. Once the fix for this issue has been included in an official release this issue will be closed.

@uiryuu
Copy link
Member

uiryuu commented Jun 24, 2024

Fixed in 1.3.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment