-
Notifications
You must be signed in to change notification settings - Fork 478
Add ffmpeg with 16kb page size support, add prebuilt 16kb aligned libraries in the README #543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis update introduces support for 16KB memory page sizes in the Android-GPUImage-Plus project. It modifies build scripts, Gradle configurations, and documentation to distinguish between 4KB and 16KB page size variants. The JNI build system and FFmpeg integration are updated to support conditional selection of libraries and flags. New FFmpeg configuration headers are added for each architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant VSCode as VSCode Tasks
participant BuildScript as tasks.sh/buildJNI
participant Env as Build Environment
participant JNI as JNI Build System
participant FFmpeg as FFmpeg Libraries
Dev->>VSCode: Select "Enable 16KB Page Size" task
VSCode->>BuildScript: Run tasks.sh --enable-16kb-page-size
BuildScript->>Env: Set CGE_ENABLE_16KB_PAGE_SIZE=1
Env->>JNI: Start build with 16KB page size flag
JNI->>FFmpeg: Link against ffmpeg-16kb libs and includes
JNI->>JNI: Add linker flag -Wl,-z,max-page-size=16384
JNI-->>Dev: Build output with 16KB page size
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (24)
📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (12)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
🧹 Nitpick comments (14)
library/src/main/jni/ffmpeg/include/config.x86_64.h (2)
5-6
: Confirm GPL-v3 obligations
#define FFMPEG_LICENSE "GPL version 3 or later"
plusCONFIG_GPL 1
andCONFIG_LIBX264 1
places the resulting AAR / .so under GPLv3. Distributing on Google Play or embedding in closed-source apps requires the whole app to comply.Please verify this is acceptable for your downstream consumers.
396-415
: Example programs left enabled – unnecessary binary bloatMacros such as
CONFIG_ENCODE_AUDIO_EXAMPLE 1
,CONFIG_MUXING_EXAMPLE 1
, ... are still1
even though all CLI tools are disabled. They pull in extra objects and enlarge.so
size.Add
--disable-programs --disable-doc --disable-examples
to yourconfigure
flags and regenerate, or patch them to0
.library/src/main/jni/ffmpeg-16kb/include/config.x86.h (2)
1-3
: Generated artefact committed to VCS – reconsider
config.x86.h
is a huge, fully-generated header. Committing it bloats the repo and creates painful merge conflicts every time FFmpeg is re-configured. Prefer adding the generation step to the build pipeline (or vendoring a minimal patch) and git-ignoring the file.
511-520
: CUDA/NVENC enabled in an Android/x86 build – unnecessary weight
CONFIG_CUDA 1
,CONFIG_NVENC 1
,CONFIG_CUVID 1
, etc. significantly inflate binary size and pull extra dependencies that are unusable on Android devices lacking Nvidia GPUs. Disabling these flags (--disable-cuda --disable-nvenc --disable-cuvid
) trims ~3-5 MB per ABI.library/src/main/jni/ffmpeg-16kb/include/config.armeabi-v7a.h (3)
4-4
: Hard-coded absolute paths reduce build reproducibility
FFMPEG_CONFIGURATION
embeds absolute paths from the author’s local machine.
This leaks environment details and makes external or CI builds non-deterministic.
Consider re-runningconfigure
in a container / repo-relative path (or post-processing the header withsed
) so only portable, reproducible paths are stored.
9-9
: Tool-chain fingerprint constantly churns the header
CC_IDENT
includes the full clang revision hash.
Every NDK upgrade will change this line and bloat future diffs.
You can safely strip or overwrite it after configure to keep the file stable.
41-47
: NEON optimisations disabled – verify necessity
--disable-neon
⇒HAVE_NEON 0
, yet the target isarmv7-a
.
Unless the 16 KB page-size build genuinely breaks NEON, this will
forfeit a large performance gain on most devices.
Double-check if the restriction is still required.library/src/main/jni/ffmpeg-16kb/include/config.x86_64.h (2)
534-539
: Double-check license flagsBoth
CONFIG_GPL
andCONFIG_VERSION3
are1
, meaning the resulting binaries
are GPLv3. Ensure that:
- Your project’s license and distribution model are compatible with GPLv3.
- Any downstream apps (especially on Play Store) are aware of this obligation.
If GPL compliance is undesired, rebuild with
--disable-gpl
/ remove
GPL-only components such aslibx264
.
2354-2356
: No trailing newlineMinor, but the header ends without a final newline; some compilers warn on this.
library/src/main/jni/ffmpeg/include/config.x86.h (2)
9-9
:CC_IDENT
also leaks local revision metadata & paths
CC_IDENT
captures the full clang tool-chain description including an internal git hash.
Consider passing--disable-ident
(or overridingCC_IDENT
with a sanitized string) duringconfigure
so that binaries don’t expose build host information.
1-2356
: Generated file should be flagged as such and excluded from manual editsAdd a top-of-file notice or
.gitattributes
entry marking allconfig.*.h
as generated to:
- Skip them in clang-format / static-analysis runs.
- Avoid accidental edits during code reviews.
Example:
library/src/main/jni/ffmpeg/include/config.*.h linguist-generatedlibrary/src/main/jni/ffmpeg/include/config.armeabi-v7a.h (1)
1-2356
: Consider excluding this 2 .3 k-line auto-generated header from VCS.
config.armeabi-v7a.h
is produced by FFmpeg’sconfigure
script and changes whenever build flags or toolchain versions change.
Checking it in:
- Bloats the repository and causes noisy diffs.
- Forces manual updates whenever FFmpeg is rebuilt.
- Couples native build logic to source control, undermining reproducible builds.
Typical pattern: keep only the build scripts and let the CI/Gradle task regenerate the header into
$(buildDir)/generated/ffmpeg/include
. If shipping pre-built binaries is required, store headers in an artifacts archive rather than the main tree.If keeping the file is unavoidable, document the regeneration procedure and add a linter rule preventing accidental edits.
library/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h (1)
1-4
: Huge auto-generated header committed to VCSThe 2 300-line config header is machine-generated and changes on every rebuild.
Keeping it in git bloats the repo and causes noisy diffs.Consider adding it to
.gitignore
and generating it during the CMake configure step.library/src/main/jni/CMakeLists.txt (1)
109-116
: Imported ffmpeg target may miss SONAME & interface settingsWhen importing a pre-built
.so
, set properties to avoid link-time surprises:set_target_properties(ffmpeg PROPERTIES IMPORTED_LOCATION "${FFMPEG_DIR}/libs/${ANDROID_ABI}/libffmpeg.so" IMPORTED_NO_SONAME TRUE INTERFACE_COMPILE_DEFINITIONS CGE_USE_FFMPEG=1)At a minimum, add
IMPORTED_NO_SONAME TRUE
to suppress duplicatesoname
warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
library/src/main/jni/ffmpeg-16kb/libs/arm64-v8a/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg-16kb/libs/armeabi-v7a/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg-16kb/libs/x86/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg-16kb/libs/x86_64/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/arm64-v8a/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/armeabi-v7a/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/libs/arm64-v8a/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/libs/armeabi-v7a/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/libs/x86/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/libs/x86_64/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/x86/libffmpeg.so
is excluded by!**/*.so
library/src/main/jni/ffmpeg/x86_64/libffmpeg.so
is excluded by!**/*.so
library/src/main/libs/arm64-v8a/libCGE.so
is excluded by!**/*.so
library/src/main/libs/arm64-v8a/libCGEExt.so
is excluded by!**/*.so
library/src/main/libs/arm64-v8a/libffmpeg.so
is excluded by!**/*.so
library/src/main/libs/armeabi-v7a/libCGE.so
is excluded by!**/*.so
library/src/main/libs/armeabi-v7a/libCGEExt.so
is excluded by!**/*.so
library/src/main/libs/armeabi-v7a/libffmpeg.so
is excluded by!**/*.so
library/src/main/libs/x86/libCGE.so
is excluded by!**/*.so
library/src/main/libs/x86/libCGEExt.so
is excluded by!**/*.so
library/src/main/libs/x86/libffmpeg.so
is excluded by!**/*.so
library/src/main/libs/x86_64/libCGE.so
is excluded by!**/*.so
library/src/main/libs/x86_64/libCGEExt.so
is excluded by!**/*.so
library/src/main/libs/x86_64/libffmpeg.so
is excluded by!**/*.so
📒 Files selected for processing (17)
.vscode/tasks.json
(1 hunks)README.md
(2 hunks)build.gradle
(1 hunks)library/build.gradle
(2 hunks)library/src/main/jni/Android.mk
(2 hunks)library/src/main/jni/Application.mk
(1 hunks)library/src/main/jni/CMakeLists.txt
(2 hunks)library/src/main/jni/buildJNI
(1 hunks)library/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h
(1 hunks)library/src/main/jni/ffmpeg-16kb/include/config.armeabi-v7a.h
(1 hunks)library/src/main/jni/ffmpeg-16kb/include/config.x86.h
(1 hunks)library/src/main/jni/ffmpeg-16kb/include/config.x86_64.h
(1 hunks)library/src/main/jni/ffmpeg-16kb/version.h
(1 hunks)library/src/main/jni/ffmpeg/include/config.arm64-v8a.h
(1 hunks)library/src/main/jni/ffmpeg/include/config.armeabi-v7a.h
(1 hunks)library/src/main/jni/ffmpeg/include/config.x86.h
(1 hunks)library/src/main/jni/ffmpeg/include/config.x86_64.h
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (21)
build.gradle (1)
34-34
: LGTM! Appropriate version bump for new feature.The minor version increment from 3.0.0 to 3.1.0 correctly follows semantic versioning for the new 16KB page size feature addition.
library/src/main/jni/Application.mk (1)
16-18
: LGTM! Correct conditional logic for flexible page sizes.The conditional block properly enables
APP_SUPPORT_FLEXIBLE_PAGE_SIZES
when the 16KB page size feature is enabled, which is the correct approach for Android NDK builds targeting devices with different page sizes.library/src/main/jni/buildJNI (2)
53-57
: LGTM! Well-implemented command-line option.The new
--16kb
option correctly:
- Supports case-insensitive variants (
--16kb
and--16KB
)- Sets the appropriate environment variable
CGE_ENABLE_16KB_PAGE_SIZE=1
- Provides clear user feedback
- Follows the existing argument parsing pattern
58-61
: Good improvement to error handling.Adding a default case to handle unknown options with clear error messages improves the script's usability and debugging experience.
.vscode/tasks.json (1)
181-208
: LGTM! Well-structured VS Code tasks for 16KB page size feature.The new tasks are properly configured and provide convenient IDE integration for toggling the 16KB page size feature. They follow the existing task structure and naming conventions consistently.
library/src/main/jni/Android.mk (2)
112-114
: LGTM! Proper linker flag for 16KB page size support.The conditional addition of
-Wl,-z,max-page-size=16384
is correct for enabling 16KB page size support. This aligns with the Android documentation for flexible page sizes.
168-174
: Verify consistency with CMake build system.The conditional FFmpeg path switching is well-implemented. However, ensure that the CMake build system (CMakeLists.txt) uses the same conditional logic and paths for consistency.
#!/bin/bash # Verify CMake build system uses consistent FFmpeg paths and conditions grep -A 10 -B 5 "16KB\|16kb\|ffmpeg-16kb" library/src/main/jni/CMakeLists.txtREADME.md (3)
18-18
: Good documentation improvement.Adding the GitHub repository URL in the comment improves clarity for developers.
24-44
: Comprehensive dependency documentation.The new dependency variants clearly document the different page size and feature combinations. The naming convention (adding
-16k
and-min
suffixes) is intuitive and well-organized.
95-96
: Clear instructions for 16KB page size enablement.The environment variable approach is consistent with the build system implementation and provides a simple way for developers to enable 16KB page size support.
library/build.gradle (2)
132-140
: Well-implemented version naming logic.The conditional version naming correctly implements the scheme documented in the README. The logic properly handles both 16KB page size (
-16k
) and minimal build (-min
) suffixes, allowing for combined suffixes like3.1.0-16k-min
.
150-154
: Improved Maven publishing configuration.The updated publishing configuration properly handles different Android components and explicitly includes the AAR artifact, which should improve the reliability of artifact publishing.
library/src/main/jni/ffmpeg/include/config.arm64-v8a.h (2)
44-45
: Appropriate ARM64 CPU features enabled.The configuration correctly enables ARM64-v8a specific features:
HAVE_ARMV8 1
andHAVE_ARMV8_EXTERNAL 1
HAVE_NEON 1
andHAVE_NEON_EXTERNAL 1
HAVE_ARMV8_INLINE 1
andHAVE_NEON_INLINE 1
These settings are appropriate for ARM64-v8a architecture and will enable optimized code paths.
Also applies to: 88-89, 132-133
1-11
: ```shell
#!/bin/bashCheck license consistency across all architecture config headers
echo "License strings in config headers:"
find library/src/main/jni -type f -name "config.*.h" -print0 | xargs -0 rg --no-heading "#define FFMPEG_LICENSE"echo
Check CONFIG_THIS_YEAR consistency across all architecture config headers
echo "CONFIG_THIS_YEAR in config headers:"
find library/src/main/jni -type f -name "config.*.h" -print0 | xargs -0 rg --no-heading "#define CONFIG_THIS_YEAR"</details> <details> <summary>library/src/main/jni/ffmpeg/include/config.x86_64.h (1)</summary> `511-520`: **CUDA / NVENC flags enabled on Android – check link-time failures** `CONFIG_CUDA 1`, `CONFIG_NVENC 1`, `CONFIG_CUVID 1`, etc. are set, but CUDA/NVENC tool-chains are not available in the Android NDK for x86_64. Expect unresolved symbols at link time or bigger binaries with dead code. Re-run `configure` with `--disable-cuda --disable-nvenc --disable-cuvid` (or similar) unless you explicitly ship custom CUDA stubs. </details> <details> <summary>library/src/main/jni/ffmpeg-16kb/include/config.armeabi-v7a.h (2)</summary> `533-535`: **Static + small build flags are appropriate** `--enable-small` + static linking fit mobile footprints; no action needed. --- `196-198`: ```shell #!/bin/bash # List all FFmpeg config headers and check HAVE_X86ASM values echo "Config headers and HAVE_X86ASM definitions:" for file in library/src/main/jni/ffmpeg-16kb/include/config.*.h; do echo "=== $file ===" grep -n "HAVE_X86ASM" "$file" || echo " (no HAVE_X86ASM)" done
library/src/main/jni/ffmpeg-16kb/include/config.x86_64.h (1)
511-520
: Validate CUDA / CUVID / NVENC flags for Android-x86_64
CONFIG_CUDA
,CONFIG_CUVID
,CONFIG_NVENC
are enabled (1
) yet Android-x86_64
NDK tool-chains rarely ship the required CUDA SDK, and the resulting objects
will pull unresolved symbols (cudaMalloc
,nvEncodePicture
, …).Confirm that:
- The pre-built 16 KB libraries you ship actually contain/need these stubs.
- Java side loads
libcuda.so
/libnvenc.so
from the device (unlikely).If they are not strictly required, disable them in
configure
to save ~4 MB
of binary size and avoid runtime link errors on devices without CUDA support.library/src/main/jni/ffmpeg/include/config.x86.h (1)
4-4
: Machine-specific absolute paths hurt reproducibility and leak local env details
FFMPEG_CONFIGURATION
embeds/Volumes/HikData/...
and other absolute paths from the author’s workstation.
These paths:
- Break deterministic builds – anyone rebuilding in another location will get a different binary hash.
- Expose your internal directory layout in public artefacts / crash logs.
- Complicate CI caching because paths become embedded in objects.
Regenerate the header with
--prefix
,--extra-cflags
,--extra-ldflags
, … that use environment variables or relative paths, or post-process the string to scrub absolute segments before committing.#!/bin/bash # Detect any hard-coded absolute paths in all ffmpeg config headers fd -e h config | xargs grep -nE '/(Users|Volumes|home|C:)' || truelibrary/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h (2)
4-9
: ```shell
#!/bin/bashVerify that absolute build-machine paths are present in the generated header
grep -n "/Volumes/HikData" library/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h
--- `195-199`: ```shell #!/bin/bash # Inspect the top of the config file for architecture and ISA defines sed -n '1,60p' library/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h # Locate the HAVE_X86ASM definition grep -n "#define HAVE_X86ASM" library/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h # Find any ARCH_* macros in this config grep -n "#define ARCH" library/src/main/jni/ffmpeg-16kb/include/config.arm64-v8a.h # Search for how HAVE_X86ASM is used elsewhere in the FFmpeg tree grep -R "HAVE_X86ASM" -n library/src/main/jni/ffmpeg-16kb
…dd tasks for 16kb page size changing, ref: <#542>
8da32a7
to
31dfbe8
Compare
Add ffmpeg with 16kb page size support, fix cmake with 16kb ffmpeg, add tasks for 16kb page size changing, ref: #542
Summary by CodeRabbit
New Features
Documentation
Build & Release