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

video/out/gpu/context: add target_csp callback to ra_swapchain #15279

Merged
merged 3 commits into from
Nov 23, 2024

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Nov 9, 2024

No description provided.

@kasper93
Copy link
Contributor Author

kasper93 commented Nov 9, 2024

/cc @Dudemanguy

Copy link

github-actions bot commented Nov 9, 2024

Download the artifacts for this pull request:

Windows
macOS

video/out/vo_gpu_next.c Outdated Show resolved Hide resolved
@Dudemanguy
Copy link
Member

Also auto being implemented in this fashion would make everything else (like drm) basically go back to behaving like no by default again.

@kasper93
Copy link
Contributor Author

Also auto being implemented in this fashion would make everything else (like drm) basically go back to behaving like no by default again.

The intention for now is that when the target_csp function is not implemented, it will assume HDR is always available. Added comment about that.

struct ra_swapchain_fns {
// Gets the current framebuffer depth in bits (0 if unknown). Optional.
int (*color_depth)(struct ra_swapchain *sw);

// Target device color space. Optional.
pl_color_space_t (*target_csp)(struct ra_swapchain *sw);
Copy link
Member

Choose a reason for hiding this comment

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

Kind of nitpicky but is there a reason to return a colorspace? It seems like you only actually use the display's peak brightness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial version was using only target peak, but it turns out it is also returned for SDR displays. Like I have one with 270 nits. Correct way is to check the color space returned from the API. Also primaries might be useful in the future.

There is still one thing to fix, because I noticed that lib placebo completely fails to render SDR correctly if the target peak is not 203. I may have time tomorrow to fix that.

Copy link
Member

Choose a reason for hiding this comment

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

You could only return the MaxLuminance value if the colorspace is actually an HDR one and return 203 otherwise.

@Dudemanguy
Copy link
Member

Just the same comment I had before. Why return the csp if you don't actually use it? SDR could always just return 0 for the target peak or whatever value works.

@kasper93
Copy link
Contributor Author

kasper93 commented Nov 23, 2024

Just the same comment I had before. Why return the csp if you don't actually use it? SDR could always just return 0 for the target peak or whatever value works.

I have also commit to support --target-contrast using min_luma value. I think returning only target_peak is limiting, but if you feel like it is better way I can do that.

EDIT: I pushed contrast commit, if that's not ok, I will revert to target luma only.

@Dudemanguy
Copy link
Member

I have also commit to support --target-contrast using min_luma value.

Ah you're using multiple values now, it's fine then and my nitpick is moot. No real point in using pointers or whatever when returning a pl_color_space works just fine.

@kasper93
Copy link
Contributor Author

Let's try it. There is possibility that those reported display values will be not optimal for some people. But we will never know if we don't test it out.

@kasper93 kasper93 merged commit 4908a63 into mpv-player:master Nov 23, 2024
26 checks passed
@kasper93 kasper93 deleted the target_csp branch November 23, 2024 21:16
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.

2 participants