[Don't Merge!!!] Test SonarQube #1
Conversation
* update .gitignore * optimize transcode.py for callback develop
* update distributed_node cmake
JackLau1222
force-pushed
the
fork_by_oc
branch
from
c8a51d1 to
d4a6065
Compare
WalkthroughThis pull request introduces several enhancements to the BMF (Byte Media Framework) project, focusing on callback mechanisms, build configurations, and development tooling. The changes span multiple files across the project, including modifications to Changes
Sequence DiagramsequenceDiagram
participant User
participant Graph
participant Decoder
participant Encoder
User->>Graph: Create Video Processing Graph
User->>Decoder: Set Decoder Callback
User->>Encoder: Set Encoder Callback
Graph->>Decoder: Process Video
Decoder->>Graph: Callback with Frame Info
Graph->>Encoder: Process Frames
Encoder->>Graph: Callback with Encoding Progress
Graph->>User: Return Processed Video
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🔭 Outside diff range comments (1)
bmf/test/distributed_node/CMakeLists.txt (1)
Line range hint
6-6: Avoid hardcoding debug flags.The build flags
-g -O0are hardcoded, which forces debug builds. Consider making this configurable based on build type.Apply this diff to make it configurable:
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -O0 -fPIC") +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC") +set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -g -O0") +set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O3")
🧹 Nitpick comments (8)
bmf/demo/transcode/test_cb.cpp (3)
7-13: Consider avoiding or minimizing global variables.
The global variables (frame_number_total,frame_number_global,process_number, andrest_time) risk complicating potential multithreading or reusability. Refactoring them into class members or local scope can enhance maintainability and avoid unintended side effects.
32-39: Remove or clarify commented-out code.
Theformat_timefunction is entirely commented out. Unless you plan to enable or maintain it, consider removing it or adding a specific comment explaining why it is left in.
68-71: Clarify log messages and naming.
You log"Extracted Total Frame Number"when assigning toframe_number_global, which may confuse future maintainers becauseframe_number_totalis used for the actual total frames. Rename or revise the log statement to avoid confusion.-BMFLOG(BMF_DEBUG) << "Extracted Total Frame Number: " << frame_number_global; +BMFLOG(BMF_DEBUG) << "Extracted Current Frame Number: " << frame_number_global;bmf/demo/transcode/transcode.py (1)
151-153: Remove unused variable or provide justification.
The local variableexpect_resultis assigned but never used. Consider removing it or using it if necessary.- expect_result = '../transcode/cb.mp4|240|320|7.615000|MOV,MP4,M4A,3GP,3G2,MJ2|366635|348991|h264|{"fps": "30.0662251656"}'🧰 Tools
🪛 Ruff (0.8.2)
153-153: Local variable
expect_resultis assigned to but never usedRemove assignment to unused variable
expect_result(F841)
bmf/engine/connector/src/builder.cpp (3)
136-137: Use a more descriptive key for JSON array creation.
Creating"callback_binding"as an array is clear, but describing the data structure or using a more self-evident field name might help maintain clarity in the JSON schema.
139-141: Validate callback binding representation.
Storing callback keys and UIDs as strings joined by “:” is workable but might be error-prone if keys or UIDs contain special characters in the future. Consider storing them as a structured object or separate fields to avoid potential parsing issues.
678-683: Avoid direct usage of iostream in high-frequency code paths.
Printing the address of the callback withstd::coutmight clutter logs in production. Consider using a logger with adjustable verbosity levels to avoid potential performance overhead.bmf/c_modules/src/ffmpeg_encoder.cpp (1)
415-423: Clean up commented code.
Any computations you no longer need (e.g.,curr_time) should be removed if they are irrelevant. Keeping them in comments can mislead readers into thinking they’re still relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignore(1 hunks)activate_env.sh(1 hunks)bmf/builder/bmf_graph.py(1 hunks)bmf/c_modules/include/ffmpeg_decoder.h(2 hunks)bmf/c_modules/include/ffmpeg_encoder.h(1 hunks)bmf/c_modules/src/ffmpeg_decoder.cpp(2 hunks)bmf/c_modules/src/ffmpeg_encoder.cpp(1 hunks)bmf/demo/transcode/build.sh(1 hunks)bmf/demo/transcode/test_cb.cpp(1 hunks)bmf/demo/transcode/transcode.py(3 hunks)bmf/engine/connector/include/builder.hpp(2 hunks)bmf/engine/connector/src/builder.cpp(3 hunks)bmf/test/distributed_node/CMakeLists.txt(1 hunks)build_osx.sh(1 hunks)version.sh(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- bmf/demo/transcode/build.sh
- version.sh
- .gitignore
- bmf/builder/bmf_graph.py
🧰 Additional context used
🪛 Shellcheck (0.10.0)
activate_env.sh
[warning] 3-3: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 5-5: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 6-6: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 Ruff (0.8.2)
bmf/demo/transcode/transcode.py
153-153: Local variable expect_result is assigned to but never used
Remove assignment to unused variable expect_result
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build and test win
- GitHub Check: build and test
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
build_osx.sh (1)
20-21: LGTM on cleanup changes, but verify PR intent.The additional cleanup commands for removing
outputandCMakeFilesdirectories are reasonable and align with good build hygiene practices.However, given that this PR is marked as "Don't Merge!!!" and is intended to test SonarQube integration, please verify:
- Are these changes intended to be permanent?
- Should this PR be converted to a draft PR to prevent accidental merging?
✅ Verification successful
Cleanup changes are consistent with codebase patterns
The removal of
outputandCMakeFilesdirectories aligns with cleanup patterns found across multiple build scripts in the repository, includingbuild_android.sh,build.sh, and others. These changes maintain consistency with the project's build hygiene practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if these directories are commonly cleaned up in other build scripts # Check if other build scripts have similar cleanup patterns rg -A 5 "rm -rf (output|CMakeFiles)" --type shLength of output: 1303
bmf/c_modules/include/ffmpeg_encoder.h (1)
141-142: Check concurrency implications forframe_number.
This variable may be updated in a multithreaded context. If so, consider introducing synchronization or using an atomic type to prevent data races onframe_number.bmf/c_modules/include/ffmpeg_decoder.h (1)
176-177: Validate callback endpoint thread safety.
callback_endpoint_might be invoked from parallel contexts. If the decoder runs in multiple threads or must handle concurrency, confirm that chasing or invoking the function is safe.bmf/demo/transcode/transcode.py (1)
165-166: Verify necessity of multiple callback registrations.
You add the same callback (cb) at lines 165–166 and 177, which may result in duplicate invocations. Confirm whether both calls are needed or if only one suffices.Also applies to: 177-177
bmf/engine/connector/include/builder.hpp (2)
144-146: Ensure consistent function naming and usage.
The new overload ofAddCallbacktakes astd::function, mirroring the existing overload that takes aBMFCallback. This is good for flexibility; however, review all call sites to confirm consistency and minimize confusion.
457-459: Coordinate callback mechanism with RealNode.
This overload inNodeessentially delegates toRealNode::AddCallback. Confirm that all callbacks are consistently registered and invoked, and that no race conditions occur when managing callbacks across the Node/RealNode boundary.bmf/engine/connector/src/builder.cpp (1)
234-239: Confirm callback concurrency safety.
TheRealNodeimplementation stashes the callback incallbackInstances_. If callbacks might be accessed concurrently, ensure thread safety for the underlying data structures (e.g., using locks or atomic operations).bmf/c_modules/src/ffmpeg_decoder.cpp (1)
2533-2537: Verify multi-threaded callback usage.
Similar to the encoder, confirm concurrency safety if this callback is invoked from multiple threads. Also confirm the lifetimes ofCFFDecoderif the callback might outlive the decoder instance.bmf/test/distributed_node/CMakeLists.txt (1)
21-21: Verify hmp library dependency.The 'hmp' library has been added as a dependency. Please ensure:
- The library is available in all build environments
- Version compatibility with other dependencies
- Impact on build time and runtime performance
Run this script to check the library availability and version:
Also applies to: 26-26
✅ Verification successful
HMP library dependency is valid and properly integrated
The 'hmp' library is an internal component of the BMF project, located in the
bmf/hmldirectory. It's built as part of the project build process and properly integrated through CMake configurations. The dependency is correctly specified in the test's CMakeLists.txt.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if hmp library exists and get its version find . -type f -name "libhmp*" -o -name "hmp.pc" pkg-config --modversion hmp 2>/dev/null || echo "hmp package config not found"Length of output: 145
Script:
#!/bin/bash # Search for hmp-related files and CMake configurations echo "=== HMP source files ===" fd -t f "hmp" -e cpp -e hpp -e h -e c echo -e "\n=== CMake files defining hmp ===" rg -l "add_library.*hmp" --type cmake echo -e "\n=== HMP includes ===" rg -l "#include.*hmp" --type cpp --type hppLength of output: 641
| std::chrono::system_clock::time_point last_encoder_call; // Track last call time | ||
| bool first_encoder_call = true; // Flag for first call | ||
|
|
There was a problem hiding this comment.
Remove or unify duplicate encoder timing variables.
You have a global last_encoder_call and first_encoder_call, but also a static local last_encoder_call_time in line 73. This duplication can cause confusion or errors. Consider removing the unused global variables or consolidating them into a single approach for tracking encoder call times.
-std::chrono::system_clock::time_point last_encoder_call; // Possibly remove
-bool first_encoder_call = true; // Possibly remove
...
static auto last_encoder_call_time = std::chrono::system_clock::now();Also applies to: 73-73
bmf/c_modules/src/ffmpeg_decoder.cpp
Outdated
| if (callback_endpoint_ != NULL) { | ||
| std::string info = "total frame number: " + std::to_string(video_stream_->nb_frames); | ||
| auto para = CBytes::make((uint8_t *)info.c_str(), info.size()); | ||
| callback_endpoint_(0, para); | ||
| } | ||
|
|
There was a problem hiding this comment.
Confirm availability of video_stream_.
You’re accessing video_stream_->nb_frames before verifying video_stream_ != nullptr. Ensure video_stream_ is valid at this point to prevent null dereferences in corner cases.
if (callback_endpoint_ != NULL) {
- std::string info = "total frame number: " + std::to_string(video_stream_->nb_frames);
+ if (video_stream_) {
+ std::string info = "total frame number: " + std::to_string(video_stream_->nb_frames);
+ auto para = CBytes::make((uint8_t*)info.c_str(), info.size());
+ callback_endpoint_(0, para);
+ }
}📝 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 (callback_endpoint_ != NULL) { | |
| std::string info = "total frame number: " + std::to_string(video_stream_->nb_frames); | |
| auto para = CBytes::make((uint8_t *)info.c_str(), info.size()); | |
| callback_endpoint_(0, para); | |
| } | |
| if (callback_endpoint_ != NULL) { | |
| if (video_stream_) { | |
| std::string info = "total frame number: " + std::to_string(video_stream_->nb_frames); | |
| auto para = CBytes::make((uint8_t*)info.c_str(), info.size()); | |
| callback_endpoint_(0, para); | |
| } | |
| } | |
| export C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include | ||
| export CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include | ||
| export LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib | ||
| export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential masking of return values in export statements.
The current inline export assignments could mask return values. Consider separating the declaration and assignment.
Apply this diff to fix the issue:
-export C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include
-export CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include
-export LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib
-export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib
-export PYTHONPATH=$(pwd)/output/bmf/lib:$(pwd)/output
+C_INCLUDE_PATH=${C_INCLUDE_PATH}:$(pwd)/output/bmf/include
+CPLUS_INCLUDE_PATH=${CPLUS_INCLUDE_PATH}:$(pwd)/output/bmf/include
+LIBRARY_PATH=${LIBRARY_PATH}:$(pwd)/output/bmf/lib
+LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:$(pwd)/output/bmf/lib
+PYTHONPATH=$(pwd)/output/bmf/lib:$(pwd)/output
+export C_INCLUDE_PATH CPLUS_INCLUDE_PATH LIBRARY_PATH LD_LIBRARY_PATH PYTHONPATHAlso applies to: 9-9
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 3-3: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 5-5: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 6-6: Declare and assign separately to avoid masking return values.
(SC2155)
|
|
||
| # only set if you want to use BMF in python | ||
| export PYTHONPATH=$(pwd)/output/bmf/lib:$(pwd)/output | ||
| export DYLD_LIBRARY_PATH=~/Documents/Programs/Git/Github/bmf/output/bmf/lib |
There was a problem hiding this comment.
Remove hard-coded path and add directory validation.
The script contains a hard-coded path that won't work on other systems. Additionally, the script should validate that required directories exist.
Apply this diff to fix the issues:
-export DYLD_LIBRARY_PATH=~/Documents/Programs/Git/Github/bmf/output/bmf/lib
+# Check if BMF directories exist
+BMF_OUTPUT_DIR="$(pwd)/output/bmf"
+if [ ! -d "$BMF_OUTPUT_DIR" ]; then
+ echo "Error: BMF output directory not found: $BMF_OUTPUT_DIR"
+ return 1
+fi
+
+# Set DYLD_LIBRARY_PATH relative to current directory
+DYLD_LIBRARY_PATH=${BMF_OUTPUT_DIR}/lib
+export DYLD_LIBRARY_PATH📝 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.
| export DYLD_LIBRARY_PATH=~/Documents/Programs/Git/Github/bmf/output/bmf/lib | |
| # Check if BMF directories exist | |
| BMF_OUTPUT_DIR="$(pwd)/output/bmf" | |
| if [ ! -d "$BMF_OUTPUT_DIR" ]; then | |
| echo "Error: BMF output directory not found: $BMF_OUTPUT_DIR" | |
| return 1 | |
| fi | |
| # Set DYLD_LIBRARY_PATH relative to current directory | |
| DYLD_LIBRARY_PATH=${BMF_OUTPUT_DIR}/lib | |
| export DYLD_LIBRARY_PATH |
* change the default ffmpeg build version to 5.1.6
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/build_ffmpeg.sh (2)
130-139: Improve version selection logic and add error handling.The version selection logic could be more robust and maintainable.
Apply this diff to improve the version handling:
- # 设置FFmpeg版本的下载链接和目录 - if [ "$version" == "5" ]; then - ffmpeg_version="ffmpeg-5.1.6" - ffmpeg_tar="ffmpeg-5.1.6.tar.bz2" - ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar" - else - ffmpeg_version="ffmpeg-4.4" - ffmpeg_tar="ffmpeg-4.4.tar.bz2" - ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar" - fi + # Set FFmpeg version, tarball name, and download URL + declare -A FFMPEG_VERSIONS=( + [5]="5.1.6" + [4]="4.4" + ) + ffmpeg_ver="${FFMPEG_VERSIONS[$version]}" + ffmpeg_version="ffmpeg-${ffmpeg_ver}" + ffmpeg_tar="${ffmpeg_version}.tar.bz2" + ffmpeg_url="https://ffmpeg.org/releases/${ffmpeg_tar}"
127-161: Consider adding SHA256 verification for security.The script downloads FFmpeg tarballs without verifying their integrity.
Consider adding SHA256 verification to ensure the downloaded tarballs haven't been tampered with. You can maintain a mapping of versions to their SHA256 hashes and verify the downloads:
declare -A FFMPEG_SHA256=( ["5.1.6"]="fe484df5d2624ff8db0f7b701d0b8c1a7f4a5d3f8b0a8a17974a9b06ccc97cf" ["4.4"]="42093549751b582cf0f338a21a3664f52e0a9fbe0d238d3c992005e493607d0e" ) # After download, verify the SHA256 if ! echo "${FFMPEG_SHA256[$ffmpeg_ver]} $ffmpeg_tar" | sha256sum --check; then echo "Error: SHA256 verification failed for $ffmpeg_tar" >&2 rm -f "$ffmpeg_tar" exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bmf/c_modules/src/ffmpeg_decoder.cpp(2 hunks)bmf/c_modules/src/ffmpeg_encoder.cpp(1 hunks)scripts/build_ffmpeg.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bmf/c_modules/src/ffmpeg_decoder.cpp
- bmf/c_modules/src/ffmpeg_encoder.cpp
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build and test
- GitHub Check: build and test win
scripts/build_ffmpeg.sh
Outdated
| # 默认版本是4 | ||
| version=${version:-5} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use explicit variable declaration and add input validation.
The version variable should be explicitly declared and validated to prevent unexpected behavior.
Apply this diff to improve variable handling:
- # 默认版本是4
- version=${version:-5}
+ # Default version is 5
+ : "${version:=5}"
+ if ! [[ "$version" =~ ^[45]$ ]]; then
+ echo "Error: version must be either 4 or 5" >&2
+ 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.
| # 默认版本是4 | |
| version=${version:-5} | |
| # Default version is 5 | |
| : "${version:=5}" | |
| if ! [[ "$version" =~ ^[45]$ ]]; then | |
| echo "Error: version must be either 4 or 5" >&2 | |
| exit 1 | |
| fi | |
scripts/build_ffmpeg.sh
Outdated
| # 如果没有找到 tar 文件，则下载 | ||
| if [ ! -e $ffmpeg_tar ]; then | ||
| curl -O -L $ffmpeg_url | ||
| fi | ||
| tar xjvf ffmpeg-4.4.tar.bz2 | ||
| cd ffmpeg-4.4 | ||
|
|
||
| # 如果已经存在解压目录，则删除 | ||
| if [ -d $ffmpeg_version ]; then | ||
| rm -rf $ffmpeg_version | ||
| fi | ||
|
|
||
| tar xjvf $ffmpeg_tar | ||
| cd $ffmpeg_version |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for download and extraction operations.
The download and extraction operations lack proper error handling and verification.
Apply this diff to improve error handling:
- # 如果没有找到 tar 文件，则下载
- if [ ! -e $ffmpeg_tar ]; then
- curl -O -L $ffmpeg_url
- fi
-
- # 如果已经存在解压目录，则删除
- if [ -d $ffmpeg_version ]; then
- rm -rf $ffmpeg_version
- fi
-
- tar xjvf $ffmpeg_tar
- cd $ffmpeg_version
+ # Download FFmpeg if tarball not found
+ if [ ! -e "$ffmpeg_tar" ]; then
+ echo "Downloading FFmpeg ${ffmpeg_ver}..."
+ if ! curl -O -L --fail "$ffmpeg_url"; then
+ echo "Error: Failed to download FFmpeg" >&2
+ exit 1
+ fi
+ fi
+
+ # Verify the downloaded tarball
+ if ! tar tf "$ffmpeg_tar" &>/dev/null; then
+ echo "Error: Invalid or corrupted tarball: $ffmpeg_tar" >&2
+ rm -f "$ffmpeg_tar"
+ exit 1
+ fi
+
+ # Remove existing directory if present
+ if [ -d "$ffmpeg_version" ]; then
+ rm -rf "$ffmpeg_version"
+ fi
+
+ # Extract the tarball
+ echo "Extracting FFmpeg ${ffmpeg_ver}..."
+ if ! tar xjf "$ffmpeg_tar"; then
+ echo "Error: Failed to extract FFmpeg" >&2
+ exit 1
+ fi
+
+ # Change to FFmpeg directory
+ cd "$ffmpeg_version" || {
+ echo "Error: Failed to change to FFmpeg directory" >&2
+ exit 1
+ }📝 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.
| # 如果没有找到 tar 文件，则下载 | |
| if [ ! -e $ffmpeg_tar ]; then | |
| curl -O -L $ffmpeg_url | |
| fi | |
| tar xjvf ffmpeg-4.4.tar.bz2 | |
| cd ffmpeg-4.4 | |
| # 如果已经存在解压目录，则删除 | |
| if [ -d $ffmpeg_version ]; then | |
| rm -rf $ffmpeg_version | |
| fi | |
| tar xjvf $ffmpeg_tar | |
| cd $ffmpeg_version | |
| # Download FFmpeg if tarball not found | |
| if [ ! -e "$ffmpeg_tar" ]; then | |
| echo "Downloading FFmpeg ${ffmpeg_ver}..." | |
| if ! curl -O -L --fail "$ffmpeg_url"; then | |
| echo "Error: Failed to download FFmpeg" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| # Verify the downloaded tarball | |
| if ! tar tf "$ffmpeg_tar" &>/dev/null; then | |
| echo "Error: Invalid or corrupted tarball: $ffmpeg_tar" >&2 | |
| rm -f "$ffmpeg_tar" | |
| exit 1 | |
| fi | |
| # Remove existing directory if present | |
| if [ -d "$ffmpeg_version" ]; then | |
| rm -rf "$ffmpeg_version" | |
| fi | |
| # Extract the tarball | |
| echo "Extracting FFmpeg ${ffmpeg_ver}..." | |
| if ! tar xjf "$ffmpeg_tar"; then | |
| echo "Error: Failed to extract FFmpeg" >&2 | |
| exit 1 | |
| fi | |
| # Change to FFmpeg directory | |
| cd "$ffmpeg_version" || { | |
| echo "Error: Failed to change to FFmpeg directory" >&2 | |
| exit 1 | |
| } |
scripts/build_ffmpeg.sh
Outdated
| # # 目标目录和并行编译核心数等 | ||
| # target_dir=${other_args[0]} # 第一个参数为目标目录 | ||
| # num_cores=${other_args[1]} # 第二个参数为并行编译的核心数 | ||
| # extra_config_args="${other_args[@]:2}" # 从第三个参数开始为额外的配置选项 | ||
|
|
||
| # # 进入目标目录 | ||
| # cd $target_dir | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Remove commented configuration code
These commented lines are dead code with no active usage in the codebase. They should be removed to improve code maintainability.
🔗 Analysis chain
Remove or uncomment configuration code.
The commented-out configuration code should either be removed or uncommented if it's needed.
Let's verify if this configuration code is used elsewhere in the codebase:
If the configuration is not used, remove these comments. If it is needed, uncomment and implement proper error handling:
- # # 目标目录和并行编译核心数等
- # target_dir=${other_args[0]} # 第一个参数为目标目录
- # num_cores=${other_args[1]} # 第二个参数为并行编译的核心数
- # extra_config_args="${other_args[@]:2}" # 从第三个参数开始为额外的配置选项
-
- # # 进入目标目录
- # cd $target_dir🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to target_dir, num_cores, or extra_config_args
rg -l "target_dir|num_cores|extra_config_args" --type sh
Length of output: 82
Script:
#!/bin/bash
# Check the context and usage of these variables in build_ffmpeg.sh
rg "target_dir|num_cores|extra_config_args" scripts/build_ffmpeg.sh -C 5
Length of output: 469
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build_ffmpeg.sh (1)
Line range hint
1-161: Maintain consistent language and error handling patterns.The script would benefit from:
- Using English consistently for all comments (currently mixing English and Chinese)
- Implementing consistent error handling patterns across all operations
Apply this pattern for error handling throughout the script:
function some_operation() { local operation_name="$1" + echo "Starting ${operation_name}..." + if ! command_that_might_fail; then + echo "Error: ${operation_name} failed" >&2 + return 1 + fi + echo "${operation_name} completed successfully" }🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 163-163: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)scripts/build_ffmpeg.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build and test
- GitHub Check: build and test win
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
CMakeLists.txt (1)
19-19:⚠️ Potential issue
⚠️ Critical: Disabling essential features could break functionalityThe changes disable three important features that might be essential for the project:
- CUDA support (GPU acceleration)
- Python support (Python bindings)
- Test compilation
Given that:
- The PR title suggests this is a test PR ("[Don't Merge!!!] Test SonarQube")
- These features are typically enabled by default for good reasons
- Disabling them could break functionality for users
I recommend:
- Keeping these options enabled by default (
ON)- If specific testing scenarios require disabling them, do so via command-line arguments (e.g.,
-DBMF_ENABLE_CUDA=OFF) rather than changing defaultsLet's verify the usage of these features in the codebase:
Also applies to: 21-21, 26-26
✅ Verification successful
⚠️ CUDA support should remain configurable but enabled when neededThe codebase has extensive CUDA integration for GPU acceleration. While having it OFF by default is reasonable (not all users have CUDA hardware), users who need GPU acceleration should enable it via:
cmake -DBMF_ENABLE_CUDA=ON ...This approach:
- Makes the default build work everywhere
- Allows easy opt-in for GPU acceleration
- Preserves all CUDA capabilities when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for CUDA usage echo "Checking CUDA usage..." rg "cuda|gpu" --type cpp # Check for Python bindings echo "Checking Python usage..." rg "PyObject|PyModule|Python" --type cpp # Check for test files echo "Checking test files..." fd "test.*\.(cpp|cc|h|hpp)$"Length of output: 60297
scripts/build_ffmpeg.sh (2)
142-149: Remove commented configuration code.These commented lines are dead code with no active usage in the codebase. They should be removed to improve code maintainability.
150-161: Add error handling for download and extraction operations.The download and extraction operations lack proper error handling and verification.
scripts/build_ffmpeg.sh
Outdated
| # 默认版本是5 | ||
| version=${version:-5} | ||
|
|
||
| # 设置FFmpeg版本的下载链接和目录 | ||
| if [ "$version" == "5" ]; then | ||
| ffmpeg_version="ffmpeg-5.1.6" | ||
| ffmpeg_tar="ffmpeg-5.1.6.tar.bz2" | ||
| ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar" | ||
| else | ||
| ffmpeg_version="ffmpeg-4.4" | ||
| ffmpeg_tar="ffmpeg-4.4.tar.bz2" | ||
| ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar" | ||
| fi |
There was a problem hiding this comment.
Improve version handling and validation.
The version handling needs more robust validation and explicit supported version declaration.
Apply this diff to improve version handling:
- # 默认版本是5
- version=${version:-5}
+ # Default version is 5
+ : "${version:=5}"
+
+ # Validate supported versions
+ case "$version" in
+ 4|4.4|5|5.1.6)
+ ;;
+ *)
+ echo "Error: Unsupported FFmpeg version. Supported versions: 4.4, 5.1.6" >&2
+ exit 1
+ ;;
+ esac
+
+ # Normalize version numbers
+ if [ "$version" = "4" ]; then
+ version="4.4"
+ elif [ "$version" = "5" ]; then
+ version="5.1.6"
+ fi
# 设置FFmpeg版本的下载链接和目录
- if [ "$version" == "5" ]; then
- ffmpeg_version="ffmpeg-5.1.6"
- ffmpeg_tar="ffmpeg-5.1.6.tar.bz2"
+ ffmpeg_version="ffmpeg-${version}"
+ ffmpeg_tar="ffmpeg-${version}.tar.bz2"
+ ffmpeg_url="https://ffmpeg.org/releases/${ffmpeg_tar}"
- ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar"
- else
- ffmpeg_version="ffmpeg-4.4"
- ffmpeg_tar="ffmpeg-4.4.tar.bz2"
- ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar"
- 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.
| # 默认版本是5 | |
| version=${version:-5} | |
| # 设置FFmpeg版本的下载链接和目录 | |
| if [ "$version" == "5" ]; then | |
| ffmpeg_version="ffmpeg-5.1.6" | |
| ffmpeg_tar="ffmpeg-5.1.6.tar.bz2" | |
| ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar" | |
| else | |
| ffmpeg_version="ffmpeg-4.4" | |
| ffmpeg_tar="ffmpeg-4.4.tar.bz2" | |
| ffmpeg_url="https://ffmpeg.org/releases/$ffmpeg_tar" | |
| fi | |
| # Default version is 5 | |
| : "${version:=5}" | |
| # Validate supported versions | |
| case "$version" in | |
| 4|4.4|5|5.1.6) | |
| ;; | |
| *) | |
| echo "Error: Unsupported FFmpeg version. Supported versions: 4.4, 5.1.6" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| # Normalize version numbers | |
| if [ "$version" = "4" ]; then | |
| version="4.4" | |
| elif [ "$version" = "5" ]; then | |
| version="5.1.6" | |
| fi | |
| # 设置FFmpeg版本的下载链接和目录 | |
| ffmpeg_version="ffmpeg-${version}" | |
| ffmpeg_tar="ffmpeg-${version}.tar.bz2" | |
| ffmpeg_url="https://ffmpeg.org/releases/${ffmpeg_tar}" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
83-83: Remove trailing spaces.Remove trailing spaces to improve code quality and fix the yamllint error.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 83-83: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci.yml(2 hunks)3rd_party/dlpack(1 hunks)3rd_party/glog(1 hunks)bmf/hml/third_party/backward(1 hunks)bmf/hml/third_party/benchmark(1 hunks)bmf/hml/third_party/dlpack(1 hunks)bmf/hml/third_party/fmt(1 hunks)bmf/hml/third_party/gtest(1 hunks)bmf/hml/third_party/pybind11(1 hunks)bmf/hml/third_party/spdlog(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- bmf/hml/third_party/backward
- 3rd_party/glog
- bmf/hml/third_party/fmt
- bmf/hml/third_party/dlpack
- 3rd_party/dlpack
- bmf/hml/third_party/spdlog
- bmf/hml/third_party/benchmark
- bmf/hml/third_party/pybind11
- bmf/hml/third_party/gtest
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci.yml
[error] 83-83: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
36-37: 🛠️ Refactor suggestionReplace
sedcommands with CMake command-line options.The current approach of modifying
CMakeLists.txtwithsedis fragile and could break if the file formatting changes. Instead, pass these options directly to CMake:- sed -i 's/option(BMF_ENABLE_PYTHON "Enable build with python support" OFF)/option(BMF_ENABLE_PYTHON "Enable build with python support" ON)/' CMakeLists.txt - sed -i 's/option(BMF_ENABLE_TEST "Compile examples and tests" OFF)/option(BMF_ENABLE_TEST "Compile examples and tests" ON)/' CMakeLists.txt + ./build.sh debug -DBMF_ENABLE_PYTHON=ON -DBMF_ENABLE_TEST=ONAlso, given the PR title "Don't Merge!!!", please clarify if these changes are intended for production.
| - name: Setup tmate session | ||
| if: ${{ failure() }} | ||
| uses: mxschmitt/action-tmate@v3 |
There was a problem hiding this comment.
Security Risk: Remove tmate session before merging.
The tmate session provides SSH access to the CI environment, which could expose sensitive information. This should not be merged into production as it poses a security risk.
Consider using GitHub's built-in debugging features or artifact uploads instead.
JackLau1222
force-pushed
the
fork_by_oc
branch
from
5459dd0 to
62abfa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
84-86:⚠️ Potential issueSecurity Risk: Remove tmate session before merging.
The tmate session provides SSH access to the CI environment, which could expose sensitive information. This should not be merged into production as it poses a security risk.
Consider using GitHub's built-in debugging features or artifact uploads instead.
- - name: Setup tmate session - if: ${{ failure() }} - uses: mxschmitt/action-tmate@v3
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
83-83: Remove trailing whitespace.There are trailing spaces on line 83 that should be removed.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 83-83: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci.yml
[error] 83-83: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build and test
- GitHub Check: build and test win
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
36-37: LGTM: Build configuration changes look good.The
sedcommands appropriately enable Python support and test compilation for the CI environment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/build_ffmpeg.sh(6 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/build_ffmpeg.sh
[warning] 420-420: Quote this to prevent word splitting.
(SC2046)
[warning] 427-427: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (4)
scripts/build_ffmpeg.sh (4)
24-26: LGTM: Command-line argument handling is well-implemented.The new
--install-dirargument is properly integrated into the existing argument parsing structure.
130-141: Improve version handling and validation.The version handling needs more robust validation and explicit supported version declaration.
145-152: Remove commented configuration code.These commented lines are dead code with no active usage in the codebase. They should be removed to improve code maintainability.
153-164: Add error handling for download and extraction operations.The download and extraction operations lack proper error handling and verification.
| if [[ ${arg} != "--install-dir"* ]] # Exclude --install-dir from build arguments | ||
| then | ||
| disable_asm="" | ||
| fi | ||
| (build_${arg}_unix $(pwd)/ffmpeg_source ${cores}) | ||
| if [ ${arg} == "nasm" ] || [ ${arg} == "yasm" ] | ||
| then | ||
| disable_asm="" | ||
| fi | ||
|
|
||
| set +e | ||
| (check_lib $(pwd)/ffmpeg_source ${arg}) | ||
| if [ $? -eq 0 ] | ||
| then | ||
| ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}" | ||
| set +e | ||
| (check_lib $(pwd)/ffmpeg_source ${arg}) | ||
| if [ $? -eq 0 ] | ||
| then | ||
| ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}" | ||
| fi | ||
| set -e |
There was a problem hiding this comment.
Fix word splitting in command arguments.
The script has potential word splitting issues in the build argument handling.
Apply this diff to fix the word splitting issues:
- if [[ ${arg} != "--install-dir"* ]] # Exclude --install-dir from build arguments
+ if [[ "${arg}" != "--install-dir"* ]] # Exclude --install-dir from build arguments
then
- (build_${arg}_unix $(pwd)/ffmpeg_source ${cores})
+ (build_"${arg}"_unix "$(pwd)/ffmpeg_source" "${cores}")
if [ ${arg} == "nasm" ] || [ ${arg} == "yasm" ]
then
disable_asm=""
fi
set +e
- (check_lib $(pwd)/ffmpeg_source ${arg})
+ (check_lib "$(pwd)/ffmpeg_source" "${arg}")
if [ $? -eq 0 ]
then
ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}"
fi
set -e
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 [[ ${arg} != "--install-dir"* ]] # Exclude --install-dir from build arguments | |
| then | |
| disable_asm="" | |
| fi | |
| (build_${arg}_unix $(pwd)/ffmpeg_source ${cores}) | |
| if [ ${arg} == "nasm" ] || [ ${arg} == "yasm" ] | |
| then | |
| disable_asm="" | |
| fi | |
| set +e | |
| (check_lib $(pwd)/ffmpeg_source ${arg}) | |
| if [ $? -eq 0 ] | |
| then | |
| ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}" | |
| set +e | |
| (check_lib $(pwd)/ffmpeg_source ${arg}) | |
| if [ $? -eq 0 ] | |
| then | |
| ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}" | |
| fi | |
| set -e | |
| if [[ "${arg}" != "--install-dir"* ]] # Exclude --install-dir from build arguments | |
| then | |
| (build_"${arg}"_unix "$(pwd)/ffmpeg_source" "${cores}") | |
| if [ ${arg} == "nasm" ] || [ ${arg} == "yasm" ] | |
| then | |
| disable_asm="" | |
| fi | |
| set +e | |
| (check_lib "$(pwd)/ffmpeg_source" "${arg}") | |
| if [ $? -eq 0 ] | |
| then | |
| ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}" | |
| fi | |
| set -e |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 420-420: Quote this to prevent word splitting.
(SC2046)
[warning] 427-427: Quote this to prevent word splitting.
(SC2046)
| @@ -145,6 +173,7 @@ function build_ffmpeg_unix() { | |||
| case $3 in | |||
| x86_64) | |||
| ./configure \ | |||
| --prefix=$INSTALL_DIR \ | |||
There was a problem hiding this comment.
💡 Codebase verification
The installation directory validation is indeed missing and should be added
The script uses $INSTALL_DIR for FFmpeg installation but lacks essential validation:
- No check if the directory path is provided
- No verification of directory existence
- No permission checks
The suggested fix in the review comment is appropriate and follows the script's existing error handling patterns. It should be implemented as proposed:
+# Validate installation directory
+if [ -z "$INSTALL_DIR" ]; then
+ echo "Error: --install-dir is required" >&2
+ exit 1
+fi
+
+# Ensure installation directory exists and is writable
+mkdir -p "$INSTALL_DIR" || {
+ echo "Error: Failed to create installation directory: $INSTALL_DIR" >&2
+ exit 1
+}🔗 Analysis chain
Verify installation directory existence and permissions.
The script uses $INSTALL_DIR for FFmpeg installation but doesn't verify if the directory exists or has proper permissions.
Apply this diff to add directory validation:
+# Validate installation directory
+if [ -z "$INSTALL_DIR" ]; then
+ echo "Error: --install-dir is required" >&2
+ exit 1
+fi
+
+# Ensure installation directory exists and is writable
+mkdir -p "$INSTALL_DIR" || {
+ echo "Error: Failed to create installation directory: $INSTALL_DIR" >&2
+ exit 1
+}
+
case $3 in
x86_64)
./configure \Also applies to: 187-187, 198-198
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if INSTALL_DIR exists and has proper permissions
# Expected results: Directory should exist and be writable
# Search for any instances where INSTALL_DIR is used without existence checks
rg -l "INSTALL_DIR" --type sh
# Search for any permission or directory checks before using INSTALL_DIR
rg -B 5 -A 5 "INSTALL_DIR.*mkdir|test.*INSTALL_DIR" --type sh
Length of output: 238
Script:
#!/bin/bash
# Check how INSTALL_DIR is defined and used in the main scripts
rg -B 5 -A 5 "INSTALL_DIR=" scripts/build_ffmpeg.sh scripts/build_wasm_ffmpeg.sh
# Look for any validation patterns in these scripts
rg -B 5 -A 5 "(\[|test).*(INSTALL_DIR|-d|mkdir)" scripts/build_ffmpeg.sh scripts/build_wasm_ffmpeg.sh
# Get the overall structure of the main script
head -n 50 scripts/build_ffmpeg.sh
Length of output: 3252
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build_ffmpeg.sh (1)
57-57: Ensure consistent use of quotes in configure options.The
--prefixoption values should be quoted to handle paths with spaces.Apply this diff pattern to all configure commands:
- ./configure --enable-shared --prefix=$INSTALL_DIR + ./configure --enable-shared --prefix="$INSTALL_DIR"Also applies to: 67-67, 76-76, 85-85, 95-95, 105-105, 115-115, 124-124, 177-177, 188-188, 199-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/build_ffmpeg.sh(14 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/build_ffmpeg.sh
[warning] 421-421: Quote this to prevent word splitting.
(SC2046)
[warning] 428-428: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build and test
- GitHub Check: build and test win
🔇 Additional comments (4)
scripts/build_ffmpeg.sh (4)
131-142: Improve version handling and validation.The version handling needs more robust validation and explicit supported version declaration.
146-153: Remove commented configuration code.These commented lines are dead code with no active usage in the codebase.
154-165: Add error handling for download and extraction operations.The download and extraction operations lack proper error handling and verification.
419-433: Fix word splitting in command arguments.The script has potential word splitting issues in the build argument handling.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 421-421: Quote this to prevent word splitting.
(SC2046)
[warning] 428-428: Quote this to prevent word splitting.
(SC2046)
| @@ -10,6 +10,7 @@ then | |||
| ARCH=$(uname -m) | |||
| fi | |||
|
|
|||
| INSTALL_DIR=${INSTALL_DIR:-/usr/local} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the installation directory.
The script needs validation for the installation directory to ensure it exists and has proper write permissions.
Apply this diff to add directory validation:
INSTALL_DIR=${INSTALL_DIR:-/usr/local}
+
+# Validate installation directory
+if [ -z "$INSTALL_DIR" ]; then
+ echo "Error: Installation directory is required" >&2
+ exit 1
+fi
+
+# Ensure installation directory exists and is writable
+mkdir -p "$INSTALL_DIR" || {
+ echo "Error: Failed to create installation directory: $INSTALL_DIR" >&2
+ exit 1
+}Also applies to: 25-27
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/build_ffmpeg.sh(14 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/build_ffmpeg.sh
[warning] 409-409: Quote this to prevent word splitting.
(SC2046)
[warning] 416-416: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build and test
- GitHub Check: build and test win
🔇 Additional comments (4)
scripts/build_ffmpeg.sh (4)
13-13: Add validation for the installation directory.The installation directory handling needs validation to ensure it exists and has proper permissions.
Apply this diff to add directory validation:
INSTALL_DIR=${INSTALL_DIR:-/usr/local} + +# Validate installation directory +if [ -z "$INSTALL_DIR" ]; then + echo "Error: Installation directory is required" >&2 + exit 1 +fi + +# Ensure installation directory exists and is writable +mkdir -p "$INSTALL_DIR" || { + echo "Error: Failed to create installation directory: $INSTALL_DIR" >&2 + exit 1 +}Also applies to: 25-27
131-141: Improve version handling and validation.The version handling needs more robust validation and explicit supported version declaration.
Apply this diff to improve version handling:
- version=${version:-5} + # Default version is 5 + : "${version:=5}" + + # Validate supported versions + case "$version" in + 4|4.4|5|5.1.6) + ;; + *) + echo "Error: Unsupported FFmpeg version. Supported versions: 4.4, 5.1.6" >&2 + exit 1 + ;; + esac
144-153: Add error handling for download and extraction operations.The download and extraction operations lack proper error handling and verification.
Apply this diff to improve error handling:
- if [ ! -e $ffmpeg_tar ]; then - curl -O -L $ffmpeg_url - fi - - if [ -d $ffmpeg_version ]; then - rm -rf $ffmpeg_version - fi - - tar xjvf $ffmpeg_tar - cd $ffmpeg_version + # Download FFmpeg if tarball not found + if [ ! -e "$ffmpeg_tar" ]; then + echo "Downloading FFmpeg ${ffmpeg_ver}..." + if ! curl -O -L --fail "$ffmpeg_url"; then + echo "Error: Failed to download FFmpeg" >&2 + exit 1 + fi + fi + + # Verify the downloaded tarball + if ! tar tf "$ffmpeg_tar" &>/dev/null; then + echo "Error: Invalid or corrupted tarball: $ffmpeg_tar" >&2 + rm -f "$ffmpeg_tar" + exit 1 + fi + + # Remove existing directory if present + if [ -d "$ffmpeg_version" ]; then + rm -rf "$ffmpeg_version" + fi + + # Extract the tarball + echo "Extracting FFmpeg ${ffmpeg_ver}..." + if ! tar xjf "$ffmpeg_tar"; then + echo "Error: Failed to extract FFmpeg" >&2 + exit 1 + fi + + # Change to FFmpeg directory + cd "$ffmpeg_version" || { + echo "Error: Failed to change to FFmpeg directory" >&2 + exit 1 + }
407-421:⚠️ Potential issueFix word splitting issues in build argument handling.
The script has potential word splitting issues in the build argument handling.
Apply this diff to fix the word splitting issues:
- if [[ ${arg} != "--install-dir"* ]] # Exclude --install-dir from build arguments + if [[ "${arg}" != "--install-dir"* ]] # Exclude --install-dir from build arguments then - (build_${arg}_unix $(pwd)/ffmpeg_source ${cores}) + (build_"${arg}"_unix "$(pwd)/ffmpeg_source" "${cores}") if [ ${arg} == "nasm" ] || [ ${arg} == "yasm" ] then disable_asm="" fi set +e - (check_lib $(pwd)/ffmpeg_source ${arg}) + (check_lib "$(pwd)/ffmpeg_source" "${arg}") if [ $? -eq 0 ] then ffmpeg_opts="${ffmpeg_opts} --enable-lib${arg}" fi set -e fiLikely invalid or redundant comment.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 409-409: Quote this to prevent word splitting.
(SC2046)
[warning] 416-416: Quote this to prevent word splitting.
(SC2046)
| @@ -50,7 +54,7 @@ function build_nasm_unix() { | |||
| tar xjvf nasm-2.15.05.tar.bz2 | |||
| cd nasm-2.15.05 | |||
| ./autogen.sh | |||
| ./configure --enable-shared | |||
| ./configure --enable-shared --prefix=$INSTALL_DIR | |||
There was a problem hiding this comment.
Fix quoting in build functions to handle paths with spaces.
The INSTALL_DIR variable is not quoted in the build functions, which could cause issues if the installation path contains spaces.
Apply this diff to fix the quoting:
- ./configure --enable-shared --prefix=$INSTALL_DIR
+ ./configure --enable-shared --prefix="$INSTALL_DIR"
- ./configure --prefix=$INSTALL_DIR
+ ./configure --prefix="$INSTALL_DIR"
- ./configure --enable-shared --prefix=$INSTALL_DIR
+ ./configure --enable-shared --prefix="$INSTALL_DIR"
- cmake -G "Unix Makefiles" -DENABLE_SHARED:bool=off -DCMAKE_INSTALL_PREFIX=$INSTALL_DIR ../../source
+ cmake -G "Unix Makefiles" -DENABLE_SHARED:bool=off -DCMAKE_INSTALL_PREFIX="$INSTALL_DIR" ../../source
- ./configure --enable-shared --prefix=$INSTALL_DIR
+ ./configure --enable-shared --prefix="$INSTALL_DIR"
- ./configure --enable-shared --enable-nasm --prefix=$INSTALL_DIR
+ ./configure --enable-shared --enable-nasm --prefix="$INSTALL_DIR"
- ./configure --enable-shared --prefix=$INSTALL_DIR
+ ./configure --enable-shared --prefix="$INSTALL_DIR"
- ./configure --disable-examples --disable-unit-tests --enable-vp9-highbitdepth --as=yasm --prefix=$INSTALL_DIR
+ ./configure --disable-examples --disable-unit-tests --enable-vp9-highbitdepth --as=yasm --prefix="$INSTALL_DIR"Also applies to: 67-67, 76-76, 85-85, 95-95, 105-105, 115-115, 124-124
|
Summary by CodeRabbit
Chores
.gitignoreto exclude build artifacts and test-related files.New Features
Bug Fixes