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] Extend high precision setting to use 10 bit or better for all streams #23617

Merged
merged 2 commits into from Aug 14, 2023

Conversation

CrystalP
Copy link
Contributor

Description

Modified CRendererDXVA::ChooseConversion and CRendererShaders::CalcIntermediateTargetFormat that select the intermediate target format for dxva and pixel shaders render method.

  • Before PR: find compatible format of 10 bits or better when the setting 'High precision processing" is enabled and the source is 10 bit HDR.
  • After PR: find a compatible format of 10 bits or better when the setting "High precision processing" is enabled (regardless of the number of bits of the source or its SDR / HDR type)

The exception of using 8 bits for VSR was preserved.

Even the case of 8 bit stream going to 8 bit backbuffer should benefit a bit, since there is scaling, optional LUT and dithering after the color conversion. Extra cost, if any, should be minimal (cheap bit shifting to go 8 bit <=> 10 bits, probably built in gpu and free)
It could be special cased and excluded though, but complex to detect when there is no post-processing after color conversion.

This removes the need to change the same functions to remove duplication of tests that distinguish SDR from HDR (ex PR #23599)

Motivation and context

Found out after a bit of research that the conversion of 8 bit YUV to RGB is not exact with 8 bits destination texture, justifying the use of 10 bits.

How has this been tested?

Windows 10 with sdr, hdr streams, some that activate VSR => 8 bit and 10 bit backbuffers.

  • rgb10 or fp16 used then setting is true (except VSR, rgb8)
  • rgb8 or rgb10 depending on backbuffers when setting is false.

What is the effect on users?

Slightly improved precision for SDR streams.

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)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • 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

Copy link
Member

@thexai thexai left a comment

Choose a reason for hiding this comment

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

OK, seems fine to me but please remove unused parameters:

CRendererShaders::CalcIntermediateTargetFormat(const VideoPicture& picture) const

to

CRendererShaders::CalcIntermediateTargetFormat() const

// - trying VSR scaling, which requires RGB8 processor output format
// - the user opted out of high quality and the swap chain is 8 bits.
if (!m_tryVSR && (DX::Windowing()->IsHighPrecisionProcessingSettingEnabled() ||
DX::Windowing()->GetBackBuffer().GetFormat() != DXGI_FORMAT_B8G8R8A8_UNORM))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DX::Windowing()->GetBackBuffer().GetFormat() != DXGI_FORMAT_B8G8R8A8_UNORM))
DX::Windowing()->GetBackBuffer().GetFormat() == DXGI_FORMAT_R10G10B10A2_UNORM))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

@CrystalP
Copy link
Contributor Author

Removed the unnecessary parameters in pixel shaders and dxva.
Included the removal of unnecessary m_intermediateTargetFormat that you noticed, thanks.

const ProcessorConversion chosenConversion =
ChooseConversion(conversions, picture.colorBits, picture.color_transfer);
m_intermediateTargetFormat = chosenConversion.m_outputFormat;
const ProcessorConversion chosenConversion = ChooseConversion(conversions);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const ProcessorConversion chosenConversion = ChooseConversion(conversions);
m_conversion = ChooseConversion(conversions);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sure

@@ -55,13 +55,10 @@ class CRendererDXVA : public CRendererHQ
* \param tryVSR yes/no favor a conversion that enables Video Super Resolution scaling
* \return
*/
DXVA::ProcessorConversion ChooseConversion(const DXVA::ProcessorConversions& conversions,
Copy link
Member

Choose a reason for hiding this comment

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

Now doxy prams are obsolete

…ll streams

8 bit YUV converted to RGB cannot be represented exactly with 8 bits so use 10 bits or float when the setting is enabled.
Keep VSR exception.
@thexai thexai merged commit 3dcf006 into xbmc:master Aug 14, 2023
2 checks passed
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

4 participants