-
-
Notifications
You must be signed in to change notification settings - Fork 960
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
base: master
Are you sure you want to change the base?
feat(capture): add AMD Display Capture for AFMF #3171
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this 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_amd.cpp
Outdated
/** | ||
* @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 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* @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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment was marked as off-topic.
This comment was marked as off-topic.
cbe742c
to
708db84
Compare
* Drain queued frames before termination
* Remove AMD Trace and debug statements * Add code comments
708db84
to
38763bb
Compare
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
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. |
Possibly another regression exposed with the ffmpeg 7.1 update? |
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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. |
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
.github/...
)Checklist
Known bugs
Known limitations
To do: