Skip to content

Fix cannot apply filters to multiple windows, #4273 #4288

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix cannot apply filters to multiple windows, #4273 #4288

wants to merge 2 commits into from

Conversation

low-batt
Copy link
Contributor

This commit will:

  • Change FilterWindowController to use the active PlayerCore

  • Change FilterWindowController.reloadTable to check if the active PlayerCore is shutting down

  • Disable the ability to add a filter if the active core is stopping

  • Change PlayerCore.playbackStopped to clear all audio and video filters

  • I have read CONTRIBUTING.md

  • This implements/fixes issue Cannot apply video filters to multiple windows #4273.

Description:

@low-batt
Fix cannot apply filters to multiple windows, #4273
f98921c 
This commit will:
- Change FilterWindowController to use the active PlayerCore
- Change FilterWindowController.reloadTable to check if the active
  PlayerCore is shutting down
- Disable the ability to add a filter if the active core is stopping
- Change PlayerCore.playbackStopped to clear all audio and video filters
@low-batt low-batt linked an issue Mar 23, 2023 that may be closed by this pull request
Cannot apply video filters to multiple windows #4273
Closed
@low-batt
Copy link
Contributor Author

low-batt commented Mar 23, 2023 •
edited
Loading

NOTE the change to clear filters in PlayerCore.playbackStopped raises the question as to what users expect and what other setting IINA is failing to reset when reusing a PlayerCore object along with its mpv core. See issue #4287 for an example of random behavior due to sometimes inheriting previous settings when reusing a mpv core.

@low-batt
Fix cannot apply filters to multiple windows, #4273
cf60895 
This commit will:
- Change FilterWindowController to use the active PlayerCore
- Change FilterWindowController.reloadTable to check if the active
  PlayerCore is shutting down
- Disable the ability to add a filter if the active core is stopping
  if IINA is configured to support multiple windows
- Change PlayerCore.playbackStopped to clear all audio and video filters
  if IINA is configured to support multiple windows
@low-batt
Copy link
Contributor Author

The mpv player carries over many settings when playing the next video. This behavior is useful when playing a series of videos such as the episodes of a show. However IINA differs from mpv in that if the preference Always open media in a new window is enabled then there can be multiple videos open and playable at the same time each with their own mpv core. Each mpv core has its own set of settings. Once there is more than one player/mpv core in the cache will appear random to the user as to which settings are applied when another video is opened. This means we want to preserve the mpv behavior of applying settings to the next video when Always open media in a new window is not enabled. However if this preference is enabled then we want to clear certain settings to avoid random behavior. The latest commit takes this attitude and clears audio and video filters if Always open media in a new window is enabled.

Trouble is Always open media in a new window is enabled by default. Users used to settings carrying over when IINA is just playing a playlist in a single window would not like this. I'm thinking maybe we are going to have to add a new preference.

Thoughts?

I can pull the change to clear filters for now and we can discuss how to handle the random behavior when there are multiple mpv cores in the next release.

uiryuu
uiryuu requested changes Mar 31, 2023
View reviewed changes
Copy link
Member

@uiryuu uiryuu left a comment

Choose a reason for hiding this comment

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

You are assuming that if "Always open media in a new window" is disabled, then there must be one player core at a time, which is not true. One can open multiple window, and then uncheck that option. So logically I think we shouldn't implement the behavior based on that preference.

iina/FilterWindowController.swift
// being stopped and about to be added to the player core cache. With that preference enabled
// there can be multiple player/mpv cores cached which would result in random behavior as to
// what filters would be applied to the next video.
addButton.isEnabled = !Preference.bool(for: .alwaysOpenInNewWindow)
Copy link
Member

Choose a reason for hiding this comment

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

Please add parentheses for better readability.

@svobs
Copy link
Contributor

svobs commented Mar 31, 2023

Examined this one a bit more, and I think this isn't quite a good fit for Always open media in a new window. With this existing proposal, it's still possible to open a new window in other ways, in which case the filters won't be cleared. And the description of that checkbox doesn't suggest any special behavior besides just opening a new window.

A better solution would be to always clear the filters for the mpv core when the window state goes from closed to opened. From the description in the issue, the user expected to have to re-enable the filter for every window already.

Trouble is Always open media in a new window is enabled by default. Users used to settings carrying over when IINA is just playing a playlist in a single window would not like this.

If the user is switching between files in a playlist, then Always open media in a new window doesn't apply - right? The same window is used. So it should always reuse the same mpv core, and it makes sense to retain the filter settings in this case. But you won't have to add extra logic for that if you only clear filters for windows which are changing state from "closed" -> "opened".

@low-batt
Copy link
Contributor Author

Yes, I regretted pushing that proposed change after making it. That change is going into the toilet due to the problems you pointed out as well as concerns from @uiryuu.

I will pull the changes for clearing filters. We will take up the issue of mpv settings when there are multiple cores in the feature release.

By the way while testing this I think I found another error. I need to reproduce it again and dig into it so I've not filed an issue yet. Retained filters can fail when you open a new video. From the UI you don't know this has happened. I noticed this in mpv log messages.

@low-batt low-batt mentioned this pull request Apr 1, 2023
Merged
2 tasks
@low-batt
Copy link
Contributor Author

low-batt commented Apr 1, 2023

This PR has been replaced by PR #4316 which does not attempt to deal with the issue of filters remaining configured in the mpv core and being applied to the next video.

@low-batt low-batt closed this Apr 1, 2023
@low-batt low-batt removed a link to an issue Apr 1, 2023
Cannot apply video filters to multiple windows #4273
Closed
@uiryuu uiryuu deleted the fix-4273 branch August 17, 2024 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uiryuu uiryuu uiryuu requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@low-batt @svobs @uiryuu