Change the build tool from qmake to cmake #36
Conversation
|
Note No issue was found based on Codety's default factory settings! 🎉 🎉 |
WalkthroughThis pull request introduces a comprehensive refactoring of the OpenConverter project, focusing on restructuring the project's directory layout, improving header file organization, and enhancing the transcoding architecture. The changes include modifying include paths, updating class inheritance structures, introducing a new base Changes
Sequence DiagramsequenceDiagram
participant Main as Main Application
participant Converter as Converter
participant Transcoder as Base Transcoder
participant TranscoderImpl as Transcoder Implementation
Main Application->>Converter: Create with ProcessParameter, EncodeParameter
Converter->>Transcoder: Initialize with parameters
Transcoder->>TranscoderImpl: Inherit base class
Main Application->>Converter: Call convert_Format()
Converter->>TranscoderImpl: Invoke transcode()
TranscoderImpl-->>Converter: Return transcoding result
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/transcoder/src/transcoder.cpp (1)
235-236: Validate codec string usage
Switching tostd::stringand callingc_str()is safer than using raw pointers. Consider handling an empty or invalid codec name more explicitly (e.g., logging a warning or returning an error code) ifcodec.c_str()is empty or invalid.src/CMakeLists.txt (3)
1-3: Appropriate minimum CMake version
Using3.5is fine for cross-platform compatibility, though consider a more recent version for modern CMake features.
79-100: Qt major version logic
The conditional approach is fine. For a large codebase or multiple targets, consider separate CMake scripts or continuous integration that tests both Qt5 and Qt6 build paths.
107-130: Extensive linking
You link many libraries manually. For better maintainability, consider grouping or searching for them with CMake’s find modules (e.g.find_package(FFmpeg REQUIRED)if available).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)src/CMakeLists.txt(1 hunks)src/common/src/process_parameter.cpp(1 hunks)src/engine/include/converter.h(1 hunks)src/open_converter.pro(0 hunks)src/transcoder/src/transcoder.cpp(1 hunks)src/transcoder/src/transcoder_bmf.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/open_converter.pro
✅ Files skipped from review due to trivial changes (2)
- src/engine/include/converter.h
- src/common/src/process_parameter.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/transcoder/src/transcoder_bmf.cpp (1)
155-155: Good addition of return value
By returningtrue, you convey success without forcing the caller to infer it from side effects..gitignore (1)
2-6: Appropriate project housekeeping
Ignoringbuild-*and.DS_Storeis standard practice. Includingsrc/CMakeLists.txt.userhelps avoid committing user-specific CMake settings.src/CMakeLists.txt (2)
12-13: Robust Qt detection
Your approach handles both Qt5 and Qt6. This is good for flexibility; just be sure to test each environment to confirm no hidden issues.
143-147: Cross-platform installation
Your install directives place executables and libraries under typical sections. Ensure that other platforms (e.g., Windows) can gracefully handle these paths.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/CMakeLists.txt (4)
1-1: Consider updating the minimum CMake version requirement.CMake 3.5 is quite old (released in 2016). Modern Qt features and better CMake practices are available in newer versions. Consider updating to at least CMake 3.16 for better Qt6 support and modern CMake features.
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.16)
18-19: Specify version requirements for FFmpeg libraries.Consider adding minimum version requirements for FFmpeg libraries to ensure compatibility:
-pkg_check_modules(FFMPEG REQUIRED libavcodec libavformat libavutil) +pkg_check_modules(FFMPEG REQUIRED + libavcodec>=58.0.0 + libavformat>=58.0.0 + libavutil>=56.0.0 +)
85-89: Remove commented Qt wrapper commands.These commands are no longer needed as they are handled by
CMAKE_AUTOUIC,CMAKE_AUTOMOC, andCMAKE_AUTORCCwhich are already enabled.Remove these commented lines as they may confuse future maintainers.
114-128: Clean up library linking configuration.
- Remove commented FFmpeg libraries as they're already included in
${FFMPEG_LDFLAGS}.- Consider adding version requirements for BMF libraries.
target_link_libraries(OpenConverter PRIVATE Qt${QT_VERSION_MAJOR}::Widgets ${FFMPEG_LDFLAGS} -# avformat -# avcodec -# avutil -# pthread -# swresample engine bmf_module_sdk hmp builtin_modules )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/CMakeLists.txt(1 hunks)src/builder/include/encode_setting.h(1 hunks)src/builder/include/open_converter.h(1 hunks)src/builder/src/encode_setting.cpp(1 hunks)src/builder/src/open_converter.cpp(1 hunks)src/common/src/encode_parameter.cpp(1 hunks)src/common/src/info.cpp(1 hunks)src/common/src/process_parameter.cpp(2 hunks)src/common/src/stream_context.cpp(1 hunks)src/engine/include/converter.h(2 hunks)src/engine/src/converter.cpp(1 hunks)src/main.cpp(2 hunks)src/transcoder/include/transcoder.h(1 hunks)src/transcoder/include/transcoder_bmf.h(1 hunks)src/transcoder/src/transcoder.cpp(2 hunks)src/transcoder/src/transcoder_bmf.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (11)
- src/builder/src/encode_setting.cpp
- src/transcoder/include/transcoder.h
- src/common/src/encode_parameter.cpp
- src/main.cpp
- src/builder/include/open_converter.h
- src/builder/include/encode_setting.h
- src/engine/src/converter.cpp
- src/common/src/stream_context.cpp
- src/transcoder/include/transcoder_bmf.h
- src/common/src/info.cpp
- src/builder/src/open_converter.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/engine/include/converter.h
- src/common/src/process_parameter.cpp
- src/transcoder/src/transcoder.cpp
- src/transcoder/src/transcoder_bmf.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/CMakeLists.txt (2)
21-25: Remove or implement the commented x264/x265 configurations.The commented-out x264/x265 configurations should either be removed if not needed or properly implemented if required for the project.
Please clarify if x264/x265 support is planned for future implementation. If not, we should remove these comments.
27-34: Replace hard-coded BMF path with a more portable solution.The current default BMF path is user-specific and won't work across different systems.
Consider these alternatives:
- Use CMake's
find_packagewith a proper Find module for BMF- Allow configuration through CMake cache variables
- Require the environment variable and fail if not set
Example implementation:
if(NOT DEFINED ENV{BMF_ROOT_PATH}) message(FATAL_ERROR "BMF_ROOT_PATH environment variable must be set") endif() set(BMF_ROOT_PATH $ENV{BMF_ROOT_PATH})
| add_library(OpenConverter SHARED | ||
| ${PROJECT_SOURCES} | ||
| ) |
There was a problem hiding this comment.
Fix undefined ${PROJECT_SOURCES} variable.
The ${PROJECT_SOURCES} variable is used but never defined. This will result in empty targets.
- ${PROJECT_SOURCES}
+ ${SOURCES}
+ ${HEADERS}
+ ${UI_FILES}
+ ${RESOURCE_FILES}Also applies to: 108-110
| install(TARGETS OpenConverter | ||
| BUNDLE DESTINATION . | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
| RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include GNUInstallDirs for installation paths.
The CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR variables are undefined. Include GNUInstallDirs module to define these.
Add at the beginning of the file:
include(GNUInstallDirs)| ) | ||
|
|
||
| # Set translation files | ||
| set(TRANSLATIONS ${CMAKE_SOURCE_DIR}/resources/lang_chinese.ts) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CMAKE_CURRENT_SOURCE_DIR for translation file path.
Similar to earlier paths, use CMAKE_CURRENT_SOURCE_DIR for better relocatability.
-set(TRANSLATIONS ${CMAKE_SOURCE_DIR}/resources/lang_chinese.ts)
+set(TRANSLATIONS ${CMAKE_CURRENT_SOURCE_DIR}/resources/lang_chinese.ts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(TRANSLATIONS ${CMAKE_SOURCE_DIR}/resources/lang_chinese.ts) | |
| set(TRANSLATIONS ${CMAKE_CURRENT_SOURCE_DIR}/resources/lang_chinese.ts) |
src/CMakeLists.txt
Outdated
| # Add source files | ||
| set(SOURCES | ||
| ${CMAKE_SOURCE_DIR}/main.cpp | ||
| ${CMAKE_SOURCE_DIR}/builder/src/encode_setting.cpp | ||
| ${CMAKE_SOURCE_DIR}/builder/src/open_converter.cpp | ||
| ${CMAKE_SOURCE_DIR}/common/src/encode_parameter.cpp | ||
| ${CMAKE_SOURCE_DIR}/common/src/info.cpp | ||
| ${CMAKE_SOURCE_DIR}/common/src/process_parameter.cpp | ||
| ${CMAKE_SOURCE_DIR}/common/src/stream_context.cpp | ||
| ${CMAKE_SOURCE_DIR}/engine/src/converter.cpp | ||
| ${CMAKE_SOURCE_DIR}/transcoder/src/transcoder.cpp | ||
| ${CMAKE_SOURCE_DIR}/transcoder/src/transcoder_bmf.cpp | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ${CMAKE_CURRENT_SOURCE_DIR} instead of ${CMAKE_SOURCE_DIR}.
Using ${CMAKE_SOURCE_DIR} makes the project non-relocatable as a subdirectory. Use ${CMAKE_CURRENT_SOURCE_DIR} instead.
set(SOURCES
- ${CMAKE_SOURCE_DIR}/main.cpp
+ ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp
- ${CMAKE_SOURCE_DIR}/builder/src/encode_setting.cpp
+ ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/encode_setting.cpp
# ... apply similar changes to other paths
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Add source files | |
| set(SOURCES | |
| ${CMAKE_SOURCE_DIR}/main.cpp | |
| ${CMAKE_SOURCE_DIR}/builder/src/encode_setting.cpp | |
| ${CMAKE_SOURCE_DIR}/builder/src/open_converter.cpp | |
| ${CMAKE_SOURCE_DIR}/common/src/encode_parameter.cpp | |
| ${CMAKE_SOURCE_DIR}/common/src/info.cpp | |
| ${CMAKE_SOURCE_DIR}/common/src/process_parameter.cpp | |
| ${CMAKE_SOURCE_DIR}/common/src/stream_context.cpp | |
| ${CMAKE_SOURCE_DIR}/engine/src/converter.cpp | |
| ${CMAKE_SOURCE_DIR}/transcoder/src/transcoder.cpp | |
| ${CMAKE_SOURCE_DIR}/transcoder/src/transcoder_bmf.cpp | |
| ) | |
| # Add source files | |
| set(SOURCES | |
| ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/encode_setting.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/open_converter.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/common/src/encode_parameter.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/common/src/info.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/common/src/process_parameter.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/common/src/stream_context.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/engine/src/converter.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/transcoder/src/transcoder.cpp | |
| ${CMAKE_CURRENT_SOURCE_DIR}/transcoder/src/transcoder_bmf.cpp | |
| ) |
| link_directories( | ||
| ${FFMPEG_LIBRARIES} | ||
| ${BMF_ROOT_PATH}/lib | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix incorrect FFMPEG variable usage and avoid link_directories.
There are two issues here:
${FFMPEG_LIBRARIES}contains library names, not paths. Use${FFMPEG_LIBRARY_DIRS}for paths.link_directoriesis discouraged in modern CMake. Use full paths in target_link_libraries instead.
-link_directories(
- ${FFMPEG_LIBRARIES}
- ${BMF_ROOT_PATH}/lib
-)Committable suggestion skipped: line range outside the PR's diff.
src/CMakeLists.txt
Outdated
| # Add include directories for external libraries | ||
| include_directories( | ||
| ${FFMPEG_INCLUDE_DIRS} | ||
| ${BMF_ROOT_PATH}/include | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use target_include_directories instead of global include_directories.
Global include directories are discouraged in modern CMake. Use target-specific includes instead.
-include_directories(
- ${FFMPEG_INCLUDE_DIRS}
- ${BMF_ROOT_PATH}/include
-)
+target_include_directories(OpenConverter PRIVATE
+ ${FFMPEG_INCLUDE_DIRS}
+ ${BMF_ROOT_PATH}/include
+)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/CMakeLists.txt (4)
1-1: Consider using a more recent CMake version.CMake 3.5 is quite old (2016). Modern CMake versions (>=3.16) provide better features for Qt projects, improved dependency handling, and better error messages.
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.16)
86-89: Remove commented Qt build steps.These manual MOC/UIC/RCC steps are unnecessary as they're handled automatically by CMAKE_AUTOMOC, CMAKE_AUTOUIC, and CMAKE_AUTORCC.
-# Automatically handle UI files using UIC -#qt_wrap_ui(UI_HEADERS ${UI_FILES}) -# qt_add_resources(RESOURCE_OBJECTS ${RESOURCE_FILES}) -# qt_wrap_cpp(MOC_HEADERS ${HEADERS})
97-99: Document or remove Android configuration.The commented Android configuration should either be documented if planned for future use or removed if not needed.
Also applies to: 105-106
117-122: Remove redundant FFmpeg library comments.These commented-out FFmpeg libraries are redundant as pkg-config (FFMPEG_LDFLAGS) already handles the linking.
-# avformat -# avcodec -# avutil -# pthread -# swresample
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
src/CMakeLists.txt(1 hunks) -
src/engine/src/converter.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/src/converter.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
src/CMakeLists.txt (6)
12-13: LGTM! Proper Qt version handling.The code correctly handles both Qt5 and Qt6 compatibility.
36-40: Use target_include_directories instead of global include_directories.Global include directories are discouraged in modern CMake. Use target-specific includes instead.
42-45: Fix incorrect FFMPEG variable usage and avoid link_directories.The FFMPEG_LIBRARIES variable contains library names, not paths, and link_directories is discouraged in modern CMake.
47-83: Use ${CMAKE_CURRENT_SOURCE_DIR} instead of ${CMAKE_SOURCE_DIR}.Using CMAKE_SOURCE_DIR makes the project non-relocatable as a subdirectory.
130-130: Use CMAKE_CURRENT_SOURCE_DIR for translation file path.Similar to earlier paths, use CMAKE_CURRENT_SOURCE_DIR for better relocatability.
140-144: Include GNUInstallDirs for installation paths.The CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR variables are undefined.
src/CMakeLists.txt
Outdated
| else() | ||
| message(STATUS "BMF_ROOT_PATH environment variable is not set, using default paths.") | ||
| # You can set a default path here if you want | ||
| set(BMF_ROOT_PATH "/Users/jacklau/Documents/Programs/Git/Github/bmf/output/bmf") |
There was a problem hiding this comment.
Remove hard-coded BMF path.
The default BMF path is system-specific and won't work across different environments.
- set(BMF_ROOT_PATH "/Users/jacklau/Documents/Programs/Git/Github/bmf/output/bmf")
+ message(FATAL_ERROR "BMF_ROOT_PATH environment variable must be set")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(BMF_ROOT_PATH "/Users/jacklau/Documents/Programs/Git/Github/bmf/output/bmf") | |
| message(FATAL_ERROR "BMF_ROOT_PATH environment variable must be set") |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
.github/workflows/cd.yaml (1)
78-82: Optimize artifact upload configuration.The artifact upload can be improved with:
- Latest action version
- Retention period
- Compression for faster uploads
Apply this configuration:
- name: Upload artifacts uses: actions/upload-artifact@v4 with: name: qt-build-artifacts path: src/build/ retention-days: 7 compression-level: 9🧰 Tools
🪛 actionlint (1.7.4)
79-79: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/cd.yaml(1 hunks).github/workflows/ci.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd.yaml
6-6: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
9-9: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
44-44: shellcheck reported issue in this script: SC2046:warning:3:8: Quote this to prevent word splitting
(shellcheck)
50-50: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
79-79: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/cd.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 33-33: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
.github/workflows/cd.yaml
Outdated
| git clone https://github.com/JackLau1222/bmf.git | ||
| cd bmf | ||
| git checkout fork_by_oc | ||
| # install ffmpeg 5.1.6 | ||
| sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus | ||
| ./build.sh | ||
| export BMF_ROOT_PATH=/bmf/output/bmf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve BMF repository configuration maintainability.
Several issues need attention:
- Hardcoded repository URL and branch name create maintenance overhead
- The
BMF_ROOT_PATHexport won't persist across steps - Absolute path
/bmf/output/bmfmight cause issues
Consider these improvements:
- Use repository inputs/variables for flexibility
- Set environment variables at the workflow level
- Use relative paths with the GitHub workspace
Example:
env:
BMF_REPO: ${{ vars.BMF_REPOSITORY_URL }}
BMF_BRANCH: ${{ vars.BMF_BRANCH }}
BMF_ROOT_PATH: ${{ github.workspace }}/bmf/output/bmf
steps:
- name: Checkout BMF repository
run: |
git clone $BMF_REPO
cd bmf
git checkout $BMF_BRANCH🧰 Tools
🪛 yamllint (1.35.1)
[error] 33-33: trailing spaces
(trailing-spaces)
.github/workflows/cd.yaml
Outdated
| - name: Build with CMake | ||
| run: | | ||
| cd src | ||
| cmake -B build | ||
| make -j$(nproc) | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve build configuration and performance.
Several improvements can be made to the build step:
- Quote shell expansions
- Specify CMake build type
- Add dependency caching
Consider this improved configuration:
- name: Cache Qt dependencies
uses: actions/cache@v3
with:
path: |
~/.cache/pip
~/.qt
key: ${{ runner.os }}-qt-${{ hashFiles('**/CMakeLists.txt') }}
- name: Build with CMake
run: |
cd src
cmake -B build -DCMAKE_BUILD_TYPE=Release
make -j"$(nproc)"🧰 Tools
🪛 actionlint (1.7.4)
44-44: shellcheck reported issue in this script: SC2046:warning:3:8: Quote this to prevent word splitting
(shellcheck)
.github/workflows/cd.yaml
Outdated
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | ||
| tar xf patchelf-0.9.tar.bz2 | ||
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | ||
| # appimage | ||
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | ||
| sudo chmod a+x /usr/local/bin/appimagetool |
There was a problem hiding this comment.
Secure tool downloads and installations.
The current setup downloads tools without verifying checksums or using secure URLs:
- patchelf download uses HTTP instead of HTTPS
- No checksum verification for downloaded artifacts
Implement secure downloads:
- name: Install tools
run: |
PATCHELF_SHA256="6699f0e9373340b5f582f8d9fb453e3838c31a9d3c2f8d16634b8677c7a0b4d9"
wget -q https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c
# Continue with installation if checksum verification passes
.github/workflows/cd.yaml
Outdated
| - name: Copy runtime | ||
| run: | | ||
| mkdir /src/build/lib | ||
| cp /usr/local/lib/libswscale.so.6 /src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 /src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 /src/build/lib | ||
| cp /bmf/output/bmf/lib/libbuiltin_modules.so /src/build/lib | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve library management.
The current approach of manually copying libraries is error-prone and hard to maintain:
- Hardcoded library versions
- Absolute paths
- No verification of library existence
Consider using a manifest file to manage library dependencies and versions:
- name: Copy runtime libraries
run: |
mkdir -p "${GITHUB_WORKSPACE}/src/build/lib"
while IFS= read -r lib; do
cp --no-dereference "$lib" "${GITHUB_WORKSPACE}/src/build/lib/"
done < runtime-libs.txtCreate a runtime-libs.txt file to list required libraries and their locations.
.github/workflows/cd.yaml
Outdated
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions/checkout to the latest version.
The current version might have known vulnerabilities or compatibility issues.
Apply this diff:
- uses: actions/checkout@v3
+ uses: actions/checkout@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout code | |
| uses: actions/checkout@v3 | |
| - name: Checkout code | |
| uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/cd.yaml (1)
78-82: Update actions/upload-artifact to v4.The current version of actions/upload-artifact is outdated.
- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4🧰 Tools
🪛 actionlint (1.7.4)
79-79: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd.yaml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
44-44: shellcheck reported issue in this script: SC2046:warning:3:8: Quote this to prevent word splitting
(shellcheck)
50-50: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
79-79: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/cd.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 33-33: trailing spaces
(trailing-spaces)
🪛 GitHub Check: Codacy Static Code Analysis
.github/workflows/cd.yaml
[warning] 20-20: .github/workflows/cd.yaml#L20
An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
.github/workflows/cd.yaml (6)
16-17: Update actions/checkout to v4.The current version of actions/checkout is outdated.
- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
19-22: Remove tmate debugging session from production workflow.The tmate debugging session with sudo access poses a security risk in production.
Remove these lines from the production workflow.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 20-20: .github/workflows/cd.yaml#L20
An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release.
43-48: Improve build configuration and performance.The build step needs several improvements:
- Missing CMake build type
- Unquoted shell expansion
- No dependency caching
Apply these improvements:
- name: Cache Qt dependencies uses: actions/cache@v3 with: path: | ~/.cache/pip ~/.qt key: ${{ runner.os }}-qt-${{ hashFiles('**/CMakeLists.txt') }} - name: Build with CMake run: | cd src cmake -B build -DCMAKE_BUILD_TYPE=Release make -j"$(nproc)"🧰 Tools
🪛 actionlint (1.7.4)
44-44: shellcheck reported issue in this script: SC2046:warning:3:8: Quote this to prevent word splitting
(shellcheck)
49-69: Secure tool downloads and installations.Current setup has several security issues:
- Insecure HTTP downloads
- No checksum verification
- Hardcoded paths
Implement secure downloads with checksum verification:
- name: Install deployment tools run: | # linuxdeployqt git clone --depth 1 https://github.com/probonopd/linuxdeployqt.git cd linuxdeployqt qmake make -j"$(nproc)" sudo make install # patchelf PATCHELF_SHA256="6699f0e9373340b5f582f8d9fb453e3838c31a9d3c2f8d16634b8677c7a0b4d9" wget -q https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c tar xf patchelf-0.9.tar.bz2 cd patchelf-0.9/ ./configure && make -j"$(nproc)" && sudo make install # appimagetool APPIMAGETOOL_SHA256="$(wget -q https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage.sha256 -O - | cut -d' ' -f1)" wget -q "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O appimagetool echo "$APPIMAGETOOL_SHA256 appimagetool" | sha256sum -c chmod +x appimagetool sudo mv appimagetool /usr/local/bin/🧰 Tools
🪛 actionlint (1.7.4)
50-50: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
70-77: Improve library management.The current approach of manually copying libraries is error-prone and hard to maintain.
Create a
runtime-libs.txtfile:/usr/local/lib/libswscale.so.6 /usr/local/lib/libavfilter.so.8 /usr/local/lib/libpostproc.so.56 ${BMF_ROOT_PATH}/lib/libbuiltin_modules.soThen update the copy step:
- name: Copy runtime libraries run: | mkdir -p "${GITHUB_WORKSPACE}/src/build/lib" while IFS= read -r lib; do lib_path=$(eval echo "$lib") if [ ! -f "$lib_path" ]; then echo "Error: Library $lib_path not found" exit 1 fi cp --no-dereference "$lib_path" "${GITHUB_WORKSPACE}/src/build/lib/" done < runtime-libs.txt
24-38: 🛠️ Refactor suggestion
⚠️ Potential issueImprove BMF repository configuration and dependency management.
Several critical issues need attention:
- Hardcoded repository URL and branch name
- Non-persistent environment variable
- Commented out Python dependencies without explanation
Apply these improvements:
+env: + BMF_ROOT_PATH: ${{ github.workspace }}/bmf/output/bmf + BMF_REPO: ${{ vars.BMF_REPOSITORY_URL }} + BMF_BRANCH: ${{ vars.BMF_BRANCH }} + steps: - name: Checkout BMF repository run: | sudo apt update sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev - # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa + sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa - git clone https://github.com/JackLau1222/bmf.git + git clone $BMF_REPO cd bmf - git checkout fork_by_oc + git checkout $BMF_BRANCH sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus ./build.sh - export BMF_ROOT_PATH=/bmf/output/bmfLikely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 33-33: trailing spaces
(trailing-spaces)
.github/workflows/cd.yaml
Outdated
| on: | ||
| push: | ||
| branches: | ||
| - '**' | ||
| pull_request: | ||
| branches: | ||
| - '**' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restrict CD workflow triggers for production deployment.
The current configuration runs the CD workflow on all branches. For production deployments, it's recommended to restrict the workflow to specific branches (e.g., main/master, release/*).
on:
push:
branches:
- - '**'
+ - main
+ - release/*
pull_request:
branches:
- - '**'
+ - main
+ - release/*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on: | |
| push: | |
| branches: | |
| - '**' | |
| pull_request: | |
| branches: | |
| - '**' | |
| on: | |
| push: | |
| branches: | |
| - main | |
| - release/* | |
| pull_request: | |
| branches: | |
| - main | |
| - release/* | |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/cd.yaml (2)
57-59: Improve build configuration.The build step needs improvements for better reliability and performance:
- Quote command substitution
- Specify CMake build type
- Consider adding dependency caching
- (cd src && cmake -B build && cd build && make -j$(nproc)) + cd src + cmake -B build -DCMAKE_BUILD_TYPE=Release + cmake --build build --parallel "$(nproc)"🧰 Tools
🪛 actionlint (1.7.4)
58-58: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
96-100: Update actions/upload-artifact to the latest version.The current version might have known vulnerabilities or compatibility issues.
- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4🧰 Tools
🪛 actionlint (1.7.4)
97-97: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd.yaml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
20-20: shellcheck reported issue in this script: SC2155:warning:12:8: Declare and assign separately to avoid masking return values
(shellcheck)
20-20: shellcheck reported issue in this script: SC2155:warning:13:8: Declare and assign separately to avoid masking return values
(shellcheck)
20-20: shellcheck reported issue in this script: SC2155:warning:14:8: Declare and assign separately to avoid masking return values
(shellcheck)
20-20: shellcheck reported issue in this script: SC2155:warning:15:8: Declare and assign separately to avoid masking return values
(shellcheck)
58-58: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
68-68: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
97-97: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/cd.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 51-51: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
🔇 Additional comments (6)
.github/workflows/cd.yaml (6)
3-9: Restrict CD workflow triggers for production deployment.The current configuration runs CD workflow on all PR branches. For production deployments, it's recommended to restrict the workflow to specific branches.
16-17: Update actions/checkout to the latest version.The current version might have known vulnerabilities or compatibility issues.
🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
61-65: Remove tmate debugging session from production workflow.The tmate debugging session with sudo access poses a security risk in production.
79-84: Secure tool downloads and installations.Downloads should use HTTPS and verify checksums.
88-95: Improve library management.The current approach of manually copying libraries is error-prone and hard to maintain.
26-27: 🛠️ Refactor suggestionUse repository configuration from workflow variables.
Hardcoding repository URL and branch name creates maintenance overhead. Use repository inputs/variables for flexibility.
- git clone https://github.com/JackLau1222/bmf.git - (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) + git clone ${BMF_REPO:-https://github.com/JackLau1222/bmf.git} + (cd bmf && git checkout ${BMF_BRANCH:-fork_by_oc} && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/cd.yaml (3)
13-13: Consider upgrading to Ubuntu 22.04.Ubuntu 20.04 is approaching its end of life. It's recommended to upgrade to Ubuntu 22.04 for long-term support and security updates.
- runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04🧰 Tools
🪛 yamllint (1.35.1)
[error] 13-13: trailing spaces
(trailing-spaces)
21-23: Improve package installation reliability.The package installation commands should check for errors and use version pinning for consistency.
- sudo apt update - sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 - sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev + sudo apt-get update && sudo apt-get upgrade -y + DEBIAN_FRONTEND=noninteractive sudo apt-get install -y \ + make=4.2.1* \ + git=1:2.25.1* \ + pkg-config=0.29.1* \ + # ... (other packages with versions) + || exit 1
95-97: Update actions/upload-artifact to the latest version.The current version of the action is outdated.
- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4🧰 Tools
🪛 actionlint (1.7.4)
96-96: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd.yaml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
20-20: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
57-57: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
96-96: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/cd.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
.github/workflows/cd.yaml (7)
3-9: Restrict CD workflow triggers for production deployment.The current configuration runs the CD workflow on all branches. For production deployments, it's recommended to restrict the workflow to specific branches.
16-17: Update actions/checkout to the latest version.The current version might have known vulnerabilities or compatibility issues.
🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
26-27: Improve BMF repository configuration maintainability.The hardcoded repository URL and branch create maintenance overhead.
56-58: Improve build configuration and performance.The build step needs improvements in error handling and configuration.
🧰 Tools
🪛 actionlint (1.7.4)
57-57: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
60-64: Remove tmate debugging session from production workflow.The tmate debugging session with sudo access poses a security risk in production.
78-83: Secure tool downloads and installations.The current setup downloads tools without verifying checksums or using secure URLs.
87-94: Improve library management.The current approach of manually copying libraries is error-prone and hard to maintain.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/cd.yaml (1)
75-80:⚠️ Potential issueSecure tool downloads and installations.
The current setup downloads tools without verifying checksums or using secure URLs. Apply this diff:
- wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 + PATCHELF_SHA256="6699f0e9373340b5f582f8d9fb453e3838c31a9d3c2f8d16634b8677c7a0b4d9" + wget -q https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 + echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c
🧹 Nitpick comments (1)
.github/workflows/cd.yaml (1)
95-98: Centralize version management.Version numbers are hardcoded in multiple places, making maintenance difficult.
- Add version to workflow level:
env: APP_VERSION: "1.3"
- Apply this diff:
- VERSION="1.3" + VERSION="${APP_VERSION}" BUILD_DIR="src/build" PACKAGE_NAME="OpenConverter_v${VERSION}_Linux_x86.tar.gz"
- Update release creation:
- VERSION="1.3" + VERSION="${APP_VERSION}" PACKAGE_NAME="OpenConverter_v${VERSION}_Linux_x86.tar.gz"Also applies to: 113-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd.yaml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
20-20: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
57-57: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
102-102: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/cd.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
.github/workflows/cd.yaml (4)
3-9: Restrict CD workflow triggers for production deployment.Running CD workflows on all PR branches is not recommended for production deployments.
Apply this diff to restrict the workflow:
on: push: branches: - 'main' pull_request: branches: - - '**' + - 'main' + - 'release/*'
16-17: Update actions/checkout to the latest version.Apply this diff:
- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
60-64: Remove tmate debugging session from production workflow.The tmate debugging session with sudo access poses a security risk in production. Remove these commented lines from the production workflow.
56-59: 🛠️ Refactor suggestionImprove build configuration and performance.
The build step can be improved for better reliability and performance.
Apply this diff:
- (cd src && cmake -B build && cd build && make -j$(nproc)) + cd src + cmake -B build \ + -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_INSTALL_PREFIX=/usr + cmake --build build --parallel "$(nproc)"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
57-57: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 59-59: trailing spaces
(trailing-spaces)
.github/workflows/cd.yaml
Outdated
| git clone https://github.com/JackLau1222/bmf.git | ||
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) |
There was a problem hiding this comment.
Improve BMF repository configuration security and maintainability.
- Hardcoded repository URL and branch create maintenance overhead
- No verification of the repository's authenticity
Apply this diff:
- git clone https://github.com/JackLau1222/bmf.git
- (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
+ git clone --depth 1 --branch "$BMF_BRANCH" "$BMF_REPO"
+ cd bmf
+ git verify-commit HEAD
+ sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus
+ ./build.shAlso, add these environment variables at the workflow level:
env:
BMF_REPO: ${{ vars.BMF_REPOSITORY_URL }}
BMF_BRANCH: ${{ vars.BMF_BRANCH }}| # mkdir /src/build/lib | ||
| cp /usr/local/lib/libswscale.so.6 src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | ||
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve library management.
The current approach of manually copying libraries is error-prone. Create a manifest file to manage dependencies:
- Create
runtime-libs.txt:
/usr/local/lib/libswscale.so.6
/usr/local/lib/libavfilter.so.8
/usr/local/lib/libpostproc.so.56
bmf/output/bmf/lib/libbuiltin_modules.so
- Apply this diff:
- # mkdir /src/build/lib
- cp /usr/local/lib/libswscale.so.6 src/build/lib
- cp /usr/local/lib/libavfilter.so.8 src/build/lib
- cp /usr/local/lib/libpostproc.so.56 src/build/lib
- cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib
+ mkdir -p src/build/lib
+ while IFS= read -r lib; do
+ cp --no-dereference "$lib" src/build/lib/
+ done < runtime-libs.txt
.github/workflows/cd.yaml
Outdated
| # - name: Upload artifacts | ||
| # uses: actions/upload-artifact@v3 | ||
| # with: | ||
| # name: qt-build-artifacts | ||
| # path: src/build/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate artifact upload step.
This artifact upload step is redundant as the artifacts are already being uploaded in the previous step.
Remove these commented lines to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
.github/workflows/cd.yaml (7)
3-9:⚠️ Potential issueRestrict CD workflow triggers for production deployment.
The current configuration runs CD workflow on all PR branches, which is a security risk for production deployments.
Apply this diff:
on: push: branches: - 'main' pull_request: branches: - - '**' + - 'main' + - 'release/*'
16-17:⚠️ Potential issueUpdate actions/checkout to v4.
The current version v3 is outdated and might have known vulnerabilities.
- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
26-27:⚠️ Potential issueImprove BMF repository security and configuration.
The hardcoded repository URL and branch create maintenance overhead and security risks.
- Add environment variables at workflow level:
env: BMF_REPO: ${{ vars.BMF_REPOSITORY_URL }} BMF_BRANCH: ${{ vars.BMF_BRANCH }}
- Apply this diff:
- git clone https://github.com/JackLau1222/bmf.git - (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) + git clone --depth 1 --branch "$BMF_BRANCH" "$BMF_REPO" + cd bmf + git verify-commit HEAD + sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus + ./build.sh
56-59: 🛠️ Refactor suggestionImprove build configuration.
The current build configuration lacks build type specification and proper error handling.
Apply this diff:
- (cd src && cmake -B build && cd build && make -j$(nproc)) + cd src + cmake -B build -DCMAKE_BUILD_TYPE=Release + cmake --build build --parallel "$(nproc)"🧰 Tools
🪛 actionlint (1.7.4)
57-57: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 59-59: trailing spaces
(trailing-spaces)
60-65:⚠️ Potential issueRemove tmate debugging configuration.
The tmate debugging session with sudo access poses a security risk in production.
Remove these commented lines from the production workflow.
75-80:⚠️ Potential issueSecure tool downloads.
Downloads lack checksum verification and use insecure URLs.
Implement secure downloads:
- wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 - tar xf patchelf-0.9.tar.bz2 - ( cd patchelf-0.9/ && ./configure && make && sudo make install ) + PATCHELF_SHA256="6699f0e9373340b5f582f8d9fb453e3838c31a9d3c2f8d16634b8677c7a0b4d9" + wget -q https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 + echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c + tar xf patchelf-0.9.tar.bz2 + ( cd patchelf-0.9/ && ./configure && make && sudo make install )
87-92: 🛠️ Refactor suggestionImprove library management.
Manual library copying is error-prone and hard to maintain.
- Create a manifest file
runtime-libs.txt:/usr/local/lib/libswscale.so.6 /usr/local/lib/libavfilter.so.8 /usr/local/lib/libpostproc.so.56 bmf/output/bmf/lib/libbuiltin_modules.so
- Apply this diff:
- # mkdir /src/build/lib - cp /usr/local/lib/libswscale.so.6 src/build/lib - cp /usr/local/lib/libavfilter.so.8 src/build/lib - cp /usr/local/lib/libpostproc.so.56 src/build/lib - cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib + mkdir -p src/build/lib + while IFS= read -r lib; do + cp --no-dereference "$lib" src/build/lib/ + done < runtime-libs.txt
🧹 Nitpick comments (3)
.github/workflows/cd.yaml (3)
31-35: Optimize environment variable configuration.Multiple individual redirects to
$GITHUB_ENVcan be combined for better maintainability.- cd bmf - echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + cd bmf + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" + } >> $GITHUB_ENV
37-51: Remove commented configuration.These commented sections should be removed as they are redundant or superseded by active configuration.
Remove these commented sections to improve maintainability.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
96-99: Use environment variables for version management.Hardcoded version numbers create maintenance overhead and risk of inconsistency.
- VERSION="1.3" + VERSION="${GITHUB_REF_NAME#v}" BUILD_DIR="src/build" PACKAGE_NAME="OpenConverter_v${VERSION}_Linux_x86.tar.gz"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cd.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd.yaml
17-17: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
20-20: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
57-57: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
103-103: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/cd.yaml
[error] 13-13: trailing spaces
(trailing-spaces)
[warning] 16-16: wrong indentation: expected 6 but found 4
(indentation)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 59-59: trailing spaces
(trailing-spaces)
[error] 83-83: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/cd.yaml (1)
121-125: Remove duplicate artifact upload.This commented artifact upload step is redundant.
Remove these commented lines to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
109-109: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
117-117: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
117-117: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
117-117: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
121-121: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
.github/workflows/release.yaml
Outdated
| on: | ||
| release: | ||
| types: | ||
| - created | ||
| push: | ||
| tags: | ||
| - "v*.*.*" | ||
| # push: | ||
| # branches: | ||
| # - 'main' | ||
| pull_request: | ||
| branches: | ||
| - '**' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize workflow triggers for better resource utilization
The current trigger configuration runs the complete release workflow on every PR, which might be unnecessary and resource-intensive. Consider:
- Splitting the workflow into two:
- A build/test workflow for PRs
- A release workflow for tags/releases
- Removing commented-out code for cleaner maintenance
on:
release:
types:
- created
push:
tags:
- "v*.*.*"
- # push:
- # branches:
- # - 'main'
pull_request:
branches:
- - '**'
+ - 'main'
+ paths:
+ - 'src/**'
+ - '.github/workflows/**'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on: | |
| release: | |
| types: | |
| - created | |
| push: | |
| tags: | |
| - "v*.*.*" | |
| # push: | |
| # branches: | |
| # - 'main' | |
| pull_request: | |
| branches: | |
| - '**' | |
| on: | |
| release: | |
| types: | |
| - created | |
| push: | |
| tags: | |
| - "v*.*.*" | |
| pull_request: | |
| branches: | |
| - 'main' | |
| paths: | |
| - 'src/**' | |
| - '.github/workflows/**' |
| - name: Get GitHub Release information | ||
| id: release_info | ||
| run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})" | ||
|
|
||
|
|
||
| - name: Upload Release Asset | ||
| uses: softprops/action-gh-release@v1 | ||
| if: startsWith(github.ref, 'refs/tags/') | ||
| with: | ||
| files: ./build/OpenConverter_Linux_x86.tar.gz | ||
|
|
||
| - name: Finish | ||
| run: echo "Release upload complete" No newline at end of file |
There was a problem hiding this comment.
Update deprecated syntax and fix path inconsistencies
Several updates are needed:
- Replace deprecated
set-outputcommand - Fix path mismatch in release asset upload
- Update GitHub Actions to latest versions
- name: Get GitHub Release information
id: release_info
- run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"
+ run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT
- name: Upload Release Asset
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2
if: startsWith(github.ref, 'refs/tags/')
with:
- files: ./build/OpenConverter_Linux_x86.tar.gz
+ files: ./OpenConverter_Linux_x86.tar.gzAlso update other action versions:
- name: Checkout code
- uses: actions/checkout@v3
+ uses: actions/checkout@v4
- name: Upload build artifact
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.4)
117-117: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
117-117: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
117-117: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
121-121: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
| - name: Set up Qt | ||
| run: | | ||
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | ||
| - name: Build with CMake | ||
| run: | | ||
| (cd src && cmake -B build && cd build && make -j$(nproc)) | ||
| # - name: Start tmate session for debugging | ||
| # uses: mxschmitt/action-tmate@v3 | ||
| # with: | ||
| # sudo: true | ||
| # duration: 7200 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance build environment setup and performance
Consider these improvements:
- Pin Qt version for reproducibility
- Remove debugging configuration from production workflow
- Add build caching to speed up workflows
+ - name: Cache Qt
+ uses: actions/cache@v3
+ with:
+ path: |
+ /usr/lib/qt5
+ ~/.cache/pip
+ key: ${{ runner.os }}-qt5-${{ hashFiles('**/CMakeLists.txt') }}
+
- name: Set up Qt
run: |
- sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
+ sudo apt-get install -y \
+ qt5-qmake=5.12.8* \
+ qtbase5-dev=5.12.8* \
+ qtchooser=66-2* \
+ qtbase5-dev-tools=5.12.8* \
+ cmake \
+ build-essential
- # - name: Start tmate session for debugging
- # uses: mxschmitt/action-tmate@v3
- # with:
- # sudo: true
- # duration: 7200📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Set up Qt | |
| run: | | |
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | |
| - name: Build with CMake | |
| run: | | |
| (cd src && cmake -B build && cd build && make -j$(nproc)) | |
| # - name: Start tmate session for debugging | |
| # uses: mxschmitt/action-tmate@v3 | |
| # with: | |
| # sudo: true | |
| # duration: 7200 | |
| - name: Cache Qt | |
| uses: actions/cache@v3 | |
| with: | |
| path: | | |
| /usr/lib/qt5 | |
| ~/.cache/pip | |
| key: ${{ runner.os }}-qt5-${{ hashFiles('**/CMakeLists.txt') }} | |
| - name: Set up Qt | |
| run: | | |
| sudo apt-get install -y \ | |
| qt5-qmake=5.12.8* \ | |
| qtbase5-dev=5.12.8* \ | |
| qtchooser=66-2* \ | |
| qtbase5-dev-tools=5.12.8* \ | |
| cmake \ | |
| build-essential | |
| - name: Build with CMake | |
| run: | | |
| (cd src && cmake -B build && cd build && make -j$(nproc)) |
🧰 Tools
🪛 actionlint (1.7.4)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 65-65: trailing spaces
(trailing-spaces)
| - name: Copy libs | ||
| run: | | ||
| # linuxdeployqt | ||
| sudo apt-get -y install git g++ libgl1-mesa-dev | ||
| git clone https://github.com/probonopd/linuxdeployqt.git | ||
| # Then build in Qt Creator, or use | ||
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | ||
| (cd linuxdeployqt && qmake && make && sudo make install) | ||
| # patchelf | ||
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | ||
| tar xf patchelf-0.9.tar.bz2 | ||
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | ||
| # appimage | ||
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | ||
| sudo chmod a+x /usr/local/bin/appimagetool | ||
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | ||
| continue-on-error: true | ||
|
|
||
|
|
||
| - name: Copy runtime | ||
| run: | | ||
| # mkdir /src/build/lib | ||
| cp /usr/local/lib/libswscale.so.6 src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | ||
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | ||
| # Step to package the build directory | ||
| - name: Create tar.gz package | ||
| run: | | ||
| # VERSION="1.3" | ||
| BUILD_DIR="src/build" | ||
| PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz" | ||
| tar -czvf $PACKAGE_NAME -C $BUILD_DIR . | ||
There was a problem hiding this comment.
Fix critical build and packaging issues
Several critical issues need attention:
- Remove
continue-on-errorfrom AppImage creation - Verify copied libraries
- Use consistent build directory references
- continue-on-error: true
+ run: |
+ # Verify libraries exist before copying
+ LIBS=(
+ "/usr/local/lib/libswscale.so.6"
+ "/usr/local/lib/libavfilter.so.8"
+ "/usr/local/lib/libpostproc.so.56"
+ "bmf/output/bmf/lib/libbuiltin_modules.so"
+ )
+
+ for lib in "${LIBS[@]}"; do
+ if [ ! -f "$lib" ]; then
+ echo "Error: Required library $lib not found"
+ exit 1
+ fi
+ done
+
+ # Create lib directory if it doesn't exist
+ mkdir -p src/build/lib
+
+ # Copy and verify libraries
+ for lib in "${LIBS[@]}"; do
+ cp "$lib" src/build/lib/
+ if [ ! -f "src/build/lib/$(basename "$lib")" ]; then
+ echo "Error: Failed to copy $lib"
+ exit 1
+ fi
+ doneCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
🪛 yamllint (1.35.1)
[error] 89-89: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/CMakeLists.txt (4)
31-33:⚠️ Potential issueRemove hard-coded BMF path.
Using a hard-coded path makes the build system non-portable and won't work across different environments.
- message(STATUS "BMF_ROOT_PATH environment variable is not set, using default paths.") - # You can set a default path here if you want - set(BMF_ROOT_PATH "/Users/jacklau/Documents/Programs/Git/Github/bmf/output/bmf") + message(FATAL_ERROR "BMF_ROOT_PATH environment variable must be set")
36-45: 🛠️ Refactor suggestionUse modern CMake target-based approach.
Global include and link directories are discouraged in modern CMake.
- Replace include_directories with target_include_directories
- Avoid link_directories and use full paths in target_link_libraries
47-83: 🛠️ Refactor suggestionUse CMAKE_CURRENT_SOURCE_DIR and improve file organization.
- Using CMAKE_SOURCE_DIR makes the project non-relocatable.
- Consider grouping source files by component for better maintainability.
Additional suggestion for better organization:
# Group files by component set(BUILDER_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/encode_setting.cpp ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/open_converter.cpp ) set(COMMON_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/common/src/encode_parameter.cpp # ... other common sources ) set(SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp ${BUILDER_SOURCES} ${COMMON_SOURCES} # ... other component sources )
140-144: 🛠️ Refactor suggestionInclude GNUInstallDirs for installation paths.
Add at the beginning of the file:
include(GNUInstallDirs)
🧹 Nitpick comments (3)
src/CMakeLists.txt (3)
1-1: Consider using a more recent CMake version.CMake 3.5 is quite old (2016). Modern CMake versions (>=3.16) provide better features for Qt projects and improved error messages. Consider upgrading to at least 3.16 for better Qt6 support.
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.16)
86-89: Remove redundant commented commands.These Qt wrapper commands are unnecessary since CMAKE_AUTOUIC, CMAKE_AUTOMOC, and CMAKE_AUTORCC are already enabled.
-# Automatically handle UI files using UIC -#qt_wrap_ui(UI_HEADERS ${UI_FILES}) -# qt_add_resources(RESOURCE_OBJECTS ${RESOURCE_FILES}) -# qt_wrap_cpp(MOC_HEADERS ${HEADERS})
114-127: Clean up FFmpeg linking configuration.The FFMPEG_LDFLAGS already includes all necessary FFmpeg libraries, making the commented-out individual library links redundant.
target_link_libraries(OpenConverter PRIVATE Qt${QT_VERSION_MAJOR}::Widgets ${FFMPEG_LDFLAGS} -# avformat -# avcodec -# avutil -# pthread -# swresample engine bmf_module_sdk hmp builtin_modules )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/CMakeLists.txt (4)
12-13: LGTM! Good Qt version handling.The flexible Qt version detection allows the project to work with both Qt5 and Qt6.
21-25: Clarify the status of x264/x265 support.The commented-out codec configurations suggest incomplete or planned codec support. If these codecs are needed, they should be implemented; if not, the commented code should be removed.
Please clarify if x264/x265 support is planned for a future update or if these comments can be removed.
96-99: Clarify Android support status.The commented Android configuration suggests incomplete Android support. Either implement the Android-specific configuration or remove the comments if Android support isn't planned.
Please clarify if Android support is planned for a future update or if these comments can be removed.
Also applies to: 105-107
130-130: Use CMAKE_CURRENT_SOURCE_DIR for translation file path.-set(TRANSLATIONS ${CMAKE_SOURCE_DIR}/resources/lang_chinese.ts) +set(TRANSLATIONS ${CMAKE_CURRENT_SOURCE_DIR}/resources/lang_chinese.ts)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/CMakeLists.txt (4)
27-34:⚠️ Potential issueRemove hard-coded BMF path.
The default BMF path is user-specific and won't work across different environments. Consider:
- Making BMF_ROOT_PATH mandatory
- Providing clear instructions in the error message
if (DEFINED ENV{BMF_ROOT_PATH}) set(BMF_ROOT_PATH $ENV{BMF_ROOT_PATH}) else() - message(STATUS "BMF_ROOT_PATH environment variable is not set, using default paths.") - # You can set a default path here if you want - set(BMF_ROOT_PATH "/Users/jacklau/Documents/Programs/Git/Github/bmf/output/bmf") + message(FATAL_ERROR "BMF_ROOT_PATH environment variable must be set. Please set it to your BMF installation directory.") endif()
36-45: 🛠️ Refactor suggestionUse target-specific include and link directories.
Global include and link directories are discouraged in modern CMake. Use target-specific commands instead.
-include_directories( - ${FFMPEG_INCLUDE_DIRS} - ${BMF_ROOT_PATH}/include -) - -link_directories( - ${FFMPEG_LIBRARY_DIRS} - ${BMF_ROOT_PATH}/lib -) +target_include_directories(OpenConverter PRIVATE + ${FFMPEG_INCLUDE_DIRS} + ${BMF_ROOT_PATH}/include +) +target_link_directories(OpenConverter PRIVATE + ${FFMPEG_LIBRARY_DIRS} + ${BMF_ROOT_PATH}/lib +)
47-59: 🛠️ Refactor suggestionUse CMAKE_CURRENT_SOURCE_DIR for better relocatability.
Using CMAKE_SOURCE_DIR makes the project non-relocatable as a subdirectory. Use CMAKE_CURRENT_SOURCE_DIR instead.
set(SOURCES - ${CMAKE_SOURCE_DIR}/main.cpp - ${CMAKE_SOURCE_DIR}/builder/src/encode_setting.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/encode_setting.cpp # ... apply similar changes to other paths )
140-144: 🛠️ Refactor suggestionInclude GNUInstallDirs for installation paths.
The CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_BINDIR variables are undefined. Include GNUInstallDirs module to define these.
Add at the beginning of the file:
include(GNUInstallDirs).github/workflows/release.yaml (2)
25-42: 🛠️ Refactor suggestionImprove BMF repository setup reliability.
Several improvements needed:
- Use repository variables instead of hard-coded values
- Optimize environment variable exports
- git clone https://github.com/JackLau1222/bmf.git - (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) + git clone ${BMF_REPO_URL:-https://github.com/JackLau1222/bmf.git} + cd bmf + git checkout ${BMF_BRANCH:-fork_by_oc} + sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus + ./build.sh - echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib" + } >> "$GITHUB_ENV"🧰 Tools
🪛 actionlint (1.7.4)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 31-31: trailing spaces
(trailing-spaces)
72-100:⚠️ Potential issueFix critical build and packaging issues.
Several critical issues need attention:
- Remove
continue-on-errorfrom AppImage creation- Verify copied libraries
- Use consistent build directory references
Add library verification:
# Verify libraries exist before copying LIBS=( "/usr/local/lib/libswscale.so.6" "/usr/local/lib/libavfilter.so.8" "/usr/local/lib/libpostproc.so.56" "bmf/output/bmf/lib/libbuiltin_modules.so" ) for lib in "${LIBS[@]}"; do if [ ! -f "$lib" ]; then echo "Error: Required library $lib not found" exit 1 fi done🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
🪛 yamllint (1.35.1)
[error] 89-89: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (1)
src/CMakeLists.txt (1)
1-3: Update minimum CMake version.CMake 3.5 is quite old. Consider updating to at least 3.13 to leverage modern CMake features and best practices.
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.13)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/version.logis excluded by!**/*.log
📒 Files selected for processing (5)
.github/workflows/ci.yaml(0 hunks).github/workflows/release.yaml(1 hunks)src/CMakeLists.txt(1 hunks)src/engine/src/converter.cpp(2 hunks)src/transcoder/LICENSE(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/transcoder/LICENSE
- src/engine/src/converter.cpp
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
111-111: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
119-119: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
119-119: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
119-119: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
123-123: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 129-129: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/release.yaml (1)
3-16: Optimize workflow triggers for better resource utilization.The current trigger configuration could be optimized. Consider:
- Splitting into separate workflows for PR validation and releases
- Removing commented-out code
on: release: types: - created push: tags: - "v*.*.*" - # push: - # branches: - # - 'main' - # pull_request: - # branches: - # - '**'
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
Update GitHub Actions versions and deprecated syntax.
Several updates needed:
- Update actions to latest versions
- Replace deprecated
set-outputcommand
- uses: actions/checkout@v3
+ uses: actions/checkout@v4
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
- run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"
+ run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2Also applies to: 110-112, 117-119, 122-124
🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
.github/workflows/review.yaml (2)
21-21: Remove commented codeRemove commented code to improve maintainability. If these are TODO items, create GitHub issues to track them instead.
Also applies to: 26-27
37-37: Pin Qt version for reproducible buildsConsider pinning the Qt version to ensure reproducible builds across different environments.
- sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential + sudo apt-get install -y qt5-qmake=5.12.8* qtbase5-dev=5.12.8* qtchooser=66-2* qtbase5-dev-tools=5.12.8* cmake build-essential
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
81-81: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/review.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 87-87: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| uses: actions/upload-artifact@v3 | ||
| with: | ||
| name: OpenConverter_Linux_x86 | ||
| path: OpenConverter_Linux_x86.tar.gz |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update upload-artifact action version
The upload-artifact action version needs to be updated as per static analysis:
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/upload-artifact@v3 | |
| with: | |
| name: OpenConverter_Linux_x86 | |
| path: OpenConverter_Linux_x86.tar.gz | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: OpenConverter_Linux_x86 | |
| path: OpenConverter_Linux_x86.tar.gz |
🧰 Tools
🪛 actionlint (1.7.4)
81-81: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
Outdated
| name: Release | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - '**' | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-20.04 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename workflow to reflect its purpose
The workflow name "Release" is misleading as it runs on pull requests. Consider renaming it to "Build" or "PR Build" to better reflect its purpose.
-name: Release
+name: PR BuildAlso, consider upgrading to Ubuntu 22.04 as Ubuntu 20.04 is approaching EOL:
- runs-on: ubuntu-20.04
+ runs-on: ubuntu-22.04📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Release | |
| on: | |
| pull_request: | |
| branches: | |
| - '**' | |
| jobs: | |
| build: | |
| runs-on: ubuntu-20.04 | |
| name: PR Build | |
| on: | |
| pull_request: | |
| branches: | |
| - '**' | |
| jobs: | |
| build: | |
| runs-on: ubuntu-22.04 |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | ||
| continue-on-error: true |
There was a problem hiding this comment.
Remove continue-on-error for critical packaging step
The AppImage creation is a critical step and should not silently continue on failure. Remove continue-on-error and add proper error handling:
- (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
- continue-on-error: true
+ if ! linuxdeployqt "./src/${BUILD_DIR}/OpenConverter" -appimage; then
+ echo "::error::Failed to create AppImage"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | |
| continue-on-error: true | |
| if ! linuxdeployqt "./src/${BUILD_DIR}/OpenConverter" -appimage; then | |
| echo "::error::Failed to create AppImage" | |
| exit 1 | |
| fi |
| - name: Build with CMake | ||
| run: | | ||
| (cd src && cmake -B build && cd build && make -j$(nproc)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix CMake build command
The build command needs proper quoting and should use a configurable build directory:
- (cd src && cmake -B build && cd build && make -j$(nproc))
+ BUILD_DIR="${BUILD_DIR:-build}"
+ (cd src && cmake -B "$BUILD_DIR" && cd "$BUILD_DIR" && make "-j$(nproc)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (cd src && cmake -B build && cd build && make -j$(nproc)) | |
| BUILD_DIR="${BUILD_DIR:-build}" | |
| (cd src && cmake -B "$BUILD_DIR" && cd "$BUILD_DIR" && make "-j$(nproc)") |
.github/workflows/review.yaml
Outdated
| git clone https://github.com/JackLau1222/bmf.git | ||
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) |
There was a problem hiding this comment.
Use official BMF repository instead of personal fork
Using a personal fork with a specific branch creates maintenance and security risks. Consider:
- Using the official BMF repository
- Making the repository URL and branch configurable via workflow inputs
- git clone https://github.com/JackLau1222/bmf.git
- (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
+ git clone https://github.com/BabitMF/bmf.git
+ (cd bmf && git checkout ${BMF_VERSION:-master} && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| git clone https://github.com/JackLau1222/bmf.git | |
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) | |
| git clone https://github.com/BabitMF/bmf.git | |
| (cd bmf && git checkout ${BMF_VERSION:-master} && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) |
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | ||
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix environment variable concatenation
The environment variables should be properly quoted to prevent word splitting:
- echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
- echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
- echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
- echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
+ {
+ echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+"$C_INCLUDE_PATH:"}$(pwd)/output/bmf/include"
+ echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+"$CPLUS_INCLUDE_PATH:"}$(pwd)/output/bmf/include"
+ echo "LIBRARY_PATH=${LIBRARY_PATH:+"$LIBRARY_PATH:"}$(pwd)/output/bmf/lib"
+ echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+"$LD_LIBRARY_PATH:"}$(pwd)/output/bmf/lib"
+ } >> "$GITHUB_ENV"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | |
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | |
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | |
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | |
| { | |
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+"$C_INCLUDE_PATH:"}$(pwd)/output/bmf/include" | |
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+"$CPLUS_INCLUDE_PATH:"}$(pwd)/output/bmf/include" | |
| echo "LIBRARY_PATH=${LIBRARY_PATH:+"$LIBRARY_PATH:"}$(pwd)/output/bmf/lib" | |
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+"$LD_LIBRARY_PATH:"}$(pwd)/output/bmf/lib" | |
| } >> "$GITHUB_ENV" |
| sudo apt-get -y install git g++ libgl1-mesa-dev | ||
| git clone https://github.com/probonopd/linuxdeployqt.git | ||
| # Then build in Qt Creator, or use | ||
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | ||
| (cd linuxdeployqt && qmake && make && sudo make install) | ||
| # patchelf |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pre-built linuxdeployqt
Building linuxdeployqt from source using qmake in a CMake workflow is inconsistent. Consider using pre-built releases:
- sudo apt-get -y install git g++ libgl1-mesa-dev
- git clone https://github.com/probonopd/linuxdeployqt.git
- export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
- (cd linuxdeployqt && qmake && make && sudo make install)
+ LINUXDEPLOYQT_VERSION="8"
+ wget -c -nv "https://github.com/probonopd/linuxdeployqt/releases/download/${LINUXDEPLOYQT_VERSION}/linuxdeployqt-continuous-x86_64.AppImage"
+ chmod a+x linuxdeployqt-continuous-x86_64.AppImage
+ sudo mv linuxdeployqt-continuous-x86_64.AppImage /usr/local/bin/linuxdeployqt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo apt-get -y install git g++ libgl1-mesa-dev | |
| git clone https://github.com/probonopd/linuxdeployqt.git | |
| # Then build in Qt Creator, or use | |
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | |
| (cd linuxdeployqt && qmake && make && sudo make install) | |
| # patchelf | |
| LINUXDEPLOYQT_VERSION="8" | |
| wget -c -nv "https://github.com/probonopd/linuxdeployqt/releases/download/${LINUXDEPLOYQT_VERSION}/linuxdeployqt-continuous-x86_64.AppImage" | |
| chmod a+x linuxdeployqt-continuous-x86_64.AppImage | |
| sudo mv linuxdeployqt-continuous-x86_64.AppImage /usr/local/bin/linuxdeployqt | |
| # patchelf |
.github/workflows/review.yaml
Outdated
| cp /usr/local/lib/libswscale.so.6 src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | ||
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | ||
| touch src/build/activate_env.sh | ||
| echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh |
There was a problem hiding this comment.
Fix runtime library setup
The runtime library setup has several issues:
- Hardcoded library versions
- Undefined BASEDIR variable
- cp /usr/local/lib/libswscale.so.6 src/build/lib
- cp /usr/local/lib/libavfilter.so.8 src/build/lib
- cp /usr/local/lib/libpostproc.so.56 src/build/lib
- cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib
- touch src/build/activate_env.sh
- echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh
+ LIB_DIR="src/${BUILD_DIR}/lib"
+ mkdir -p "$LIB_DIR"
+ for lib in swscale.6 avfilter.8 postproc.56; do
+ cp "/usr/local/lib/lib${lib}" "$LIB_DIR/"
+ done
+ cp "bmf/output/bmf/lib/libbuiltin_modules.so" "$LIB_DIR/"
+ cat > "src/${BUILD_DIR}/activate_env.sh" << 'EOF'
+ #!/bin/bash
+ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+ export LD_LIBRARY_PATH="${SCRIPT_DIR}/lib${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}"
+ EOFCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/release.yaml (1)
17-130: Add robustness measures to the workflowConsider adding the following improvements:
- Add error handling and logging to shell scripts
- Include cleanup steps to remove temporary files
- Add verification steps for build artifacts
# Example verification step if [ ! -f "OpenConverter_Linux_x86.tar.gz" ]; then echo "Error: Build artifact not found" exit 1 fi # Verify artifact contents tar -tvf OpenConverter_Linux_x86.tar.gz || { echo "Error: Invalid archive" exit 1 }🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
112-112: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
120-120: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
124-124: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 130-130: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks).github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
112-112: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
120-120: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
124-124: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
82-82: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 130-130: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/review.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 88-88: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
.github/workflows/review.yaml (8)
1-10: Rename workflow and upgrade Ubuntu versionThe workflow name "Release" is misleading as it runs on pull requests. Additionally, Ubuntu 20.04 is approaching EOL.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 10-10: trailing spaces
(trailing-spaces)
23-24: Use official BMF repositoryUsing a personal fork creates maintenance and security risks.
29-32: Fix environment variable concatenationEnvironment variables should be properly quoted to prevent word splitting.
41-41: Fix CMake build commandThe build command needs proper quoting and should use a configurable build directory.
46-51: Use pre-built linuxdeployqtBuilding linuxdeployqt from source using qmake in a CMake workflow is inconsistent.
58-59: Remove continue-on-error for critical packaging stepThe AppImage creation is a critical step and should not silently continue on failure.
64-71: Fix runtime library setupThe runtime library setup has several issues including hardcoded library versions and undefined BASEDIR variable.
82-85: Update upload-artifact action versionThe upload-artifact action version needs to be updated.
🧰 Tools
🪛 actionlint (1.7.4)
82-82: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yaml (6)
3-15: Optimize workflow triggers for better resource utilizationThe current trigger configuration includes commented-out sections that should be cleaned up. Additionally, running the complete release workflow on PRs would be resource-intensive.
25-42: Improve BMF repository setup reliability and maintainabilityThe BMF repository setup needs improvements in reliability and maintainability:
- Hard-coded repository URL and branch should use repository variables
- Environment variable exports should be combined
- System setup should be separated from BMF build
🧰 Tools
🪛 actionlint (1.7.4)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 31-31: trailing spaces
(trailing-spaces)
58-65: Enhance build environment setup and performanceConsider these improvements:
- Pin Qt version for reproducibility
- Quote the command substitution in make command
- Add build caching to speed up workflows
🧰 Tools
🪛 actionlint (1.7.4)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 65-65: trailing spaces
(trailing-spaces)
72-89: Fix critical build and packaging issuesSeveral critical issues need attention:
- Remove
continue-on-errorfrom AppImage creation- Verify libraries exist before copying
- Use consistent build directory references
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
🪛 yamllint (1.35.1)
[error] 89-89: trailing spaces
(trailing-spaces)
110-127: Update deprecated syntax and fix path inconsistenciesSeveral updates are needed:
- Replace deprecated
set-outputcommand- Fix path mismatch in release asset upload
- Update GitHub Actions to latest versions
🧰 Tools
🪛 actionlint (1.7.4)
112-112: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
120-120: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
120-120: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
120-120: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
124-124: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
91-101:⚠️ Potential issueFix runtime setup issues
The runtime setup has several issues that need to be addressed:
- Missing verification of source files before copying
- The activation script uses an undefined
BASEDIRvariable-echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh +BASEDIR='$(dirname "$(readlink -f "$0")")' +echo "export LD_LIBRARY_PATH=\"$BASEDIR/lib\":\$LD_LIBRARY_PATH" >> src/build/activate_env.shLikely invalid or redundant comment.
| - name: Set up Qt | ||
| run: | | ||
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary qmake installation
Since the project is moving to CMake, qmake-related packages are no longer needed.
- sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
+ sudo apt-get install -y qtbase5-dev cmake build-essential📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | |
| sudo apt-get install -y qtbase5-dev cmake build-essential |
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update checkout action version
The checkout action version should be updated to v4 for better compatibility and security.
- uses: actions/checkout@v3
+ uses: actions/checkout@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/checkout@v3 | |
| uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/review.yaml (1)
25-27: Remove commented codeRemove commented-out code that sets BMF_ROOT_PATH as it adds noise to the workflow file.
- # setup bmf root path - # echo "BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf" >> $GITHUB_ENV - # export BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks).github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
116-116: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
124-124: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
128-128: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
86-86: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 134-134: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/review.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 92-92: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
.github/workflows/review.yaml (11)
1-1: Rename workflow to reflect its purposeThe workflow name "Review" is misleading as it runs on pull requests. Consider renaming it to "PR Build" to better reflect its purpose.
10-10: Update Ubuntu runner versionUbuntu 20.04 is approaching EOL. Consider upgrading to Ubuntu 22.04.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
14-14: Update checkout action versionThe checkout action version should be updated to v4 for better compatibility and security.
🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
23-24: Use official BMF repository instead of personal forkUsing a personal fork with a specific branch creates maintenance and security risks.
29-32: Fix environment variable concatenationThe environment variables should be properly quoted to prevent word splitting.
37-37: Remove unnecessary qmake installationSince the project is moving to CMake, qmake-related packages are no longer needed.
41-41: Fix CMake build commandThe build command needs proper quoting and should use a configurable build directory.
46-51: Use pre-built linuxdeployqtBuilding linuxdeployqt from source using qmake in a CMake workflow is inconsistent.
58-59: Remove continue-on-error for critical packaging stepThe AppImage creation is a critical step and should not silently continue on failure.
64-71: Fix runtime library setupThe runtime library setup has several issues including hardcoded library versions and undefined BASEDIR variable.
86-89: Update upload-artifact action versionThe upload-artifact action version needs to be updated to v4.
🧰 Tools
🪛 actionlint (1.7.4)
86-86: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yaml (2)
1-16: 🛠️ Refactor suggestionOptimize workflow triggers and fix YAML formatting.
The workflow configuration needs improvement:
- Remove commented-out triggers for cleaner maintenance
- Fix trailing spaces in YAML
Apply this diff:
-name: Release +name: Release on: release: types: - created push: tags: - "v*.*.*" - # push: - # branches: - # - 'main' - # pull_request: - # branches: - # - '**'Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: trailing spaces
(trailing-spaces)
115-134:⚠️ Potential issueUpdate GitHub Actions versions and fix release asset path.
Several critical updates needed:
- Update actions to latest versions
- Fix deprecated
set-outputcommand- Fix release asset path
Apply this diff:
- name: Upload build artifact - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: OpenConverter_Linux_x86 path: OpenConverter_Linux_x86.tar.gz - name: Get GitHub Release information id: release_info - run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})" + run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> "$GITHUB_OUTPUT" - name: Upload Release Asset - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 if: startsWith(github.ref, 'refs/tags/') with: - files: ./build/OpenConverter_Linux_x86.tar.gz + files: OpenConverter_Linux_x86.tar.gz - name: Finish - run: echo "Release upload complete" + run: echo "Release upload complete" +Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
116-116: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
124-124: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
128-128: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 134-134: no new line character at the end of file
(new-line-at-end-of-file)
| run: | | ||
| (cd src && cmake -B build && cd build && make -j$(nproc)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add build validation steps
The workflow should validate the build artifacts before packaging:
- name: Build with CMake
run: |
(cd src && cmake -B build && cd build && make -j$(nproc))
+ # Validate build artifacts
+ if [ ! -f "src/build/OpenConverter" ]; then
+ echo "::error::Build failed: OpenConverter binary not found"
+ exit 1
+ fi
# ... other steps ...
- name: Create tar.gz package
run: |
+ # Validate required files before packaging
+ required_files=("OpenConverter" "lib" "BUILTIN_CONFIG.json" "activate_env.sh")
+ for file in "${required_files[@]}"; do
+ if [ ! -e "$BUILD_DIR/$file" ]; then
+ echo "::error::Required file/directory not found: $file"
+ exit 1
+ fi
+ done
BUILD_DIR="src/build"Also applies to: 75-82
🧰 Tools
🪛 actionlint (1.7.4)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
| - name: Checkout BMF repository (specific branch) | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | ||
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | ||
| # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa | ||
| git clone https://github.com/JackLau1222/bmf.git | ||
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) | ||
| # setup bmf root path | ||
| # echo "BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf" >> $GITHUB_ENV | ||
| # export BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf | ||
| cd bmf | ||
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | ||
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | ||
| - name: Set up Qt | ||
| run: | | ||
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | ||
| - name: Build with CMake | ||
| run: | | ||
| (cd src && cmake -B build && cd build && make -j$(nproc)) | ||
| - name: Copy libs | ||
| run: | | ||
| # linuxdeployqt | ||
| sudo apt-get -y install git g++ libgl1-mesa-dev | ||
| git clone https://github.com/probonopd/linuxdeployqt.git | ||
| # Then build in Qt Creator, or use | ||
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | ||
| (cd linuxdeployqt && qmake && make && sudo make install) | ||
| # patchelf | ||
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | ||
| tar xf patchelf-0.9.tar.bz2 | ||
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | ||
| # appimage | ||
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | ||
| sudo chmod a+x /usr/local/bin/appimagetool | ||
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | ||
| continue-on-error: true | ||
|
|
||
|
|
||
| - name: Copy runtime | ||
| run: | | ||
| # mkdir /src/build/lib | ||
| cp /usr/local/lib/libswscale.so.6 src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | ||
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | ||
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | ||
| touch src/build/activate_env.sh | ||
| echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dependency caching
The workflow rebuilds all dependencies on every run. Consider adding caching to speed up the workflow:
+ - name: Cache dependencies
+ uses: actions/cache@v4
+ with:
+ path: |
+ bmf
+ patchelf-0.9
+ key: ${{ runner.os }}-deps-${{ hashFiles('**/CMakeLists.txt') }}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.4)
17-17: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
🪛 yamllint (1.35.1)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
| - name: Set up Qt | ||
| run: | | ||
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | ||
| - name: Build with CMake | ||
| run: | | ||
| (cd src && cmake -B build && cd build && make -j$(nproc)) | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Qt setup and build reliability.
- Pin Qt package versions for reproducibility
- Add error handling for build steps
- Fix command substitution in make command
Apply this diff:
- name: Set up Qt
run: |
+ set -eo pipefail
sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential
- name: Build with CMake
run: |
+ set -eo pipefail
- (cd src && cmake -B build && cd build && make -j$(nproc))
+ cd src
+ cmake -B build
+ cd build
+ make -j"$(nproc)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Set up Qt | |
| run: | | |
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | |
| - name: Build with CMake | |
| run: | | |
| (cd src && cmake -B build && cd build && make -j$(nproc)) | |
| - name: Set up Qt | |
| run: | | |
| set -eo pipefail | |
| sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential | |
| - name: Build with CMake | |
| run: | | |
| set -eo pipefail | |
| cd src | |
| cmake -B build | |
| cd build | |
| make -j"$(nproc)" | |
🧰 Tools
🪛 actionlint (1.7.4)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 65-65: trailing spaces
(trailing-spaces)
| - name: Copy runtime | ||
| run: | | ||
| # mkdir /src/build/lib | ||
| cp /usr/local/lib/libswscale.so.6 src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | ||
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | ||
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | ||
| touch src/build/activate_env.sh | ||
| echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve runtime library management.
- Add error handling for library copying
- Fix commented mkdir command
- Improve environment script
Apply this diff:
- name: Copy runtime
run: |
+ set -eo pipefail
- # mkdir /src/build/lib
+ mkdir -p src/build/lib
+
+ # Verify libraries exist before copying
+ LIBS=(
+ "/usr/local/lib/libswscale.so.6"
+ "/usr/local/lib/libavfilter.so.8"
+ "/usr/local/lib/libpostproc.so.56"
+ "bmf/output/bmf/lib/libbuiltin_modules.so"
+ )
+
+ for lib in "${LIBS[@]}"; do
+ if [ ! -f "$lib" ]; then
+ echo "Error: Required library $lib not found"
+ exit 1
+ fi
+ cp "$lib" src/build/lib/
+ done
+
+ if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then
+ echo "Error: BUILTIN_CONFIG.json not found"
+ exit 1
+ fi
cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
- touch src/build/activate_env.sh
- echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh
+
+ cat > src/build/activate_env.sh << 'EOF'
+ #!/bin/bash
+ BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+ export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
+ EOF
+ chmod +x src/build/activate_env.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Copy runtime | |
| run: | | |
| # mkdir /src/build/lib | |
| cp /usr/local/lib/libswscale.so.6 src/build/lib | |
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | |
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | |
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | |
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | |
| touch src/build/activate_env.sh | |
| echo export LD_LIBRARY_PATH="$BASEDIR/lib" >> src/build/activate_env.sh | |
| - name: Copy runtime | |
| run: | | |
| set -eo pipefail | |
| mkdir -p src/build/lib | |
| # Verify libraries exist before copying | |
| LIBS=( | |
| "/usr/local/lib/libswscale.so.6" | |
| "/usr/local/lib/libavfilter.so.8" | |
| "/usr/local/lib/libpostproc.so.56" | |
| "bmf/output/bmf/lib/libbuiltin_modules.so" | |
| ) | |
| for lib in "${LIBS[@]}"; do | |
| if [ ! -f "$lib" ]; then | |
| echo "Error: Required library $lib not found" | |
| exit 1 | |
| fi | |
| cp "$lib" src/build/lib/ | |
| done | |
| if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then | |
| echo "Error: BUILTIN_CONFIG.json not found" | |
| exit 1 | |
| fi | |
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | |
| cat > src/build/activate_env.sh << 'EOF' | |
| #!/bin/bash | |
| BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" | |
| EOF | |
| chmod +x src/build/activate_env.sh |
| - name: Copy libs | ||
| run: | | ||
| # linuxdeployqt | ||
| sudo apt-get -y install git g++ libgl1-mesa-dev | ||
| git clone https://github.com/probonopd/linuxdeployqt.git | ||
| # Then build in Qt Creator, or use | ||
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | ||
| (cd linuxdeployqt && qmake && make && sudo make install) | ||
| # patchelf | ||
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | ||
| tar xf patchelf-0.9.tar.bz2 | ||
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | ||
| # appimage | ||
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | ||
| sudo chmod a+x /usr/local/bin/appimagetool | ||
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | ||
| continue-on-error: true | ||
|
|
There was a problem hiding this comment.
Fix critical issues in library setup.
Several critical issues need attention:
- Remove
continue-on-errorfrom AppImage creation - Add error handling
- Fix PATH manipulation
- Verify dependencies before proceeding
Apply this diff:
- name: Copy libs
run: |
+ set -eo pipefail
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
+
+ # Verify Qt installation
+ if ! command -v qmake &> /dev/null; then
+ echo "Error: Qt installation not found"
+ exit 1
+ fi
+
git clone https://github.com/probonopd/linuxdeployqt.git
- # Then build in Qt Creator, or use
- export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
+
+ # Set Qt path safely
+ qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true
+ if [ -n "$qt_path" ]; then
+ export PATH="$qt_path:$PATH"
+ fi
+
(cd linuxdeployqt && qmake && make && sudo make install)
+
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
+
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
- (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
- continue-on-error: true
+ linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Copy libs | |
| run: | | |
| # linuxdeployqt | |
| sudo apt-get -y install git g++ libgl1-mesa-dev | |
| git clone https://github.com/probonopd/linuxdeployqt.git | |
| # Then build in Qt Creator, or use | |
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | |
| (cd linuxdeployqt && qmake && make && sudo make install) | |
| # patchelf | |
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | |
| tar xf patchelf-0.9.tar.bz2 | |
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | |
| # appimage | |
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | |
| sudo chmod a+x /usr/local/bin/appimagetool | |
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | |
| continue-on-error: true | |
| - name: Copy libs | |
| run: | | |
| set -eo pipefail | |
| # linuxdeployqt | |
| sudo apt-get -y install git g++ libgl1-mesa-dev | |
| # Verify Qt installation | |
| if ! command -v qmake &> /dev/null; then | |
| echo "Error: Qt installation not found" | |
| exit 1 | |
| fi | |
| git clone https://github.com/probonopd/linuxdeployqt.git | |
| # Set Qt path safely | |
| qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true | |
| if [ -n "$qt_path" ]; then | |
| export PATH="$qt_path:$PATH" | |
| fi | |
| (cd linuxdeployqt && qmake && make && sudo make install) | |
| # patchelf | |
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | |
| tar xf patchelf-0.9.tar.bz2 | |
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | |
| # appimage | |
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | |
| sudo chmod a+x /usr/local/bin/appimagetool | |
| linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage |
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
🪛 yamllint (1.35.1)
[error] 89-89: trailing spaces
(trailing-spaces)
| - name: Checkout BMF repository (specific branch) | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | ||
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | ||
| # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa | ||
| git clone https://github.com/JackLau1222/bmf.git | ||
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) | ||
| # setup bmf root path | ||
| # echo "BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf" >> $GITHUB_ENV | ||
| # export BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf | ||
| cd bmf | ||
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | ||
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve BMF repository setup reliability.
Several improvements needed:
- Use repository variables for BMF repository URL and branch
- Fix environment variable exports
- Add error handling for BMF build steps
Apply this diff:
- name: Checkout BMF repository (specific branch)
+ env:
+ BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }}
+ BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }}
run: |
+ set -eo pipefail
sudo apt update
sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
- # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa
- git clone https://github.com/JackLau1222/bmf.git
- (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
+ git clone "$BMF_REPO" bmf
+ cd bmf
+ git checkout "$BMF_BRANCH"
+ sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus
+ ./build.sh
- # setup bmf root path
- # echo "BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf" >> $GITHUB_ENV
- # export BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf
- cd bmf
- echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
- echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
- echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
- echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
+ {
+ echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include"
+ echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include"
+ echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib"
+ echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib"
+ } >> "$GITHUB_ENV"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout BMF repository (specific branch) | |
| run: | | |
| sudo apt update | |
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | |
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | |
| # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa | |
| git clone https://github.com/JackLau1222/bmf.git | |
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) | |
| # setup bmf root path | |
| # echo "BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf" >> $GITHUB_ENV | |
| # export BMF_ROOT_PATH=${BMF_ROOT_PATH}:$(pwd)/bmf/output/bmf | |
| cd bmf | |
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | |
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | |
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | |
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | |
| - name: Checkout BMF repository (specific branch) | |
| env: | |
| BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }} | |
| BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }} | |
| run: | | |
| set -eo pipefail | |
| sudo apt update | |
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | |
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | |
| git clone "$BMF_REPO" bmf | |
| cd bmf | |
| git checkout "$BMF_BRANCH" | |
| sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus | |
| ./build.sh | |
| { | |
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include" | |
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include" | |
| echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib" | |
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib" | |
| } >> "$GITHUB_ENV" |
🧰 Tools
🪛 actionlint (1.7.4)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 31-31: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/release.yaml (2)
43-57: Remove commented environment configuration.Remove the commented-out environment configuration to improve maintainability.
- # - name: set envs - # run: | - # echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - # echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - # echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - # echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib:/usr/local/lib" >> $GITHUB_ENV - # echo "PYTHONPATH=$(pwd)/output/bmf/lib:$(pwd)/output" >> $GITHUB_ENV - # echo "PATH=${PATH}:$(pwd)/output/bmf/bin" >> $GITHUB_ENV - # echo "HMP_TEST_DATA_ROOT=$(pwd)/bmf/hml/tests/data" >> $GITHUB_ENV - - # env: - # BMF_REPO: ${{ vars.BMF_REPOSITORY_URL }} - # BMF_BRANCH: ${{ vars.BMF_BRANCH }} - # BMF_ROOT_PATH: ${{ github.workspace }}/bmf/output/bmf🧰 Tools
🪛 yamllint (1.35.1)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
66-70: Remove commented debugging configuration.Remove the commented-out debugging configuration to improve maintainability.
- # - name: Start tmate session for debugging - # uses: mxschmitt/action-tmate@v3 - # with: - # sudo: true - # duration: 7200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks).github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
116-116: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
124-124: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
128-128: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
17-17: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
17-17: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
86-86: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 54-54: trailing spaces
(trailing-spaces)
[error] 55-55: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 134-134: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/review.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 92-92: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (18)
.github/workflows/review.yaml (9)
1-10: Rename workflow and upgrade Ubuntu version
- The workflow name "Review" is misleading as it runs on pull requests. Consider renaming it to "Build" or "PR Build".
- Consider upgrading to Ubuntu 22.04 as Ubuntu 20.04 is approaching EOL.
-name: Review +name: PR Build on: pull_request: branches: - '**' jobs: build: - runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
13-14: Update checkout action versionThe checkout action version should be updated to v4 for better compatibility and security.
- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
23-24: Use official BMF repository instead of personal forkUsing a personal fork with a specific branch creates maintenance and security risks.
- git clone https://github.com/JackLau1222/bmf.git - (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) + git clone https://github.com/BabitMF/bmf.git + (cd bmf && git checkout ${BMF_VERSION:-master} && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
29-32: Fix environment variable concatenationThe environment variables should be properly quoted to prevent word splitting.
- echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+"$C_INCLUDE_PATH:"}$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+"$CPLUS_INCLUDE_PATH:"}$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH:+"$LIBRARY_PATH:"}$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+"$LD_LIBRARY_PATH:"}$(pwd)/output/bmf/lib" + } >> "$GITHUB_ENV"
37-37: Remove unnecessary qmake installationSince the project is moving to CMake, qmake-related packages are no longer needed.
- sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential + sudo apt-get install -y qtbase5-dev cmake build-essential
40-41: Fix CMake build commandThe build command needs proper quoting and should use a configurable build directory.
- (cd src && cmake -B build && cd build && make -j$(nproc)) + BUILD_DIR="${BUILD_DIR:-build}" + (cd src && cmake -B "$BUILD_DIR" && cd "$BUILD_DIR" && make "-j$(nproc)")🧰 Tools
🪛 actionlint (1.7.4)
40-40: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
46-51: Use pre-built linuxdeployqtBuilding linuxdeployqt from source using qmake in a CMake workflow is inconsistent.
- sudo apt-get -y install git g++ libgl1-mesa-dev - git clone https://github.com/probonopd/linuxdeployqt.git - export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH - (cd linuxdeployqt && qmake && make && sudo make install) + LINUXDEPLOYQT_VERSION="8" + wget -c -nv "https://github.com/probonopd/linuxdeployqt/releases/download/${LINUXDEPLOYQT_VERSION}/linuxdeployqt-continuous-x86_64.AppImage" + chmod a+x linuxdeployqt-continuous-x86_64.AppImage + sudo mv linuxdeployqt-continuous-x86_64.AppImage /usr/local/bin/linuxdeployqt
58-59: Remove continue-on-error for critical packaging stepThe AppImage creation is a critical step and should not silently continue on failure.
- (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) - continue-on-error: true + if ! linuxdeployqt "./src/${BUILD_DIR}/OpenConverter" -appimage; then + echo "::error::Failed to create AppImage" + exit 1 + fi
86-89: Update upload-artifact action versionThe upload-artifact action version needs to be updated.
- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4🧰 Tools
🪛 actionlint (1.7.4)
86-86: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yaml (9)
10-15: Remove commented workflow triggers.Remove the commented-out workflow triggers to improve maintainability.
- # push: - # branches: - # - 'main' - # pull_request: - # branches: - # - '**'
22-23: 🛠️ Refactor suggestionUpdate GitHub Actions versions.
The
actions/checkoutaction version is outdated. Update to the latest version.- uses: actions/checkout@v3 + uses: actions/checkout@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
115-119: 🛠️ Refactor suggestionUpdate upload-artifact action version.
The
actions/upload-artifactaction version is outdated. Update to the latest version.- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
116-116: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
58-64: 🛠️ Refactor suggestionImprove Qt setup and build reliability.
Add error handling and fix command substitution in the build steps.
- name: Set up Qt run: | + set -eo pipefail sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential - name: Build with CMake run: | + set -eo pipefail - (cd src && cmake -B build && cd build && make -j$(nproc)) + cd src + cmake -B build + cd build + make -j"$(nproc)"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
63-63: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
127-131:⚠️ Potential issueUpdate gh-release action and fix file path.
- Update the action version
- Fix the incorrect file path for the release asset
- uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 if: startsWith(github.ref, 'refs/tags/') with: - files: ./build/OpenConverter_Linux_x86.tar.gz + files: ./OpenConverter_Linux_x86.tar.gzLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
128-128: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
122-124: 🛠️ Refactor suggestionReplace deprecated set-output command.
The
set-outputcommand is deprecated. Use the new syntax withGITHUB_OUTPUT.- run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})" + run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUTLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
124-124: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
72-89:⚠️ Potential issueFix critical issues in library setup.
- Remove
continue-on-errorfrom AppImage creation- Add error handling
- Fix PATH manipulation
- name: Copy libs run: | + set -eo pipefail # linuxdeployqt sudo apt-get -y install git g++ libgl1-mesa-dev git clone https://github.com/probonopd/linuxdeployqt.git - export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH + qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true + if [ -n "$qt_path" ]; then + export PATH="$qt_path:$PATH" + fi (cd linuxdeployqt && qmake && make && sudo make install) # patchelf wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 tar xf patchelf-0.9.tar.bz2 ( cd patchelf-0.9/ && ./configure && make && sudo make install ) # appimage sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool sudo chmod a+x /usr/local/bin/appimagetool - (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) - continue-on-error: true + linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimageLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
73-73: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
🪛 yamllint (1.35.1)
[error] 89-89: trailing spaces
(trailing-spaces)
91-100: 🛠️ Refactor suggestionImprove runtime library management.
Add error handling and improve environment script setup.
- name: Copy runtime run: | + set -eo pipefail - # mkdir /src/build/lib + mkdir -p src/build/lib + + # Verify libraries exist before copying + LIBS=( + "/usr/local/lib/libswscale.so.6" + "/usr/local/lib/libavfilter.so.8" + "/usr/local/lib/libpostproc.so.56" + "bmf/output/bmf/lib/libbuiltin_modules.so" + ) + + for lib in "${LIBS[@]}"; do + if [ ! -f "$lib" ]; then + echo "Error: Required library $lib not found" + exit 1 + fi + cp "$lib" src/build/lib/ + done + + if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then + echo "Error: BUILTIN_CONFIG.json not found" + exit 1 + fi cp bmf/output/bmf/BUILTIN_CONFIG.json src/build - touch src/build/activate_env.sh - echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh + cat > src/build/activate_env.sh << 'EOF' + #!/bin/bash + BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" + EOF + chmod +x src/build/activate_env.shLikely invalid or redundant comment.
25-41: 🛠️ Refactor suggestionImprove BMF repository setup and environment configuration.
- Use repository variables for BMF repository URL and branch
- Add error handling
- Optimize environment variable exports
- name: Checkout BMF repository (specific branch) + env: + BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }} + BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }} run: | + set -eo pipefail sudo apt update sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev - git clone https://github.com/JackLau1222/bmf.git - (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) + git clone "$BMF_REPO" bmf + cd bmf + git checkout "$BMF_BRANCH" + sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus + ./build.sh - cd bmf - echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib" + } >> "$GITHUB_ENV"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
26-26: shellcheck reported issue in this script: SC2129:style:12:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:12:70: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:13:78: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:14:62: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:15:68: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 31-31: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (7)
src/transcoder/src/transcoder_ffmpeg.cpp (5)
Line range hint
169-180: Logical error in return value checks incopyFrameIn
TranscoderFFmpeg::copyFrame, the return value checks forav_frame_copyandav_frame_copy_propsseem incorrect. Normally, a return value of zero indicates success, and negative values indicate an error. The current checks may lead to unexpected behavior.Correct the conditional statements to properly handle success and failure:
response = av_frame_copy(newFrame, oldFrame); - if (response >= 0) { + if (response < 0) { return false; } response = av_frame_copy_props(newFrame, oldFrame); - if (response == 0) { + if (response < 0) { return false; }
Line range hint
314-318: Error handling is incomplete after failing to find video codecIf
decoder->videoCodecisnullptr, the function logs an error but does not return or handle the error appropriately. This may lead to a null pointer dereference later.After logging the error, add a return statement to handle the failure:
if (!decoder->videoCodec) { av_log(NULL, AV_LOG_ERROR, "Couldn't find codec: %s \n", avcodec_get_name(decoder->videoStream->codecpar->codec_id)); return false; }
Line range hint
323-327: Error handling is incomplete after failing to find audio codecSimilarly, if
decoder->audioCodecisnullptr, the function logs an error but does not handle the error.After logging the error, add a return statement:
if (!decoder->audioCodec) { av_log(NULL, AV_LOG_ERROR, "Couldn't find codec: %s \n", avcodec_get_name(decoder->audioStream->codecpar->codec_id)); return false; }
Line range hint
330-334: Incorrect null check after allocatingdecoder->videoCodecCtxThe condition should check if
decoder->videoCodecCtxisnullptr, notdecoder->videoCodec.Correct the condition:
decoder->videoCodecCtx = avcodec_alloc_context3(decoder->videoCodec); - if (!decoder->videoCodec) { + if (!decoder->videoCodecCtx) { av_log(NULL, AV_LOG_ERROR, "No memory!\n"); return false; }
Line range hint
336-340: Incorrect null check after allocatingdecoder->audioCodecCtxSimilarly, the condition should check if
decoder->audioCodecCtxisnullptr, notdecoder->audioCodec.Correct the condition:
decoder->audioCodecCtx = avcodec_alloc_context3(decoder->audioCodec); - if (!decoder->audioCodec) { + if (!decoder->audioCodecCtx) { av_log(NULL, AV_LOG_ERROR, "No memory!\n"); return false; }src/common/include/process_parameter.h (1)
Line range hint
29-47: Inconsistency betweenprocessNumbertype and method signaturesThe member variable
processNumberis nowint64_t, butget_Process_Number()returnsdouble, and theupdate_Process_Numbersignal uses adoubleparameter. This inconsistency may lead to unintended type conversions.Update the method and signal to use
int64_t:- double get_Process_Number(); + int64_t get_Process_Number();And:
- void update_Process_Number(double result); + void update_Process_Number(int64_t result);Ensure that all calculations and usages of
processNumberare updated accordingly.src/transcoder/src/transcoder_bmf.cpp (1)
Line range hint
157-182: Add error handling for graph operations.The graph operations should include proper error handling and logging.
auto decoder = graph.Decode(bmf_sdk::JsonParam(decoder_para), "", scheduler_cnt++); + if (!decoder) { + BMFLOG(BMF_ERROR) << "Failed to create decoder node"; + return false; + } auto encoder = graph.Encode(decoder["video"], decoder["audio"], bmf_sdk::JsonParam(encoder_para), "", scheduler_cnt++); + if (!encoder) { + BMFLOG(BMF_ERROR) << "Failed to create encoder node"; + return false; + }
🧹 Nitpick comments (15)
src/common/include/stream_context.h (1)
32-32: Good improvement on const correctness!Making the string content const prevents accidental modifications of the filename string.
Consider these additional improvements:
- Make the pointer itself const if it's not meant to be reassigned:
const char * const filename = NULL;- Use
std::stringinstead of C-style strings for better safety and easier memory management:-const char *filename = NULL; +std::string filename;src/transcoder/include/transcoder.h (3)
25-26: Consider using smart pointers to manage resourcesUsing smart pointers like
std::unique_ptrorstd::shared_ptrforProcessParameterandEncodeParametercan help manage object lifetimes and prevent memory leaks.Apply this diff to update the constructor:
- Transcoder(ProcessParameter *processParameter, EncodeParameter *encodeParameter) + Transcoder(std::shared_ptr<ProcessParameter> processParameter, std::shared_ptr<EncodeParameter> encodeParameter)And update member variable declarations accordingly.
32-34: Usenullptrinstead ofNULLfor pointer initializationIn modern C++, it's recommended to use
nullptrinstead ofNULLfor pointer initialization to ensure type safety.Apply this diff to update the member variables:
- ProcessParameter *processParameter = NULL; - EncodeParameter *encodeParameter = NULL; + ProcessParameter *processParameter = nullptr; + EncodeParameter *encodeParameter = nullptr;
32-34: Encapsulate member variables by making them private or protectedThe member variables
processParameterandencodeParameterare currently public. It's a good practice to encapsulate member variables to maintain control over their access and modification.Apply this diff to modify the access specifier:
class Transcoder { public: Transcoder( /*...*/ ); virtual ~Transcoder(); virtual bool transcode(std::string input_path, std::string output_path) = 0; - ProcessParameter *processParameter = nullptr; - EncodeParameter *encodeParameter = nullptr; +protected: + ProcessParameter *processParameter = nullptr; + EncodeParameter *encodeParameter = nullptr; };src/transcoder/src/transcoder_ffmpeg.cpp (2)
27-67: Avoid usinggotostatements for error handlingThe use of
gotostatements for error handling can make the code harder to read and maintain. Consider refactoring the code to use structured error handling mechanisms, such as exceptions or scoped cleanup helpers, to manage resource deallocation.
Line range hint
351-399: Use of hardcoded values for frame rates and pixel formatsHardcoding values like frame rates
(AVRational){60, 1}and pixel formatsAV_PIX_FMT_YUV420Pmay reduce flexibility. Consider retrieving these values from the decoder or making them configurable.For example, you can set the encoder's
time_baseandframeratebased on the decoder's values:encoder->videoCodecCtx->time_base = decoder->videoCodecCtx->time_base; encoder->videoCodecCtx->framerate = decoder->videoCodecCtx->framerate;src/transcoder/include/transcoder_ffmpeg.h (1)
59-61: Document member variable usage.The boolean flags
copyVideoandcopyAudioneed documentation explaining their purpose and when they're modified.private: - // encoder's parameters + // Flags controlling stream handling behavior + // copyVideo: true to copy video stream without re-encoding + // copyAudio: true to copy audio stream without re-encoding bool copyVideo; bool copyAudio;src/transcoder/include/transcoder_bmf.h (1)
Line range hint
51-57: Consider encapsulating timing-related members.The timing-related variables and constants could be encapsulated in a separate timing manager class to improve modularity.
+class TimingManager { + std::vector<double> duration_history; + static constexpr size_t max_history_size = 20; + static constexpr double min_duration_threshold = 10.0; + std::chrono::system_clock::time_point last_encoder_call; + bool first_encoder_call = true; +public: + double compute_smooth_duration(double new_duration); + // ... other timing-related methods +};src/transcoder/src/transcoder_bmf.cpp (1)
4-7: Initialize member variables in constructor.Member variables should be initialized in the constructor's initialization list.
TranscoderBMF::TranscoderBMF(ProcessParameter *processParameter, EncodeParameter *encodeParameter) - : Transcoder(processParameter, encodeParameter) { - frameTotalNumber = 0; + : Transcoder(processParameter, encodeParameter) + , frameTotalNumber(0) + , frame_number_global(0) + , process_number(0) + , rest_time(0) + , first_encoder_call(true) {src/engine/src/converter.cpp (1)
3-9: Consider using CMake configuration options instead of preprocessor definitions.Since the project is moving to CMake, consider using CMake options to control which transcoder to use instead of preprocessor definitions.
Example CMake configuration:
option(USE_BMF "Use BMF transcoder" OFF) option(USE_FFMPEG "Use FFmpeg transcoder" ON) if(USE_BMF AND USE_FFMPEG) message(FATAL_ERROR "Only one transcoder can be selected") endif() if(NOT USE_BMF AND NOT USE_FFMPEG) message(FATAL_ERROR "At least one transcoder must be selected") endif()src/CMakeLists.txt (5)
1-1: Consider updating minimum CMake version.CMake 3.5 is quite old (2016). Modern Qt features and better CMake practices are available in newer versions. Consider updating to at least 3.16 for modern Qt support.
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.16)
23-28: Remove commented x264/x265 configuration.If x264/x265 support is not currently needed, remove these commented sections to keep the CMake configuration clean. If it will be needed in the future, consider adding a TODO comment instead.
75-88: Remove redundant commented source files section.This commented section is redundant with the active source files configuration above. Remove it to maintain cleaner CMake configuration.
114-119: Remove unnecessary commented Qt wrapper commands.These wrapper commands are redundant since
CMAKE_AUTOUIC,CMAKE_AUTOMOC, andCMAKE_AUTORCCare already enabled at the top of the file.
146-151: Remove commented FFmpeg library links.These commented library links are redundant since
${FFMPEG_LDFLAGS}already includes all necessary FFmpeg libraries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/CMakeLists.txt(1 hunks)src/common/include/process_parameter.h(1 hunks)src/common/include/stream_context.h(1 hunks)src/common/src/process_parameter.cpp(4 hunks)src/engine/include/converter.h(2 hunks)src/engine/src/converter.cpp(1 hunks)src/transcoder/include/transcoder.h(1 hunks)src/transcoder/include/transcoder_bmf.h(1 hunks)src/transcoder/include/transcoder_ffmpeg.h(2 hunks)src/transcoder/src/transcoder_bmf.cpp(2 hunks)src/transcoder/src/transcoder_ffmpeg.cpp(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/engine/include/converter.h
- src/common/src/process_parameter.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/common/include/stream_context.h (1)
32-32: Verify the impact of const correctness change.The change from
char*toconst char*could affect code that was previously modifying the filename string content.Let's verify the usage:
✅ Verification successful
The const correctness change is safe
The change from
char*toconst char*aligns with the actual usage in the codebase. All assignments tofilenameare fromconst char*sources (viac_str()), and no code modifies the string content through this pointer.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct modifications to StreamContext::filename ast-grep --pattern 'streamContext->filename' -A 2 rg -p "filename.*=" --type cpp # Check for any assignments or modifications ast-grep --pattern $'StreamContext { $$$ void $_($$_) { $$$ filename = $_ $$$ } $$$ }'Length of output: 657
src/transcoder/include/transcoder_ffmpeg.h (2)
30-34:⚠️ Potential issueConsider adding virtual destructor in base class.
Since
TranscoderFFmpeginherits fromTranscoder, the base class should declare a virtual destructor to ensure proper cleanup of derived classes.
36-36:⚠️ Potential issueConsider making
transcodemethod virtual.The
transcodemethod should be marked virtual in the base class to ensure proper polymorphic behavior.src/transcoder/include/transcoder_bmf.h (1)
26-26: Ensure consistent inheritance model with TranscoderFFmpeg.Both transcoder implementations inherit from the same base class. Verify that they follow the same patterns for virtual methods and destructor.
✅ Verification successful
Inheritance model is correctly implemented
The base class
Transcoderhas a virtual destructor and bothTranscoderBMFandTranscoderFFmpegproperly inherit from it. No changes needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Compare inheritance patterns between implementations ast-grep --pattern 'class TranscoderBMF : public Transcoder { $$$ virtual ~TranscoderBMF() $$$ $$$ }'Length of output: 109
Script:
#!/bin/bash # Find both transcoder class definitions echo "=== TranscoderBMF class definition ===" ast-grep --pattern 'class TranscoderBMF : public Transcoder { $$$ }' echo -e "\n=== TranscoderFFmpeg class definition ===" ast-grep --pattern 'class TranscoderFFmpeg : public Transcoder { $$$ }' echo -e "\n=== Searching for destructors ===" rg "~Transcoder(BMF|FFmpeg)" -A 2Length of output: 6668
Script:
#!/bin/bash # Find base Transcoder class definition echo "=== Base Transcoder class definition ===" ast-grep --pattern 'class Transcoder { $$$ }' # Also search for any virtual destructor echo -e "\n=== Searching for virtual destructors ===" rg "virtual.*~" -A 2Length of output: 1421
| StreamContext *decoder = new StreamContext; | ||
| StreamContext *encoder = new StreamContext; |
There was a problem hiding this comment.
Potential memory leak: decoder and encoder are not deleted
The dynamically allocated StreamContext objects decoder and encoder are not deleted, which may result in a memory leak. Consider deleting these objects to release the allocated memory.
Add the following code before the return flag; statement at the end of the transcode function:
if (decoder) {
delete decoder;
decoder = nullptr;
}
if (encoder) {
delete encoder;
encoder = nullptr;
}| if (!copyAudio) { | ||
|
|
||
| } else { | ||
| remux(decoder->pkt, encoder->fmtCtx, | ||
| decoder->audioStream, encoder->audioStream); | ||
| } |
There was a problem hiding this comment.
Incomplete implementation for audio transcoding
The if (!copyAudio) block is empty, indicating that the audio transcoding functionality is not implemented when copyAudio is false. This may result in the audio stream being omitted or processed incorrectly.
Consider implementing the audio transcoding logic within this block to handle audio data appropriately.
|
|
||
| private: | ||
| double processNumber; | ||
| int64_t processNumber; |
There was a problem hiding this comment.
Changing processNumber from double to int64_t may lead to loss of precision
If processNumber represents a progress percentage or a fractional value, changing its type to int64_t may cause loss of precision and affect progress calculations.
Consider keeping processNumber as double if it needs to represent fractional values, or ensure that all related calculations and methods accommodate the integer type.
Alternatively, update the return type of get_Process_Number() and the parameter of the update_Process_Number signal to int64_t for consistency if an integer type is appropriate.
| // bool Converter::transcode(char *src, char *dst) { | ||
| // bool flag = true; | ||
| // int ret = -1; | ||
| // // deal with arguments | ||
|
|
||
| // StreamContext *decoder = new StreamContext; | ||
| // StreamContext *encoder = new StreamContext; | ||
|
|
||
| // av_log_set_level(AV_LOG_DEBUG); | ||
|
|
||
| // decoder->filename = src; | ||
| // encoder->filename = dst; | ||
|
|
||
| // transcoder->open_Media(decoder, encoder); | ||
|
|
||
| // if (!transcoder->prepare_Decoder(decoder)) { | ||
| // flag = false; | ||
| // goto end; | ||
| // } | ||
|
|
||
| // if (!copyVideo) { | ||
| // if (!transcoder->prepare_Encoder_Video(decoder, encoder)) { | ||
| // flag = false; | ||
| // goto end; | ||
| // } | ||
| // } else { | ||
| // transcoder->prepare_Copy(encoder->fmtCtx, &encoder->videoStream, | ||
| // decoder->videoStream->codecpar); | ||
| // } | ||
|
|
||
| // if (!copyAudio) { | ||
| // if (!transcoder->prepare_Encoder_Audio(decoder, encoder)) { | ||
| // flag = false; | ||
| // goto end; | ||
| // } | ||
| // } else { | ||
| // transcoder->prepare_Copy(encoder->fmtCtx, &encoder->audioStream, | ||
| // decoder->audioStream->codecpar); | ||
| // } | ||
|
|
||
| // // binding | ||
| // ret = avio_open2(&encoder->fmtCtx->pb, encoder->filename, AVIO_FLAG_WRITE, | ||
| // NULL, NULL); | ||
| // if (ret < 0) { | ||
| // // av_log(encoder->fmtCtx, AV_LOG_ERROR, "%s", av_err2str(ret)); | ||
| // flag = false; | ||
| // goto end; | ||
| // } | ||
| // /* Write the stream header, if any. */ | ||
| // ret = avformat_write_header(encoder->fmtCtx, NULL); | ||
| // if (ret < 0) { | ||
| // // fprintf(stderr, "Error occurred when opening output file: %s\n", | ||
| // // av_err2str(ret)); | ||
| // flag = false; | ||
| // goto end; | ||
| // } | ||
|
|
||
| // // read video data from multimedia files to write into destination file | ||
| // while (av_read_frame(decoder->fmtCtx, decoder->pkt) >= 0) { | ||
| // if (decoder->pkt->stream_index == decoder->videoIdx) { | ||
| // if (!copyVideo) { | ||
| // transcoder->transcode_Video(decoder, encoder); | ||
| // } else { | ||
| // transcoder->remux(decoder->pkt, encoder->fmtCtx, | ||
| // decoder->videoStream, encoder->videoStream); | ||
| // } | ||
|
|
||
| // // encode(oFmtCtx, outCodecCtx, outFrame, outPkt, inStream, | ||
| // // outStream); | ||
| // } else if (decoder->pkt->stream_index == decoder->audioIdx) { | ||
| // if (!copyAudio) { | ||
|
|
||
| // } else { | ||
| // transcoder->remux(decoder->pkt, encoder->fmtCtx, | ||
| // decoder->audioStream, encoder->audioStream); | ||
| // } | ||
| // } | ||
| // } | ||
| // if (!copyVideo) { | ||
| // encoder->frame = NULL; | ||
| // // write the buffered frame | ||
| // transcoder->encode_Video(decoder->videoStream, encoder); | ||
| // } | ||
|
|
||
| // processParameter->set_Process_Number(1, 1); | ||
|
|
||
| // av_write_trailer(encoder->fmtCtx); | ||
|
|
||
| // // free memory | ||
| // end: | ||
| // if (decoder->fmtCtx) { | ||
| // avformat_close_input(&decoder->fmtCtx); | ||
| // decoder->fmtCtx = NULL; | ||
| // } | ||
| // if (decoder->videoCodecCtx) { | ||
| // avcodec_free_context(&decoder->videoCodecCtx); | ||
| // decoder->videoCodecCtx = NULL; | ||
| // } | ||
| // if (decoder->frame) { | ||
| // av_frame_free(&decoder->frame); | ||
| // decoder->frame = NULL; | ||
| // } | ||
| // if (decoder->pkt) { | ||
| // av_packet_free(&decoder->pkt); | ||
| // decoder->pkt = NULL; | ||
| // } | ||
|
|
||
| // if (encoder->fmtCtx && !(encoder->fmtCtx->oformat->flags & AVFMT_NOFILE)) { | ||
| // avio_closep(&encoder->fmtCtx->pb); | ||
| // } | ||
| // if (encoder->fmtCtx) { | ||
| // avformat_free_context(encoder->fmtCtx); | ||
| // encoder->fmtCtx = NULL; | ||
| // } | ||
| // if (encoder->videoCodecCtx) { | ||
| // avcodec_free_context(&encoder->videoCodecCtx); | ||
| // encoder->videoCodecCtx = NULL; | ||
| // } | ||
| // if (encoder->frame) { | ||
| // av_frame_free(&encoder->frame); | ||
| // encoder->frame = NULL; | ||
| // } | ||
| // if (encoder->pkt) { | ||
| // av_packet_free(&encoder->pkt); | ||
| // encoder->pkt = NULL; | ||
| // } | ||
| // return flag; | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
Large blocks of commented-out code should be removed. If this code needs to be preserved, it should be moved to version control history.
This commented-out implementation can be safely removed as it's been replaced by the new transcoder architecture.
| : processParameter(processParamter), encodeParameter(encodeParamter) { | ||
| #if defined(USE_BMF) | ||
| transcoder = new TranscoderBMF(this->processParameter, this->encodeParameter); | ||
| #elif defined(USE_FFMPEG) | ||
| transcoder = new TranscoderFFmpeg(this->processParameter, this->encodeParameter); | ||
| #endif | ||
|
|
||
|
|
||
| this->encodeParameter = encodeParamter; | ||
| } |
There was a problem hiding this comment.
Fix potential memory leak in transcoder initialization.
The transcoder pointer is not deleted in the destructor, which could lead to memory leaks.
+Converter::~Converter() {
+ delete transcoder;
+}Committable suggestion skipped: line range outside the PR's diff.
| target_link_libraries(OpenConverter | ||
| PRIVATE Qt${QT_VERSION_MAJOR}::Widgets | ||
| ${FFMPEG_LDFLAGS} | ||
| # avformat | ||
| # avcodec | ||
| # avutil | ||
| # pthread | ||
| # swresample | ||
|
|
||
| engine | ||
| bmf_module_sdk | ||
| hmp | ||
| builtin_modules | ||
| ) |
There was a problem hiding this comment.
Conditionally link BMF libraries based on USE_BMF_TRANSCODER.
BMF libraries are currently always linked, even when USE_BMF_TRANSCODER is OFF. This should be conditional.
target_link_libraries(OpenConverter
PRIVATE Qt${QT_VERSION_MAJOR}::Widgets
${FFMPEG_LDFLAGS}
- engine
- bmf_module_sdk
- hmp
- builtin_modules
)
+if(USE_BMF_TRANSCODER)
+ target_link_libraries(OpenConverter PRIVATE
+ engine
+ bmf_module_sdk
+ hmp
+ builtin_modules
+ )
+endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_link_libraries(OpenConverter | |
| PRIVATE Qt${QT_VERSION_MAJOR}::Widgets | |
| ${FFMPEG_LDFLAGS} | |
| # avformat | |
| # avcodec | |
| # avutil | |
| # pthread | |
| # swresample | |
| engine | |
| bmf_module_sdk | |
| hmp | |
| builtin_modules | |
| ) | |
| target_link_libraries(OpenConverter | |
| PRIVATE Qt${QT_VERSION_MAJOR}::Widgets | |
| ${FFMPEG_LDFLAGS} | |
| # avformat | |
| # avcodec | |
| # avutil | |
| # pthread | |
| # swresample | |
| ) | |
| if(USE_BMF_TRANSCODER) | |
| target_link_libraries(OpenConverter PRIVATE | |
| engine | |
| bmf_module_sdk | |
| hmp | |
| builtin_modules | |
| ) | |
| endif() |
| include_directories(${BMF_ROOT_PATH}/include) | ||
| link_directories(${BMF_ROOT_PATH}/lib) |
There was a problem hiding this comment.
💡 Codebase verification
BMF library existence checks are indeed missing
The codebase directly links against BMF libraries without verifying their existence. The suggested solution using find_library commands is appropriate and would prevent silent failures when BMF libraries are missing.
- In
src/CMakeLists.txt, replace the current link_directories approach with the suggested library checks.
🔗 Analysis chain
Add BMF library existence checks.
The code assumes BMF libraries exist at the specified location. Add proper checks to ensure required BMF libraries are available.
find_library(BMF_MODULE_SDK bmf_module_sdk PATHS ${BMF_ROOT_PATH}/lib REQUIRED)
find_library(HMP_LIB hmp PATHS ${BMF_ROOT_PATH}/lib REQUIRED)
find_library(BUILTIN_MODULES builtin_modules PATHS ${BMF_ROOT_PATH}/lib REQUIRED)
if(NOT BMF_MODULE_SDK OR NOT HMP_LIB OR NOT BUILTIN_MODULES)
message(FATAL_ERROR "Required BMF libraries not found")
endif()🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for existing BMF library checks
rg "find_library.*bmf" --type cmake -A 3 -B 3
# Look for BMF-related CMake configuration
fd "CMakeLists.txt|.cmake$" --exec grep -l "BMF" {}
# Search for any existing library checks or BMF configuration
rg "BMF.*lib|bmf_module_sdk|hmp|builtin_modules" --type cmake
Length of output: 419
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/CMakeLists.txt (6)
35-38:⚠️ Potential issueRemove hard-coded BMF path.
The default BMF path is system-specific and won't work across different environments.
40-41: 🛠️ Refactor suggestionUse target_include_directories instead of global include_directories.
Global include directories are discouraged in modern CMake. Use target-specific includes instead.
55-61: 🛠️ Refactor suggestionUse target_include_directories and avoid link_directories.
Global include directories and link_directories are discouraged in modern CMake.
64-73: 🛠️ Refactor suggestionUse ${CMAKE_CURRENT_SOURCE_DIR} instead of ${CMAKE_SOURCE_DIR}.
Using
${CMAKE_SOURCE_DIR}makes the project non-relocatable as a subdirectory.Also applies to: 90-101, 104-112
173-177:⚠️ Potential issueInclude GNUInstallDirs for installation paths.
The
CMAKE_INSTALL_LIBDIRandCMAKE_INSTALL_BINDIRvariables are undefined.
163-163: 🛠️ Refactor suggestionUse CMAKE_CURRENT_SOURCE_DIR for translation file path.
Similar to earlier paths, use
CMAKE_CURRENT_SOURCE_DIRfor better relocatability.
🧹 Nitpick comments (5)
src/CMakeLists.txt (5)
1-1: Consider using a more recent CMake version.CMake 3.5 is quite old (2016). Consider using at least 3.16 for better features and improved Qt support. Recent versions provide:
- Better dependency management
- Improved generator expressions
- Enhanced configure-time checks
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.16)
12-12: Consider defaulting USE_BMF_TRANSCODER to OFF.Optional features should typically default to OFF to ensure users explicitly opt-in. This prevents unexpected dependencies and build failures.
-option(USE_BMF_TRANSCODER "Use BMF Transcoder" ON) +option(USE_BMF_TRANSCODER "Use BMF Transcoder" OFF)
23-28: Remove commented x264/x265 configuration code.If x264/x265 support is planned for the future, consider adding a TODO comment instead of keeping commented code.
-## Use pkg-config to find x264 -#pkg_check_modules(X264 REQUIRED x264) - -## Use pkg-config to find x265 (if necessary) -#pkg_check_modules(X265 REQUIRED x265) +# TODO: Add support for x264/x265 encoders
75-88: Remove commented source files section.This section duplicates the source files already defined above. If it's no longer needed, it should be removed to maintain code clarity.
115-118: Remove redundant commented Qt wrapper commands.These commands are unnecessary when using
CMAKE_AUTOUIC,CMAKE_AUTOMOC, andCMAKE_AUTORCC.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/CMakeLists.txt (1)
52-53: 🛠️ Refactor suggestionUse target_include_directories and target_link_directories.
Global include and link directories are discouraged in modern CMake. Use target-specific commands instead.
- include_directories(${BMF_ROOT_PATH}/include) - link_directories(${BMF_ROOT_PATH}/lib) + target_include_directories(OpenConverter PRIVATE ${BMF_ROOT_PATH}/include) + target_link_directories(OpenConverter PRIVATE ${BMF_ROOT_PATH}/lib)
🧹 Nitpick comments (3)
src/CMakeLists.txt (3)
1-1: Consider updating minimum CMake version.CMake 3.5 is quite old (2016). Modern Qt features and better CMake functionality are available in newer versions. Consider updating to at least 3.16 for Qt 6 support or 3.21 for modern CMake features.
-cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.16)
76-85: Consider grouping source files by component.The current list structure makes it difficult to maintain. Consider using separate variables for each component (builder, common, engine, transcoder).
-list(APPEND SOURCES - ${CMAKE_SOURCE_DIR}/main.cpp - ${CMAKE_SOURCE_DIR}/builder/src/encode_setting.cpp - # ... other files -) +# Group sources by component +set(BUILDER_SOURCES + ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/encode_setting.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/builder/src/open_converter.cpp +) + +set(COMMON_SOURCES + ${CMAKE_CURRENT_SOURCE_DIR}/common/src/encode_parameter.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/common/src/info.cpp + # ... other common files +) + +list(APPEND SOURCES + ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp + ${BUILDER_SOURCES} + ${COMMON_SOURCES} +)Also applies to: 88-99
114-116: Remove commented-out code.These Qt wrapper commands are no longer needed as they're handled automatically by CMAKE_AUTOUIC, CMAKE_AUTOMOC, and CMAKE_AUTORCC.
-#qt_wrap_ui(UI_HEADERS ${UI_FILES}) -# qt_add_resources(RESOURCE_OBJECTS ${RESOURCE_FILES}) -# qt_wrap_cpp(MOC_HEADERS ${HEADERS})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)src/CMakeLists.txt(1 hunks)src/common/include/encode_parameter.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/common/include/encode_parameter.h
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| message(STATUS "FFMPEG_ROOT_PATH environment variable is not set, using default path.") | ||
| set(FFMPEG_ROOT_PATH "D:/ffmpeg/ffmpeg-n5.1.6-16-g6e63e49496-win64-gpl-shared-5.1/ffmpeg-n5.1.6-16-g6e63e49496-win64-gpl-shared-5.1") | ||
| endif() |
There was a problem hiding this comment.
Remove hardcoded FFmpeg path.
Using a hardcoded Windows path reduces portability and may fail on other systems. Consider using an environment variable or CMake's find_package mechanism.
- message(STATUS "FFMPEG_ROOT_PATH environment variable is not set, using default path.")
- set(FFMPEG_ROOT_PATH "D:/ffmpeg/ffmpeg-n5.1.6-16-g6e63e49496-win64-gpl-shared-5.1/ffmpeg-n5.1.6-16-g6e63e49496-win64-gpl-shared-5.1")
+ message(FATAL_ERROR "FFMPEG_ROOT_PATH environment variable must be set for Windows builds")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message(STATUS "FFMPEG_ROOT_PATH environment variable is not set, using default path.") | |
| set(FFMPEG_ROOT_PATH "D:/ffmpeg/ffmpeg-n5.1.6-16-g6e63e49496-win64-gpl-shared-5.1/ffmpeg-n5.1.6-16-g6e63e49496-win64-gpl-shared-5.1") | |
| endif() | |
| message(FATAL_ERROR "FFMPEG_ROOT_PATH environment variable must be set for Windows builds") | |
| endif() |
| target_link_libraries(OpenConverter | ||
| PRIVATE Qt${QT_VERSION_MAJOR}::Widgets | ||
| avformat | ||
| avcodec | ||
| avutil | ||
| pthread | ||
| swresample | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve FFmpeg library linking.
Direct library names should be replaced with variables from pkg-config or find_package results.
target_link_libraries(OpenConverter
PRIVATE Qt${QT_VERSION_MAJOR}::Widgets
- avformat
- avcodec
- avutil
- pthread
- swresample
+ ${FFMPEG_LIBRARIES}
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_link_libraries(OpenConverter | |
| PRIVATE Qt${QT_VERSION_MAJOR}::Widgets | |
| avformat | |
| avcodec | |
| avutil | |
| pthread | |
| swresample | |
| ) | |
| target_link_libraries(OpenConverter | |
| PRIVATE Qt${QT_VERSION_MAJOR}::Widgets | |
| ${FFMPEG_LIBRARIES} | |
| ) |
| # set_property(TARGET OpenConverter APPEND PROPERTY QT_ANDROID_PACKAGE_SOURCE_DIR | ||
| # ${CMAKE_CURRENT_SOURCE_DIR}/android) | ||
| # For more information, see https://doc.qt.io/qt-6/qt-add-executable.html#target-creation |
There was a problem hiding this comment.
💡 Codebase verification
Remove commented Android configuration template code
The commented Android configuration lines are just template examples from Qt's CMake setup. Since the project doesn't have any Android-specific files or actual Android support implementation, these comments should be removed to avoid confusion.
- Remove lines 124-126 (Qt 6 Android template comment)
- Remove lines 133-134 (Qt 5 Android template comment)
🔗 Analysis chain
Complete Android configuration.
Android-specific properties are commented out. Either complete the Android configuration or remove the commented code.
Also applies to: 133-134
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any Android-specific files in the project
fd -t f "android" -x echo "Found Android file: {}"
# Check if the project actually supports Android
rg -l "android|ANDROID" --type cmake
Length of output: 108
Script:
#!/bin/bash
# Get the complete content of CMakeLists.txt to check for other Android configurations
cat src/CMakeLists.txt
# Also search for any Qt-related Android configurations
rg -i "qt.*android|android.*qt" --type cmake
Length of output: 6086
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/release.yaml (3)
10-15: Remove commented workflow triggers.Remove the commented-out triggers to improve maintainability. If these triggers are needed in the future, they can be retrieved from version control history.
- # push: - # branches: - # - 'main' - # pull_request: - # branches: - # - '**'
65-68: Optimize environment variable exports.Use a more efficient approach to set multiple environment variables.
- echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib" + } >> "$GITHUB_ENV"
79-83: Remove debugging configuration.Remove the commented-out debugging configuration to improve maintainability.
- # - name: Start tmate session for debugging - # uses: mxschmitt/action-tmate@v3 - # with: - # sudo: true - # duration: 7200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks).github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
35-35: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
63-63: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
86-86: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
137-137: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
137-137: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
137-137: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
141-141: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
33-33: could not parse as YAML: yaml: line 33: mapping values are not allowed in this context
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[error] 147-147: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/review.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 33-33: syntax error: mapping values are not allowed here
(syntax)
🔇 Additional comments (15)
.github/workflows/review.yaml (10)
1-10: Rename workflow and upgrade Ubuntu versionThe workflow configuration needs updates:
- The name "Review" is misleading as this is a build workflow
- Ubuntu 20.04 is approaching EOL
🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
14-14: Update checkout action version
16-24: Use official BMF repository instead of personal fork🧰 Tools
🪛 yamllint (1.35.1)
[error] 22-22: trailing spaces
(trailing-spaces)
56-59: Fix environment variable concatenation
64-64: Remove unnecessary qmake installation
67-68: Fix CMake build command and add validation
73-78: Use pre-built linuxdeployqt
85-86: Remove continue-on-error for critical packaging step
91-98: Fix runtime library setup
113-116: Update upload-artifact action version.github/workflows/release.yaml (5)
23-23: 🛠️ Refactor suggestionUpdate GitHub Actions to latest versions.
Update
actions/checkoutto the latest version for improved security and features.- uses: actions/checkout@v3 + uses: actions/checkout@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
100-101:⚠️ Potential issueRemove continue-on-error from AppImage creation.
The AppImage creation should fail the workflow if unsuccessful, rather than silently continuing.
- (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) - continue-on-error: true + linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimageLikely invalid or redundant comment.
77-77: 🛠️ Refactor suggestionImprove build command reliability.
Add error handling and fix command substitution in the build command.
- (cd src && cmake -B build && cd build && make -j$(nproc)) + set -eo pipefail + cd src + cmake -B build + cd build + make -j"$(nproc)"Likely invalid or redundant comment.
32-32: 🛠️ Refactor suggestionUse repository variables for BMF configuration.
Replace hardcoded repository URL and branch with repository variables for better maintainability and flexibility.
- git clone https://github.com/JackLau1222/bmf.git + BMF_REPO="${BMF_REPO:-https://github.com/JackLau1222/bmf.git}" + git clone "$BMF_REPO"Likely invalid or redundant comment.
106-113: 🛠️ Refactor suggestionImprove runtime library management.
Add error handling and improve the environment script.
- # mkdir /src/build/lib - cp /usr/local/lib/libswscale.so.6 src/build/lib - cp /usr/local/lib/libavfilter.so.8 src/build/lib - cp /usr/local/lib/libpostproc.so.56 src/build/lib - cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib - cp bmf/output/bmf/BUILTIN_CONFIG.json src/build - touch src/build/activate_env.sh - echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh + set -eo pipefail + mkdir -p src/build/lib + + # Verify and copy libraries + for lib in \ + "/usr/local/lib/libswscale.so.6" \ + "/usr/local/lib/libavfilter.so.8" \ + "/usr/local/lib/libpostproc.so.56" \ + "bmf/output/bmf/lib/libbuiltin_modules.so" + do + if [ ! -f "$lib" ]; then + echo "Error: Required library $lib not found" + exit 1 + fi + cp "$lib" src/build/lib/ + done + + # Create environment script + cat > src/build/activate_env.sh << 'EOF' + #!/bin/bash + BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" + EOF + chmod +x src/build/activate_env.shLikely invalid or redundant comment.
.github/workflows/review.yaml
Outdated
| restore-keys: | | ||
| ${{ runner.os }}-ffmpeg- |
There was a problem hiding this comment.
Fix YAML syntax in cache configuration
The restore-keys field has incorrect indentation.
- restore-keys: |
- ${{ runner.os }}-ffmpeg-
+ restore-keys: |
+ ${{ runner.os }}-ffmpeg-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- | |
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- |
🧰 Tools
🪛 actionlint (1.7.4)
33-33: could not parse as YAML: yaml: line 33: mapping values are not allowed in this context
(syntax-check)
🪛 yamllint (1.35.1)
[error] 33-33: syntax error: mapping values are not allowed here
(syntax)
| run: | | ||
| BUILD_DIR="src/build" | ||
| PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz" | ||
| OUTPUT_DIR="OpenConverter_Linux_x86" | ||
| mkdir -p $OUTPUT_DIR | ||
| cp -r $BUILD_DIR/* $OUTPUT_DIR/ | ||
| tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR . | ||
| rm -rf $OUTPUT_DIR |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to package creation
The tar.gz package creation lacks proper error handling and validation.
BUILD_DIR="src/build"
PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz"
OUTPUT_DIR="OpenConverter_Linux_x86"
+
+# Validate source directory
+if [ ! -d "$BUILD_DIR" ]; then
+ echo "::error::Build directory not found: $BUILD_DIR"
+ exit 1
+fi
+
mkdir -p $OUTPUT_DIR
-cp -r $BUILD_DIR/* $OUTPUT_DIR/
+if ! cp -r $BUILD_DIR/* $OUTPUT_DIR/; then
+ echo "::error::Failed to copy build files"
+ exit 1
+fi
+
-tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .
+if ! tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .; then
+ echo "::error::Failed to create package"
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| BUILD_DIR="src/build" | |
| PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz" | |
| OUTPUT_DIR="OpenConverter_Linux_x86" | |
| mkdir -p $OUTPUT_DIR | |
| cp -r $BUILD_DIR/* $OUTPUT_DIR/ | |
| tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR . | |
| rm -rf $OUTPUT_DIR | |
| run: | | |
| BUILD_DIR="src/build" | |
| PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz" | |
| OUTPUT_DIR="OpenConverter_Linux_x86" | |
| # Validate source directory | |
| if [ ! -d "$BUILD_DIR" ]; then | |
| echo "::error::Build directory not found: $BUILD_DIR" | |
| exit 1 | |
| fi | |
| mkdir -p $OUTPUT_DIR | |
| if ! cp -r $BUILD_DIR/* $OUTPUT_DIR/; then | |
| echo "::error::Failed to copy build files" | |
| exit 1 | |
| fi | |
| if ! tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR .; then | |
| echo "::error::Failed to create package" | |
| exit 1 | |
| fi | |
| rm -rf $OUTPUT_DIR |
.github/workflows/release.yaml
Outdated
| git clone https://github.com/JackLau1222/bmf.git | ||
| - name: Cache FFmpeg build | ||
| uses: actions/cache@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update cache action versions.
Update both instances of actions/cache to the latest version.
- uses: actions/cache@v3
+ uses: actions/cache@v4Also applies to: 46-46
🧰 Tools
🪛 actionlint (1.7.4)
35-35: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| # Step to upload the tar.gz package as an artifact | ||
| - name: Upload build artifact | ||
| uses: actions/upload-artifact@v3 |
There was a problem hiding this comment.
Update deprecated syntax and actions.
Several updates are needed:
- Replace deprecated
set-outputcommand - Update GitHub Actions to latest versions
- Fix release asset path
- uses: actions/upload-artifact@v3
+ uses: actions/upload-artifact@v4
- run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})"
+ run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2
with:
- files: ./build/OpenConverter_Linux_x86.tar.gz
+ files: ./OpenConverter_Linux_x86.tar.gzAlso applies to: 137-137, 141-141
🧰 Tools
🪛 actionlint (1.7.4)
129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/review.yaml (3)
14-14: 🛠️ Refactor suggestionUpdate checkout action to v4
The checkout action version should be updated for better compatibility and security.
🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
64-64: 🛠️ Refactor suggestionRemove qmake-related packages
Since the PR's objective is to change from qmake to CMake, installing qmake-related packages is unnecessary.
113-116: 🛠️ Refactor suggestionUpdate upload-artifact action to v4
The upload-artifact action version should be updated for better compatibility and security.
🧰 Tools
🪛 actionlint (1.7.4)
113-113: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (1)
.github/workflows/review.yaml (1)
118-119: Add cleanup step and fix file format
- Add a cleanup step for temporary files
- Add a newline at the end of the file
Apply this diff:
- name: Finish - run: echo "Release upload complete" + run: | + echo "Release upload complete" + # Cleanup temporary files + rm -rf linuxdeployqt patchelf-0.9 patchelf-0.9.tar.bz2 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 119-119: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
37-37: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
54-54: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
71-71: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
113-113: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/review.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 119-119: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
.github/workflows/review.yaml (2)
1-10: 🛠️ Refactor suggestionUpdate workflow configuration for better maintainability
- The workflow name "Review" doesn't reflect its purpose as a build workflow
- Ubuntu 20.04 is approaching EOL (End of Life) in April 2024
Apply this diff to improve the configuration:
-name: Review +name: Build and Package on: pull_request: branches: - '**' jobs: build: - runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
21-21: Remove or uncomment the pip install commandThe commented pip install command suggests missing Python dependencies. Either remove it if unnecessary or uncomment if these dependencies are required.
.github/workflows/review.yaml
Outdated
| - name: Cache FFmpeg build | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: | | ||
| /usr/local/bin/ffmpeg | ||
| /usr/local/lib/ | ||
| /usr/local/include/ | ||
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-ffmpeg- | ||
| - name: Cache BMF build | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: bmf/output/ | ||
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-bmf- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update cache action to v4
Both instances of the cache action should be updated to v4 for better compatibility and security.
Apply this diff:
- uses: actions/cache@v3
+ uses: actions/cache@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Cache FFmpeg build | |
| uses: actions/cache@v3 | |
| with: | |
| path: | | |
| /usr/local/bin/ffmpeg | |
| /usr/local/lib/ | |
| /usr/local/include/ | |
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- | |
| - name: Cache BMF build | |
| uses: actions/cache@v3 | |
| with: | |
| path: bmf/output/ | |
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-bmf- | |
| - name: Cache FFmpeg build | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| /usr/local/bin/ffmpeg | |
| /usr/local/lib/ | |
| /usr/local/include/ | |
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- | |
| - name: Cache BMF build | |
| uses: actions/cache@v4 | |
| with: | |
| path: bmf/output/ | |
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-bmf- |
🧰 Tools
🪛 actionlint (1.7.4)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
37-37: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | ||
| tar xf patchelf-0.9.tar.bz2 | ||
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | ||
| # appimage | ||
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | ||
| sudo chmod a+x /usr/local/bin/appimagetool |
There was a problem hiding this comment.
Add checksum verification for downloaded binaries
Downloaded binaries should be verified using checksums to ensure integrity and security.
Add SHA256 verification:
+PATCHELF_SHA256="f2aa40a6148cb3b0ca807a1bf836b081793e55ec9e5540a5356d800132be7e0a"
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
+echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c
+APPIMAGETOOL_SHA256="df3baf5ca5facbecfc2f3fa6713c29ab9cefa8fd4992c78f35b635c9e27cf3e5"
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
+echo "$APPIMAGETOOL_SHA256 /usr/local/bin/appimagetool" | sha256sum -c📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | |
| tar xf patchelf-0.9.tar.bz2 | |
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | |
| # appimage | |
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | |
| sudo chmod a+x /usr/local/bin/appimagetool | |
| PATCHELF_SHA256="f2aa40a6148cb3b0ca807a1bf836b081793e55ec9e5540a5356d800132be7e0a" | |
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | |
| echo "$PATCHELF_SHA256 patchelf-0.9.tar.bz2" | sha256sum -c | |
| tar xf patchelf-0.9.tar.bz2 | |
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | |
| # appimage | |
| APPIMAGETOOL_SHA256="df3baf5ca5facbecfc2f3fa6713c29ab9cefa8fd4992c78f35b635c9e27cf3e5" | |
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | |
| echo "$APPIMAGETOOL_SHA256 /usr/local/bin/appimagetool" | sha256sum -c | |
| sudo chmod a+x /usr/local/bin/appimagetool |
fix blocker impact bug fix function wasn't implement update builder code
* fix cmake file for qt5 or other platform compiling add github CD fix github cd update cd Update .github/workflows/cd.yaml Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> add release test ignore error log update release.yaml fix ffmpeg lib path
* move LGPL LICENSE to proper place
add BUILTIN_CONFIG.json fix compressed file name fix LD_LIBRARY_PATH
…nal transcoder selection Refactor Transcoder class and update CMake configuration for conditional transcoder selection fix bmf transcoder libraries link method
* fix cmake for windows platform fix windows compile bug fix typo * optimizing code Co-authored-by: JackLau1222 <[email protected]> * optimizing code
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (3)
src/transcoder/include/transcoder_bmf.h (1)
Line range hint
26-59: Initialize member variables in the constructor.Several member variables are not properly initialized in the constructor, which could lead to undefined behavior. Consider initializing them in the constructor's initialization list:
TranscoderBMF(ProcessParameter *processParameter, EncodeParameter *encodeParamter) - : Transcoder(processParameter, encodeParamter); + : Transcoder(processParameter, encodeParamter), + frameTotalNumber(0), + copyVideo(false), + copyAudio(false), + frame_number_total(0), + frame_number_global(0), + process_number(0), + rest_time(0), + first_encoder_call(true) { +}src/transcoder/src/transcoder_ffmpeg.cpp (2)
Line range hint
162-186: Fix inverted return values in error handling.The function has incorrect logic for error conditions:
- Returns false when
av_frame_copysucceeds (ret >= 0)- Returns false when
av_frame_copy_propssucceeds (ret == 0)Apply this diff to fix the logic:
response = av_frame_copy(newFrame, oldFrame); - if (response >= 0) { + if (response < 0) { return false; } response = av_frame_copy_props(newFrame, oldFrame); - if (response == 0) { + if (response != 0) { return false; }
Line range hint
351-457: Parameterize hardcoded video encoding settings.The function uses hardcoded values for:
- Frame rates (60 fps)
- Pixel format (YUV420P)
These should be configurable through
encodeParameter.Apply this diff:
- encoder->videoCodecCtx->time_base = (AVRational){1, 60}; - encoder->videoCodecCtx->framerate = (AVRational){60, 1}; + encoder->videoCodecCtx->time_base = (AVRational){1, encodeParameter->get_Frame_Rate()}; + encoder->videoCodecCtx->framerate = (AVRational){encodeParameter->get_Frame_Rate(), 1}; - encoder->videoCodecCtx->pix_fmt = AV_PIX_FMT_YUV420P; + encoder->videoCodecCtx->pix_fmt = encodeParameter->get_Pixel_Format();Add these methods to the
EncodeParameterclass:int get_Frame_Rate() const; AVPixelFormat get_Pixel_Format() const;
♻️ Duplicate comments (1)
src/transcoder/src/transcoder_ffmpeg.cpp (1)
17-18:⚠️ Potential issueMemory leak:
decoderandencoderare not deleted.The dynamically allocated
StreamContextobjects are not deleted before returning.Add the following code before the
return flag;statement:+ if (decoder) { + delete decoder; + decoder = nullptr; + } + if (encoder) { + delete encoder; + encoder = nullptr; + } return flag;Also applies to: 138-139
🧹 Nitpick comments (6)
src/transcoder/include/transcoder.h (2)
19-21: Consider using angle brackets for non-local includes.The relative paths to common headers make the code less portable and more dependent on the project structure. Consider using angle brackets and configuring include paths in CMake:
-#include "../../common/include/process_parameter.h" -#include "../../common/include/encode_parameter.h" -#include "../../common/include/stream_context.h" +#include <common/process_parameter.h> +#include <common/encode_parameter.h> +#include <common/stream_context.h>
30-30: Optimize string parameter passing.Pass string parameters by const reference to avoid unnecessary copying:
- virtual bool transcode(std::string input_path, std::string output_path) = 0; + virtual bool transcode(const std::string& input_path, const std::string& output_path) = 0;src/transcoder/include/transcoder_ffmpeg.h (1)
Line range hint
36-50: Add method documentation.Methods lack documentation explaining their purpose, parameters, and return values. Add doxygen-style comments:
/** * Transcodes the input file to the output file. * @param input_path Path to the input media file * @param output_path Path to the output media file * @return true if transcoding was successful, false otherwise */ bool transcode(std::string input_path, std::string output_path); /** * Opens input and output media contexts. * @param decoder Decoder context to initialize * @param encoder Encoder context to initialize * @return true if successful, false otherwise */ bool open_Media(StreamContext *decoder, StreamContext *encoder);.github/workflows/release.yaml (3)
3-15: Clean up commented workflow triggers.Remove the commented-out triggers for better maintainability. The current active triggers (release creation and version tags) are appropriate for a release workflow.
on: release: types: - created push: tags: - "v*.*.*" - # push: - # branches: - # - 'main' - # pull_request: - # branches: - # - '**'
79-83: Remove commented debugging section.Remove the commented-out debugging configuration for cleaner maintenance.
- # - name: Start tmate session for debugging - # uses: mxschmitt/action-tmate@v3 - # with: - # sudo: true - # duration: 7200
146-147: Add newline at end of file.Add a newline character at the end of the file to comply with POSIX standards.
- name: Finish - run: echo "Release upload complete" + run: echo "Release upload complete" +🧰 Tools
🪛 yamllint (1.35.1)
[error] 147-147: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/version.logis excluded by!**/*.log
📒 Files selected for processing (27)
.github/workflows/ci.yaml(0 hunks).github/workflows/release.yaml(1 hunks).github/workflows/review.yaml(1 hunks).gitignore(1 hunks)src/CMakeLists.txt(1 hunks)src/builder/include/encode_setting.h(1 hunks)src/builder/include/open_converter.h(1 hunks)src/builder/src/encode_setting.cpp(1 hunks)src/builder/src/open_converter.cpp(1 hunks)src/common/include/encode_parameter.h(1 hunks)src/common/include/process_parameter.h(1 hunks)src/common/include/stream_context.h(1 hunks)src/common/src/encode_parameter.cpp(1 hunks)src/common/src/info.cpp(1 hunks)src/common/src/process_parameter.cpp(4 hunks)src/common/src/stream_context.cpp(1 hunks)src/converter.cpp(0 hunks)src/engine/include/converter.h(2 hunks)src/engine/src/converter.cpp(1 hunks)src/main.cpp(2 hunks)src/open_converter.pro(0 hunks)src/transcoder/LICENSE(1 hunks)src/transcoder/include/transcoder.h(1 hunks)src/transcoder/include/transcoder_bmf.h(1 hunks)src/transcoder/include/transcoder_ffmpeg.h(2 hunks)src/transcoder/src/transcoder_bmf.cpp(2 hunks)src/transcoder/src/transcoder_ffmpeg.cpp(9 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/ci.yaml
- src/open_converter.pro
- src/converter.cpp
🚧 Files skipped from review as they are similar to previous changes (18)
- src/common/include/encode_parameter.h
- src/common/include/stream_context.h
- src/engine/include/converter.h
- src/common/src/info.cpp
- src/transcoder/LICENSE
- src/main.cpp
- src/builder/include/encode_setting.h
- src/common/include/process_parameter.h
- src/builder/src/encode_setting.cpp
- src/common/src/stream_context.cpp
- src/common/src/process_parameter.cpp
- src/transcoder/src/transcoder_bmf.cpp
- .gitignore
- src/engine/src/converter.cpp
- src/common/src/encode_parameter.cpp
- src/builder/src/open_converter.cpp
- src/builder/include/open_converter.h
- src/CMakeLists.txt
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
35-35: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
63-63: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
86-86: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
137-137: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
137-137: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
137-137: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
141-141: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
37-37: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
54-54: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
54-54: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
67-67: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
71-71: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
113-113: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[error] 147-147: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/review.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
[error] 119-119: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
.github/workflows/release.yaml (5)
22-23: 🛠️ Refactor suggestionUpdate GitHub Actions to latest versions.
Update the checkout action to the latest version for improved security and features.
- name: Checkout code - uses: actions/checkout@v3 + uses: actions/checkout@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
128-132: 🛠️ Refactor suggestionUpdate artifact upload action.
Update to the latest version of the upload-artifact action.
- name: Upload build artifact - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: OpenConverter_Linux_x86 path: OpenConverter_Linux_x86.tar.gzLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
135-144:⚠️ Potential issueFix release asset upload.
Update the release action and fix deprecated commands and paths.
- name: Get GitHub Release information id: release_info - run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})" + run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT - name: Upload Release Asset - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 if: startsWith(github.ref, 'refs/tags/') with: - files: ./build/OpenConverter_Linux_x86.tar.gz + files: ./OpenConverter_Linux_x86.tar.gzLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
137-137: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
137-137: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
137-137: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
141-141: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
71-77: 🛠️ Refactor suggestionImprove build reliability.
Add error handling and pin Qt package versions for reproducible builds.
- name: Set up Qt run: | + set -eo pipefail - sudo apt-get install -y qt5-qmake qtbase5-dev qtchooser qtbase5-dev-tools cmake build-essential + sudo apt-get install -y \ + qt5-qmake=5.12.8* \ + qtbase5-dev=5.12.8* \ + qtchooser=66-2* \ + qtbase5-dev-tools=5.12.8* \ + cmake \ + build-essential - name: Build with CMake run: | + set -eo pipefail - (cd src && cmake -B build && cd build && make -j$(nproc)) + cd src + cmake -B build + cd build + make -j"$(nproc)"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
76-76: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
85-101:⚠️ Potential issueFix critical issues in AppImage creation.
Several critical issues need attention:
- Unsafe PATH manipulation
- Using continue-on-error masks potential issues
- Missing error handling
- name: Copy libs run: | + set -eo pipefail # linuxdeployqt sudo apt-get -y install git g++ libgl1-mesa-dev git clone https://github.com/probonopd/linuxdeployqt.git - # Then build in Qt Creator, or use - export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH + + # Set Qt path safely + qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true + if [ -n "$qt_path" ]; then + export PATH="$qt_path:$PATH" + fi + (cd linuxdeployqt && qmake && make && sudo make install) # patchelf wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 tar xf patchelf-0.9.tar.bz2 ( cd patchelf-0.9/ && ./configure && make && sudo make install ) # appimage sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool sudo chmod a+x /usr/local/bin/appimagetool - (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) - continue-on-error: true + linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimageLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
86-86: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
src/transcoder/include/transcoder_bmf.h (2)
19-19: LGTM!The include directive for the base class header is correctly placed before other includes.
26-26: LGTM!The class correctly inherits from the
Transcoderbase class using public inheritance.
| ProcessParameter *processParameter = NULL; | ||
|
|
||
| EncodeParameter *encodeParameter = NULL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate member variables.
Public member variables break encapsulation. Consider making them protected and providing accessor methods:
- ProcessParameter *processParameter = NULL;
- EncodeParameter *encodeParameter = NULL;
+protected:
+ std::shared_ptr<ProcessParameter> processParameter;
+ std::shared_ptr<EncodeParameter> encodeParameter;
+public:
+ ProcessParameter* getProcessParameter() const { return processParameter.get(); }
+ EncodeParameter* getEncodeParameter() const { return encodeParameter.get(); }Committable suggestion skipped: line range outside the PR's diff.
| Transcoder(ProcessParameter *processParameter, EncodeParameter *encodeParameter) | ||
| : processParameter(processParameter), encodeParameter(encodeParameter) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using smart pointers for better memory management.
Raw pointers don't convey ownership semantics. Consider using std::shared_ptr or std::unique_ptr:
- Transcoder(ProcessParameter *processParameter, EncodeParameter *encodeParameter)
- : processParameter(processParameter), encodeParameter(encodeParameter) {}
+ Transcoder(std::shared_ptr<ProcessParameter> processParameter,
+ std::shared_ptr<EncodeParameter> encodeParameter)
+ : processParameter(std::move(processParameter))
+ , encodeParameter(std::move(encodeParameter)) {}Committable suggestion skipped: line range outside the PR's diff.
| #include "stream_context.h" | ||
|
|
||
| #define ENCODE_BIT_RATE 5000000 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define ENCODE_BIT_RATE as a configurable parameter.
Hard-coded bit rate limits flexibility. Consider making it configurable:
-#define ENCODE_BIT_RATE 5000000
+// Remove the macro and add to class
+class TranscoderFFmpeg : public Transcoder {
+public:
+ static constexpr int64_t DEFAULT_ENCODE_BIT_RATE = 5000000;
+private:
+ int64_t encodeBitRate = DEFAULT_ENCODE_BIT_RATE;
+public:
+ void setEncodeBitRate(int64_t bitRate) { encodeBitRate = bitRate; }Committable suggestion skipped: line range outside the PR's diff.
| - name: Checkout BMF repository (specific branch) | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | ||
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | ||
| # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa | ||
| git clone https://github.com/JackLau1222/bmf.git | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use repository variables for external dependencies.
Move the hardcoded BMF repository URL to repository variables for better maintainability and security.
- name: Checkout BMF repository (specific branch)
+ env:
+ BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }}
+ BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }}
run: |
sudo apt update
sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1
sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev
- git clone https://github.com/JackLau1222/bmf.git
+ git clone "$BMF_REPO" bmf
+ cd bmf
+ git checkout "$BMF_BRANCH"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout BMF repository (specific branch) | |
| run: | | |
| sudo apt update | |
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | |
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | |
| # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa | |
| git clone https://github.com/JackLau1222/bmf.git | |
| - name: Checkout BMF repository (specific branch) | |
| env: | |
| BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }} | |
| BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }} | |
| run: | | |
| sudo apt update | |
| sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 | |
| sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev | |
| # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa | |
| git clone "$BMF_REPO" bmf | |
| cd bmf | |
| git checkout "$BMF_BRANCH" |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 31-31: trailing spaces
(trailing-spaces)
| - name: Set up BMF | ||
| run: | | ||
| cd bmf | ||
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | ||
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | ||
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix environment variable setup.
The current environment variable setup has shell scripting issues and could be more maintainable.
- name: Set up BMF
run: |
cd bmf
- echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
- echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV
- echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
- echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV
+ {
+ echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include"
+ echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include"
+ echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib"
+ echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib"
+ } >> "$GITHUB_ENV"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Set up BMF | |
| run: | | |
| cd bmf | |
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | |
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV | |
| echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | |
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV | |
| - name: Set up BMF | |
| run: | | |
| cd bmf | |
| { | |
| echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include" | |
| echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include" | |
| echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib" | |
| echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib" | |
| } >> "$GITHUB_ENV" |
🧰 Tools
🪛 actionlint (1.7.4)
63-63: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/release.yaml
Outdated
| - name: Cache FFmpeg build | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: | | ||
| /usr/local/bin/ffmpeg | ||
| /usr/local/lib/ | ||
| /usr/local/include/ | ||
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-ffmpeg- | ||
| - name: Cache BMF build | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: bmf/output/ | ||
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-bmf- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update cache actions and optimize cache keys.
The cache actions are outdated and the cache keys could be more specific.
- name: Cache FFmpeg build
- uses: actions/cache@v3
+ uses: actions/cache@v4
with:
path: |
/usr/local/bin/ffmpeg
/usr/local/lib/
/usr/local/include/
- key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
+ key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh', 'bmf/scripts/deps/*.sh') }}
restore-keys: |
${{ runner.os }}-ffmpeg-
- name: Cache BMF build
- uses: actions/cache@v3
+ uses: actions/cache@v4
with:
path: bmf/output/
- key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
+ key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh', 'bmf/CMakeLists.txt') }}
restore-keys: |
${{ runner.os }}-bmf-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Cache FFmpeg build | |
| uses: actions/cache@v3 | |
| with: | |
| path: | | |
| /usr/local/bin/ffmpeg | |
| /usr/local/lib/ | |
| /usr/local/include/ | |
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- | |
| - name: Cache BMF build | |
| uses: actions/cache@v3 | |
| with: | |
| path: bmf/output/ | |
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-bmf- | |
| - name: Cache FFmpeg build | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| /usr/local/bin/ffmpeg | |
| /usr/local/lib/ | |
| /usr/local/include/ | |
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh', 'bmf/scripts/deps/*.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- | |
| - name: Cache BMF build | |
| uses: actions/cache@v4 | |
| with: | |
| path: bmf/output/ | |
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh', 'bmf/CMakeLists.txt') }} | |
| restore-keys: | | |
| ${{ runner.os }}-bmf- |
🧰 Tools
🪛 actionlint (1.7.4)
35-35: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: Copy runtime | ||
| run: | | ||
| # mkdir /src/build/lib | ||
| cp /usr/local/lib/libswscale.so.6 src/build/lib | ||
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | ||
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | ||
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | ||
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | ||
| touch src/build/activate_env.sh | ||
| echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve runtime library management.
Add error handling and verification for library copying operations.
- name: Copy runtime
run: |
+ set -eo pipefail
- # mkdir /src/build/lib
+ mkdir -p src/build/lib
+
+ # Verify libraries exist before copying
+ LIBS=(
+ "/usr/local/lib/libswscale.so.6"
+ "/usr/local/lib/libavfilter.so.8"
+ "/usr/local/lib/libpostproc.so.56"
+ "bmf/output/bmf/lib/libbuiltin_modules.so"
+ )
+
+ for lib in "${LIBS[@]}"; do
+ if [ ! -f "$lib" ]; then
+ echo "Error: Required library $lib not found"
+ exit 1
+ fi
+ cp "$lib" src/build/lib/
+ done
+
+ if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then
+ echo "Error: BUILTIN_CONFIG.json not found"
+ exit 1
+ fi
cp bmf/output/bmf/BUILTIN_CONFIG.json src/build
- touch src/build/activate_env.sh
- echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh
+
+ cat > src/build/activate_env.sh << 'EOF'
+ #!/bin/bash
+ BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+ export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
+ EOF
+ chmod +x src/build/activate_env.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Copy runtime | |
| run: | | |
| # mkdir /src/build/lib | |
| cp /usr/local/lib/libswscale.so.6 src/build/lib | |
| cp /usr/local/lib/libavfilter.so.8 src/build/lib | |
| cp /usr/local/lib/libpostproc.so.56 src/build/lib | |
| cp bmf/output/bmf/lib/libbuiltin_modules.so src/build/lib | |
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | |
| touch src/build/activate_env.sh | |
| echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh | |
| - name: Copy runtime | |
| run: | | |
| set -eo pipefail | |
| mkdir -p src/build/lib | |
| # Verify libraries exist before copying | |
| LIBS=( | |
| "/usr/local/lib/libswscale.so.6" | |
| "/usr/local/lib/libavfilter.so.8" | |
| "/usr/local/lib/libpostproc.so.56" | |
| "bmf/output/bmf/lib/libbuiltin_modules.so" | |
| ) | |
| for lib in "${LIBS[@]}"; do | |
| if [ ! -f "$lib" ]; then | |
| echo "Error: Required library $lib not found" | |
| exit 1 | |
| fi | |
| cp "$lib" src/build/lib/ | |
| done | |
| if [ ! -f "bmf/output/bmf/BUILTIN_CONFIG.json" ]; then | |
| echo "Error: BUILTIN_CONFIG.json not found" | |
| exit 1 | |
| fi | |
| cp bmf/output/bmf/BUILTIN_CONFIG.json src/build | |
| cat > src/build/activate_env.sh << 'EOF' | |
| #!/bin/bash | |
| BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" | |
| EOF | |
| chmod +x src/build/activate_env.sh |
| if [ ! -f "/usr/local/bin/ffmpeg" ]; then | ||
| echo "FFmpeg not found, starting build..." | ||
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) | ||
| else | ||
| echo "FFmpeg is already installed, skipping build." | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for BMF build.
The BMF build step lacks proper error handling. If the build fails, the workflow continues without proper error reporting.
- if [ ! -f "/usr/local/bin/ffmpeg" ]; then
- echo "FFmpeg not found, starting build..."
- (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh )
- else
- echo "FFmpeg is already installed, skipping build."
- fi
+ if [ ! -f "/usr/local/bin/ffmpeg" ]; then
+ echo "FFmpeg not found, starting build..."
+ (
+ cd bmf || exit 1
+ git checkout fork_by_oc || exit 1
+ if ! sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus; then
+ echo "::error::FFmpeg build failed"
+ exit 1
+ fi
+ if ! ./build.sh; then
+ echo "::error::BMF build failed"
+ exit 1
+ fi
+ )
+ else
+ echo "FFmpeg is already installed, skipping build."
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -f "/usr/local/bin/ffmpeg" ]; then | |
| echo "FFmpeg not found, starting build..." | |
| (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) | |
| else | |
| echo "FFmpeg is already installed, skipping build." | |
| fi | |
| if [ ! -f "/usr/local/bin/ffmpeg" ]; then | |
| echo "FFmpeg not found, starting build..." | |
| ( | |
| cd bmf || exit 1 | |
| git checkout fork_by_oc || exit 1 | |
| if ! sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus; then | |
| echo "::error::FFmpeg build failed" | |
| exit 1 | |
| fi | |
| if ! ./build.sh; then | |
| echo "::error::BMF build failed" | |
| exit 1 | |
| fi | |
| ) | |
| else | |
| echo "FFmpeg is already installed, skipping build." | |
| fi |
| bool TranscoderFFmpeg::prepare_Encoder_Audio(StreamContext *decoder, | ||
| StreamContext *encoder) { | ||
| // TODO | ||
| return true; |
There was a problem hiding this comment.
Implement audio encoder preparation.
The function is marked with TODO and returns true without implementation, which could lead to silent failures when audio encoding is required.
Would you like me to help implement the audio encoder preparation or create a GitHub issue to track this task?
| EncodeParameter *encodeParameter) | ||
| : Transcoder(processParameter, encodeParameter) { | ||
| frameTotalNumber = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize all member variables in the constructor.
The constructor only initializes frameTotalNumber. Other member variables should be initialized to prevent undefined behavior:
TranscoderFFmpeg::TranscoderFFmpeg(ProcessParameter *processParameter,
EncodeParameter *encodeParameter)
- : Transcoder(processParameter, encodeParameter) {
- frameTotalNumber = 0;
+ : Transcoder(processParameter, encodeParameter),
+ frameTotalNumber(0),
+ copyVideo(false),
+ copyAudio(false) {
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TranscoderFFmpeg::TranscoderFFmpeg(ProcessParameter *processParameter, | |
| EncodeParameter *encodeParameter) | |
| : Transcoder(processParameter, encodeParameter) { | |
| frameTotalNumber = 0; | |
| } | |
| TranscoderFFmpeg::TranscoderFFmpeg(ProcessParameter *processParameter, | |
| EncodeParameter *encodeParameter) | |
| : Transcoder(processParameter, encodeParameter), | |
| frameTotalNumber(0), | |
| copyVideo(false), | |
| copyAudio(false) { | |
| } |
fix syntax error temporarily disable compile cache
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/review.yaml (1)
25-46: Enable dependency caching to improve workflow performanceUncomment and implement the caching configuration to speed up the workflow. Update the cache action version:
- # uses: actions/cache@v3 + uses: actions/cache@v4.github/workflows/release.yaml (1)
3-15: Remove commented workflow triggers.The commented workflow triggers for branch pushes and PRs should be removed to maintain clean and maintainable code.
on: release: types: - created push: tags: - "v*.*.*" - # push: - # branches: - # - 'main' - # pull_request: - # branches: - # - '**'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks).github/workflows/review.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yaml
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
63-63: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
86-86: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
137-137: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
137-137: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
137-137: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
141-141: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/review.yaml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
58-58: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
58-58: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
71-71: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
75-75: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
117-117: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/release.yaml
[error] 1-1: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 78-78: trailing spaces
(trailing-spaces)
[error] 102-102: trailing spaces
(trailing-spaces)
[error] 147-147: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/review.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 91-91: trailing spaces
(trailing-spaces)
[error] 123-123: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
.github/workflows/review.yaml (8)
89-90:⚠️ Potential issueRemove continue-on-error for critical AppImage creation
The AppImage creation is a critical step and should not silently continue on failure:
- (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) - continue-on-error: true + if ! linuxdeployqt "./src/build/OpenConverter" -appimage; then + echo "::error::Failed to create AppImage" + exit 1 + fiLikely invalid or redundant comment.
14-14: 🛠️ Refactor suggestionUpdate checkout action version
- uses: actions/checkout@v3 + uses: actions/checkout@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
117-120: 🛠️ Refactor suggestionUpdate upload-artifact action version
- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
117-117: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
60-63:⚠️ Potential issueFix environment variable concatenation
Current implementation is unsafe and prone to word splitting:
- echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+"$C_INCLUDE_PATH:"}$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+"$CPLUS_INCLUDE_PATH:"}$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH:+"$LIBRARY_PATH:"}$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+"$LD_LIBRARY_PATH:"}$(pwd)/output/bmf/lib" + } >> "$GITHUB_ENV"Likely invalid or redundant comment.
23-23:⚠️ Potential issueUse official BMF repository for better maintainability
Using a personal fork creates maintenance and security risks. Consider using the official repository:
- git clone https://github.com/JackLau1222/bmf.git + git clone https://github.com/BabitMF/bmf.gitLikely invalid or redundant comment.
1-10: 🛠️ Refactor suggestionUpdate workflow configuration for better maintainability
- Rename the workflow to better reflect its purpose
- Upgrade to Ubuntu 22.04 as 20.04 is approaching EOL
-name: Review +name: PR Build on: pull_request: branches: - '**' jobs: build: - runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: trailing spaces
(trailing-spaces)
50-55:⚠️ Potential issueAdd proper error handling for FFmpeg build
The current implementation lacks error handling:
- if [ ! -f "/usr/local/bin/ffmpeg" ]; then - echo "FFmpeg not found, starting build..." - (cd bmf && git checkout fork_by_oc && sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus && ./build.sh ) + if [ ! -f "/usr/local/bin/ffmpeg" ]; then + echo "FFmpeg not found, starting build..." + ( + cd bmf || exit 1 + if ! sudo scripts/build_ffmpeg.sh nasm yasm x264 x265 opus; then + echo "::error::FFmpeg build failed" + exit 1 + fi + if ! ./build.sh; then + echo "::error::BMF build failed" + exit 1 + fi + )Likely invalid or redundant comment.
106-113:⚠️ Potential issueAdd error handling to package creation
The package creation lacks proper error handling and validation:
BUILD_DIR="src/build" PACKAGE_NAME="OpenConverter_Linux_x86.tar.gz" OUTPUT_DIR="OpenConverter_Linux_x86" + +# Validate source directory +if [ ! -d "$BUILD_DIR" ]; then + echo "::error::Build directory not found: $BUILD_DIR" + exit 1 +fi + mkdir -p $OUTPUT_DIR -cp -r $BUILD_DIR/* $OUTPUT_DIR/ +if ! cp -r "$BUILD_DIR"/* "$OUTPUT_DIR"/; then + echo "::error::Failed to copy build files" + exit 1 +fi + -tar -czvf $PACKAGE_NAME -C $OUTPUT_DIR . +if ! tar -czvf "$PACKAGE_NAME" -C "$OUTPUT_DIR" .; then + echo "::error::Failed to create package" + exit 1 +fiLikely invalid or redundant comment.
.github/workflows/release.yaml (7)
53-60: LGTM!The FFmpeg and BMF compilation step includes proper existence checks and error handling.
22-23: 🛠️ Refactor suggestionUpdate GitHub Actions checkout version.
The
actions/checkoutaction version is outdated. Update to the latest version for improved security and features.- uses: actions/checkout@v3 + uses: actions/checkout@v4Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[warning] 22-22: wrong indentation: expected 6 but found 4
(indentation)
62-68: 🛠️ Refactor suggestionFix environment variable setup.
The current environment variable setup has shell scripting issues that need to be addressed.
- name: Set up BMF run: | cd bmf - echo "C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include" >> $GITHUB_ENV - echo "LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV - echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib" >> $GITHUB_ENV + { + echo "C_INCLUDE_PATH=${C_INCLUDE_PATH:+$C_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH:+$CPLUS_INCLUDE_PATH:}$(pwd)/output/bmf/include" + echo "LIBRARY_PATH=${LIBRARY_PATH:+$LIBRARY_PATH:}$(pwd)/output/bmf/lib" + echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(pwd)/output/bmf/lib" + } >> "$GITHUB_ENV"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
63-63: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:2:70: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:3:78: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:4:62: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:5:68: Double quote to prevent globbing and word splitting
(shellcheck)
128-132: 🛠️ Refactor suggestionUpdate artifact upload action version.
The
actions/upload-artifactaction version is outdated.- name: Upload build artifact - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: OpenConverter_Linux_x86 path: OpenConverter_Linux_x86.tar.gzLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
129-129: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
135-144: 🛠️ Refactor suggestionFix release asset upload configuration.
Multiple issues need to be addressed in the release asset upload configuration.
- name: Get GitHub Release information id: release_info - run: echo "::set-output name=release_tag::$(echo ${GITHUB_REF#refs/tags/})" + run: echo "release_tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT - name: Upload Release Asset - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 if: startsWith(github.ref, 'refs/tags/') with: - files: ./build/OpenConverter_Linux_x86.tar.gz + files: ./OpenConverter_Linux_x86.tar.gzLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
137-137: shellcheck reported issue in this script: SC2116:style:1:38: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
(shellcheck)
137-137: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
137-137: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
141-141: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
25-33: 🛠️ Refactor suggestionUse repository variables for external dependencies.
Move the hardcoded BMF repository URL to repository variables for better maintainability and security.
- name: Checkout BMF repository (specific branch) + env: + BMF_REPO: ${{ vars.BMF_REPOSITORY_URL || 'https://github.com/JackLau1222/bmf.git' }} + BMF_BRANCH: ${{ vars.BMF_BRANCH || 'fork_by_oc' }} run: | sudo apt update sudo apt install -y make git pkg-config libssl-dev cmake binutils-dev libgoogle-glog-dev gcc g++ golang wget libgl1 sudo apt install -y python3.9 python3-dev python3-pip libsndfile1 libsndfile1-dev - # sudo python3 -m pip install timeout_decorator numpy onnxruntime pytest opencv-python librosa - git clone https://github.com/JackLau1222/bmf.git + git clone "$BMF_REPO" bmf + cd bmf + git checkout "$BMF_BRANCH"Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 31-31: trailing spaces
(trailing-spaces)
104-113: 🛠️ Refactor suggestionImprove runtime library management.
Add error handling for library copying and improve the environment activation script.
- name: Copy runtime run: | + set -eo pipefail - # mkdir /src/build/lib + mkdir -p src/build/lib + + # Verify libraries exist before copying + LIBS=( + "/usr/local/lib/libswscale.so.6" + "/usr/local/lib/libavfilter.so.8" + "/usr/local/lib/libpostproc.so.56" + "bmf/output/bmf/lib/libbuiltin_modules.so" + ) + + for lib in "${LIBS[@]}"; do + if [ ! -f "$lib" ]; then + echo "Error: Required library $lib not found" + exit 1 + fi + cp "$lib" src/build/lib/ + done + cp bmf/output/bmf/BUILTIN_CONFIG.json src/build - touch src/build/activate_env.sh - echo export LD_LIBRARY_PATH="./lib" >> src/build/activate_env.sh + cat > src/build/activate_env.sh << 'EOF' + #!/bin/bash + BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + export LD_LIBRARY_PATH="${BASEDIR}/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" + EOF + chmod +x src/build/activate_env.shLikely invalid or redundant comment.
| # - name: Cache FFmpeg build | ||
| # uses: actions/cache@v3 | ||
| # with: | ||
| # path: | | ||
| # /usr/local/bin/ffmpeg | ||
| # /usr/local/lib/ | ||
| # /usr/local/include/ | ||
| # key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | ||
| # restore-keys: | | ||
| # ${{ runner.os }}-ffmpeg- | ||
|
|
||
| # - name: Cache BMF build | ||
| # uses: actions/cache@v3 | ||
| # with: | ||
| # path: bmf/output/ | ||
| # key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | ||
| # restore-keys: | | ||
| # ${{ runner.os }}-bmf- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable and update build caching.
Enable build caching with the latest cache action version to improve workflow performance.
- # - name: Cache FFmpeg build
- # uses: actions/cache@v3
+ - name: Cache FFmpeg build
+ uses: actions/cache@v4
with:
path: |
/usr/local/bin/ffmpeg
/usr/local/lib/
/usr/local/include/
key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }}
restore-keys: |
${{ runner.os }}-ffmpeg-
- # - name: Cache BMF build
- # uses: actions/cache@v3
+ - name: Cache BMF build
+ uses: actions/cache@v4
with:
path: bmf/output/
key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }}
restore-keys: |
${{ runner.os }}-bmf-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # - name: Cache FFmpeg build | |
| # uses: actions/cache@v3 | |
| # with: | |
| # path: | | |
| # /usr/local/bin/ffmpeg | |
| # /usr/local/lib/ | |
| # /usr/local/include/ | |
| # key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | |
| # restore-keys: | | |
| # ${{ runner.os }}-ffmpeg- | |
| # - name: Cache BMF build | |
| # uses: actions/cache@v3 | |
| # with: | |
| # path: bmf/output/ | |
| # key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | |
| # restore-keys: | | |
| # ${{ runner.os }}-bmf- | |
| - name: Cache FFmpeg build | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| /usr/local/bin/ffmpeg | |
| /usr/local/lib/ | |
| /usr/local/include/ | |
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('bmf/scripts/build_ffmpeg.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-ffmpeg- | |
| - name: Cache BMF build | |
| uses: actions/cache@v4 | |
| with: | |
| path: bmf/output/ | |
| key: ${{ runner.os }}-bmf-${{ hashFiles('bmf/build.sh') }} | |
| restore-keys: | | |
| ${{ runner.os }}-bmf- |
| - name: Build with CMake | ||
| run: | | ||
| (cd src && cmake -B build && cd build && make -j$(nproc)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix build command substitution.
The build command needs proper quoting to prevent word splitting.
- name: Build with CMake
run: |
- (cd src && cmake -B build && cd build && make -j$(nproc))
+ cd src
+ cmake -B build
+ cd build
+ make -j"$(nproc)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build with CMake | |
| run: | | |
| (cd src && cmake -B build && cd build && make -j$(nproc)) | |
| - name: Build with CMake | |
| run: | | |
| cd src | |
| cmake -B build | |
| cd build | |
| make -j"$(nproc)" |
🧰 Tools
🪛 actionlint (1.7.4)
76-76: shellcheck reported issue in this script: SC2046:warning:1:49: Quote this to prevent word splitting
(shellcheck)
| - name: Copy libs | ||
| run: | | ||
| # linuxdeployqt | ||
| sudo apt-get -y install git g++ libgl1-mesa-dev | ||
| git clone https://github.com/probonopd/linuxdeployqt.git | ||
| # Then build in Qt Creator, or use | ||
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | ||
| (cd linuxdeployqt && qmake && make && sudo make install) | ||
| # patchelf | ||
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | ||
| tar xf patchelf-0.9.tar.bz2 | ||
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | ||
| # appimage | ||
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | ||
| sudo chmod a+x /usr/local/bin/appimagetool | ||
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | ||
| continue-on-error: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve AppImage creation reliability.
Remove continue-on-error and add proper error handling for the AppImage creation process.
- name: Copy libs
run: |
+ set -eo pipefail
# linuxdeployqt
sudo apt-get -y install git g++ libgl1-mesa-dev
git clone https://github.com/probonopd/linuxdeployqt.git
- # Then build in Qt Creator, or use
- export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH
+
+ # Set Qt path safely
+ qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true
+ if [ -n "$qt_path" ]; then
+ export PATH="$qt_path:$PATH"
+ fi
+
(cd linuxdeployqt && qmake && make && sudo make install)
# patchelf
wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2
tar xf patchelf-0.9.tar.bz2
( cd patchelf-0.9/ && ./configure && make && sudo make install )
# appimage
sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool
sudo chmod a+x /usr/local/bin/appimagetool
- (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage)
- continue-on-error: true
+ linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Copy libs | |
| run: | | |
| # linuxdeployqt | |
| sudo apt-get -y install git g++ libgl1-mesa-dev | |
| git clone https://github.com/probonopd/linuxdeployqt.git | |
| # Then build in Qt Creator, or use | |
| export PATH=$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/):$PATH | |
| (cd linuxdeployqt && qmake && make && sudo make install) | |
| # patchelf | |
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | |
| tar xf patchelf-0.9.tar.bz2 | |
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | |
| # appimage | |
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | |
| sudo chmod a+x /usr/local/bin/appimagetool | |
| (linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage) | |
| continue-on-error: true | |
| - name: Copy libs | |
| run: | | |
| set -eo pipefail | |
| # linuxdeployqt | |
| sudo apt-get -y install git g++ libgl1-mesa-dev | |
| git clone https://github.com/probonopd/linuxdeployqt.git | |
| # Set Qt path safely | |
| qt_path="$(readlink -f /tmp/.mount_QtCreator-*-x86_64/*/gcc_64/bin/)" || true | |
| if [ -n "$qt_path" ]; then | |
| export PATH="$qt_path:$PATH" | |
| fi | |
| (cd linuxdeployqt && qmake && make && sudo make install) | |
| # patchelf | |
| wget https://nixos.org/releases/patchelf/patchelf-0.9/patchelf-0.9.tar.bz2 | |
| tar xf patchelf-0.9.tar.bz2 | |
| ( cd patchelf-0.9/ && ./configure && make && sudo make install ) | |
| # appimage | |
| sudo wget -c "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" -O /usr/local/bin/appimagetool | |
| sudo chmod a+x /usr/local/bin/appimagetool | |
| linuxdeployqt/bin/linuxdeployqt ./src/build/OpenConverter -appimage |
🧰 Tools
🪛 actionlint (1.7.4)
86-86: shellcheck reported issue in this script: SC2155:warning:5:8: Declare and assign separately to avoid masking return values
(shellcheck)
Summary by CodeRabbit
Release Notes
New Features
Transcoderclass with specialized implementations.ReleaseandReviewworkflows for improved build and packaging automation.Bug Fixes
Refactoring
Documentation
Chores