-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
win-mf: Reintroduce win-mf plugin for Media Foundation-based encoding on WoA #11993
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
base: master
Are you sure you want to change the base?
Conversation
55aa652
to
ea69f55
Compare
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'm wary about just reintroducing the old code as-is, particularly as we have progressed to C++17 and expect modern C++ coding practices, including but not limited to:
- Use of anonymous namespaces instead of
static
functions is deprecated in C++ (static functions are only used for class methods in modern C++). - Use of
constexpr
as it's preferable over macros in almost all situations as they enable the compiler to optimise code better and sometimes resolve entire code paths at compile time - Abuse of macros instead of functions marked as
constexpr
(see above) which makes the code harder to debug
Just merging this would mean re-importing the code smell as well and I just don't see someone else volunteering to "refactor" it after it's been merged again.
plugins/win-mf/CMakeLists.txt
Outdated
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.
This file should be adapted to current CMake guidelines instead of copied-over from the legacy variant.
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.
@PatTheMav
I have Updated the CMakeLists.txt
to meet with the current guidelines. Could you please review the changes and share any suggestions you might have?
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.
The file still requires some revisions, follow the general skeleton of:
cmake_minimum_required(VERSION 3.28...3.30)
find_package(<dependencies) # If any 3rd party dependencies
add_library(<module> MODULE)
add_library(OBS::<module> ALIAS <module>)
target_sources(
<module>
PRIVATE
<source and header files, sorted alphabetically>
PUBLIC # If any public headers
<public source and header files, sorted alphabetically>
)
target_link_libraries(
<module>
PRIVATE <dependencies>
PUBLIC <dependencies>. # If any public dependencies
)
set_target_properties_obs(<module> PROPERTIES FOLDER plugins PREFIX "")
I tested this on my Snapdragon X laptop and it does work as expected. But Ideally this plugin should be either rewritten from the ground up, or replaced wholesale with just using the FFmpeg implemenation. At the very least, the AAC encoder could probably be removed. Of course since this is ancient it also doesn't support AV1 (which the X Elite chips do support). Aside from what Pat mentioned, the naming also conflicts with #10471 right now. |
ea69f55
to
109de95
Compare
Will this work with my older Surface Pro X (SQ2)? If it works I can help test it. |
13c8aac
to
2aa95e6
Compare
@dd-han Thanks for offering to help. |
You are using the Media Foundation encoder via FFmpeg. That is not what is in this PR, and this PR has not been merged, so it is not in OBS Studio 31.1.0 Beta 1. Please use our forums or Discord for assistance. |
You need to pass additional flags to make hardware encoding work with the ffmpeg backend. |
2aa95e6
to
e066af7
Compare
This commit re-introduces the base files required for Media Foundation Transform (MFT) support in the win-mf plugin, specifically targeting Windows on ARM (WoA) devices with Qualcomm hardware. It includes core implementation files, language configuration files for localization, and the initial CMakeLists.txt for integration.
This commit introduces H264 encoder support using the Media Foundation Transform (MFT) interface, specifically targeting Windows on ARM (WoA) devices with Qualcomm hardware. It adds mf-h264.cpp and mf-h264-encoder.cpp/.hpp to implement the encoding logic. Additionally, CMakeLists.txt is updated to include the new source and header files for proper integration into the win-mf plugin.
This commit introduces HEVC encoder support using the Media Foundation Transform (MFT) interface, specifically targeting Windows on ARM (WoA) devices with Qualcomm hardware. It adds mf-hevc.cpp and mf-hevc-encoder.cpp/.hpp to implement the encoding logic. Additionally, CMakeLists.txt is updated to include the new source and header files for proper integration into the win-mf plugin.
This commit introduces AAC encoder support using the Media Foundation Transform (MFT) interface, specifically targeting Windows on ARM (WoA) devices with Qualcomm hardware. It adds mf-aac.cpp and mf-aac-encoder.cpp/.hpp to implement the encoding logic. Additionally, CMakeLists.txt is updated to include the new source and header files for proper integration into the win-mf plugin
This commit updates the CMakeLists.txt file in the plugins directory to incorporate the win-mf plugin into the OBS build system. This change ensures that Media Foundation-based encoding is available for Windows on ARM (WoA) devices, utilizing Qualcomm hardware for efficient video Encoding.
e066af7
to
f7fd3aa
Compare
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.
Left a first round of reviews, will still have to look at the HEVC and H264 encoder implementations.
Except for the missing newline issues I haven't commented on every repeat issue, so things like functions marked inline
in compilation units, opaque variable names, "magic" global variables, should be fixed throughout and not just were I first encountered them in review.
|
||
project(win-mf) | ||
|
||
set( |
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.
Do not use separate _SOURCES
and _HEADERS
variables, call target_sources
with them directly (see also the other CMake build scripts in the project).
@@ -0,0 +1,43 @@ | |||
cmake_minimum_required(VERSION 3.28...3.30) | |||
|
|||
project(win-mf) |
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.
Do not specify a separate project for a plugin.
plugins/win-mf/CMakeLists.txt
Outdated
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.
The file still requires some revisions, follow the general skeleton of:
cmake_minimum_required(VERSION 3.28...3.30)
find_package(<dependencies) # If any 3rd party dependencies
add_library(<module> MODULE)
add_library(OBS::<module> ALIAS <module>)
target_sources(
<module>
PRIVATE
<source and header files, sorted alphabetically>
PUBLIC # If any public headers
<public source and header files, sorted alphabetically>
)
target_link_libraries(
<module>
PRIVATE <dependencies>
PUBLIC <dependencies>. # If any public dependencies
)
set_target_properties_obs(<module> PROPERTIES FOLDER plugins PREFIX "")
*extraData_ = extraData; | ||
*extraDataLength = sizeof(extraData); | ||
return true; | ||
} |
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.
Missing newline at the end of file.
|
||
#include <array> | ||
|
||
inline void LogAAC(const obs_encoder_t *encoder, int level, const char *format, ...) |
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.
It looks like this function (and the next) are not supposed to have external linkage and are only used within this compilation unit, so they should instead both be wrapped in an anonymous namespace and could even be marked as constexpr
:
namespace {
constexpr void LogCOMError(const obs_encoder_t *encoder, const char *operation, HRESULT hr)
{
_com_error err(hr);
LogAAC(encoder, LOG_ERROR, "%s failed, %S (0x%08lx)", operation, err.ErrorMessage(), hr);
}
}
The inline
keyword in most cases should only be used to make it explicit that a function or variable is defined "in-line" with its signature in header files, to allow what would otherwise be a violation of ODR.
Using constexpr
instead allows the compiler to resolve it at compile time (similar to how a macro would be expanded before compilation) if the associated conditions are fulfilled.
const char *id; | ||
EncoderType type; | ||
|
||
} guidNameMap[] = { |
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'd like to think that it would be better for readability and understanding to split the type definition and the initialisation of a new variable with that type into separate statements.
|
||
ComHeapPtr<WCHAR> guidW; | ||
StringFromIID(guid, &guidW); | ||
std::string guidString = MBSToString(guidW); |
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.
Correct me if I'm wrong, but C++ has no built-in conversions from wstring
to string
? IIRC they even deprecated such functions in more recent C++ versions.
bool isHardware; | ||
EncoderType type; | ||
}; | ||
}; // namespace MF |
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.
Missing newline at the end of file.
pendingRequests--; | ||
|
||
return true; | ||
} |
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.
Missing newline at the end of file.
std::atomic<UINT32> outputRequests{0}; | ||
std::atomic<UINT32> pendingRequests{0}; | ||
}; | ||
} // namespace MF |
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.
Missing newline at the end of file.
Couple more notes:
|
Description
This PR re-introduces the win-mf plugin to OBS Studio, enabling Media Foundation-based encoding for H264, HEVC, and AAC. This plugin was previously removed in PR #7372 but is now being restored to support hardware-accelerated encoding on Windows on ARM (WoA) devices, specifically those utilizing Qualcomm hardware encoders. The Key changes include:
Motivation and Context
The primary motivation for these changes is to enable hardware-accelerated encoding on Windows on ARM (WoA) devices by reintroducing the win-mf plugin in OBS Studio. With the growing adoption of WoA devices, ensuring OBS Studio can leverage hardware encoders on this platform improves performance and efficiency, reducing CPU load while maintaining high-quality encoding.
By integrating the win-mf plugin back into the build system, we enable native support for Media Foundation-based encoding on WoA, ensuring users on this platform can leverage hardware acceleration for streaming and recording. This change ensures better utilization of Qualcomm hardware encoders, providing a more efficient and optimized encoding experience.
How Has This Been Tested?
The changes have been tested by building OBS Studio on a Windows on ARM (WoA) device with Qualcomm hardware. The build completed successfully, and basic functionality tests confirmed that OBS Studio runs as expected with the reintroduced win-mf plugin. Media Foundation-based encoding for H264, HEVC, and AAC was verified to function correctly, ensuring proper hardware acceleration on WoA devices utilizing Qualcomm hardware.
Types of changes
Checklist: