adding ffmpeg v7 #1777
adding ffmpeg v7 #1777
Conversation
🦋 Changeset detectedLatest commit: db0c7df The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
""" WalkthroughThe changes add a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant BP as Build Process
participant F as ffmpeg7 Function
participant BL as Build Layer
BP->>F: Call ffmpeg7 (onBuildComplete)
Note right of F: Check if build target is "dev"
F-->>BP: Early return if "dev"
alt Non-dev target
F->>F: Log debug message for ffmpeg7 integration
F->>BL: Add build layer with ffmpeg 7 installation steps
BL->>BP: Deploy layer with FFMPEG_PATH and FFPROBE_PATH set
end
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 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 (
|
|
I am happy to hear if its needed or not |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/build/src/extensions/core/ffmpeg.ts (2)
51-52: Fix coding style inconsistenciesThere are inconsistencies in spacing after colons compared to the rest of the file.
- name:"ffmpeg7", + name: "ffmpeg7",- id:"ffmpeg", + id: "ffmpeg7",- deploy : { + deploy: {Also applies to: 60-61, 67-68
63-64:❓ Verification inconclusive
Consider pinning to a specific version
The current implementation downloads the latest FFmpeg 7.x build without version pinning. For reproducible builds, consider downloading a specific version or validating the download with checksums.
🏁 Script executed:
#!/bin/bash # Check what versions are available on johnvansickle.com curl -s https://johnvansickle.com/ffmpeg/ | grep -o "ffmpeg-7\.[0-9.]\+-amd64-static\.tar\.xz" | sort -uLength of output: 106
I'll run a new verification script using a broader regex pattern to capture any FFmpeg tarball names (both release and versioned) from the releases page. This should help us determine if version pinning is feasible or if the current URL indeed only serves the generic release file.
Next step: Running an updated verification script
Please run this script and share the output so we can conclude whether a specific version is available for pinning or if we need to consider adding checksum validation to ensure reproducibility.
🏁 Script executed:
#!/bin/bash # Use a broader regex pattern to capture any FFmpeg tarball names on the releases page curl -s https://johnvansickle.com/ffmpeg/releases/ | grep -Eo 'ffmpeg[-a-zA-Z0-9._]*static\.tar\.xz' | sort -uLength of output: 112
Action Required: Ensure Reproducibility for FFmpeg Installation
The current implementation downloads a generic FFmpeg archive (ffmpeg-release-amd64-static.tar.xz) without pinning to a specific version, which risks non-reproducible builds. Our automated checks using broader regex patterns did not return any version-specific archive names from johnvansickle.com, so it remains unclear whether a versioned file (e.g., ffmpeg-7.x.x-amd64-static.tar.xz) is available.
- File: packages/build/src/extensions/core/ffmpeg.ts (lines 63-64)
- Consider pinning to a specific FFmpeg version if one is available.
- Alternatively, add checksum validation to ensure the integrity and reproducibility of the downloaded archive.
Please perform a manual verification of the downloads at johnvansickle.com/ffmpeg to determine if a version-specific archive exists, or update the build script accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/core/ffmpeg.ts(1 hunks)
🔇 Additional comments (1)
packages/build/src/extensions/core/ffmpeg.ts (1)
41-77: Overall implementation looks goodThe implementation for adding FFmpeg 7 looks good overall. It correctly:
- Skips operation for dev targets
- Logs debug information
- Configures the necessary environment variables
- Uses a reliable source for the static binaries
The approach of using a static build from johnvansickle.com is appropriate for getting the latest FFmpeg 7 capabilities.
|
@coderabbitai is this pr looks good and ok? |
🧩 Analysis chainI'll need to gather more information about this PR to properly evaluate it. Let me check the details of the code changes and the related issue. 🏁 Script executed: Length of output: 4107 @Rudra-Sankha-Sinhamahapatra I've reviewed your PR implementing FFmpeg version 7, and it looks good! The implementation correctly addresses issue #1688 by using a static build approach to install FFmpeg 7.x instead of relying on apt-get packages. The code is well structured and maintains compatibility with the existing name:"ffmpeg7", // should be name: "ffmpeg7",Other than that, the PR looks good to go and properly solves the issue. Nice work! ✅ Actions performedComments resolved. |
|
@coderabbitai I have added space for formatting now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/build/src/extensions/core/ffmpeg.ts (4)
53-53: Add space after if for consistent formatting.There should be a space between
ifand the opening parenthesis to match the coding style used elsewhere in the file.- if(context.target === "dev") { + if (context.target === "dev") {
60-60: Add space after colon for consistent formatting.To maintain consistent formatting with the rest of the codebase, add a space after the colon.
- id:"ffmpeg7", + id: "ffmpeg7",
67-67: Remove extra space before colon for consistent formatting.To maintain consistent formatting with the rest of the codebase, remove the extra space before the colon.
- deploy : { + deploy: {
41-77: Consider adding integrity verification for the downloaded binary.While downloading from johnvansickle.com is a common practice for FFmpeg static builds, adding checksum or signature verification would improve security by ensuring the downloaded file hasn't been tampered with.
Would you like me to provide an example of how to implement checksum verification for the downloaded binary?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/core/ffmpeg.ts(1 hunks)
🔇 Additional comments (3)
packages/build/src/extensions/core/ffmpeg.ts (3)
51-51: Good naming convention for the new function.The function name "ffmpeg7" clearly indicates its purpose to add FFmpeg version 7 to the build environment, distinguishing it from the original "ffmpeg" function.
63-64: Good approach for installing FFmpeg 7.x via static build.Using a static build from johnvansickle.com is an excellent approach to ensure compatibility with FFmpeg 7.x, especially since it might not be available in standard package repositories yet. The commands properly:
- Install required dependencies
- Download and extract only the needed binaries
- Clean up temporary files and package lists
This is more reliable than depending on apt-get packages for newer FFmpeg versions.
42-47: Well-documented function with clear purpose.The JSDoc comments clearly explain the purpose of the function, how it differs from the original
ffmpegfunction, and the source of the FFmpeg binaries. This documentation helps future developers understand the implementation.
🧩 Analysis chain@Rudra-Sankha-Sinhamahapatra Great! I can see that you've fixed the formatting issue by adding the space after Since all issues have been addressed, this PR is ready to be approved. Good work on implementing FFmpeg version 7! 🐰✨ happy rabbit noises 🏁 Script executed: Length of output: 2193 @Rudra-Sankha-Sinhamahapatra Thank you for fixing the spacing issue with
These are small details, but fixing them will ensure consistent code style throughout the codebase. Would you mind addressing these minor formatting issues as well? ✅ Actions performedComments resolved. |
|
@coderabbitai I have fixed these formatting issues |
🧩 Analysis chain🏁 Script executed: Length of output: 2194 @Rudra-Sankha-Sinhamahapatra Thank you for fixing all the formatting issues! I've verified the changes and everything looks great now. Your PR implementing FFmpeg version 7 is ready to be merged. Nice work! If you found this review helpful, would you consider giving us a shout-out on X? ✅ Actions performedComments resolved. |
|
Can someone check this pr |
This reverts commit 74cd317.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/unit-tests.yml (2)
10-12: Ensure both DockerHub credentials are accounted for in the environment.The job-level
envsection only injectsDOCKERHUB_USERNAMEfromsecrets, but the login step also relies on theDOCKERHUB_TOKENsecret. If the token isn’t set, the login action will error out. Consider also exposingDOCKERHUB_TOKENat the job level or validating its presence before attempting to log in.
57-64: Make the DockerHub login guard more robust by checking both username and token.Currently, the
ifaround the login step only testsenv.DOCKERHUB_USERNAME. If that secret exists butDOCKERHUB_TOKENis missing or invalid, the login will still run and then fail. You can tighten the condition to require both credentials and avoid runtime errors. For example:# In the job’s env block: env: - DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} + DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} + DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} # Login step: - if: ${{ env.DOCKERHUB_USERNAME }} + if: ${{ env.DOCKERHUB_USERNAME && env.DOCKERHUB_TOKEN }} # Skip-login step: - if: ${{ !env.DOCKERHUB_USERNAME }} + if: ${{ !env.DOCKERHUB_USERNAME || !env.DOCKERHUB_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/thick-bikes-laugh.md(1 hunks).github/workflows/unit-tests.yml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/thick-bikes-laugh.md
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
There was a problem hiding this comment.
Thanks for this @Rudra-Sankha-Sinhamahapatra and sorry it took us a while to get this in.
I ended up merging this into the existing extension to keep things simple. Shouldn't be a breaking change. If version: 7 it will use the static build. Other version numbers will be ignored for now as they never had any effect and could even make builds fail.
Closes #1688
✅ Checklist
Testing
Changelog
I have added ffmpeg v7 so that users can have an option to choose versions manually
Screenshots
N.A
💯
Summary by CodeRabbit
Summary by CodeRabbit