Upgrade FFmpeg to 8.0 #154
Conversation
WalkthroughAdds an AVTX FFT backend (new header, implementation, and CMake wiring), extends FFmpeg discovery for avutil-tx, enables conditional build flags and config macro, and bumps packaged FFmpeg from 5.1.2 to 8.0 in the build script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
- Implement new fft_lib_avtx backend using libavutil/tx.h API - Add avtx as FFT_LIB option with auto-detection for FFmpeg 8.0+ - Maintain backward compatibility with existing avfft backend - Update CMake build system to handle both backends - Add tx.h header detection in FindFFmpeg.cmake - All tests pass with new avtx implementation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
-
CMakeLists.txt(3 hunks) -
cmake/modules/FindFFmpeg.cmake(1 hunks) -
config.h.in(1 hunks) -
src/CMakeLists.txt(1 hunks) -
src/fft_lib.h(1 hunks) -
src/fft_lib_avtx.cpp(1 hunks) -
src/fft_lib_avtx.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/fft_lib.h (4)
src/fft_lib_avfft.h (2)
FFTLib(18-33)chromaprint(16-35)src/fft_lib_fftw3.h (1)
FFTLib(26-42)src/fft_lib_vdsp.h (1)
FFTLib(14-31)src/fft.h (1)
FFT(19-47)
src/fft_lib_avtx.h (4)
src/fft_lib_avtx.cpp (6)
FFTLib(8-22)FFTLib(24-29)Load(31-36)Load(31-31)Compute(38-65)Compute(38-38)src/fft_lib_avfft.h (2)
chromaprint(16-35)FFTLib(18-33)src/fft_lib_avfft.cpp (1)
FFTLib(6-47)src/fft.h (1)
chromaprint(15-49)
src/CMakeLists.txt (4)
src/fft_lib_avfft.cpp (1)
FFTLib(6-47)src/fft_lib_avfft.h (2)
chromaprint(16-35)FFTLib(18-33)src/fft.h (1)
chromaprint(15-49)src/fft_frame.h (1)
chromaprint(9-13)
CMakeLists.txt (4)
src/fft_lib_avfft.h (2)
FFTLib(18-33)chromaprint(16-35)src/fft_lib_avfft.cpp (3)
FFTLib(6-47)FFTLib(8-18)FFTLib(20-24)src/fft_lib_vdsp.cpp (1)
FFTLib(7-48)src/fft_lib_fftw3.cpp (1)
FFTLib(6-46)
src/fft_lib_avtx.cpp (2)
src/fft_lib_avtx.h (1)
FFTLib(17-34)src/fft_lib_avfft.cpp (1)
FFTLib(6-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-linux (fftw3)
- GitHub Check: test-macos (vdsp)
- GitHub Check: test-macos (kissfft)
🔇 Additional comments (10)
cmake/modules/FindFFmpeg.cmake (1)
105-105: AVTX wiring correct USE_AVTX block in src/CMakeLists.txt links${FFMPEG_LIBAVUTIL_LIBRARIES}and includes${FFMPEG_LIBAVUTIL_INCLUDE_DIRS}, with LIBAVUTIL_TX_FOUND as the sole feature guard.src/fft_lib_avtx.h (2)
7-11: C includes in extern "C" block look correct.No issues here.
17-24: Zero outframeon AVTX init failure inCompute
Prevent stale data by replacing the early return with:- if (!m_tx_ctx || !m_tx_fn) return; + if (!m_tx_ctx || !m_tx_fn) { + std::fill(frame.begin(), frame.end(), 0.0f); + return; + }Verify that
FFTFramesupportsbegin()/end()and that a zeroed spectrum is appropriate downstream.CMakeLists.txt (4)
16-16: Expose “avtx” in FFT_LIB choices — good.Keeps cache UX consistent.
99-105: Backend flags initialized explicitly — good.Prevents stale cache surprises.
106-111: Reasonable default: prefer avtx when available.Helps migrate to FFmpeg 8 smoothly.
128-134: Backend gating and src wiring verified
src/CMakeLists.txt correctly adds fft_lib_avtx.cpp, links ${FFMPEG_LIBAVUTIL_LIBRARIES} and includes ${FFMPEG_LIBAVUTIL_INCLUDE_DIRS} when USE_AVTX is ON.src/fft_lib_avtx.cpp (3)
31-36: Load path mirrors AVFFT semantics. LGTM.Windowing and accumulation match the existing backend.
38-46: Guard against uninitialized TX context (good).Early return prevents crashes when init failed.
52-56: DC/Nyquist handling matches AVFFT layout.Correct mapping: bins 0 and N/2 are real-only.
| m_window = (float *) av_malloc(sizeof(float) * frame_size); | ||
| m_input = (float *) av_malloc(sizeof(float) * frame_size); | ||
| m_output = (AVComplexFloat *) av_malloc(sizeof(AVComplexFloat) * (frame_size / 2 + 1)); | ||
| PrepareHammingWindow(m_window, m_window + frame_size, 1.0 / INT16_MAX); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer av_malloc_array to avoid size_t overflow and consider OOM checks.
Small safety win; also verify INT16_MAX is visible in this TU (via or a transitive include).
- m_window = (float *) av_malloc(sizeof(float) * frame_size);
- m_input = (float *) av_malloc(sizeof(float) * frame_size);
- m_output = (AVComplexFloat *) av_malloc(sizeof(AVComplexFloat) * (frame_size / 2 + 1));
+ m_window = (float *) av_malloc_array(frame_size, sizeof(float));
+ m_input = (float *) av_malloc_array(frame_size, sizeof(float));
+ m_output = (AVComplexFloat *) av_malloc_array(frame_size / 2 + 1, sizeof(AVComplexFloat));📝 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.
| m_window = (float *) av_malloc(sizeof(float) * frame_size); | |
| m_input = (float *) av_malloc(sizeof(float) * frame_size); | |
| m_output = (AVComplexFloat *) av_malloc(sizeof(AVComplexFloat) * (frame_size / 2 + 1)); | |
| PrepareHammingWindow(m_window, m_window + frame_size, 1.0 / INT16_MAX); | |
| m_window = (float *)av_malloc_array(frame_size, sizeof(float)); | |
| m_input = (float *)av_malloc_array(frame_size, sizeof(float)); | |
| m_output = (AVComplexFloat *)av_malloc_array(frame_size / 2 + 1, sizeof(AVComplexFloat)); | |
| PrepareHammingWindow(m_window, m_window + frame_size, 1.0 / INT16_MAX); |
🤖 Prompt for AI Agents
In src/fft_lib_avtx.cpp around lines 9 to 13, replace raw av_malloc calls with
av_malloc_array to prevent size_t overflow and add null checks for each
allocation to handle OOM (log/error and early return/cleanup on failure); ensure
INT16_MAX is defined in this translation unit by including <cstdint> or <limits>
(or replace with std::numeric_limits<int16_t>::max()) so the
PrepareHammingWindow call uses a visible constant.
| FFTLib::~FFTLib() { | ||
| av_tx_uninit(&m_tx_ctx); | ||
| av_free(m_output); | ||
| av_free(m_input); | ||
| av_free(m_window); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Free-and-null to avoid dangling pointers.
av_freep nulls the pointer and is robust for repeated cleanup paths.
- av_tx_uninit(&m_tx_ctx);
- av_free(m_output);
- av_free(m_input);
- av_free(m_window);
+ av_tx_uninit(&m_tx_ctx);
+ av_freep((void**)&m_output);
+ av_freep((void**)&m_input);
+ av_freep((void**)&m_window);📝 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.
| FFTLib::~FFTLib() { | |
| av_tx_uninit(&m_tx_ctx); | |
| av_free(m_output); | |
| av_free(m_input); | |
| av_free(m_window); | |
| } | |
| FFTLib::~FFTLib() { | |
| av_tx_uninit(&m_tx_ctx); | |
| av_freep((void**)&m_output); | |
| av_freep((void**)&m_input); | |
| av_freep((void**)&m_window); | |
| } |
🤖 Prompt for AI Agents
In src/fft_lib_avtx.cpp around lines 24 to 29, the destructor frees m_output,
m_input and m_window with av_free which can leave dangling pointers; replace
those calls with av_freep(&m_output), av_freep(&m_input) and av_freep(&m_window)
so the pointers are nulled and safe for repeated cleanup paths (leave
av_tx_uninit(&m_tx_ctx) as-is).
| auto input = m_output; | ||
| auto output = frame.begin(); | ||
|
|
||
| // Handle DC component (index 0) | ||
| output[0] = input[0].re * input[0].re; | ||
|
|
||
| // Handle Nyquist frequency (index frame_size/2) | ||
| output[m_frame_size / 2] = input[m_frame_size / 2].re * input[m_frame_size / 2].re; | ||
|
|
||
| // Handle intermediate frequencies (indices 1 to frame_size/2-1) | ||
| output += 1; | ||
| input += 1; | ||
| for (size_t i = 1; i < m_frame_size / 2; i++) { | ||
| *output++ = input->re * input->re + input->im * input->im; | ||
| input++; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tighten up output writes: use data(), precompute half, and (optionally) ensure capacity.
This improves clarity and avoids iterator arithmetic; resizing guards against misuse by callers.
- auto input = m_output;
- auto output = frame.begin();
-
- // Handle DC component (index 0)
- output[0] = input[0].re * input[0].re;
-
- // Handle Nyquist frequency (index frame_size/2)
- output[m_frame_size / 2] = input[m_frame_size / 2].re * input[m_frame_size / 2].re;
-
- // Handle intermediate frequencies (indices 1 to frame_size/2-1)
- output += 1;
- input += 1;
- for (size_t i = 1; i < m_frame_size / 2; i++) {
- *output++ = input->re * input->re + input->im * input->im;
- input++;
- }
+ const size_t half = m_frame_size / 2;
+ // Ensure capacity if caller didn't pre-size (optional; keep if consistent with other backends)
+ if (frame.size() < half + 1) frame.resize(half + 1);
+
+ const AVComplexFloat *in = m_output;
+ double *out = frame.data();
+
+ // DC and Nyquist
+ out[0] = in[0].re * in[0].re;
+ out[half] = in[half].re * in[half].re;
+
+ // Intermediate bins
+ in += 1;
+ out += 1;
+ for (size_t i = 1; i < half; ++i) {
+ *out++ = in->re * in->re + in->im * in->im;
+ ++in;
+ }📝 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.
| auto input = m_output; | |
| auto output = frame.begin(); | |
| // Handle DC component (index 0) | |
| output[0] = input[0].re * input[0].re; | |
| // Handle Nyquist frequency (index frame_size/2) | |
| output[m_frame_size / 2] = input[m_frame_size / 2].re * input[m_frame_size / 2].re; | |
| // Handle intermediate frequencies (indices 1 to frame_size/2-1) | |
| output += 1; | |
| input += 1; | |
| for (size_t i = 1; i < m_frame_size / 2; i++) { | |
| *output++ = input->re * input->re + input->im * input->im; | |
| input++; | |
| } | |
| auto const size_t half = m_frame_size / 2; | |
| // Ensure capacity if caller didn't pre-size (optional; keep if consistent with other backends) | |
| if (frame.size() < half + 1) | |
| frame.resize(half + 1); | |
| auto const AVComplexFloat* in = m_output; | |
| auto double* out = frame.data(); | |
| // DC and Nyquist | |
| out[0] = in[0].re * in[0].re; | |
| out[half] = in[half].re * in[half].re; | |
| // Intermediate bins | |
| in += 1; | |
| out += 1; | |
| for (size_t i = 1; i < half; ++i) { | |
| *out++ = in->re * in->re + in->im * in->im; | |
| +in; | |
| } |
🤖 Prompt for AI Agents
In src/fft_lib_avtx.cpp around lines 49 to 64, the code uses iterator arithmetic
for writes into frame and input which is fragile and less clear; change to use
frame.data() and m_output (or input) data pointers, precompute size_t half =
m_frame_size / 2, and write DC and Nyquist via frame_data[0] and
frame_data[half]; then loop i from 1 to half-1 and write to frame_data[i] using
input_data[i].re/im to compute magnitude squared. Also ensure frame has capacity
(e.g., resize or assert frame.size() >= half+1) before writing to avoid UB.
| #include "fft_frame.h" | ||
| #include "utils.h" | ||
|
|
There was a problem hiding this comment.
Make the header self-contained: include and .
Public API uses int16_t and size_t. Don’t rely on transitive includes.
Apply:
#include "fft_frame.h"
#include "utils.h"
+#include <cstddef>
+#include <cstdint>📝 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.
| #include "fft_frame.h" | |
| #include "utils.h" | |
| #include "fft_frame.h" | |
| #include "utils.h" | |
| #include <cstddef> | |
| #include <cstdint> |
🤖 Prompt for AI Agents
In src/fft_lib_avtx.h around lines 12 to 14, the header is not self-contained
because it uses int16_t and size_t from the public API but doesn't include
<cstdint> and <cstddef>; add #include <cstdint> and #include <cstddef> at the
top of this header (before or alongside the existing includes) so the types are
defined without relying on transitive includes.
| av_tx_fn m_tx_fn; | ||
| }; | ||
|
|
||
| }; // namespace chromaprint |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nit: drop the trailing semicolon after closing the namespace.
Not harmful, but inconsistent with existing headers.
-}; // namespace chromaprint
+} // namespace chromaprint📝 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.
| }; // namespace chromaprint | |
| } // namespace chromaprint |
🤖 Prompt for AI Agents
In src/fft_lib_avtx.h around line 36, the namespace closing line currently ends
with an unnecessary trailing semicolon ("}; // namespace chromaprint"); remove
the extra semicolon so the namespace is closed as "} // namespace chromaprint"
to match existing header style and keep consistency.
| #ifdef USE_AVTX | ||
| #include "fft_lib_avtx.h" | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include looks fine; add a global backend exclusivity check here.
To prevent multiple FFTLib class definitions, add a preprocessor guard (see suggestion in config.h.in comment).
Add near line 11 (after config.h):
#if (defined(USE_AVTX) + defined(USE_AVFFT) + defined(USE_FFTW3) + defined(USE_FFTW3F) + defined(USE_KISSFFT) + defined(USE_VDSP)) != 1
#error "Exactly one FFT backend (AVTX, AVFFT, FFTW3, FFTW3F, KISSFFT, vDSP) must be enabled."
#endif🤖 Prompt for AI Agents
In src/fft_lib.h around lines 11 to 13, add a preprocessor exclusivity check
immediately after the existing config.h include to ensure exactly one FFT
backend macro is defined; implement the check that sums the defined backend
macros (USE_AVTX, USE_AVFFT, USE_FFTW3, USE_FFTW3F, USE_KISSFFT, USE_VDSP) and
triggers a compile-time #error when the sum is not equal to 1 so multiple or
zero backend definitions are caught early.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (ret < 0) { | ||
| // Handle initialization error - this should not happen with valid parameters | ||
| m_tx_ctx = NULL; | ||
| m_tx_fn = NULL; | ||
| } |
There was a problem hiding this comment.
The initialization error handling in FFTLib::FFTLib() sets m_tx_ctx and m_tx_fn to NULL but doesn't release the memory allocated for m_window, m_input, and m_output. This creates a potential memory leak if initialization fails.
Consider either:
- Freeing these resources when initialization fails
- Throwing an exception to prevent object creation with invalid state
- Adding error checking in
Compute()to ensure allocated resources are properly used
This would ensure consistent resource management throughout the object's lifecycle.
| if (ret < 0) { | |
| // Handle initialization error - this should not happen with valid parameters | |
| m_tx_ctx = NULL; | |
| m_tx_fn = NULL; | |
| } | |
| if (ret < 0) { | |
| // Handle initialization error - this should not happen with valid parameters | |
| m_tx_ctx = NULL; | |
| m_tx_fn = NULL; | |
| // Free allocated resources to prevent memory leaks | |
| if (m_window) free(m_window); | |
| if (m_input) free(m_input); | |
| if (m_output) free(m_output); | |
| m_window = NULL; | |
| m_input = NULL; | |
| m_output = NULL; | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| } | ||
| } | ||
|
|
||
| }; // namespace chromaprint No newline at end of file |
There was a problem hiding this comment.
The file is missing a newline at the end, which can trigger warnings in some compilers and version control systems. Adding a newline character at the end of the file would follow standard coding practices and prevent potential issues during compilation or when viewing diffs.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
1 participant
No description provided.