Skip to content

Description improvement and some behavior change for sdr-adjust-gamma #17007

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+15 −2
Open

Description improvement and some behavior change for sdr-adjust-gamma #17007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a6863d
DOCS/man/options: add some description for sdr-adjust-gamma
Headcrabed Nov 7, 2025
77476a9
video/out/vo_gpu_next: don't make sdr-adjust-gamma=auto behave like "…
Headcrabed Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions DOCS/man/options.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7377,10 +7377,19 @@ them.
``yes``, it will be converted based on the available metadata.

``auto`` (default) behaves like ``no``, except when ``--target-trc`` is
explicitly set, in which case it behaves like ``yes``.
explicitly set, in which case it behaves like ``yes``. On macOS it
should always behave like ``yes``.

Generally it's recommended to enable this option, if you can ensure that
both source and target metadata is correct.
both source and target metadata is correct. Besides, on color managed
platforms, the output metadata will most likely to be correct. For example,
macOS, Windows with ACM enabled, or Wayland that compositor has color
management protocol support are all globally color managed.

However, your render API should also support passing output metadata to MPV,
or able to let MPV pick color space description for output surface. Setting
``--gpu-api`` to ``d3d11`` or ``vulkan`` (``opengl`` won't work) would allow
this.

(Only for ``--vo=gpu-next``)

Expand Down
4 changes: 4 additions & 0 deletions video/out/vo_gpu_next.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,10 @@ static bool draw_frame(struct vo *vo, struct vo_frame *frame)
case PL_COLOR_TRC_BT_1886:
case PL_COLOR_TRC_GAMMA22:
case PL_COLOR_TRC_SRGB:
#ifdef __APPLE__
if (opts->sdr_adjust_gamma == 0)
Copy link
Member

@kasper93 kasper93 Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we don't do that. the option is should be respected. You could set default value to yes for macos, but I'm not sure it's correct.

/cc @Akemi

Copy link
Contributor Author

@Headcrabed Headcrabed Nov 18, 2025

Choose a reason for hiding this comment

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

Using different default values on different OSes looks weird... I didn't see any existing options doing like this, is there any?

Copy link
Member

@Akemi Akemi Nov 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

historical we never set different default values for different platforms, or at least not via a conditional preprocessor or similar. not sure if we have such cases now?

if we needed a different behaviour or default we usual let the context/backend/platform specific code decide such things, for example with a callback or similar (like these

mpv/video/out/gpu/context.h

Lines 73 to 92 in 7482608

struct ra_ctx_params {
// For special contexts (i.e. wayland) that want to check visibility
// before drawing a frame.
bool (*check_visible)(struct ra_ctx *ctx);
// See ra_swapchain_fns.color_depth.
int (*color_depth)(struct ra_ctx *ctx);
// Preferred device color space. Optional.
pl_color_space_t (*preferred_csp)(struct ra_ctx *ctx);
// See ra_swapchain_fns.get_vsync.
void (*get_vsync)(struct ra_ctx *ctx, struct vo_vsync_info *info);
// Set to the platform-specific function to swap buffers, like
// glXSwapBuffers, eglSwapBuffers etc. This will be called by
// ra_gl_ctx_swap_buffers. Required unless you either never call that
// function or if you override it yourself.
void (*swap_buffers)(struct ra_ctx *ctx);
};
).

i am also not sure of the necessity of this change, but admittedly this might be a bit over my head atm. it would probably be a good idea to properly describe the problem, how this fixes the problem and what the difference are (before/after).

maybe in the end it would just be okay to document that setting it to yes is preferred on macOS and leave it up to the user to set it. we have quite a few cases (in the past) where the defaults are not optimal for every platform.

Copy link
Member

@kasper93 kasper93 Nov 18, 2025

Choose a reason for hiding this comment

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

i am also not sure of the necessity of this change

I also don't see why it's needed and how does this differ from other platforms.

break;
#endif
target.color.transfer = frame->current->params.color.transfer;
}
}
Expand Down
Loading