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

feat(capture): add AMD Display Capture for AFMF #3171

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

radugrecu97
Copy link

@radugrecu97 radugrecu97 commented Sep 11, 2024

Description

This PR adds a new capture option under advanced settings - AMD Display capture.
The main purpose of this new capture method would be to capture AFMF frames.

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

Known bugs

  • Mouse isn't captured unless it's of a very large size
  • When using AFMF, frames might be captured out of order during and could result in temporary "ghosted" image. Some testing showed that it's caused by display changes and moonlight pause and resume. This is fixed after disabling and re-enabling AFMF or sometimes waiting. Different capturing mode parameters might fix this issue and a proper testing methodology should prove the current best approach.

Known limitations

  • AMD card is required to use this capturing mode, regardless of use of AFMF
  • AFMF can't be used with VDD because it Adrenaline doesn't recognize the virtual display and therefore no special features can be used. So a monitor or edid emulator has to be used as the capturing display.

To do:

  • Fix mouse cursor streaming
  • Add screenshot showing new UI option
  • Fix quality check
  • Maybe add a new field for changing Display Capture, capturing mode parameter.

@moi952

This comment was marked as off-topic.

@radugrecu97

This comment was marked as off-topic.

@moi952

This comment was marked as off-topic.

@radugrecu97

This comment was marked as off-topic.

@moi952

This comment was marked as off-topic.

@radugrecu97

This comment was marked as resolved.

Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. This will be a nice addition!

src/platform/windows/display.h Outdated Show resolved Hide resolved
src/platform/windows/display.h Show resolved Hide resolved
Comment on lines 53 to 63
/**
* @brief Get the next frame from the producer thread.
* If not available, the capture thread blocks until one is, or the wait times out.
* @param timeout how long to wait for the next frame
* @param out a texture containing the frame just captured
* @param out_time the timestamp of the frame just captured
*/
Copy link
Member

@ReenigneArcher ReenigneArcher Sep 30, 2024

Choose a reason for hiding this comment

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

Suggested change
/**
* @brief Get the next frame from the producer thread.
* If not available, the capture thread blocks until one is, or the wait times out.
* @param timeout how long to wait for the next frame
* @param out a texture containing the frame just captured
* @param out_time the timestamp of the frame just captured
*/
/**
* @brief Get the next frame from the producer thread.
* If not available, the capture thread blocks until one is, or the wait times out.
* @param timeout how long to wait for the next frame
* @param out a texture containing the frame just captured
* @param out_time the timestamp of the frame just captured
*/

This should also probably be moved to the header (doxygen can complain if it exists in the header and implementation if there is any mismatch).

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what you mean here. I fixed the misaligned comment.

Copy link
Member

Choose a reason for hiding this comment

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

I mean to move the documentation block to the .h file instead of the .cpp file.

@@ -1051,6 +1053,16 @@ namespace platf {
}
}

if (config::video.capture == "amd") {
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
if (config::video.capture == "amd") {
if (config::video.capture == "amd" || config::video.capture.empty()) {

This should allot it to attempt amd capture if the user hasn't explicitly set anything.

What should be our automatic order? The way it is now will try in this order:

  • ddx
  • amd
  • wgc

Copy link
Author

Choose a reason for hiding this comment

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

I moved amd first as AMD users would prefer it over ddx because it supports AFMF.

Only downside right now is the mouse cursor not being captured.

Copy link
Member

Choose a reason for hiding this comment

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

I think it will be fine to be first once the issues like this are ironed out. No mouse capture is a pretty big downside at the moment.

src/platform/windows/display_vram.cpp Outdated Show resolved Hide resolved
src/stream.cpp Outdated Show resolved Hide resolved
src/video.cpp Outdated Show resolved Hide resolved
@HibernalGlow

This comment was marked as off-topic.

* Drain queued frames before termination
* Remove AMD Trace and debug statements
* Add code comments
Copy link

sonarcloud bot commented Oct 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
10 New issues
9.8% Duplication on New Code (required ≤ 2%)
2 Duplicated Blocks on New Code (required ≤ 0)
10 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@radugrecu97
Copy link
Author

radugrecu97 commented Oct 10, 2024

So the latest AMD driver seems to have had a quality regression for AFMF 2 compared to the preview.

I'll test again later but AFMF 2 looked so blurry and jittery on the latest drivers.

@ReenigneArcher ReenigneArcher changed the title AMD Display Capture for AFMF feat(capture): add AMD Display Capture for AFMF Oct 10, 2024
@ReenigneArcher
Copy link
Member

So the latest AMD driver seems to have had a quality regression for AFMF 2 compared to the preview.

I'll test again later but AFMF 2 looked so blurry and jittery when streamed compared to the preview.

Possibly another regression exposed with the ffmpeg 7.1 update?

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 130 lines in your changes missing coverage. Please review.

Project coverage is 7.56%. Comparing base (d378c18) to head (38763bb).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
src/platform/windows/display_amd.cpp 0.00% 82 Missing ⚠️
src/platform/windows/display_vram.cpp 0.00% 38 Missing ⚠️
src/platform/windows/display_base.cpp 0.00% 7 Missing ⚠️
src/platform/windows/display.h 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #3171      +/-   ##
=========================================
- Coverage    9.74%   7.56%   -2.18%     
=========================================
  Files         101     102       +1     
  Lines       17975   18038      +63     
  Branches     8420    8477      +57     
=========================================
- Hits         1751    1364     -387     
- Misses      13330   13858     +528     
+ Partials     2894    2816      -78     
Flag Coverage Δ
Linux 7.27% <ø> (ø)
Windows 0.00% <0.00%> (-5.11%) ⬇️
macOS-13 10.54% <ø> (-0.04%) ⬇️
macOS-14 ?

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

Files with missing lines Coverage Δ
src/video.cpp 26.15% <ø> (-1.03%) ⬇️
src/platform/windows/display.h 0.00% <0.00%> (-7.25%) ⬇️
src/platform/windows/display_base.cpp 0.00% <0.00%> (-13.60%) ⬇️
src/platform/windows/display_vram.cpp 0.00% <0.00%> (-1.85%) ⬇️
src/platform/windows/display_amd.cpp 0.00% <0.00%> (ø)

... and 39 files with indirect coverage changes

@radugrecu97
Copy link
Author

So the latest AMD driver seems to have had a quality regression for AFMF 2 compared to the preview.
I'll test again later but AFMF 2 looked so blurry and jittery when streamed compared to the preview.

Possibly another regression exposed with the ffmpeg 7.1 update?

FFmpeg 7.1 changelog doesn't tell me much, BUT I was surprised that a different Display Capture mode is not displaying frames out of order any more compared to previous experience.

The AMF sdk release does mention that they updated ffmpeg to 7.0, so maybe they didn't test with 7.1.

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.

4 participants