Support FFmpeg 9 and deterministic provider selection - #201
Conversation
📝 WalkthroughWalkthroughAdded FFmpeg v9 support to the media module. The change bundles FFmpeg v9 public headers, detects and orders multiple FFmpeg installations, dynamically loads required APIs, and adds H.264 video encoding, re-encoding, image conversion, and audio muxing. CMake now includes the v9 sources and optional media tests. Tests cover version detection, path ordering, custom paths, incomplete libraries, and stable candidate ordering. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/media/internal/ffmpeg/v9/ffmpeglibhandler.cpp`:
- Around line 114-132: Update FFmpegLibHandler::loadLib to call unload before
returning false when any tryLoadPath fails, ensuring all partially opened
libraries and member handles are cleared before the next candidate is attempted.
Also update loadApi so failure during RESOLVE_FROM clears every previously
resolved API pointer before returning false, preventing stale symbols from
surviving a partial load.
- Around line 40-52: Remove the RTLD_DEFAULT fallback from
FFmpegLibHandler::getSymbol so failed muse::getLibFunc lookups return nullptr
and loadApi can fail for missing handle-local symbols. Apply the same change to
all existing FFmpeg handlers containing this dlsym(RTLD_DEFAULT, ...) fallback,
preserving resolution exclusively through the loaded library handle.
In `@framework/media/internal/ffmpeg/v9/ffmpeglibhandler.h`:
- Line 110: Move the private access specifier onto its own line above the
getSymbol declaration, matching the surrounding class style and sibling v4-v8
handler declarations.
In `@framework/media/internal/ffmpeg/v9/videoencoder.cpp`:
- Line 342: Update encodeImage to check the boolean result of convertImage_sws
before encoding; when conversion fails, stop processing and return failure
instead of using m_ffmpeg->ppicture. Preserve the existing successful conversion
and encoding path.
- Line 90: Initialize the AVPacket pointer member pkt in the FFmpeg class
definition with a null default initializer, matching the initialization pattern
of the other members and preventing reads before assignment from using an
indeterminate pointer.
- Around line 774-777: Correct the spacing in the `srcstride` assignment within
the video encoder so `srcstride[2] = 0;` matches the formatting of the preceding
`srcstride` assignments.
- Around line 215-221: Update the allocation flow around
av_image_get_buffer_size and m_ffmpeg->picture_buf: validate that size is
non-negative before allocating, and handle allocation failures using the
exception behavior of new rather than checking for a null result. Preserve the
existing frame cleanup and false return for both buffer-size and allocation
failures.
- Around line 593-599: Update the VideoEncoder lifecycle around addAudio and
close to enforce or preserve the required finishEncode → addAudio → close
sequence. Ensure addAudio cannot silently fail after close: either reject that
state with an explicit documented error or retain the video path and muxer state
until addAudio completes, while keeping m_outputPath handling consistent.
- Around line 498-499: Validate the results of av_frame_alloc and
av_packet_alloc in the encoder initialization flow before they are used by
avcodec_receive_frame or av_read_frame, and handle allocation failure through
the existing cleanup/error path. Apply the same checks to the corresponding
allocations in addAudio, ensuring neither decodedFrame nor packet can reach
FFmpeg APIs when null.
- Around line 133-236: Ensure every early return in VideoEncoder::open releases
all allocated and opened FFmpeg resources, including formatCtx, codecCtx,
ppicture, picture_buf, and pb, even when opened remains false. Reuse or add a
single cleanup helper/DEFER guard consistent with encodeVideo and addAudio, and
make close use the same cleanup path so retries cannot overwrite leaked state.
- Around line 401-441: Update encodeVideo() to set inputFmtCtx->flags |=
AVFMT_FLAG_CUSTOM_IO after assigning inputFmtCtx->pb, retain the original
avioBuf pointer, and free that buffer only after avio_context_free() in the
DEFER cleanup. Also free avioBuf when avio_alloc_context() fails before
returning, preserving the existing allocation-error handling.
In `@framework/media/internal/ffmpeg/v9/videoencoder.h`:
- Around line 53-56: Replace the owning raw pointer m_ffmpeg in the video
encoder class with std::unique_ptr<FFmpeg>, following the ownership pattern used
by the v4–v8 encoders. Preserve the out-of-line destructor declaration and
update construction, access, and cleanup in the implementation to use unique_ptr
semantics, ensuring copy operations remain unavailable.
In `@framework/media/internal/ffmpegutils.cpp`:
- Around line 73-74: Update the path normalization flow around
defaultSearchPaths to preserve its configured order, since
VideoEncoderResolver::startWatchingFfmpegsDirs() relies on that order. Remove
the std::sort step and deduplicate using normalizedSearchPaths before or without
sorting, ensuring duplicate entries are removed while the configured FFmpeg
directory remains last.
In `@framework/media/internal/videoencoderresolver.cpp`:
- Around line 71-87: Update loadFFmpeg and the successful encoder-loading flow
around makeEncoder so the discovered FFmpeg directory is stored in a
runtime-only field rather than configuration()->setFFmpegLibsDir. Preserve the
user-configured FFmpeg directory across automatic fallback discovery, and update
the persistent configuration only through the user-setting change path.
In `@framework/media/thirdparty/ffmpeg/v9/libavutil/avconfig.h`:
- Line 22: Update the hardcoded AV_HAVE_BIGENDIAN definition in avconfig.h and
matching v4–v8 copies to add a compile-time guard that detects big-endian
targets and fails the build explicitly, while preserving the current
little-endian configuration.
In `@framework/media/thirdparty/ffmpeg/v9/libavutil/common.h`:
- Around line 163-169: Add a clang-tidy exclusion for
framework/media/thirdparty/ covering FFmpeg v4 through v9, disabling checks and
preventing header analysis while preserving all vendored files byte-identical.
In framework/media/thirdparty/ffmpeg/v9/libavutil/common.h lines 163-169,
libavformat/avformat.h lines 312-329, libavformat/avio.h lines 415-424,
libavutil/macros.h line 52, and libswscale/swscale.h line 439, leave the
upstream code unchanged; each is covered by the shared exclusion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9e080c6-11ea-43f7-ba29-91a18632cee5
📒 Files selected for processing (53)
framework/media/CMakeLists.txtframework/media/internal/ffmpeg/v9/ffmpeglibhandler.cppframework/media/internal/ffmpeg/v9/ffmpeglibhandler.hframework/media/internal/ffmpeg/v9/videoencoder.cppframework/media/internal/ffmpeg/v9/videoencoder.hframework/media/internal/ffmpegutils.cppframework/media/internal/ffmpegutils.hframework/media/internal/videoencoderresolver.cppframework/media/tests/CMakeLists.txtframework/media/tests/ffmpegutils_tests.cppframework/media/thirdparty/ffmpeg/download_ffmpeg_headers.shframework/media/thirdparty/ffmpeg/v9/libavcodec/avcodec.hframework/media/thirdparty/ffmpeg/v9/libavcodec/codec.hframework/media/thirdparty/ffmpeg/v9/libavcodec/codec_desc.hframework/media/thirdparty/ffmpeg/v9/libavcodec/codec_id.hframework/media/thirdparty/ffmpeg/v9/libavcodec/codec_par.hframework/media/thirdparty/ffmpeg/v9/libavcodec/defs.hframework/media/thirdparty/ffmpeg/v9/libavcodec/packet.hframework/media/thirdparty/ffmpeg/v9/libavcodec/version.hframework/media/thirdparty/ffmpeg/v9/libavcodec/version_major.hframework/media/thirdparty/ffmpeg/v9/libavformat/avformat.hframework/media/thirdparty/ffmpeg/v9/libavformat/avio.hframework/media/thirdparty/ffmpeg/v9/libavformat/version.hframework/media/thirdparty/ffmpeg/v9/libavformat/version_major.hframework/media/thirdparty/ffmpeg/v9/libavutil/attributes.hframework/media/thirdparty/ffmpeg/v9/libavutil/avconfig.hframework/media/thirdparty/ffmpeg/v9/libavutil/avstring.hframework/media/thirdparty/ffmpeg/v9/libavutil/avutil.hframework/media/thirdparty/ffmpeg/v9/libavutil/buffer.hframework/media/thirdparty/ffmpeg/v9/libavutil/channel_layout.hframework/media/thirdparty/ffmpeg/v9/libavutil/common.hframework/media/thirdparty/ffmpeg/v9/libavutil/dict.hframework/media/thirdparty/ffmpeg/v9/libavutil/error.hframework/media/thirdparty/ffmpeg/v9/libavutil/frame.hframework/media/thirdparty/ffmpeg/v9/libavutil/hwcontext.hframework/media/thirdparty/ffmpeg/v9/libavutil/imgutils.hframework/media/thirdparty/ffmpeg/v9/libavutil/intfloat.hframework/media/thirdparty/ffmpeg/v9/libavutil/log.hframework/media/thirdparty/ffmpeg/v9/libavutil/macros.hframework/media/thirdparty/ffmpeg/v9/libavutil/mathematics.hframework/media/thirdparty/ffmpeg/v9/libavutil/mem.hframework/media/thirdparty/ffmpeg/v9/libavutil/opt.hframework/media/thirdparty/ffmpeg/v9/libavutil/pixdesc.hframework/media/thirdparty/ffmpeg/v9/libavutil/pixfmt.hframework/media/thirdparty/ffmpeg/v9/libavutil/rational.hframework/media/thirdparty/ffmpeg/v9/libavutil/samplefmt.hframework/media/thirdparty/ffmpeg/v9/libavutil/version.hframework/media/thirdparty/ffmpeg/v9/libswresample/swresample.hframework/media/thirdparty/ffmpeg/v9/libswresample/version.hframework/media/thirdparty/ffmpeg/v9/libswresample/version_major.hframework/media/thirdparty/ffmpeg/v9/libswscale/swscale.hframework/media/thirdparty/ffmpeg/v9/libswscale/version.hframework/media/thirdparty/ffmpeg/v9/libswscale/version_major.h
| uint8_t* const dst[], const int dstStride[]) = nullptr; | ||
| void (*sws_freeContext)(SwsContext* swsContext) = nullptr; | ||
|
|
||
| private: void* getSymbol(void* lib, const char* name) const; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Put the private: label on its own line.
The declaration of getSymbol shares the line with the access specifier. This differs from the rest of the class and from the sibling v4-v8 handlers.
♻️ Proposed formatting fix
-private: void* getSymbol(void* lib, const char* name) const;
+private:
+ void* getSymbol(void* lib, const char* name) const;
bool tryLoadPath(void*& lib, const io::path_t& fullPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/internal/ffmpeg/v9/ffmpeglibhandler.h` at line 110, Move the
private access specifier onto its own line above the getSymbol declaration,
matching the surrounding class style and sibling v4-v8 handler declarations.
| // Conversion | ||
| SwsContext* img_convert_ctx = nullptr; | ||
| // Packet | ||
| AVPacket* pkt; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Initialize pkt.
Every other member of FFmpeg has a default initializer. pkt does not. Any future read before the first assignment gives an indeterminate pointer.
🐛 Proposed fix
- AVPacket* pkt;
+ AVPacket* pkt = nullptr;📝 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.
| AVPacket* pkt; | |
| AVPacket* pkt = nullptr; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/internal/ffmpeg/v9/videoencoder.cpp` at line 90, Initialize
the AVPacket pointer member pkt in the FFmpeg class definition with a null
default initializer, matching the initialization pattern of the other members
and preventing reads before assignment from using an indeterminate pointer.
Source: Linters/SAST tools
| m_ffmpeg->videoStream = m_ffmpegHandler->avformat_new_stream(m_ffmpeg->formatCtx, nullptr); | ||
| if (!m_ffmpeg->videoStream) { | ||
| LOGE() << "failed allocate stream"; | ||
| return false; | ||
| } | ||
|
|
||
| m_ffmpeg->videoStream->time_base.den = options.fps; | ||
| m_ffmpeg->videoStream->time_base.num = 1; | ||
|
|
||
| // find the video encoder | ||
| m_ffmpeg->codec = m_ffmpegHandler->avcodec_find_encoder(AV_CODEC_ID_H264); | ||
| if (!m_ffmpeg->codec) { | ||
| LOGE() << "not found codec"; | ||
| return false; | ||
| } | ||
| m_ffmpeg->codecCtx = m_ffmpegHandler->avcodec_alloc_context3(m_ffmpeg->codec); | ||
| if (m_ffmpeg->codecCtx == nullptr) { | ||
| LOGE() << "failed to allocate AV context"; | ||
| return false; | ||
| } | ||
|
|
||
| m_ffmpeg->videoStream->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; | ||
| m_ffmpeg->videoStream->codecpar->codec_id = AV_CODEC_ID_H264; | ||
| m_ffmpeg->videoStream->codecpar->width = options.width; | ||
| m_ffmpeg->videoStream->codecpar->height = options.height; | ||
| m_ffmpeg->videoStream->codecpar->format = AV_PIX_FMT_YUV420P; | ||
| m_ffmpeg->videoStream->codecpar->bit_rate = options.bitrate; | ||
|
|
||
| if (m_ffmpegHandler->avcodec_parameters_to_context(m_ffmpeg->codecCtx, m_ffmpeg->videoStream->codecpar) < 0) { | ||
| m_ffmpegHandler->avcodec_free_context(&m_ffmpeg->codecCtx); | ||
| LOGE() << "failed to set codec parameters from stream"; | ||
| return false; | ||
| } | ||
|
|
||
| m_ffmpeg->codecCtx->profile = AV_PROFILE_H264_MAIN; | ||
| m_ffmpeg->codecCtx->time_base.den = options.fps; | ||
| m_ffmpeg->codecCtx->time_base.num = 1; | ||
| m_ffmpeg->codecCtx->gop_size = options.gop; | ||
| m_ffmpeg->codecCtx->thread_count = 10; | ||
| m_ffmpeg->codecCtx->max_b_frames = 0; | ||
| m_ffmpegHandler->av_opt_set_int(m_ffmpeg->codecCtx, "b_strategy", 1, 0); | ||
|
|
||
| m_ffmpeg->codecCtx->me_cmp = 1; | ||
| m_ffmpeg->codecCtx->me_range = 16; | ||
| m_ffmpegHandler->av_opt_set_int(m_ffmpeg->codecCtx, "hex", 1, 0); | ||
|
|
||
| m_ffmpeg->codecCtx->qmin = 10; | ||
| m_ffmpeg->codecCtx->qmax = 51; | ||
| m_ffmpegHandler->av_opt_set_int(m_ffmpeg->codecCtx, "sc_threshold", 40, 0); | ||
| m_ffmpeg->codecCtx->flags |= AV_CODEC_FLAG_LOOP_FILTER; | ||
| m_ffmpeg->codecCtx->me_subpel_quality = 5; | ||
| m_ffmpeg->codecCtx->i_quant_factor = 0.71f; | ||
| m_ffmpeg->codecCtx->qcompress = 0.6f; | ||
| m_ffmpeg->codecCtx->max_qdiff = 4; | ||
|
|
||
| if (m_ffmpeg->formatCtx->oformat->flags & AVFMT_GLOBALHEADER) { | ||
| m_ffmpeg->codecCtx->flags |= AV_CODEC_FLAG_GLOBAL_HEADER; | ||
| } | ||
|
|
||
| if (m_ffmpegHandler->avcodec_open2(m_ffmpeg->codecCtx, m_ffmpeg->codec, nullptr) < 0) { | ||
| LOGE() << "failed open codec"; | ||
| return false; | ||
| } | ||
|
|
||
| // Copy codec params to stream AFTER open - codec adds extradata (SPS/PPS) during open | ||
| if (m_ffmpegHandler->avcodec_parameters_from_context(m_ffmpeg->videoStream->codecpar, m_ffmpeg->codecCtx) < 0) { | ||
| m_ffmpegHandler->avcodec_free_context(&m_ffmpeg->codecCtx); | ||
| LOGE() << "failed to copy codec parameters to stream"; | ||
| return false; | ||
| } | ||
|
|
||
| // Allocate the YUV frame | ||
| m_ffmpeg->ppicture = m_ffmpegHandler->av_frame_alloc(); | ||
| if (!m_ffmpeg->ppicture) { | ||
| LOGE() << "failed allocate frame"; | ||
| return false; | ||
| } | ||
|
|
||
| m_ffmpeg->ppicture->width = options.width; | ||
| m_ffmpeg->ppicture->height = options.height; | ||
| m_ffmpeg->ppicture->format = AV_PIX_FMT_YUV420P; | ||
|
|
||
| int size = m_ffmpegHandler->av_image_get_buffer_size(AV_PIX_FMT_YUV420P, m_ffmpeg->width, m_ffmpeg->height, 1); | ||
| m_ffmpeg->picture_buf = new uint8_t[size]; | ||
| if (!m_ffmpeg->picture_buf) { | ||
| LOGE() << "failed allocate frame buf"; | ||
| m_ffmpegHandler->av_frame_free(&m_ffmpeg->ppicture); | ||
| return false; | ||
| } | ||
|
|
||
| // Setup the planes | ||
| m_ffmpegHandler->av_image_fill_arrays(m_ffmpeg->ppicture->data, m_ffmpeg->ppicture->linesize, m_ffmpeg->picture_buf, | ||
| AV_PIX_FMT_YUV420P, m_ffmpeg->width, m_ffmpeg->height, 1); | ||
|
|
||
| if (m_ffmpegHandler->avio_open(&m_ffmpeg->formatCtx->pb, fileName.c_str(), AVIO_FLAG_WRITE) < 0) { | ||
| LOGE() << "failed open file: " << fileName; | ||
| return false; | ||
| } | ||
|
|
||
| int ret = m_ffmpegHandler->avformat_write_header(m_ffmpeg->formatCtx, NULL); | ||
| if (ret < 0) { | ||
| LOGE() << "failed write AV header"; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A failed open leaks the FFmpeg objects, and close cannot recover them.
open returns early at lines 136, 146, 151, 164, 194, 201, 208, 220, 229, and 235. It sets m_ffmpeg->opened = true only at line 240. close returns immediately at lines 297-299 when opened is false. So after any failed open, the allocated formatCtx, codecCtx, ppicture, picture_buf, and the opened pb are never released. A caller that retries open also overwrites the previous pointers, which multiplies the leak.
Release everything on each failure path. A single cleanup helper, or a DEFER guard that runs unless a success flag is set, keeps this manageable. encodeVideo and addAudio already use DEFER in this file.
🔒 Proposed structure
bool VideoEncoder::open(const muse::io::path_t& fileName, const Options& options)
{
+ bool success = false;
+ DEFER {
+ if (!success) {
+ m_ffmpeg->opened = true; // allow close() to release partial state
+ close();
+ }
+ };Adjust the guard to match the cleanup helper you prefer. Check the v4 to v8 encoders, because they likely share this defect.
🧰 Tools
🪛 Clang (14.0.6)
[note] 134-134: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 135-135: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 144-144: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 145-145: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 149-149: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 150-150: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 161-161: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 163-163: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 188-188: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 192-192: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 193-193: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 198-198: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 200-200: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 206-206: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 207-207: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 217-217: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 218-218: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 227-227: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 228-228: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 233-233: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 234-234: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[warning] 184-184: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 185-185: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 215-215: variable 'size' is not initialized
(cppcoreguidelines-init-variables)
[warning] 217-217: implicit conversion 'uint8_t *' (aka 'unsigned char *') -> bool
(readability-implicit-bool-conversion)
[warning] 232-232: variable 'ret' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/internal/ffmpeg/v9/videoencoder.cpp` around lines 133 - 236,
Ensure every early return in VideoEncoder::open releases all allocated and
opened FFmpeg resources, including formatCtx, codecCtx, ppicture, picture_buf,
and pb, even when opened remains false. Reuse or add a single cleanup
helper/DEFER guard consistent with encodeVideo and addAudio, and make close use
the same cleanup path so retries cannot overwrite leaked state.
| std::shared_ptr<FFmpegLibHandler> m_ffmpegHandler = nullptr; | ||
| FFmpeg* m_ffmpeg = nullptr; | ||
| muse::io::path_t m_outputPath; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Own FFmpeg with std::unique_ptr to satisfy the rule of three.
m_ffmpeg is a raw owning pointer that the destructor deletes. The class declares no copy or move operations, so a copy produces a double free. std::unique_ptr<FFmpeg> removes the risk and also deletes the copy operations. The destructor stays declared here and defined in the .cpp, where FFmpeg is complete.
♻️ Proposed refactor
- std::shared_ptr<FFmpegLibHandler> m_ffmpegHandler = nullptr;
- FFmpeg* m_ffmpeg = nullptr;
+ std::shared_ptr<FFmpegLibHandler> m_ffmpegHandler;
+ std::unique_ptr<FFmpeg> m_ffmpeg;
muse::io::path_t m_outputPath;Check the v4 to v8 encoders first, and keep the pattern consistent across versions.
📝 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.
| std::shared_ptr<FFmpegLibHandler> m_ffmpegHandler = nullptr; | |
| FFmpeg* m_ffmpeg = nullptr; | |
| muse::io::path_t m_outputPath; | |
| }; | |
| std::shared_ptr<FFmpegLibHandler> m_ffmpegHandler; | |
| std::unique_ptr<FFmpeg> m_ffmpeg; | |
| muse::io::path_t m_outputPath; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/internal/ffmpeg/v9/videoencoder.h` around lines 53 - 56,
Replace the owning raw pointer m_ffmpeg in the video encoder class with
std::unique_ptr<FFmpeg>, following the ownership pattern used by the v4–v8
encoders. Preserve the out-of-line destructor declaration and update
construction, access, and cleanup in the implementation to use unique_ptr
semantics, ensuring copy operations remain unavailable.
| std::sort(paths.begin(), paths.end()); | ||
| paths.erase(std::unique(paths.begin(), paths.end()), paths.end()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of defaultSearchPaths and inspect how the order is used.
rg -nP -C 10 '\bdefaultSearchPaths\s*\(' --type=cpp --type=cppm -g '!**/thirdparty/**' || \
rg -nP -C 10 '\bdefaultSearchPaths\s*\('Repository: musescore/muse_framework
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -u
echo "== find defaultSearchPaths definition/callers =="
rg -n -C 12 '\bdefaultSearchPaths\b' --glob '*.{cpp,cppm,c,cxx,h,hpp,hxx,cc,hpp}' . 2>/dev/null || true
echo
echo "== file outline =="
ast-grep outline framework/media/internal/ffmpegutils.cpp --view expanded 2>/dev/null || true
echo
echo "== relevant ffmpegutils.cpp lines =="
sed -n '1,140p' framework/media/internal/ffmpegutils.cppRepository: musescore/muse_framework
Length of output: 14037
🏁 Script executed:
#!/bin/bash
set -u
echo "== deterministic ordering probe for defaultSearchPaths platform sections =="
python3 - <<'PY'
sections = {
"macOS": [
("/Library/Application Support/FFmpeg/Providers/lib1", "/opt/homebrew/lib"),
("/Library/Application Support/FFmpeg/Providers/lib1", "/usr/local/lib"),
("/Library/Application Support/FFmpeg/Providers/lib1", "/usr/lib"),
],
"Linux_AMD64": ["/usr/lib/x86_64-linux-gnu", "/usr/lib64", "/usr/lib"],
"Linux_ARM64": ["/usr/lib64", "/usr/lib/arm64-linux-gnu", "/usr/lib"],
"Linux_AMD64_duplicate": ["/usr/lib/x86_64-linux-gnu", "/usr/lib/x86_64-linux-gnu", "/usr/lib"],
}
for name, paths in sections.items():
sorted_paths = sorted(paths)
unique_then_sorted_paths = sorted(set(paths))
print(f"--- {name} ---")
print("prefetched:", paths)
print("std::sort output:", sorted_paths)
print("sort-then-dedup output:", sorted_paths)
print("dedup-then-sort output:", unique_then_sorted_paths)
print("unique_output:", sorted(paths).__iter__().__next__().__class__)
PY
echo
echo "== exact caller implementation =="
sed -n '96,111p' framework/media/internal/videoencoderresolver.cpp
sed -n '234,260p' framework/media/internal/ffmpegutils.cppRepository: musescore/muse_framework
Length of output: 3944
Keep defaultSearchPaths unsorted or deduplicate before sorting.
defaultSearchPaths is public and still used directly by VideoEncoderResolver::startWatchingFfmpegsDirs(), which iterates paths in the returned order and adds the configured FFmpeg directory at the end. The current std::sort removes that preference order and duplicates any duplicate default entries because std::unique runs only after sorting. Remove the sort here and deduplicate via normalizedSearchPaths, or document/use a different contract if order is no longer intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/internal/ffmpegutils.cpp` around lines 73 - 74, Update the
path normalization flow around defaultSearchPaths to preserve its configured
order, since VideoEncoderResolver::startWatchingFfmpegsDirs() relies on that
order. Remove the std::sort step and deduplicate using normalizedSearchPaths
before or without sorting, ensuring duplicate entries are removed while the
configured FFmpeg directory remains last.
| #ifndef AVUTIL_AVCONFIG_H | ||
| #define AVUTIL_AVCONFIG_H | ||
|
|
||
| #define AV_HAVE_BIGENDIAN 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Guard the hardcoded little-endian assumption.
Upstream generates avconfig.h at configure time. This vendored copy fixes AV_HAVE_BIGENDIAN to 0 for every build. libavutil/macros.h uses this value to define AV_NE(be, le), so on a big-endian target the headers silently select little-endian constants. The supported platforms are little-endian today, so nothing breaks now, but the assumption is invisible to a future port. Add a compile-time check so a big-endian build fails loudly instead of miscompiling.
Apply the same change to the v4 through v8 copies of avconfig.h if they carry the same hardcoded value.
🛡️ Proposed guard
`#ifndef` AVUTIL_AVCONFIG_H
`#define` AVUTIL_AVCONFIG_H
+/* NOTE: upstream generates this file at configure time. This bundled copy
+ * assumes a little-endian target, which covers all currently supported
+ * platforms. Fail the build instead of miscompiling on big-endian targets. */
+#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && \
+ __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#error "Bundled FFmpeg avconfig.h assumes a little-endian target"
+#endif
+
`#define` AV_HAVE_BIGENDIAN 0
`#endif` /* AVUTIL_AVCONFIG_H */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/thirdparty/ffmpeg/v9/libavutil/avconfig.h` at line 22, Update
the hardcoded AV_HAVE_BIGENDIAN definition in avconfig.h and matching v4–v8
copies to add a compile-time guard that detects big-endian targets and fails the
build explicitly, while preserving the current little-endian configuration.
| #ifndef av_log2 | ||
| av_const int av_log2(unsigned v); | ||
| #endif | ||
|
|
||
| #ifndef av_log2_16bit | ||
| av_const int av_log2_16bit(unsigned v); | ||
| #endif |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exclude the vendored FFmpeg header tree from clang-tidy.
Clang-tidy reports over 200 style findings across these five files: readability-identifier-length, bugprone-macro-parentheses, readability-braces-around-statements and bugprone-narrowing-conversions. All of them target upstream FFmpeg code. None are actionable, because the vendored headers must stay byte-identical to the upstream release for the ABI to match the runtime libraries. The shared root cause is that framework/media/thirdparty/ is inside the analysis scope. Add a path exclusion so real findings in framework/media/internal/ are not buried.
framework/media/thirdparty/ffmpeg/v9/libavutil/common.h#L163-L169: leave the upstream code unchanged; addframework/media/thirdparty/.clang-tidywith all checks disabled, or add a--exclude-header-filter/HeaderFilterRegexentry that dropsframework/media/thirdparty/.*.framework/media/thirdparty/ffmpeg/v9/libavformat/avformat.h#L312-L329: leave the upstream code unchanged; covered by the same exclusion.framework/media/thirdparty/ffmpeg/v9/libavformat/avio.h#L415-L424: leave the upstream code unchanged; covered by the same exclusion.framework/media/thirdparty/ffmpeg/v9/libavutil/macros.h#L52-L52: leave theFFSWAPdefinition unchanged; covered by the same exclusion.framework/media/thirdparty/ffmpeg/v9/libswscale/swscale.h#L439-L439: leave the upstream code unchanged; covered by the same exclusion.
Extend the exclusion to the existing v4 through v8 directories at the same time.
♻️ Proposed exclusion file
Add framework/media/thirdparty/.clang-tidy:
# Vendored third-party sources. Keep byte-identical to upstream; do not lint.
Checks: '-*'
HeaderFilterRegex: '$^'🧰 Tools
🪛 Clang (14.0.6)
[warning] 164-164: parameter name 'v' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 168-168: parameter name 'v' is too short, expected at least 3 characters
(readability-identifier-length)
📍 Affects 5 files
framework/media/thirdparty/ffmpeg/v9/libavutil/common.h#L163-L169(this comment)framework/media/thirdparty/ffmpeg/v9/libavformat/avformat.h#L312-L329framework/media/thirdparty/ffmpeg/v9/libavformat/avio.h#L415-L424framework/media/thirdparty/ffmpeg/v9/libavutil/macros.h#L52-L52framework/media/thirdparty/ffmpeg/v9/libswscale/swscale.h#L439-L439
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/media/thirdparty/ffmpeg/v9/libavutil/common.h` around lines 163 -
169, Add a clang-tidy exclusion for framework/media/thirdparty/ covering FFmpeg
v4 through v9, disabling checks and preventing header analysis while preserving
all vendored files byte-identical. In
framework/media/thirdparty/ffmpeg/v9/libavutil/common.h lines 163-169,
libavformat/avformat.h lines 312-329, libavformat/avio.h lines 415-424,
libavutil/macros.h line 52, and libswscale/swscale.h line 439, leave the
upstream code unchanged; each is covered by the shared exclusion.
Source: Linters/SAST tools
1 participant
Resolves: musescore/MuseScore#34496
This adds an FFmpeg 9 adapter for video export and makes provider selection deterministic. A configured custom directory keeps priority and is not overwritten when automatic fallback is used. Automatically discovered providers are ordered by the newest supported FFmpeg ABI and then by normalized path. If a candidate is incomplete or cannot be loaded, resolution continues with the next compatible candidate.
The FFmpeg 4 through 8 adapters remain available. FFmpeg 9 follows the existing version-specific adapter pattern and uses the library majors shipped by FFmpeg 9.0.
Tests and validation
ea7bb84e18117e32eb1b19abb14504cacae2f96apassed the full unit-test suite before the review follow-up. The current head passes the repository codestyle check andgit diff --check; compilation and application validation of the follow-up are left to maintainer CI.n9.0tag;avconfig.his generated using the existing repository pattern.a7860f77961bf60560ea8958daefee612229b1c3against framework commitea7bb84e18117e32eb1b19abb14504cacae2f96aon macOS universal, Linux x86_64, and Linux arm64. This did not include an end-to-end video-export test; application-level validation is left to the MuseScore maintainers.The implementation is ready for maintainer review.
[email protected].Build configuration
audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/main
musescore platforms: linux_x64 macos windows_x64