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

[Windows] minor refactor of DXVAHD #23101

Merged
merged 1 commit into from Apr 5, 2023
Merged

Conversation

thexai
Copy link
Member

@thexai thexai commented Apr 4, 2023

Description

Minor refactor of DXVAHD

Motivation and context

  • Avoids many calls to DX::Windowing()->UseLimitedColor()
  • No functional changes

How has this been tested?

Runtime Windows x64

What is the effect on users?

nothing

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

 - Avoids many calls to DX::Windowing()->UseLimitedColor()
 - No functional changes
static DXGI_COLOR_SPACE_TYPE GetDXGIColorSpaceSource(const DXGIColorSpaceArgs& csArgs,
bool supportHDR,
bool supportHLG);
DXGI_COLOR_SPACE_TYPE GetDXGIColorSpaceTarget(const DXGIColorSpaceArgs& csArgs,
Copy link
Contributor

Choose a reason for hiding this comment

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

not static anymore? I think it's good it doesn't rely on class members.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed static as seems a bit confusing due calls others methods inside. In the other hand GetDXGIColorSpaceSource is self contained as not calls others methods. But actually the two methods could be static or not.

@CrystalP
Copy link
Contributor

CrystalP commented Apr 4, 2023

Saving a call and reducing coupling is a good idea.
I will give it a whirl just in case.

Copy link
Contributor

@CrystalP CrystalP left a comment

Choose a reason for hiding this comment

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

No surprise, the behavior is the same before/after, so approved for me.
Unfortunately in my case it means that with limited range output, dxva fails all the formats I tried.

edit: reason is that the AMD processor doesn't support ANY DXGI_COLOR_SPACE_RGB_STUDIO as output.

@thexai
Copy link
Member Author

thexai commented Apr 5, 2023

Unfortunately in my case it means that with limited range output, dxva fails all the formats I tried.

edit: reason is that the AMD processor doesn't support ANY DXGI_COLOR_SPACE_RGB_STUDIO as output.

In the forum there are multiple examples of people using AMD and limited range with HDR10 I remember it in the thread when HDR was implemented in Windows (https://forum.kodi.tv/showthread.php?tid=349861) I would have to look for the posts. In any case this is not relevant for this PR.

@thexai thexai merged commit 1c215ce into xbmc:master Apr 5, 2023
2 checks passed
@thexai thexai deleted the dxvahd-refactor branch April 5, 2023 12:07
@CrystalP
Copy link
Contributor

CrystalP commented Apr 5, 2023

I don't doubt it and it wouldn't be surprising that different AMD GPU generations, models, and maybe even different driver versions have different support (range, but other things as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants