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

[RFC] vo_gpu: remove deprecated options --gamma-factor and --gamma-auto #14369

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Akemi
Copy link
Member

@Akemi Akemi commented Jun 16, 2024

--gamma-factor and --gamma-auto were deprecated in 0.35 (ac39661 and 2207236). the latter was only used on macOS.
the equivalent client API functionality wasn't officially deprecated.

what i would like to discuss:
do we want to remove those options already?
if yes, how do we want to remove them? since the client API equivalent wasn't deprecated, removal of the functionality can't be done immediately?
do we want to deprecate the client API functionality first in the next release and remove the functionality in one of the releases after the next one?
the client API deprecation change should probably be taken separately?

hope i found all the dependent code and removed it properly.

@@ -40,7 +40,7 @@ enum {
VO_EVENT_ICC_PROFILE_CHANGED = 1 << 2,
// Some other window state changed (position, window state, fps)
VO_EVENT_WIN_STATE = 1 << 3,
// The ambient light conditions changed and need to be reloaded
// Deprecated. The ambient light conditions changed and need to be reloaded
VO_EVENT_AMBIENT_LIGHTING_CHANGED = 1 << 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed too, no? It is not public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i wasn't sure how to handle this since we would get a 'hole' in that enum or would we want to renumber it?

Copy link
Contributor

Choose a reason for hiding this comment

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

hole is fine, I think, to keep it simple. There are plenty of bits left for new values.

Copy link
Member

Choose a reason for hiding this comment

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

If something new is added later, we can always just fill the hole in too.

Copy link

Download the artifacts for this pull request:

Windows
macOS

@ruihe774
Copy link
Contributor

FWIW I wonder what is the alternative of --gamma-factor? I am using --gamma-factor=1.2239 on macOS to match the color with QuickTime Player.

@hooke007
Copy link
Contributor

hooke007 commented Jun 16, 2024

what is the alternative

#10699 (comment) #11153
But it doesn't give the real answer.

@na-na-hi
Copy link
Contributor

Note that after deprecating --gamma-factor, there is no longer a meaningful way to set gamma anymore. The value of --gamma is essentially a meaningless implementation detail:

params->gamma = exp(log(8.0) * eq->values[MP_CSP_EQ_GAMMA] / 100.0);

I suggest to undeprecate --gamma-factor. Even though it was implemented as a part of something which no longer works, the option still has valid uses.

@Dudemanguy
Copy link
Member

I'm not sure --gamma-auto has any use but I agree with @na-na-hi that --gamma-factor is worth keeping. It's just a simple multiplier anyway so it's not like there's great code complexity.

@Akemi
Copy link
Member Author

Akemi commented Jun 20, 2024

why was this option deprecated in the first place?

@hooke007
Copy link
Contributor

https://github.com/mpv-player/mpv/wiki/GPU-Next-vs-GPU

--gamma-factor and --gamma-auto: considered deprecated by design, the new BPC logic handles this much better

Not sure if it was related.

@Akemi
Copy link
Member Author

Akemi commented Jun 20, 2024

from the commits and the PR i could only gather "outdated information" and "--gamma-auto not working". though both aren't really a reason to deprecate the options. for the former update the info, for the latter fix it or open an issue (without knowing what exactly is broken, hard to judge).

seems like the initial use case for --gamma-factor is gone, though it still has a more general use case of being the only way to adjust gamma at all (besides video filters via ffmpeg i guess?).

then i am also of the opinion to undeprecate both options.

@ruihe774
Copy link
Contributor

https://github.com/mpv-player/mpv/wiki/GPU-Next-vs-GPU

--gamma-factor and --gamma-auto: considered deprecated by design, the new BPC logic handles this much better

Not sure if it was related.

I wonder what is "BPC"? I cannot find the word in any other places.

@Akemi
Copy link
Member Author

Akemi commented Jun 20, 2024

black point compensation, is my guess. at least that's what i see when working with colour profiles and related.

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

6 participants