search

mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
26.65k stars 2.83k forks source link

auto_profiles.lua: add a way to apply profiles only once #14436

Open Dudemanguy opened 4 days ago

Dudemanguy commented 4 days ago

This allows users to specify the maximum amount of times they want a profile to be applied. Fixes #14426.

Not really sure about the name tbh.

github-actions[bot] commented 4 days ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1636524131.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1636529780.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1636563637.zip)
macOS * [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1636520543.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1636522794.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1636518353.zip)
hooke007 commented 4 days ago

Emm... I think the issue's problem is different. The window behav is changed recently (I guess). It doesn't happen for me with the previous version of mpv.

hooke007 commented 4 days ago

To make it clear, set autofit at runtime will change the position of window. Which is not expected by @escapezn

guidocella commented 4 days ago

once or oneshot would be much better names unless you can think a use case for running profiles n times. mp.add_periodic_timer has a oneshot argument, and addEventListener() in Javascript has a once argument.

Emm... I think the issue's problem is different. The window behav is changed recently (I guess). It doesn't happen for me with the previous version of mpv.

4370dc0cb6 fixed runtime updates on Windows.

na-na-hi commented 3 days ago

4370dc0 fixed runtime updates on Windows.

True, but setting it to the same value without changing it is only applied after #13566. The issue can be fixed by requiring the value to be changed if it's applied in a profile.

kasper93 commented 3 days ago

True, but setting it to the same value without changing it is only applied after https://github.com/mpv-player/mpv/pull/13566.

So we are adding workarounds to workarounds. Bit annoying.

To me it is not profile application issue. Profiles system is designed to apply value always and it is property job to act in a sane way.

Also this makes it so you have to duplicate profiles, one with count and the other without the count.

I don't see what is the best approach here, the force_update flag is inherently not compatible with profiles. I don't think count fixes it, because what if I have playlist with audio and video. audio -> audio switch shouldn't apply the property video -> audio switch should apply the property cannot fix that with count.

once or oneshot

I had exact same idea when reading. I don't see how would anything more than 1 make sense for apply count in practice.

Dudemanguy commented 3 days ago

I didn't see any reason why anyone would would anything other than 1 or inf either, but at the same time I didn't want to artificially limit it. But we can change it to just once if you really want.

what if I have playlist with audio and video.

If your profile condition doesn't match the audio case but matches the video case. It should work.

guidocella commented 3 days ago

I didn't see any reason why anyone would would anything other than 1 or inf either, but at the same time I didn't want to artificially limit it. But we can change it to just once if you really want.

I don't think conditional profiles need to cover such edge cases as applying profiles n times, you should use scripts for that.

what if I have playlist with audio and video.

If your profile condition doesn't match the audio case but matches the video case. It should work.

I think he meant if you keep switching between video and audio files. I don't see a good solution either.

Dudemanguy commented 3 days ago

I think he meant if you keep switching between video and audio files.

Do you mean that the once setting should be "refreshed" if switching to an audio file? That could just be handled similar to profile-restore.

guidocella commented 3 days ago

Ideally it would reset when switching to a video file.

Dudemanguy commented 3 days ago

I changed it to profile-once instead as requested. Independent of the problems with switching tracks, etc. I still think this is a reasonable feature.

kasper93 commented 3 days ago

I still think this is a reasonable feature.

Could you give example how it can be used?

I don't see how once (or N) profile application can be useful. It is not obvious to track when the profile is applied, depending if this is playlist, live stream, ytdl, localfile, things will work differently. I feel like state machines like that to trigger some behavior only once or multiple times is better to be as a lua script. But I may be missing something.

Dudemanguy commented 3 days ago

It seems completely reasonable to me that you may only want a profile applied once. It's not hard to imagine that someone maybe have some playlist and want to only trigger whatever condition to apply fullscreen once. If the user changes this on his/her own later, they may not want the fullscreen to happen again if they go back in the playlist or something. The behavior in the linked issue is due to an implementation detail of how that option works (which is working exactly as intended), but clearly they only actually want the profile to apply once since they are moving the window on their own and such later.

Additionally, adding some way for the autofit options to work but without moving the window might be valuable as well.

kasper93 commented 3 days ago

I think it would be better as an example lua script. IMHO it doesn't fit profile use-case, but if others agree it is fine to merge.

guidocella commented 3 days ago

This feels like a stopgap solution that fixes the issue if you only have audio files in the playlist, but the general solution would either for properties like track-list and path to instantly switch from the values in the previous file to the values in the next file, or not to evalute conditional profiles while they're unselected or unavailable, but I don't know if that's feasible. I don't think the current behavior of unapplying and reapplying profiles on each file switch is ever intuitive or desirable.

kasper93 commented 3 days ago

I don't think the current behavior of unapplying and reapplying profiles on each file switch is ever intuitive or desirable.

It doesn't have to be. It is consistent at the end. Profiles are designed to be enabled for current state of the player. And it works well.

That's why I don't like adding additional state (once flag) to it, it just doesn't fit in there. Or we have to redesign when the profiles are applied.

guidocella commented 3 days ago

How do behaviors not have to be desirable?

Here's another case:

[image]
profile-cond=p['current-tracks/video/image']
profile-restore=copy
linear-downscaling=no # don't make black and white manga brighter

Because tracks become unselected while switching files, linear-downscaling is reenabled and disabled on every image switch, and you therefore see manga pages linearly downscaled (i.e. brighter) for an instant and then being re-rendered. This isn't fixed by profile-once because I want to be able to switch between images and videos in the same playlist.

hooke007 commented 2 days ago

Well, I found a niche use case. https://github.com/mpv-player/mpv/issues/9759#issuecomment-1020441726

guidocella commented 2 days ago

I also thought of load-script in profile-cond but I don't think we should encourage it vs editing the script, as scripts will stay loaded after switching from network to local files.