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

Move FFmpeg hwaccel checks to CMake, eliminate #pragma messages #645

Merged
merged 5 commits into from Jun 5, 2021

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Mar 15, 2021

This PR renames the HAVE_HW_ACCEL preprocessor symbol to USE_HW_ACCEL, and adds a new CMake build option of the same name (default ON). This allows hardware acceleration to be disabled even when building with FFmpeg 3.4+ (by adding -DUSE_HW_ACCEL=0 to the CMake command line).

In addition, the #pragma messages about hardware acceleration are removed from FFmpeg{Reader,Writer}.cpp and replaced with a CMake FeatureSummary message that accounts for both the value of USE_HW_ACCEL, and whether the FFmpeg version in question supports acceleration.

Normal output (with FFmpeg 3.4+)

$ cmake -B build -S . [...]
-- Displaying feature summary

Build configuration:
-- The following features have been enabled:

 * FFmpeg hwaccel, GPU-accelerated routines (FFmpeg 3.4+)
 * OpenCV algorithms, Use OpenCV algorithms
 * Documentation, Build API documentation with 'make doc'
 * Unit tests, Compile unit tests for library functions
 * Testrunner, Run unit tests with 'make test'

-- The following OPTIONAL packages have been found:

 * ImageMagick
 * cppzmq
 * OpenCV (required version >= 4)
 * Threads
 * PythonInterp (required version >= 3)
 * PythonLibs (required version >= 3)
 * Ruby
 * Doxygen
 * PkgConfig

-- The following RECOMMENDED packages have been found:

 * UnitTest++
   Unit testing framework

-- The following REQUIRED packages have been found:

 * OpenShotAudio (required version >= 0.2.0)
 * jsoncpp
 * Qt5Widgets
 * FFmpeg
 * OpenMP
 * ZeroMQ
 * Protobuf (required version >= 3)
 * Qt5Core (required version >= 5.15.2)
 * Qt5Gui
 * Qt5
 * SWIG (required version >= 3.0)

-- The following features have been disabled:

 * Coverage, analyze test coverage and generate report
 * IWYU (include-what-you-use), Scan all source files with 'iwyu'

-- The following OPTIONAL packages have not been found:

 * Resvg

Manually disabled

$ cmake -B build -S . -DUSE_HW_ACCEL=0 [...]
-- Displaying feature summary

Build configuration:
-- The following features have been enabled:

 * OpenCV algorithms, Use OpenCV algorithms
 * Documentation, Build API documentation with 'make doc'
 * Unit tests, Compile unit tests for library functions
 * Testrunner, Run unit tests with 'make test'

-- The following OPTIONAL packages have been found:

 * ImageMagick
 * cppzmq
 * OpenCV (required version >= 4)
 * Threads
 * PythonInterp (required version >= 3)
 * PythonLibs (required version >= 3)
 * Ruby
 * Doxygen
 * PkgConfig

-- The following RECOMMENDED packages have been found:

 * UnitTest++
   Unit testing framework

-- The following REQUIRED packages have been found:

 * OpenShotAudio (required version >= 0.2.0)
 * jsoncpp
 * Qt5Widgets
 * FFmpeg
 * OpenMP
 * ZeroMQ
 * Protobuf (required version >= 3)
 * Qt5Core (required version >= 5.15.2)
 * Qt5Gui
 * Qt5
 * SWIG (required version >= 3.0)

-- The following features have been disabled:

 * Coverage, analyze test coverage and generate report
 * IWYU (include-what-you-use), Scan all source files with 'iwyu'
 * FFmpeg hwaccel, GPU-accelerated routines (FFmpeg 3.4+)

-- The following OPTIONAL packages have not been found:

 * Resvg

As a fallback, if USE_HW_ACCEL is not defined in the generated compile commands at all (which it will not be, if USE_HWACCEL is TRUE, but the FFmpeg version cannot be confirmed to support acceleration), then FFmpegUtilities.h will set USE_HW_ACCEL using the previous version-detection code.

@ferdnyc ferdnyc added hardware Issues involving hardware detection or de/encoding code Source code cleanup, streamlining, or style tweaks build Issues related to compiling or installing libopenshot and its dependencies labels Mar 15, 2021
@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #645 (e6d4ab3) into develop (691fc83) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #645   +/-   ##
========================================
  Coverage    51.53%   51.53%           
========================================
  Files          151      151           
  Lines        12231    12231           
========================================
  Hits          6303     6303           
  Misses        5928     5928           
Impacted Files Coverage Δ
src/FFmpegReader.cpp 68.35% <ø> (ø)
src/FFmpegReader.h 66.66% <ø> (ø)
src/FFmpegUtilities.h 100.00% <ø> (ø)
src/FFmpegWriter.cpp 59.71% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 691fc83...e6d4ab3. Read the comment docs.

@github-actions github-actions bot added the conflicts A PR with unresolved merge conflicts label Apr 9, 2021
@github-actions
Copy link

github-actions bot commented Apr 9, 2021

Merge conflicts have been detected on this PR, please resolve.

@ferdnyc ferdnyc removed the conflicts A PR with unresolved merge conflicts label Apr 17, 2021
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 5, 2021

I don't see a down side to this, and hearing no objections...

@ferdnyc ferdnyc merged commit 8a81452 into OpenShot:develop Jun 5, 2021
@ferdnyc ferdnyc deleted the move-hwaccel-checks branch June 5, 2021 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to compiling or installing libopenshot and its dependencies code Source code cleanup, streamlining, or style tweaks hardware Issues involving hardware detection or de/encoding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant