Skip to content

vo_gpu_next: set color directly using Wayland protocol #17345

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
kasper93 wants to merge 9 commits into
base: master
Choose a base branch
from
+140 −44
Open

vo_gpu_next: set color directly using Wayland protocol #17345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
170c5a0
wayland_common: only bind color surface when actually used
kasper93 Feb 28, 2026
eb1d839
vo_gpu_next: set color directly using Wayland protocol
kasper93 Feb 28, 2026
c0f21a5
wayland_common: set luminances
kasper93 Jan 28, 2026
d61b80e
wayland_common: scale preferred luminances
mahkoh Jan 28, 2026
463c7c9
vo_wlshm: use image params after conversion
kasper93 Mar 3, 2026
cf06281
video/out/opengl/context_wayland: export preferred colorspace
kasper93 Mar 3, 2026
7fa9c8e
vo_dmabuf_wayland: use color parameters prepared in reconfig
kasper93 Mar 3, 2026
71910a7
vo_dmabuf_wayland: add BT.2100 mapping
kasper93 Mar 3, 2026
d40f956
vo_dmabuf_wayland: don't crash on not supported color representation
kasper93 Mar 3, 2026
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
9 changes: 9 additions & 0 deletions video/out/gpu/d3d11_helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,15 @@ struct pl_color_space mp_dxgi_desc_to_color_space(const DXGI_OUTPUT_DESC1 *desc)
break;
}

if (!pl_color_transfer_is_hdr(ret.transfer)) {
// Don't use reported display peak in SDR mode, setting target peak in
// SDR mode is very specific usecase, needs proper calibration, users
// can set it manually.
ret.hdr.max_luma = 0;
ret.hdr.max_cll = 0;
ret.hdr.max_fall = 0;
}

return ret;
}

Expand Down
6 changes: 6 additions & 0 deletions video/out/opengl/context_wayland.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ static bool wayland_egl_check_visible(struct ra_ctx *ctx)
return vo_wayland_check_visible(ctx->vo);
}

static pl_color_space_t wayland_egl_preferred_csp(struct ra_ctx *ctx)
{
return vo_wayland_preferred_csp(ctx->vo);
}

static bool wayland_egl_set_color(struct ra_ctx *ctx, struct mp_image_params *params)
{
vo_wayland_handle_color(ctx->vo->wl, params);
Expand Down Expand Up @@ -112,6 +117,7 @@ static bool egl_create_context(struct ra_ctx *ctx)

struct ra_ctx_params params = {
.check_visible = wayland_egl_check_visible,
.preferred_csp = wayland_egl_preferred_csp,
.set_color = wayland_egl_set_color,
.swap_buffers = wayland_egl_swap_buffers,
.get_vsync = wayland_egl_get_vsync,
Expand Down
2 changes: 1 addition & 1 deletion video/out/vo_dmabuf_wayland.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ static bool draw_frame(struct vo *vo, struct vo_frame *frame)
pts = frame->current ? frame->current->pts : 0;
if (frame->current) {
buf = buffer_get(vo, frame);
vo_wayland_handle_color(wl, &frame->current->params);
vo_wayland_handle_color(wl, &p->target_params);

if (buf && buf->frame) {
struct mp_image *image = buf->frame->current;
Expand Down
22 changes: 4 additions & 18 deletions video/out/vo_gpu_next.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,10 +961,11 @@ static bool set_colorspace_hint(struct priv *p, struct pl_color_space *hint)
},
};

if (sw->fns->set_color && sw->fns->set_color(sw, &params)) {
if (hint)
if (sw->fns->set_color && sw->fns->set_color(sw, hint ? &params : NULL)) {
if (hint) {
*hint = params.color;
return true;
return true;
}
}
pl_swapchain_colorspace_hint(p->sw, hint);
return false;
Expand Down Expand Up @@ -1118,13 +1119,6 @@ static bool draw_frame(struct vo *vo, struct vo_frame *frame)
// limit min_luma to 1000:1 contrast ratio in SDR mode
if (target_csp.hdr.min_luma > PL_COLOR_SDR_WHITE / PL_COLOR_SDR_CONTRAST)
target_csp.hdr.min_luma = 0;
// Don't use reported display peak in SDR mode. Mostly because libplacebo
// forcefully switches to PQ if hinting hdr metadata, ignoring the transfer
// set in the hint. But also because setting target peak in SDR mode is
// very specific usecase, needs proper calibration, users can set it manually.
target_csp.hdr.max_luma = 0;
target_csp.hdr.max_cll = 0;
target_csp.hdr.max_fall = 0;
}
// maxFALL in display metadata is in fact MaxFullFrameLuminance. Wayland
// reports it as maxFALL directly, but this doesn't mean the same thing.
Expand Down Expand Up @@ -1156,14 +1150,6 @@ static bool draw_frame(struct vo *vo, struct vo_frame *frame)
pl_color_space_merge(&hint, source);
if (target_unknown && !opts->target_trc && !pl_color_transfer_is_hdr(source->transfer))
hint = *source;
// Vulkan doesn't have support for gamma 2.2 transfer function,
// so even though requested preferred color space is gamma 2.2, we
// fallback to sRGB. sRGB itself is ambiguous, but at least we have
// options to control the behavior.
// TODO: Revise this after fix for linear transfers lands in libplacebo.
// <https://code.videolan.org/videolan/libplacebo/-/merge_requests/759>
if (hint.transfer == PL_COLOR_TRC_GAMMA22)
hint.transfer = PL_COLOR_TRC_SRGB;
// Restore target luminance if it was present, note that we check
// max_luma only, this make sure that max_cll/max_fall is not take
// from source.
Expand Down
2 changes: 1 addition & 1 deletion video/out/vo_wlshm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ static bool draw_frame(struct vo *vo, struct vo_frame *frame)
}
}
if (src) {
vo_wayland_handle_color(wl, &src->params);
vo_wayland_handle_color(wl, &p->sws->dst);
struct mp_image dst = buf->mpi;
struct mp_rect src_rc;
struct mp_rect dst_rc;
Expand Down
27 changes: 21 additions & 6 deletions video/out/vulkan/context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,13 +528,28 @@ static void get_vsync(struct ra_swapchain *sw,

static bool set_color(struct ra_swapchain *sw, struct mp_image_params *params)
{
// Needs VK_COLOR_SPACE_PASS_THROUGH_EXT for Wayland. This is currently the
// only backend that supports set_color, and likely will stay that way.
// Everything else can use Vulkan API colorspace without issues.
bool supported = strcmp(sw->ctx->fns->name, "waylandvk") != 0;
struct priv *p = sw->priv;
if (supported && p->params.set_color)
return p->params.set_color(sw->ctx, params);

// Vulkan Wayland needs special handling to avoid duplicated color surface.
bool waylandvk = strcmp(sw->ctx->fns->name, "waylandvk") == 0;
// Assume anything else is supported when set_color is defined. However,
// everything else can use Vulkan API colorspace without issues, so unlikely
// that set_color is used outside of Wayland.
bool supported = PL_API_VER >= 361 || !waylandvk;
if (supported && p->params.set_color) {
if (waylandvk && params) {
// Request VK_COLOR_SPACE_PASS_THROUGH_EXT
pl_swapchain_colorspace_hint(p->vk->swapchain, &(struct pl_color_space){0});
// Do the resize in case surface format needs to change.
pl_swapchain_resize(p->vk->swapchain, &(int){0}, &(int){0});
}
bool ret = p->params.set_color(sw->ctx, params);
// To avoid ping-pong between VK_COLOR_SPACE_PASS_THROUGH_EXT and others,
// we assume that internal Wayland set_color always work, and update
// params to fallback values if needed.
mp_assert(ret || !waylandvk);
return ret;
}
// Technically we could call pl_swapchain_colorspace_hint() here directly,
// but we want to know when parameters are set "externally".
return false;
Expand Down
115 changes: 97 additions & 18 deletions video/out/wayland_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2029,6 +2029,10 @@ static void supported_feature(void *data, struct wp_color_manager_v1 *color_mana
MP_VERBOSE(wl, "Compositor supports setting mastering display primaries.\n");
wl->supports_display_primaries = true;
break;
case WP_COLOR_MANAGER_V1_FEATURE_SET_LUMINANCES:
MP_VERBOSE(wl, "Compositor supports setting primary color luminances.\n");
wl->supports_set_luminances = true;
break;
}
}

Expand Down Expand Up @@ -2144,20 +2148,31 @@ static void info_done(void *data, struct wp_image_description_info_v1 *image_des
if (!wd->icc_file) {
MP_VERBOSE(wl, "Preferred surface feedback received:\n");
log_color_space(wl->log, wd);
// We don't support extended ranges output where luminance exceeds
// maximum nominal luminance range (1.0), so switch to PQ.
if (fabsf(wd->csp.hdr.max_luma / wd->ref_luma - 1.0f) > 1e-4f &&
!pl_color_transfer_is_hdr(wd->csp.transfer)) {
MP_VERBOSE(wl, "Setting preferred transfer to PQ for HDR output.\n");
wd->csp.transfer = PL_COLOR_TRC_PQ;
}
// Wayland luminances are always in reference to the reference luminance. That is,
// if max_luma == 2*ref_luma, then there is 2x headroom above paper white. On the
// other hand, libplacebo hardcodes PL_COLOR_SDR_WHITE as the reference luminance.
// We must scale all wayland values to correspond to the libplacebo scale,
// otherwise libplacebo will assume that there is too little or too much headroom
// when ref_luma != PL_COLOR_SDR_WHITE.
float a = wd->min_luma;
float b = (PL_COLOR_SDR_WHITE - PL_COLOR_HDR_BLACK) / (wd->ref_luma - a);
wd->csp.hdr.min_luma = (wd->csp.hdr.min_luma - a) * b + PL_COLOR_HDR_BLACK;
wd->csp.hdr.max_luma = (wd->csp.hdr.max_luma - a) * b + PL_COLOR_HDR_BLACK;
// Wayland treats all transfers as display referred, don't scale min
// luminance, and hope compositors will do the right thing mapping it,
// or to be specific, not mapping it, because we set the same value.
float c = wd->min_luma;
float b = (PL_COLOR_SDR_WHITE - c) / (wd->ref_luma - a);
wd->csp.hdr.min_luma = (wd->csp.hdr.min_luma - a) * b + c;
wd->csp.hdr.max_luma = (wd->csp.hdr.max_luma - a) * b + c;
if (wd->csp.hdr.max_cll != 0)
wd->csp.hdr.max_cll = (wd->csp.hdr.max_cll - a) * b + PL_COLOR_HDR_BLACK;
wd->csp.hdr.max_cll = (wd->csp.hdr.max_cll - a) * b + c;
if (wd->csp.hdr.max_fall != 0)
wd->csp.hdr.max_fall = (wd->csp.hdr.max_fall - a) * b + PL_COLOR_HDR_BLACK;
wd->csp.hdr.max_fall = (wd->csp.hdr.max_fall - a) * b + c;
// Ensure that min_luma doesn't become negative.
wd->csp.hdr.min_luma = MPMAX(wd->csp.hdr.min_luma, 0.0);
// Since we want to do some exact comparisons of max_luma with PL_COLOR_SDR_WHITE,
Expand All @@ -2170,10 +2185,6 @@ static void info_done(void *data, struct wp_image_description_info_v1 *image_des
wd->csp.hdr.max_fall = MPMIN(wd->csp.hdr.max_fall, wd->csp.hdr.max_luma);
}
wl->preferred_csp = wd->csp;
if (wd->csp.hdr.max_luma != PL_COLOR_SDR_WHITE && !pl_color_transfer_is_hdr(wd->csp.transfer)) {
MP_VERBOSE(wl, "Setting preferred transfer to PQ for HDR output.\n");
wl->preferred_csp.transfer = PL_COLOR_TRC_PQ;
}
} else {
if (wl->icc_size) {
munmap(wl->icc_file, wl->icc_size);
Expand Down Expand Up @@ -2351,6 +2362,10 @@ static void supported_coefficients_and_ranges(void *data, struct wp_color_repres
wl->coefficients_map[PL_COLOR_SYSTEM_BT_2020_C] = WP_COLOR_REPRESENTATION_SURFACE_V1_COEFFICIENTS_BT2020_CL;
wl->range_map[PL_COLOR_SYSTEM_BT_2020_C + offset] = range;
break;
case WP_COLOR_REPRESENTATION_SURFACE_V1_COEFFICIENTS_ICTCP:
wl->coefficients_map[PL_COLOR_SYSTEM_BT_2100_PQ] = WP_COLOR_REPRESENTATION_SURFACE_V1_COEFFICIENTS_ICTCP;
wl->range_map[PL_COLOR_SYSTEM_BT_2100_PQ + offset] = range;
break;
}
}

Expand Down Expand Up @@ -3513,6 +3528,48 @@ static void set_color_management(struct vo_wayland_state *wl, struct pl_color_sp
wp_image_description_creator_params_v1_set_tf_named(image_creator_params, transfer);

struct pl_hdr_metadata hdr = color->hdr;

if (wl->supports_set_luminances) {
switch (color->transfer) {
case PL_COLOR_TRC_PQ:
// Set min luminance to 0 for PQ as per SMPTE ST 2084
wp_image_description_creator_params_v1_set_luminances(image_creator_params,
0 * WAYLAND_MIN_LUM_FACTOR, 10000, PL_COLOR_SDR_WHITE);
MP_VERBOSE(wl, "Setting PQ luminance range: min=0, max=10000, ref=%.2f\n",
PL_COLOR_SDR_WHITE);
// Mastering luminances will be set below
break;
case PL_COLOR_TRC_LINEAR:
// Our linear output is absolute scaled, meaning the 0 is absolute
// black, similar to PQ transfer. Configure it in the same way as PQ.
if (hdr.max_luma) {
wp_image_description_creator_params_v1_set_luminances(image_creator_params,
0 * WAYLAND_MIN_LUM_FACTOR, hdr.max_luma, PL_COLOR_SDR_WHITE);
MP_VERBOSE(wl, "Setting linear luminance range: min=0, max=%.5f, ref=%.2f\n",
hdr.max_luma, PL_COLOR_SDR_WHITE);
if (hdr.min_luma && hdr.max_luma) {
wp_image_description_creator_params_v1_set_mastering_luminance(image_creator_params,
lrintf(hdr.min_luma * WAYLAND_MIN_LUM_FACTOR), lrintf(hdr.max_luma));
MP_VERBOSE(wl, "Setting linear luminace mastering range: min=%.5f, max=%.2f\n",
hdr.min_luma, hdr.max_luma);
}
}
break;
case PL_COLOR_TRC_HLG:
// Leave default for HLG, we wouldn't output it directly, except for pass-through
break;
default:
// Set SDR luminance range for all relative transfers
if (hdr.min_luma && hdr.max_luma) {
wp_image_description_creator_params_v1_set_luminances(image_creator_params,
hdr.min_luma * WAYLAND_MIN_LUM_FACTOR, hdr.max_luma, PL_COLOR_SDR_WHITE);
Comment on lines +3564 to +3565
Copy link
Contributor

@mahkoh mahkoh Mar 3, 2026

Choose a reason for hiding this comment

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

I'm confused about this (and similar with the linear case above). Shouldn't max=ref in this case?

Copy link
Member Author

@kasper93 kasper93 Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If it's configured correctly, it's will be max=ref. However, it can be configured differently, setting higher max works fine.

MP_VERBOSE(wl, "Setting relative luminance range: min=%.5f, max=%.2f, ref=%.2f\n",
hdr.min_luma, hdr.max_luma, PL_COLOR_SDR_WHITE);
}
break;
}
}

bool is_hdr = pl_color_transfer_is_hdr(color->transfer);
bool use_metadata = hdr_metadata_valid(wl, &hdr);
if (!use_metadata)
Expand Down Expand Up @@ -3564,11 +3621,10 @@ static void set_color_representation(struct vo_wayland_state *wl, struct mp_imag
if (!wl->color_representation_manager)
return;

if (wl->color_representation_surface)
if (wl->color_representation_surface) {
wp_color_representation_surface_v1_destroy(wl->color_representation_surface);

wl->color_representation_surface =
wp_color_representation_manager_v1_get_surface(wl->color_representation_manager, wl->callback_surface);
wl->color_representation_surface = NULL;
}

int alpha = wl->alpha_map[params->repr.alpha];
int coefficients = wl->coefficients_map[params->repr.sys];
Expand All @@ -3582,10 +3638,17 @@ static void set_color_representation(struct vo_wayland_state *wl, struct mp_imag
MP_VERBOSE(wl, "Setting color representation:\n");

if (coefficients && range) {
wl->color_representation_surface =
wp_color_representation_manager_v1_get_surface(wl->color_representation_manager, wl->callback_surface);
Comment on lines +3641 to +3642
Copy link
Contributor

@llyyr llyyr Mar 4, 2026

Choose a reason for hiding this comment

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

I don't believe this is correct. There could be theoretical compositor that doesn't support the coefficient/range combination we want to set but does support the alpha/chromaloc. In such a case, we'll try to set them without a color repr surface

Copy link
Member Author

@kasper93 kasper93 Mar 4, 2026

Choose a reason for hiding this comment

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

Yes, I have it fixed locally, there was unrelated issue that caused problems. I will push later when I have time to work on this again. I got side-tracked again with luminances, because I'm still thinking if we should prefer PQ output and just output in gamma2.2 as compositors like to have it.

MP_VERBOSE(wl, " Coefficients: %s, Range: %s\n",
m_opt_choice_str(pl_csp_names, params->repr.sys),
m_opt_choice_str(pl_csp_levels_names, params->repr.levels));
wp_color_representation_surface_v1_set_coefficients_and_range(wl->color_representation_surface, coefficients, range);
} else {
MP_WARN(wl, "Color representation %s / %s not supported! Output can be incorrect.\n",
Copy link
Member Author

@kasper93 kasper93 Mar 4, 2026

Choose a reason for hiding this comment

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

@mahkoh: Why Jay doesn't advertise any coefficients as supported? I would expect any implementation that supports this protocol at least advertise the baseline, which it in fact supports.

I'm not sure if that's intended, because it triggers this warning. I can exclude identity, because this in practice is always supported, but that seems weird.

Copy link
Contributor

@mahkoh mahkoh Mar 4, 2026

Choose a reason for hiding this comment

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

Why Jay doesn't advertise any coefficients as supported?

Jay does not support any YCbCr formats.

I would expect any implementation that supports this protocol at least advertise the baseline, which it in fact supports.

I've implemented the protocol only for the alpha modes.

Copy link
Member Author

@kasper93 kasper93 Mar 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's fair. I think I will keep this warning, it may be weird thing to log when the output is fine. But this is the information we get. RGB/identity is not advertised as supported.

Protocol states.

When this object is created, it shall immediately send this event once for each matrix coefficient and color range combination the compositor supports.

So, I assume that implementation should send RGB/identity as supported, if it is in fact supported.

EDIT: Note to self, in case I forget, change it to Output may be incorrect., sounds better.

Copy link
Contributor

@mahkoh mahkoh Mar 4, 2026

Choose a reason for hiding this comment

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

But it doesn't support them. If you call set_coefficients_and_range at all, you will get a protocol error.

Copy link
Member Author

@kasper93 kasper93 Mar 4, 2026

Choose a reason for hiding this comment

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

But it doesn't support them.

I kindly disagree that Jay doesn't support identity. It displays fine.

If you call set_coefficients_and_range at all, you will get a protocol error.

That's why I don't call it to avoid protocol error and display warning.

Copy link
Member

@Dudemanguy Dudemanguy Mar 4, 2026

Choose a reason for hiding this comment

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

FWIW, I don't think we need to scare people with a warning here. Verbose is probably fine.

Copy link
Member Author

@kasper93 kasper93 Mar 4, 2026

Choose a reason for hiding this comment

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

I think we should warn, it would save us dozens of "wrong colors" issues that we get for dmabuf wayland. Note that it will warn only when color repr proto is actually supported, so it's quite valid thing to say that output will be wrong, if certain params are not supported.

Copy link
Member

@Dudemanguy Dudemanguy Mar 4, 2026

Choose a reason for hiding this comment

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

Honestly my guess is that people would open an issue about the warning they see instead.

Copy link
Member Author

@kasper93 kasper93 Mar 4, 2026

Choose a reason for hiding this comment

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

As they should, not for mpv probably, but their compositor.

Copy link
Contributor

@llyyr llyyr Mar 4, 2026

Choose a reason for hiding this comment

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

FWIW, I don't think we need to scare people with a warning here. Verbose is probably fine.

The warning would only be visible with dmabuf-wayland in most cases, since full range rgb should be supported on all most compositors. In which case, I do think it being a warning is fair.

m_opt_choice_str(pl_csp_names, params->repr.sys),
m_opt_choice_str(pl_csp_levels_names, params->repr.levels));
return;
}

if (alpha) {
Expand Down Expand Up @@ -4203,6 +4266,27 @@ int vo_wayland_control(struct vo *vo, int *events, int request, void *arg)

void vo_wayland_handle_color(struct vo_wayland_state *wl, struct mp_image_params *params)
{
#if HAVE_WAYLAND_PROTOCOLS_1_41
if (!params) {
if (wl->color_surface) {
wp_color_management_surface_v1_destroy(wl->color_surface);
Copy link
Member

@Dudemanguy Dudemanguy Mar 3, 2026

Choose a reason for hiding this comment

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

Why have this destroy behavior? I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?

Copy link
Member Author

@kasper93 kasper93 Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The idea is when we set --target-colorspace-hint=no we no longer hint anything. Which in practice means, Vulkan sets VK_COLOR_SPACE_SRGB_NONLINEAR_KHR, which means that Mesa take over color_surface duty. So we release our color surface, to let others use it. Conversely when we do --target-colorspace-hint=yes we switch to VK_COLOR_SPACE_PASS_THROUGH_EXT and use color surface ourselves.

I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?

Mesa supports Wayland nativelly now, so not only VK_hdr_layer is affected.

Copy link
Member

@Dudemanguy Dudemanguy Mar 3, 2026

Choose a reason for hiding this comment

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

Ah I forgot about the --target-colorspace-hint=no case. I guess it is fine as long as that is the only time this ever receives NULL params.

Copy link
Member Author

@kasper93 kasper93 Mar 3, 2026

Choose a reason for hiding this comment

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

Yes, in all other cases, hint is defined to "something". Also we might at some point get rid of this option completely, but people still complain that "color is wrong", so restoring to dumb player mode is good to debug or just workaround other bugs.

Copy link

@roberth1990 roberth1990 Mar 3, 2026

Choose a reason for hiding this comment

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

Why have this destroy behavior? I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?

Well you will remove hdr support for us nvidia users, since amd have so many issues for me at the moment, I will be forced back to windows if this happens.

Copy link
Contributor

@llyyr llyyr Mar 4, 2026

Choose a reason for hiding this comment

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

Why have this destroy behavior? I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?

Well you will remove hdr support for us nvidia users, since amd have so many issues for me at the moment, I will be forced back to windows if this happens.

this doesn't remove HDR support for nvidia. this PR makes it so that you dont need that layer for HDR support on nvidia

Copy link

@roberth1990 roberth1990 Mar 4, 2026

Choose a reason for hiding this comment

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

Why have this destroy behavior? I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?

Well you will remove hdr support for us nvidia users, since amd have so many issues for me at the moment, I will be forced back to windows if this happens.

this doesn't remove HDR support for nvidia. this PR makes it so that you dont need that layer for HDR support on nvidia

Ah okay thanks, kind of traumatised by a certain other open source project's disregard for nvidia.

Copy link
Contributor

@mahkoh mahkoh Mar 3, 2026

Choose a reason for hiding this comment

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

I don't believe this is sound as is.

Consider the following code:

static void image_description_failed(void *data, struct wp_image_description_v1 *image_description,
                                     uint32_t cause, const char *msg)
{
    struct vo_wayland_state *wl = data;
    MP_VERBOSE(wl, "Image description failed: %d, %s\n", cause, msg);
    wp_color_management_surface_v1_unset_image_description(wl->color_surface);
    wp_image_description_v1_destroy(image_description);
}

// ...

    struct wp_image_description_v1 *image_description = wp_image_description_creator_params_v1_create(image_creator_params);
    wl_proxy_set_queue((struct wl_proxy *)image_description, wl->color_queue);
    wp_image_description_v1_add_listener(image_description, &image_description_listener, wl);

If the color_surface is destroyed while the image description is pending, the callback will access a NULL pointer.

Furthermore, it is probably a good idea to store the image_description inside wl and to destroy it at the appropriate times, since otherwise the image descriptions might complete out of order.

Copy link
Member Author

@kasper93 kasper93 Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

That's very good observation. However, my expectation is that @llyyr in commit 8012dc8 made it so that image_description_listener is read or failed by the time set_color_management function returns. And we will never call set_color_management() concurrently.

Copy link
Contributor

@mahkoh mahkoh Mar 3, 2026

Choose a reason for hiding this comment

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

I don't believe that is necessarily the case. See the discussion in #17495.

wl->color_surface = NULL;
}
if (wl->color_queue) {
wl_event_queue_destroy(wl->color_queue);
wl->color_queue = NULL;
}
Comment on lines +4275 to +4278
Copy link
Contributor

@mahkoh mahkoh Mar 3, 2026

Choose a reason for hiding this comment

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

Suggested change
if (wl->color_queue) {
wl_event_queue_destroy(wl->color_queue);
wl->color_queue = NULL;
}

I think it's fine to keep the queue alive.

Copy link
Member Author

@kasper93 kasper93 Mar 3, 2026

Choose a reason for hiding this comment

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

Ok, will do.

wl->current_params = (struct mp_image_params){0};
return;
}
if (wl->color_manager) {
if (!wl->color_queue)
wl->color_queue = wl_display_create_queue_with_name(wl->display, "image description creator queue");
if (!wl->color_surface)
wl->color_surface = wp_color_manager_v1_get_surface(wl->color_manager, wl->callback_surface);
}
#endif

bool color_space_changed = !pl_color_space_equal(&wl->current_params.color, &params->color);
bool color_repr_changed = !pl_color_repr_equal(&wl->current_params.repr, &params->repr) ||
wl->current_params.chroma_location != params->chroma_location;
Expand Down Expand Up @@ -4335,11 +4419,6 @@ bool vo_wayland_init(struct vo *vo)
if (wl->color_manager) {
wl->color_surface_feedback = wp_color_manager_v1_get_surface_feedback(wl->color_manager, wl->callback_surface);
wp_color_management_surface_feedback_v1_add_listener(wl->color_surface_feedback, &surface_feedback_listener, wl);
// Only bind color surface to vo_dmabuf_wayland for now to avoid conflicting with graphics drivers
if (!strcmp(wl->vo->driver->name, "dmabuf-wayland")) {
wl->color_surface = wp_color_manager_v1_get_surface(wl->color_manager, wl->callback_surface);
wl->color_queue = wl_display_create_queue_with_name(wl->display, "image description creator queue");
}
} else {
MP_VERBOSE(wl, "Compositor doesn't support the %s protocol!\n",
wp_color_manager_v1_interface.name);
Expand Down
1 change: 1 addition & 0 deletions video/out/wayland_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ struct vo_wayland_state {
struct mp_image_params current_params;
bool supports_parametric;
bool supports_display_primaries;
bool supports_set_luminances;
int primaries_map[PL_COLOR_PRIM_COUNT];
int transfer_map[PL_COLOR_TRC_COUNT];
void *icc_file;
Expand Down
Loading