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

Headcrabed · 2025-11-04T17:50:28Z

No description provided.

Headcrabed · 2025-11-04T17:51:03Z

@kasper93 Do you think this is ok?

llyyr · 2025-11-04T18:05:35Z

DOCS/man/options.rst

-    both source and target metadata is correct.
+    both source and target metadata is correct. If you are using macOS, Windows
+    with ACM enabled, or Wayland that compositor has color management protocol
+    support, metadata should be always correct.


What does color management have to do with source metadata?

Metadata is provided by compositor's color management API, isn't it?

This option requires that both source and target metadata is correct. The compositor has nothing to do with the source, it doesn't even know anything about the source file.

This option requires that both source and target metadata is correct. The compositor has nothing to do with the source, it doesn't even know anything about the source file.

Then my opinion is to change this part to “output metadata should be always correct”, is this ok?

I prefer saying "On color managed platforms, the output metadata will most likely be correct" instead of the current way of listing gpu-apis and platforms with their settings

I've updated description, what's your opinion?

Headcrabed · 2025-11-04T18:32:42Z

Also sdr-adjust-gamma=auto should be aware of icc-profile and icc-profile-auto, but I'm not sure how to do this. Any advice?

kasper93 · 2025-11-04T18:39:09Z

Also sdr-adjust-gamma=auto should be aware of icc-profile and icc-profile-auto, but I'm not sure how to do this. Any advice?

What do you mean? icc-profile takes precedence over any other target options.

Headcrabed · 2025-11-05T00:36:17Z

Also sdr-adjust-gamma=auto should be aware of icc-profile and icc-profile-auto, but I'm not sure how to do this. Any advice?

What do you mean? icc-profile takes precedence over any other target options.

So icc-profile will be correctly working when sdr-adjust-gamma=auto?

kasper93 · 2025-11-05T00:41:01Z

Also sdr-adjust-gamma=auto should be aware of icc-profile and icc-profile-auto, but I'm not sure how to do this. Any advice?

What do you mean? icc-profile takes precedence over any other target options.

So icc-profile will be correctly working when sdr-adjust-gamma=auto?

Yes. Is there evidence that would say otherwise?

Headcrabed · 2025-11-05T01:17:14Z

Also sdr-adjust-gamma=auto should be aware of icc-profile and icc-profile-auto, but I'm not sure how to do this. Any advice?

What do you mean? icc-profile takes precedence over any other target options.

So icc-profile will be correctly working when sdr-adjust-gamma=auto?

Yes. Is there evidence that would say otherwise?

When I'm testing under windows+ACM off and set icc-profile, setting sdr-adjust-gamma=auto would result in shift+i shows srgb, while sdr-adjust-gamma=yes would result in shift+i shows bt.1886(same as previous version)

video/out/vo_gpu_next.c

github-actions · 2025-11-05T08:49:51Z

Download the artifacts for this pull request:

Windows

macOS

llyyr · 2025-11-05T08:57:50Z

DOCS/man/options.rst

+
+    However, your render API should also support passing output metadata to MPV.
+    Setting ``--gpu-api`` to ``d3d11`` or ``vulkan`` (``opengl`` won't work)
+    would allow this.


Generally, it is recommended to enable this option, if you can ensure that both source and target metadata is correct. On color managed platforms, the output metadata will most likely be correct. Such platforms include macOS, Windows with ACM and Wayland compositors with color management protocol support.

The part about render API is not necessary or even correct because Vulkan doesn't tell us anything about the target metadata. On Windows, we get it via D3D11 and on Wayland we get it via the protocol. This information is completely agnostic of the rendering API used, opengl just won't do any hinting unless we use the wayland protocol for it.

Emmm you don't treat VK_COLOR_SPACE as metadata source?

I changed the description here again, do you think this is ok?

kasper93 · 2025-11-05T09:46:26Z

Also sdr-adjust-gamma=auto should be aware of icc-profile and icc-profile-auto, but I'm not sure how to do this. Any advice?

What do you mean? icc-profile takes precedence over any other target options.

So icc-profile will be correctly working when sdr-adjust-gamma=auto?

Yes. Is there evidence that would say otherwise?

When I'm testing under windows+ACM off and set icc-profile, setting sdr-adjust-gamma=auto would result in shift+i shows srgb, while sdr-adjust-gamma=yes would result in shift+i shows bt.1886(same as previous version)

It doesn't matter. named transfer function is not used when icc profile is applied or any other lut.

Add some description about output metadata. Signed-off-by: Shengyu Qu <[email protected]>

DOCS/man/options.rst

Headcrabed · 2025-11-09T18:08:32Z

I added another commit to partially revert #17008
Since macOS is doing things correctly by default: https://projects.blender.org/blender/blender/issues/145022

Headcrabed · 2025-11-10T04:09:59Z

I added another commit to partially revert #17008 Since macOS is doing things correctly by default: https://projects.blender.org/blender/blender/issues/145022

Forget about this. My mind wasn't clear. This extra commit is not needed, I've reverted it.

Headcrabed · 2025-11-17T05:19:55Z

@kasper93 anything is blocking it from getting merged?

DOCS/man/options.rst

kasper93 · 2025-11-17T19:18:26Z

video/out/vo_gpu_next.c

        case PL_COLOR_TRC_GAMMA22:
        case PL_COLOR_TRC_SRGB:
+#ifdef __APPLE__
+            if (opts->sdr_adjust_gamma == 0)


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

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

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 se it. we have quite a few cases (in the past) where the default are not optimal for every platform.

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.

…no" on macOS On macOS, global color management is always enabled, so there's no need to make "auto" behave like "no". Signed-off-by: Shengyu Qu <[email protected]>

llyyr reviewed Nov 4, 2025

View reviewed changes

Headcrabed force-pushed the description_improvement branch from 6b1a431 to bdbb1d5 Compare November 4, 2025 18:07

Headcrabed force-pushed the description_improvement branch from bdbb1d5 to fa4801a Compare November 5, 2025 03:32

llyyr reviewed Nov 5, 2025

View reviewed changes

video/out/vo_gpu_next.c Show resolved Hide resolved

Headcrabed force-pushed the description_improvement branch from fa4801a to 6c4c803 Compare November 5, 2025 08:29

llyyr reviewed Nov 5, 2025

View reviewed changes

Headcrabed force-pushed the description_improvement branch from 6c4c803 to d202cb3 Compare November 7, 2025 03:20

DOCS/man/options: add some description for sdr-adjust-gamma

5a6863d

Add some description about output metadata. Signed-off-by: Shengyu Qu <[email protected]>

Headcrabed force-pushed the description_improvement branch from d202cb3 to f24dec3 Compare November 7, 2025 03:39

atrxia1 reviewed Nov 8, 2025

View reviewed changes

DOCS/man/options.rst Show resolved Hide resolved

DOCS/man/options.rst Show resolved Hide resolved

Headcrabed force-pushed the description_improvement branch from f24dec3 to ee1184b Compare November 9, 2025 17:50

Headcrabed force-pushed the description_improvement branch from 8a36849 to ee1184b Compare November 10, 2025 04:10

Jules-A reviewed Nov 17, 2025

View reviewed changes

DOCS/man/options.rst Outdated Show resolved Hide resolved

kasper93 reviewed Nov 17, 2025

View reviewed changes

video/out/vo_gpu_next: don't make sdr-adjust-gamma=auto behave like "…

77476a9

…no" on macOS On macOS, global color management is always enabled, so there's no need to make "auto" behave like "no". Signed-off-by: Shengyu Qu <[email protected]>

Headcrabed force-pushed the description_improvement branch from ee1184b to 77476a9 Compare November 18, 2025 02:37

	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);
	};

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

Headcrabed commented Nov 4, 2025

Uh oh!

Headcrabed commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Headcrabed commented Nov 4, 2025

Uh oh!

kasper93 commented Nov 4, 2025

Uh oh!

Headcrabed commented Nov 5, 2025

Uh oh!

kasper93 commented Nov 5, 2025

Uh oh!

Headcrabed commented Nov 5, 2025

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasper93 commented Nov 5, 2025

Uh oh!

Uh oh!

Uh oh!

Headcrabed commented Nov 9, 2025

Uh oh!

Headcrabed commented Nov 10, 2025

Uh oh!

Headcrabed commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kasper93 Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Nov 5, 2025 •

edited

Loading

Headcrabed commented Nov 17, 2025 •

edited

Loading

kasper93 Nov 17, 2025 •

edited

Loading