obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

Architecture: adding windows on ARM support

Open thirumalai-qcom opened this issue 1 year ago • 10 comments

Introduces Windows on ARM support to the OBS-studio application. This includes modifications that are guarded with ARM-specific macros to ensure compatibility without affecting the existing codebase.

Description

This pull request introduces several code refactoring changes to enhance WOA support and resolve conflicts in the OBS-Studio application. The key changes include:

  • UI: Added shellapi.h to fix compilation error, Added NOMINMAX flag to solve compilation errors, Ensured WIN32_LEAN_AND_MEAN is defined conditionally if not defined.
  • w32-pthreads: Added additional definitions of ARM macros _M_ARM and _M_ARM64.
  • libobs: updated obs-win-crash-handler.c to handle instruction pointers and stack traces, improving crash reporting on ARM64 systems .
  • obs-filters: Undefined S_THRESHOLD due to a conflicting existing macro with a numeric value.
  • obs-outputs: Replaced __tzcnt_u32() with _CountTrailingZeros() for ARM.

Motivation and Context

The primary motivation for these changes is to expand compatibility with ARM architecture (windows) by addressing conflicts with existing macros and functions. By introducing ARM-specific macros and guarding ARM-specific code, we ensure that the application runs smoothly without affecting the existing codebase.

  • UI Changes:

    • Added shellapi.h in obs-app.cpp to fix compilation errors.
    • Updated min/max values to use limits header for VolumeMeter and VolumeSlider.
    • Ensured WIN32_LEAN_AND_MEAN is included conditionally in window-basic-filters, window-basic-interaction, and window-basic-properties.
    • Updated min/max values in the DrawStripedLine function in window-basic-preview.
    • Updated min/max values for preset setting in window-basic-settings-a11y.cpp.
  • Additional ARM definitions in deps/w32-pthreads/context.h:

    • Update w32-pthreads/context.h to include definitions for _M_ARM and _M_ARM64 in addition to existing ARM definitions.
  • Libobs changes:

    • Adjust obs-win-crash-handler.c to handle instruction pointers & stack traces for crash reporting.
  • Obs-filters changes:

    • Undefines the macro S_THRESHOLD if previously defined, then redefines it with a new value "threshold".
  • Obs-outputs: Use _CountTrailingZeros() in ctz32

    • Replaced tzcnt_u32 with _CountTrailingZeros to ensure compatibility with WOA machines.

How Has This Been Tested?

The changes have been tested by building OBS Studio on an ARM machine (X Elite machine, Win11). All builds were successful, Basic functionality tests confirmed that the OBS Studio application with the modifications is functional.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • [x] My code has been run through clang-format.
  • [x] I have read the contributing document.
  • [x] My code is not on the master branch.
  • [x] The code has been tested.
  • [x] All commit messages are properly formatted and commits squashed where appropriate.
  • [x] I have included updates to all appropriate documentation.

thirumalai-qcom avatar Jul 16 '24 21:07 thirumalai-qcom

Unless you have tested QSV with your arm devices you should drop the proposed changes there and ensure the plugin is not built for arm.

kkartaltepe avatar Jul 17 '24 01:07 kkartaltepe

Please reword the PR as "adding Windows on ARM support", we already support ARM64 on macOS and OBS can be built on Linux aarch64.

Also there is no CI build check which does not help to avoid/find build issue on WoA, which is not good at all since not all contributors will have WoA machines to check if it builds.

Edit: Also, read our contribution guidelines and split your commits.

tytan652 avatar Jul 17 '24 07:07 tytan652

The commit messages need to follow the guideline. https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst#commit-guidelines

norihiro avatar Jul 29 '24 15:07 norihiro

@RytoEX I would like to know if AJA functionality is planned to be included in the Phase-1 release of the arm64 version of OBS. Currently, in AJA the __cpuid method is not supported on ARM devices, Could you provide some insights into what functionality we will miss in OBS on ARM due to this limitation ?

Edit: Identified an alternative way to retrieve the CPU string from the register (as done in libobs/obs-windows.c log_processor_info()). Can this be a replacement for the __cpuid method on ARM devices?

thirumalai-qcom avatar Aug 02 '24 16:08 thirumalai-qcom

@RytoEX I would like to know if AJA functionality is planned to be included in the Phase-1 release of the arm64 version of OBS. Currently, in AJA the __cpuid method is not supported on ARM devices, Could you provide some insights into what functionality we will miss in OBS on ARM due to this limitation ?

Edit: Identified an alternative way to retrieve the CPU string from the register (as done in libobs/obs-windows.c log_processor_info()). Can this be a replacement for the __cpuid method on ARM devices?

Poking @paulh-aja for AJA's current stats on ARM support. (sorry for double ping here, accidentally replied to the wrong comment!)

Fenrirthviti avatar Aug 14 '24 17:08 Fenrirthviti

@RytoEX I would like to know if AJA functionality is planned to be included in the Phase-1 release of the arm64 version of OBS. Currently, in AJA the __cpuid method is not supported on ARM devices, Could you provide some insights into what functionality we will miss in OBS on ARM due to this limitation ? Edit: Identified an alternative way to retrieve the CPU string from the register (as done in libobs/obs-windows.c log_processor_info()). Can this be a replacement for the __cpuid method on ARM devices?

Poking @paulh-aja for AJA's current stats on ARM support. (sorry for double ping here, accidentally replied to the wrong comment!)

My thinking on this would be that if there's an ARM64 equivalent to __cpuid, then it should (probably) be trivial to ifdef appropriately, which should be fine. That said, the linked code is in AJA's SDK, so the changes will have to be made there, not in OBS. It is worth noting that you cannot simply lift existing code from the obs-studio repo to drop into AJA's repo as the licenses, while compatible, are different. The correct way to address this is likely to file an Issue (or PR) on their repo.

It may also be worth noting that the obs-qsv11 plugin also uses __cpuid.

RytoEX avatar Aug 14 '24 17:08 RytoEX

Currently, in AJA the __cpuid method is not supported on ARM devices.

Poking @paulh-aja for AJA's current stats on ARM support.

Please open a PR or at least raise an issue in the libajantv2 repo as RytoEX mentioned.

We do not have Windows ARM support on our current release roadmap. However, if the only thing blocking it is replacing the use of __cpuid in the ajabase code, then it may be trivial to add it in. Our Mac and Linux drivers and SDK do support ARM64 today.

paulh-aja avatar Aug 14 '24 19:08 paulh-aja

Please open a PR or at least raise an issue in the libajantv2 repo as RytoEX mentioned.

@paulh-aja I've raised a PR in the libajantv2 repo to replace the __cpuid in the ajabase code.

thirumalai-qcom avatar Sep 09 '24 08:09 thirumalai-qcom

@thirumalai-qcom I did see your PR in libajantv2. I'll test it out and get it merged into our next release if it all looks good. Thank you for your contribution!

paulh-aja avatar Sep 09 '24 20:09 paulh-aja

The correct way to address this is likely to file an Issue (or PR) on their repo.

@RytoEX As the AJA PR has been successfully merged into the mainline, I was curious to know if there are any updates on this PR merging for the initial release on Windows on ARM (WOA)? Could you also provide your inputs on which fix would be appropriate to resolve the compilation errors related to std::numeric_limits<int>::min() error and S_THRESHOLDmacro redefinition warning?

thirumalai-qcom avatar Sep 27 '24 19:09 thirumalai-qcom

With #11385 merged, please drop the NOMINMAX changes from this PR. Additionally, the CMake changes seem to affect more lines than they should. Please drop those and revert the scope of this PR back to just the remaining code changes necessary to make compiling possible

@RytoEX Thanks for the information regarding NOMINMAX, I have refactored the PR with your suggestions.

thirumalai-qcom avatar Feb 10 '25 17:02 thirumalai-qcom

Please update the commit messages to reflect the current changes.

Thanks for the feedback @RytoEX, I've updated the commit messages and descriptions to reflect the current changes. Let me know if any further adjustments are needed.

thirumalai-qcom avatar Feb 12 '25 07:02 thirumalai-qcom