iina icon indicating copy to clipboard operation
iina copied to clipboard

Opening folder spawns playlist that includes non-media files

Open justinmayer opened this issue 1 year ago • 11 comments

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.

justinmayer avatar Jul 31 '23 14:07 justinmayer

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.

svobs avatar Jul 31 '23 23:07 svobs

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! 🌟

justinmayer avatar Aug 01 '23 10:08 justinmayer

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?

svobs avatar Aug 02 '23 04:08 svobs

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 https://github.com/mpv-player/mpv/issues/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.

low-batt avatar Aug 02 '23 21:08 low-batt

Going back to IINA issue https://github.com/iina/iina/issues/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.

svobs avatar Aug 03 '23 21:08 svobs

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 avatar Aug 03 '23 23:08 low-batt

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

low-batt avatar Dec 30 '23 00:12 low-batt

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

See my comment in this random place.

svobs avatar Dec 30 '23 00:12 svobs

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. 👍

justinmayer avatar Jan 02 '24 10:01 justinmayer

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 avatar Jan 02 '24 20:01 low-batt

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.

low-batt avatar Feb 03 '24 02:02 low-batt

Fixed in 1.3.5

uiryuu avatar Jun 24 '24 13:06 uiryuu