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

vo_gpu{,_next}: convert scale options to type choice #14221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guidocella
Copy link
Contributor

This allows Tab completing them in the console and zsh, and using cycle scale.

jinc is also added to --tscale=help's output, while before it was missing because validate_scaler_opt() skipped filter windows with the same name as a filter kernel, but the jinc kernel was skipped with --tscale because it is polar.

Copy link

github-actions bot commented May 24, 2024

Download the artifacts for this pull request:

Windows
macOS

video/out/gpu/video.c Outdated Show resolved Hide resolved
video/out/gpu/video.c Outdated Show resolved Hide resolved
@kasper93
Copy link
Contributor

Since we require libplacebo anyway, maybe it is good time to unify filter definitions, instead of adding another layer of paint on top of it.

libplacebo exports things like pl_scale_filters and all filters as structures. Sure, this would be breaking changes for some inconsistencies between vo_gpu, but the long-term plan is to replace it anyway, so we might do it bit by bit.

@guidocella guidocella force-pushed the scale-choice branch 2 times, most recently from 6bdc500 to 6b05499 Compare May 24, 2024 15:33
video/out/gpu/video.c Show resolved Hide resolved
video/out/gpu/video.c Show resolved Hide resolved
@guidocella
Copy link
Contributor Author

Since we require libplacebo anyway, maybe it is good time to unify filter definitions, instead of adding another layer of paint on top of it.

libplacebo exports things like pl_scale_filters and all filters as structures. Sure, this would be breaking changes for some inconsistencies between vo_gpu, but the long-term plan is to replace it anyway, so we might do it bit by bit.

Doesn't vo gpu still needs its filter functions and parameters in video/out/filter_kernels.c?

@kasper93
Copy link
Contributor

Doesn't vo gpu still needs its filter functions and parameters in video/out/filter_kernels.c?

You can get all that from pl_filter_config

@na-na-hi
Copy link
Contributor

na-na-hi commented May 24, 2024

Since we require libplacebo anyway, maybe it is good time to unify filter definitions, instead of adding another layer of paint on top of it.

Can this be done in a separate PR instead? There is no need to expand the scope of this. It's mostly semantic changes and some code simplification.

This allows Tab completing them in the console and zsh, and using cycle
scale.

jinc is also added to --tscale=help's output, while before it was
missing because validate_scaler_opt() skipped filter windows with the
same name as a filter kernel, but the jinc kernel was skipped with
--tscale because it is polar.
@kasper93
Copy link
Contributor

Can this be done in a separate PR instead? There is no need to expand the scope of this. It's mostly semantic changes and some code simplification.

What is the scope of this PR though? All I see is another enum with filters and mapping tables. All this is far from simplification. Simplification would be to remove all this code from mpv. Like I said it is painting over something that should just be replaced.

@na-na-hi
Copy link
Contributor

What is the scope of this PR though?

See commit message.

All I see is another enum with filters and mapping tables. All this is far from simplification.

Removing several special NULL handling and the validate functions and replace them with static mapping tables decreases code complexity. It's more uniform and less error prone.

Simplification would be to remove all this code from mpv. Like I said it is painting over something that should just be replaced.

If @guidocella doesn't want to do this in this PR and you don't want to merge this PR at the current state, then please open another PR which removes the code, instead of stalling valid UX improvements. Once that is resolved we can go back to this one.

@guidocella
Copy link
Contributor Author

This PR is just meant to provide Tab completion. I'm not sure it would be worth it to update all of vo gpu's code to use pl_filter_config when the plan is to remove it later.

@kasper93
Copy link
Contributor

If @guidocella doesn't want to do this in this PR and you don't want to merge this PR at the current state, then please open another PR which removes the code, instead of stalling valid UX improvements.

I'm sorry, but who are you to tell me what should I do? Also, I'm the last person who is stalling anything here.

@na-na-hi
Copy link
Contributor

So is this PR ready to merge or not? If not, someone needs to do something about it. Maybe I was wrong to assume that no one other than you is interested to do that, but if you aren't and no one else is, then this PR has no future and it's better to be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants