Skip to content

fix(record): fix compilation error in GIF palette generation#859

Open
add-uos wants to merge 1 commit into
linuxdeepin:release/eaglefrom
add-uos:release/eagle
Open

fix(record): fix compilation error in GIF palette generation#859
add-uos wants to merge 1 commit into
linuxdeepin:release/eaglefrom
add-uos:release/eagle

Conversation

@add-uos

@add-uos add-uos commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Replaces QThreadPool::start() with QtConcurrent::run() to properly execute lambda functions in background threads for palette generation. Adds explicit capture of 'this' and 'cachePalette' variables for thread-safe callback execution via queued connection. Resolves compilation issue in the newly added GIF recording method with palette support.

log: fix compilation error in GIF palette generation
bug: https://pms.uniontech.com/bug-view-362851.html

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@add-uos add-uos force-pushed the release/eagle branch 2 times, most recently from c6fac65 to 7c87590 Compare June 3, 2026 03:10
Replaces QThreadPool::start() with QtConcurrent::run() to properly execute
lambda functions in background threads for palette generation. Adds explicit
capture of 'this' and 'cachePalette' variables for thread-safe callback
execution via queued connection. Resolves compilation issue in the newly
added GIF recording method with palette support.

log: fix compilation error in GIF palette generation
bug: https://pms.uniontech.com/bug-view-362851.html
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX,你的智能编程助手。我已经仔细审查了你提供的Git Diff。

这次代码变更的主要目的是优化录屏转码(MP4转GIF)的流程,引入了FFmpeg的两步转码法(先生成调色板,再合成GIF)以提升GIF画质,并将耗时的调色板生成过程放到了 QtConcurrent::run 中异步执行,同时修复了之前使用 QString::replace 替换后缀名可能导致目录名包含"mp4"时被误伤的Bug。

整体改进思路非常好,但在语法逻辑、代码质量、性能和安全性方面,还有一些需要改进和注意的地方。以下是详细的审查意见:

1. 语法与逻辑问题

  • QPointer 跨线程使用的逻辑隐患
    onStartTranscode 中,你使用了 QPointer<RecordProcess> guard(this) 并在 lambda 中捕获。当 QtConcurrent::run 的 lambda 执行完毕后,你通过 QMetaObject::invokeMethod 将回调投递到主线程。
    问题:如果在异步任务执行期间,RecordProcess 对象被销毁,guard 会自动置为 nullptr。此时 if (guard) 判断为 false,onTranscodePaletteFinished 不会被调用。这会导致后续的正式转码流程永远无法触发,且用户得不到任何反馈(没有错误提示,程序卡死在转码中)
    建议:即使对象被销毁,也应该有失败回调或状态清理机制。或者确保在 RecordProcess 析构时,等待异步任务取消或完成(例如使用 QFutureQFutureWatcher)。

  • QProcess 在无事件循环的线程中启动
    你在 QtConcurrent::run 的线程中创建了 paletteProcess,并调用了 start()waitForFinished()
    问题:虽然 waitForFinished 可以在没有事件循环的线程中工作,但 QProcess 的某些错误处理和状态通知依赖于事件循环。如果 ffmpeg 进程启动失败,或者需要读取大量的标准错误输出,没有事件循环可能会导致死锁或信号丢失。
    建议:更好的做法是在主线程创建 QProcess,通过信号槽触发,或者使用 QFutureWatcher 配合信号槽来管理异步任务,而不是生硬地在 QtConcurrent::run 中使用 QProcess::waitForFinished

  • transcodeProcess 内存泄漏风险
    onTranscodePaletteFinished 中,transcodeProcess = new QProcess(this);。你在 finishederrorOccurred 的 lambda 中都调用了 transcodeProcess->deleteLater()
    问题:如果 ffmpeg 根本无法启动(例如未安装),errorOccurred 会触发,随后 deleteLater。但是,如果 start() 调用本身就因为缺少可执行文件而立即失败,某些情况下可能不会触发 errorOccurred,或者如果在 start 之后、事件循环处理之前发生异常,可能会有泄漏风险。
    建议:在 start() 之后增加判断:

    if (!transcodeProcess->startDetached()) { // 或者判断 error() 状态
        qWarning() << "Failed to start ffmpeg process";
        transcodeProcess->deleteLater();
        return;
    }

2. 代码质量

  • 文件路径拼接不规范
    cachePalette = captureTempDir + QDir::separator() + "deepin_screen_recorder_palette.png"; 以及 gifPath = fileInfo.absolutePath() + QDir::separator() + fileInfo.completeBaseName() + ".gif";
    问题:手动使用 +QDir::separator() 拼接路径容易出错(比如 captureTempDir 末尾是否已经带了分隔符)。
    建议:使用 QDir::filePath()QFileInfo 来拼接路径:

    QString cachePalette = QDir(captureTempDir).filePath("deepin_screen_recorder_palette.png");
    QString gifPath = QDir(fileInfo.absolutePath()).filePath(fileInfo.completeBaseName() + ".gif");
  • Lambda 表达式捕获方式
    connect(transcodeProcess, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished), [this, transcodeProcess](...)
    建议:虽然 transcodeProcessthis 的子对象,生命周期可控,但在 lambda 中同时捕获 thistranscodeProcess,且在内部调用 deleteLater,容易让人产生生命周期疑问。建议在注释中明确说明 transcodeProcess 的所有权归 this,且通过 deleteLater 安全释放。

  • 硬编码的 FFmpeg 参数
    "select=not(mod(n\\,30)),palettegen=stats_mode=diff""fps=12,paletteuse=dither=none:diff_mode=rectangle"
    建议:这些参数中的帧率采样(30帧取1帧)和输出帧率(12fps)最好提取为常量或配置项,方便后续针对不同录制时长或性能要求的设备进行调整。

3. 代码性能

  • 超时时间设置
    paletteProcess.waitForFinished(2 * 60 * 1000) 设置了2分钟的超时。
    问题:如果录制了一个非常长的视频,生成调色板可能需要超过2分钟,这会导致调色板生成被强制中断,退化为低画质GIF。
    建议:考虑根据视频时长动态计算超时时间,或者适当延长超时时间,并在超时时给出明确日志提示当前是因为超时退化。

  • 两步转码的 I/O 开销
    生成调色板需要完整读取一次视频文件,合成GIF需要再读取一次视频文件和调色板。
    建议:这是 FFmpeg 两步转码的固有开销,目前无法避免。但需确保 captureTempDircaptureSavePath 在同一磁盘分区,以利用操作系统的文件系统缓存(Page Cache)加速第二次读取。

4. 代码安全

  • 命令注入风险
    paletteProcess.start("ffmpeg", {"-i", captureSavePath, ...})transcodeProcess->start("ffmpeg", arg)
    问题:如果 captureSavePathsavePath 来源于用户输入,且包含了特殊字符(如空格、引号等),虽然 QProcess::start 的参数列表模式比 system() 安全得多,但路径如果包含空格,在某些极端情况下或被错误处理时仍可能有隐患。
    建议:确保传入的文件路径经过校验。目前的写法已经是 Qt 推荐的安全启动进程的方式(避免了 shell 注入),但需注意不要将 argQStringList::join(" ") 拼接后传给 start(string) 接口,坚持使用当前的 start(program, args) 重载版本。

  • 临时文件安全
    生成的调色板文件 deepin_screen_recorder_palette.png 保存在临时目录。
    问题:如果该目录是共享的 /tmp,可能存在符号链接攻击或信息泄露风险。
    建议:确保 saveTempDir 是应用专属的安全临时目录(如 QStandardPaths::writableLocation(QStandardPaths::CacheLocation)),并在转码完成后(无论成功失败)及时清理该调色板临时文件。

改进后的参考代码片段

针对 onStartTranscodeonTranscodePaletteFinished 的核心逻辑,我提供如下优化建议:

void RecordProcess::onStartTranscode()
{
    qInfo() << __LINE__ << __func__ << "正在转码视频(mp4 to gif)...";
    QString captureTempDir = saveTempDir;
    QString captureSavePath = savePath;
    
    // 建议使用 QFutureWatcher 来管理异步任务,而不是裸用 QtConcurrent::run + QPointer
    // 这里为了最小化改动,仍沿用你的结构,但修复了路径拼接和日志问题
    QtConcurrent::run([captureTempDir, captureSavePath, this]() {
        QString cachePalette = QDir(captureTempDir).filePath("deepin_screen_recorder_palette.png");
        QProcess paletteProcess;
        
        // 注意:如果路径含空格,QProcess的stringlist模式能安全处理
        paletteProcess.start("ffmpeg", {"-i", captureSavePath, "-vf", "select=not(mod(n\\,30)),palettegen=stats_mode=diff", cachePalette, "-y"});

        // 建议根据视频大小给更合理的超时时间
        if (!paletteProcess.waitForFinished(5 * 60 * 1000)) { 
            qWarning() << "Convert palette failed or timeout!" << paletteProcess.errorString()
                       << "\nStderr:" << paletteProcess.readAllStandardError();
            cachePalette.clear(); // 退化标志
        }

        // 回到主线程执行
        QMetaObject::invokeMethod(this, [this, cachePalette]() {
            // 检查对象是否正在关闭,如果正在关闭则不再继续
            if (this->isClosingOrDeleted()) { // 假设有类似的状态标志
                qWarning() << "RecordProcess is closing, abort transcode.";
                return;
            }
            this->onTranscodePaletteFinished(cachePalette);
        }, Qt::QueuedConnection);
    });
}

void RecordProcess::onTranscodePaletteFinished(const QString &palettePng)
{
    QProcess *transcodeProcess = new QProcess(this);
    
    // 统一在 lambda 中处理清理逻辑
    auto cleanupProcess = [transcodeProcess]() {
        transcodeProcess->deleteLater();
    };

    connect(transcodeProcess, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
    [this, cleanupProcess](int exitCode, QProcess::ExitStatus exitStatus) {
        qDebug() << "exitCode: " << exitCode << "  exitStatus: " << exitStatus;
        cleanupProcess();
        if (exitStatus == QProcess::ExitStatus::NormalExit) {
            onTranscodeFinish();
        } else {
            qWarning() << "transcodeProcess is CrashExit!";
            // 建议增加错误状态通知,避免UI一直显示转码中
            emit transcodeFailed(); 
        }
    });

    connect(transcodeProcess, &QProcess::errorOccurred, [cleanupProcess](QProcess::ProcessError processError) {
        qWarning() << "transcodeProcess error:" << processError;
        cleanupProcess();
        emit transcodeFailed();
    });

    QStringList arg;
    arg << "-i" << savePath;

    if (!palettePng.isEmpty()) {
        arg << "-i" << palettePng;
        arg << "-filter_complex";
        arg << "fps=12,paletteuse=dither=none:diff_mode=rectangle";
    } else {
        arg << "-r" << "12";
    }

    // 使用规范的路径拼接
    QFileInfo fileInfo(savePath);
    QString gifPath = QDir(fileInfo.absolutePath()).filePath(fileInfo.completeBaseName() + ".gif");
    arg << gifPath;

    transcodeProcess->start("ffmpeg", arg);
    
    // 增加启动失败判断
    if (!transcodeProcess->waitForStarted(3000)) {
        qWarning() << "Failed to start ffmpeg:" << transcodeProcess->errorString();
        cleanupProcess();
        emit transcodeFailed();
        return;
    }

#if defined(__aarch64__)
    if (Utils::isWaylandMode) {
        qDebug() << "watting transcode gif end!";
        transcodeProcess->waitForFinished(-1);
    }
#endif
}

希望这些审查意见对你有所帮助!如果有任何疑问,欢迎随时提问。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants