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

fix: automatic selection for hybrid GPU and IDDSampleDriver users #3002

Merged
merged 18 commits into from
Sep 29, 2024

Conversation

Nonary
Copy link
Collaborator

@Nonary Nonary commented Aug 10, 2024

Description

Enhances the ddprobe utility to test to see if frames can be captured and that they are also not empty frames. If one of the 10 frames captured is not empty, it will return back successful. Previously it was only testing to see if it can be duplicated, but would fail on capturing frames which would cause Sunshine to pick the wrong GPU.

Screenshot

N/A

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

Attention: Patch coverage is 46.15385% with 7 lines in your changes missing coverage. Please review.

Project coverage is 9.87%. Comparing base (5bc32cd) to head (ec488ca).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/platform/windows/display_base.cpp 46.15% 2 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #3002      +/-   ##
=========================================
- Coverage    9.88%   9.87%   -0.01%     
=========================================
  Files         101     101              
  Lines       17975   17980       +5     
  Branches     8402    8408       +6     
=========================================
- Hits         1776    1775       -1     
+ Misses      13321   13318       -3     
- Partials     2878    2887       +9     
Flag Coverage Δ
Linux 7.27% <ø> (-0.07%) ⬇️
Windows 5.10% <46.15%> (+0.02%) ⬆️
macOS-12 10.79% <ø> (-0.02%) ⬇️
macOS-13 10.74% <ø> (+0.02%) ⬆️
macOS-14 10.03% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/platform/windows/display_base.cpp 13.60% <46.15%> (+0.67%) ⬆️

... and 7 files with indirect coverage changes

tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
@FrogTheFrog
Copy link
Collaborator

I have no idea about whether the code logic is correct or not, but I've added a few remarks about other stuff.

@Nonary

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@Nonary

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@Nonary

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
@paulzzh

This comment was marked as resolved.

@ReenigneArcher

This comment was marked as resolved.

@Nonary

This comment was marked as resolved.

@Biswa96

This comment was marked as resolved.


// If duplication is successful, test frame capture
HRESULT captureResult = test_frame_capture(dup, device_ptr.Get());
if (FAILED(captureResult)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

S_FALSE is a success value, so I'm not sure how this could be working. I think we would just return S_OK in the case like we did before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch! Did not see that at all, fixed it

}

// If duplication is successful, test frame capture
HRESULT captureResult = test_frame_capture(dup, device_ptr.Get());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned we may generate false negatives for previously working systems. Can we pass a parameter into ddprobe to allow us to do a first pass with your new check followed by a second pass without if the first one finds no matching configurations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the code to make the frame capture point optional like you suggested

tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
tools/ddprobe.cpp Outdated Show resolved Hide resolved
@ReenigneArcher
Copy link
Member

Also, we now use semantic PRs. Could you make the PR title follow the conventional commit guidelines? https://www.conventionalcommits.org/en/v1.0.0/

@Nonary Nonary changed the title Fix multi GPU automatic selection for hybrid GPU and IDDSampleDriver fix: automatic selection for hybrid GPU and IDDSampleDriver users Sep 14, 2024
Nonary and others added 18 commits September 29, 2024 14:56
…ual displays

- Added `test_frame_capture` function to verify if frames are successfully captured and not empty.
- Fixes issues with virtual displays such as IDDSampleDriver when using more than one GPU.
It occoured to me there might be some desktop images that are predominately black, which could cause a false positive.
Co-authored-by: ReenigneArcher <42013603+ReenigneArcher@users.noreply.github.com>
Co-authored-by: Cameron Gutman <aicommander@gmail.com>
Copy link

sonarcloud bot commented Sep 29, 2024

@cgutman cgutman merged commit fceda35 into LizardByte:master Sep 29, 2024
38 checks passed
KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Oct 9, 2024
…zardByte#3002)

* Fix frame capture and output duplication for dual GPU setups and virtual displays

- Added `test_frame_capture` function to verify if frames are successfully captured and not empty.
- Fixes issues with virtual displays such as IDDSampleDriver when using more than one GPU.

Co-authored-by: ReenigneArcher <42013603+ReenigneArcher@users.noreply.github.com>
Co-authored-by: Cameron Gutman <aicommander@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment