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][dxva] log conversions supported by the processor #23136

Merged
merged 4 commits into from
Apr 28, 2023

Conversation

CrystalP
Copy link
Contributor

@CrystalP CrystalP commented Apr 12, 2023

Description

New function to log the conversion capabilities of a dxva processor, based on the Windows 10 function ID3D11VideoProcessorEnumerator1::CheckVideoProcessorFormatConversion().
On Windows 8 the function exits early and logs nothing.

The input format (NV12, P010, ...) is fixed by the file being played and the function will go over possible combinations of the other 3 parameters of a color conversion:

  • input color space (YCbCr only)
  • output format (RGB only)
  • output color space (RGB only)

In order to make it the logged information more readable the function adds special marks to the values currently chosen by Kodi's algorithms, the values that are a more natural mapping but may be missing processor support,

A few technical points:

  • To avoid code duplication, GetDXGIColorSpaceSource() was split in two:
    • color space mapping closest to the description of the media (new function AvToDxgiColorSpace()
    • transform the mappings above to workaround processor limitations and do part of the work in the output stage / shader
  • changed the enumerator1 to a class member to avoid creating it in multiple places
  • refactored swap chain color spaces enumeration, now also used from outside DeviceResources
  • unrelated to PR subject: improve a bit DeviceResources::GetDebugInfo
  • also unrelated to PR subject: swapchain3 is enough to call CheckColorSpaceSupport, there was no need for swapchain4,

Motivation and context

Helps with troubleshooting and fixing of dxva processor issues.
A previous PR started testing whether the conversion is supported, this PR lists possible alternatives for a fix.

How has this been tested?

Window 10: Intel 4th and 8th gen, AMD Cezanne igfx
to help with the testing, put a breakpoint in DXVAHD.cpp on the last line of ListSupportedConversions
No Windows 8 needed because the new code exits early under W8.

The processor of Intel 8th gen is strange and CheckVideoProcessorFormatConversion() rejects some conversions even though the processor is able to output a picture (maybe it falls back on a default conversion, with some visual imperfections - unknown at this time). That testing will continue.

What is the effect on users?

no direct effect and the code activates only when the log level is debug.

Screenshots (if appropriate):

Example of a couple lines - not a complete log

image

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

@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch 4 times, most recently from 7b88613 to 400d32b Compare April 13, 2023 03:07
@thexai
Copy link
Member

thexai commented Apr 13, 2023

The work and effort in making this is appreciated but there are some aspects of the design that I don't like:

Too many changes are made to the current code (which works fine) and therefore may introduce bugs. The changes do not seem clear to me and the code is difficult to review. There are also changes that do not seem necessary or are not directly related to the PR.

First I would ask to separate the deviceResources changes in a separate commit. Perhaps the changes in debug info are unnecessary and penalize performance (it is not necessary to evaluate 100 pixel formats if swap chain can only have 2). Note that this code is executed 60 times per second while the debug OSD is visible.

I don't think it's necessary to make changes to GetDXGIColorSpaceSource since according to the PR it only makes a detailed log of the possible combinations supported but should not change (either intentionally or by mistake) the current operation.

That is, the new log function should call GetDXGIColorSpaceSource as it currently is and implement additional code if necessary without changing the existing one.

I also don't think there is much value in listing combinations that are by nature impossible i.e. all X601, P601, P709 color space for videos that are BT.2020:

     (*: values picked by heuristics, N native input color space, bb supported as swap chain backbuffer)
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   R16G16B16A16_FLOAT         / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   R16G16B16A16_FLOAT         /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to * R10G10B10A2_UNORM          / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to * R10G10B10A2_UNORM          /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   R8G8B8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   R8G8B8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   B8G8R8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   B8G8R8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   B8G8R8X8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_NONE_P709_X601    to   B8G8R8X8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   R16G16B16A16_FLOAT         / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   R16G16B16A16_FLOAT         /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to * R10G10B10A2_UNORM          / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to * R10G10B10A2_UNORM          /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   R8G8B8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   R8G8B8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   B8G8R8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   B8G8R8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   B8G8R8X8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P601       to   B8G8R8X8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   R16G16B16A16_FLOAT         / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   R16G16B16A16_FLOAT         /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to * R10G10B10A2_UNORM          / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to * R10G10B10A2_UNORM          /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   R8G8B8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   R8G8B8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   B8G8R8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   B8G8R8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   B8G8R8X8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P601         to   B8G8R8X8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   R16G16B16A16_FLOAT         / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   R16G16B16A16_FLOAT         /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to * R10G10B10A2_UNORM          / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to * R10G10B10A2_UNORM          /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   R8G8B8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   R8G8B8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   B8G8R8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   B8G8R8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   B8G8R8X8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G22_LEFT_P709       to   B8G8R8X8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   R16G16B16A16_FLOAT         / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   R16G16B16A16_FLOAT         /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to * R10G10B10A2_UNORM          / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to * R10G10B10A2_UNORM          /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   R8G8B8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   R8G8B8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   B8G8R8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   B8G8R8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   B8G8R8X8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_FULL_G22_LEFT_P709         to   B8G8R8X8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   R16G16B16A16_FLOAT         / *bb RGB_FULL_G22_NONE_P709          
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   R16G16B16A16_FLOAT         /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to * R10G10B10A2_UNORM          / *bb RGB_FULL_G22_NONE_P709          
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to * R10G10B10A2_UNORM          /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   R8G8B8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   R8G8B8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   B8G8R8A8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   B8G8R8A8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   B8G8R8X8_UNORM             / *bb RGB_FULL_G22_NONE_P709          
     * P010 / *  YCBCR_STUDIO_G22_LEFT_P2020      to   B8G8R8X8_UNORM             /  bb RGB_STUDIO_G22_NONE_P709        
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   R16G16B16A16_FLOAT         /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to * R10G10B10A2_UNORM          /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   R8G8B8A8_UNORM             /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   B8G8R8A8_UNORM             /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /  N YCBCR_STUDIO_G2084_LEFT_P2020    to   B8G8R8X8_UNORM             /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   R16G16B16A16_FLOAT         /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   R16G16B16A16_FLOAT         /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to * R10G10B10A2_UNORM          /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to * R10G10B10A2_UNORM          /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   R8G8B8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   R8G8B8A8_UNORM             /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   B8G8R8A8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   B8G8R8A8_UNORM             /  bb RGB_FULL_G2084_NONE_P2020       
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   B8G8R8X8_UNORM             /     RGB_FULL_G10_NONE_P709          
     * P010 /    YCBCR_STUDIO_G2084_TOPLEFT_P2020 to   B8G8R8X8_UNORM             /  bb RGB_FULL_G2084_NONE_P2020   

Perhaps it would be better to limit the listing to color spaces that match the source only.

@CrystalP
Copy link
Contributor Author

CrystalP commented Apr 14, 2023

No worries, criticize away, there are many ways to do this and to present the information, it took me a while to settle on something but still am not very happy. I wish that information was dumped by dxdiag and this whole thing could be avoided, but it's not.

I'm going to split into multiple commits.

To address your points:

  • Useless input color spaces
    It's not that easy to define and code "common sense", filter out some lines but keep others that may or may not be useful in the future. The function exists to help with future unknown situations (missing conversion) or optimize some choices.
    With those parameters, might as well use the dumb solution of listing all and avoid complexity, extra code, enums, mappings...
    For example the HDR>SDR tonemapping path overrides the "common sense" choice of input color space (ie YCBCR_STUDIO_G2084_LEFT_P2020 or the TOPLEFT). That was not obvious. Same for HLG in HDR.

note: The input format is not very helpful to cut the list. P010 does not automatically mean BT2020. Encoding SDR with 10 bit H265 is common, same for anime in 10 bit H264.
I have one example of the opposite, G2084/BT2020 in NV12 though it's probably an exception...

  • GetDXGIColorSpaceSource
    I needed a function that provides the "naive" mapping from the Av* fields to a DXGI color space > a generic AvToDxgiColorSpace was created.
    It is quite similar to GetDXGIColorSpaceSource, except for the cases that apply workarounds for various limitations of processors. The idea with the refactor was to keep in GetDXGIColorSpaceSource only the code for the differences from the ideal situation of a perfect processor, but the more workarounds there are, the more complicated that logic gets (not a good thing, but hidden with the way GetDXGIColorSpaceSource is today).
    GetDXGIColorSpaceSource could remain unchanged, not a problem.

  • no problem reverting the lookup in the video debug OSD (DeviceResources), it made sense when I had std::map containing the texts at one point but not anymore. I'll put the flag testing in a separate commit as suggested, it's still valid I think.

@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch from 400d32b to b587340 Compare April 15, 2023 15:52
@CrystalP
Copy link
Contributor Author

Here it is, broken up in discrete steps that should be easier to follow.

Tested with SDR/HDR/HLG clips played as HDR and SDR, with HDR capable and not capable hardware.
AMD Cezanne iGPU, Intel 4th and 8th gen.

Functionality is the same as before PR in GetDXGIColorSpaceSource, with the small exception of doing for HLG what you added in 5b528b9 / PR#23109 (processor supporting TOPLEFT not LEFT)

To me the benefit of separating standard mapping from the various processor limitation workarounds is that it's much easier to keep track of them that way and it should be easier when more have to be added.
If the list grows it will be possible to refactor the "override" functions to have an identical signature, define the list of override functions in an array and simply loop over the array, for any number of them.

@CrystalP CrystalP added Type: Improvement non-breaking change which improves existing functionality Platform: Windows Component: DirectX rendering labels Apr 15, 2023
@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch 2 times, most recently from 632c72d to 442452d Compare April 15, 2023 16:47
@CrystalP
Copy link
Contributor Author

I had already corrected and pushed for Jenkins last diff suggestion - not applicable.
The diffs and github presentation are making a mess of the GetDXGIColorSpaceSource work because it finds some similar lines, but it would be better to compare full function before and after, not bits and pieces of it.

xbmc/rendering/dx/DeviceResources.cpp Outdated Show resolved Hide resolved
xbmc/rendering/dx/DeviceResources.cpp Outdated Show resolved Hide resolved
xbmc/rendering/dx/DeviceResources.cpp Outdated Show resolved Hide resolved
xbmc/rendering/dx/DeviceResources.cpp Show resolved Hide resolved
xbmc/rendering/dx/DeviceResources.cpp Outdated Show resolved Hide resolved
@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch 2 times, most recently from 8671930 to 0df867c Compare April 15, 2023 17:43
xbmc/rendering/dx/DeviceResources.cpp Outdated Show resolved Hide resolved
xbmc/rendering/dx/DeviceResources.cpp Outdated Show resolved Hide resolved
@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch 2 times, most recently from 293c675 to 5856f3b Compare April 16, 2023 17:45
@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch from 5856f3b to c2f80d2 Compare April 16, 2023 18:42
@CrystalP
Copy link
Contributor Author

Feedback merged

  • const / const&

  • renamed ProcessorOutputFormats > GetProcessorOutputFormats

  • did the same for SwapChainColorSpaces > GetSwapChainColorSpaces of commit 1

  • moved up creation of enumerator1 right after creation of enumerator

  • take the lock when entering ListSupportedConversions() or IsFormatConversionSupported() (had to remove const)

  • protected IsFormatConversionSupported() against non-existing enumerator1

  • also moved up a couple lines in ListSupportedConversion that can be executed in Windows 8 with basic enumerator

I can't say I understand the threading model of Kodi and whether it's a possible scenario, but now the enumerator/enumerator1 shouldn't be destructable while being used in ListSupportedConversions() and IsFormatConversionSupported().
Nothing can help when the actual device/interfaces get invalidated by the system, we can only catch the errors, destruct and reconstruct later.

@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch from c2f80d2 to d9bd450 Compare April 16, 2023 19:02
@CrystalP
Copy link
Contributor Author

sorry messed up the rebase, pushed a fix and will leave things alone for the rest of the day
removed the C cast pointed just earlier

@CrystalP CrystalP force-pushed the dxva-processor-list-conversions branch from d9bd450 to a76ffc9 Compare April 16, 2023 19:07
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.

Tested Windows x64 and Xbox Series S

@CrystalP
Copy link
Contributor Author

The use of the CriticalSection was bugging me so I checked. All functions of DXVAHD are called exclusively on the render thread (main thread) so I'm not sure what the critical section is supposed to protect.

Did you have other concerns or does this seem ready to merge?

The Jenkins build failure of android-arm64-docker is not related.

@thexai
Copy link
Member

thexai commented Apr 27, 2023

Jenkins build this please

@thexai thexai closed this Apr 27, 2023
@thexai thexai reopened this Apr 27, 2023
@thexai
Copy link
Member

thexai commented Apr 27, 2023

@CrystalP
All is green, merge it whenever you want

@CrystalP CrystalP merged commit 07206c6 into xbmc:master Apr 28, 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

2 participants