Repository navigation
feat: retain lossless failure keyframes（暂缓） - #1593
Draft
tianguzhang wants to merge 18 commits into
Draft
tianguzhang wants to merge 18 commits into
tianguzhang wants to merge 18 commits into
Conversation
Member
|
不太可能录视频，留关键帧还差不多 |
Member
|
大的 feat 可以先开个 issue 大家一起讨论下再做 |
Member
|
关于关键帧我有一点想法，目前还在开发工具中尝试，完善了之后能以少量图片和数据尽可能无损还原一段时间内的画面变化 |
Author
|
好 |
tianguzhang
changed the title
tianguzhang
changed the title
This was referenced Oct 30, 2026
Drain once before the terminal task notification and retain the 15-second bound. Remove unused counters and avoid building a set for action pins.
tianguzhang
marked this pull request as ready for review
November 6, 2026 13:03
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
你好——我发现了 11 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="214" />
<code_context>
+ temporary += ".partial";
+ try {
+ std::filesystem::create_directories(temporary / "frames");
+ json::array manifest;
+ cv::Mat previous;
+ for (size_t i = 0; i < job.frames.size(); ++i) {
</code_context>
<issue_to_address>
**Export follows planted symlinks**
When a local attacker can modify the configured `on_error` directory while a task failure export is being written, `create_directories` and the `ofstream` calls follow symlinks under the export path, so an attacker who can modify the shared log directory can race the writer with a symlink and make the application overwrite a file outside that directory.
Create the temporary export directory with restrictive permissions and write files using symlink-resistant, exclusive file creation, rejecting any pre-existing symlink or unexpected path component.
Also at `source/MaaFramework/Task/FailureFrames.cpp:224-226`, `source/MaaFramework/Task/FailureFrames.cpp:262-264`.
</issue_to_address>
### Comment 2
<location path="source/MaaFramework/Global/OptionMgr.cpp" line_range="124" />
<code_context>
+ return false;
+ }
+
+ save_on_error_frames_ = *reinterpret_cast<const bool*>(value);
+
+ LogInfo << "Set save on error frames" << VAR(save_on_error_frames_);
</code_context>
<issue_to_address>
**Changing the setting races tasks**
When a binding or native caller changes the option while a task is running, `set_save_on_error_frames` writes a plain `bool` while the task runner reads it in recognition and action paths, causing a C++ data race and undefined behavior; callers changing the setting during an active task can corrupt that task's execution.
Make the setting atomic or synchronize reads and writes.
Also at `source/MaaFramework/Global/OptionMgr.h:31`.
</issue_to_address>
### Comment 3
<location path="source/MaaFramework/Global/OptionMgr.h" line_range="55" />
<code_context>
bool save_draw_ = false;
bool debug_mode_ = false;
bool save_on_error_ = false;
+ bool save_on_error_frames_ = true;
int draw_quality_ = 85;
size_t reco_image_cache_limit_ = 4096;
</code_context>
<issue_to_address>
**Sensitive screens are retained by default**
When an application processes a screen containing credentials or personal data, its log directory is nonempty, and a Pipeline error occurs, `Recognizer::recognize` captures the unredacted recognition input, and `FailureFrames::write` encodes it as PNG in the configured log directory; credentials or personal data therefore remain in error artifacts accessible to anyone who can access or receives those logs.
Default failure-frame capture to off and require explicit opt-in, with clear notice that unredacted screen images will be written to the log directory.
Also at `source/MaaFramework/Task/FailureFrames.cpp:220-226`.
</issue_to_address>
### Comment 4
<location path="source/MaaFramework/Task/PipelineTask.cpp" line_range="498" />
<code_context>
+ if (option.save_on_error_frames() && tasker()) {
+ auto& frames = tasker()->failure_frames();
+ frames.observe(image, MaaInvalidId, node_name, true);
+ frames.save(directory / path(std::format("{}_{}", stamp, task_id_)), node_name);
}
-
</code_context>
<issue_to_address>
**Recovered tasks still leave failure exports**
When a node failure is handled by an error path that later recovers, or the task is stopped, `PipelineTask::save_on_error` queues the export before the final task outcome is known, and nothing cancels it after recovery or stop. The writer therefore publishes failure-frame files for tasks that succeed or stop.
Queue the export only after the task outcome is final, or cancel it when the task recovers or stops.
Also at `source/MaaFramework/Task/FailureFrames.cpp:163-164`.
</issue_to_address>
### Comment 5
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="163" />
<code_context>
+{
+ try {
+ std::scoped_lock lock(mutex_);
+ if (!active_ || pending_ || (frames_.empty() && !before_action_ && !after_action_)) {
+ LogWarn << "failure frame export unavailable or busy" << directory;
+ return;
</code_context>
<issue_to_address>
**One task produces duplicate exports**
When a later Pipeline error occurs after the first export has been accepted, `save()` checks `pending_`, but `work()` clears it when writing starts, so the later request is accepted instead of being dropped. Multiple exports for one task can create duplicate bundles or target the same timestamp-and-task-ID paths, causing a rename failure and a missing export.
Track that an export has already been accepted for the task and reject all subsequent save requests.
Also at `source/MaaFramework/Task/PipelineTask.cpp:488-498`, `source/MaaFramework/Task/FailureFrames.cpp:210-211`, `source/MaaFramework/Task/FailureFrames.cpp:266`.
</issue_to_address>
### Comment 6
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="103" />
<code_context>
+ return;
+ }
+ while (frames_.size() > 1 && (bytes_->load() + length > kMaxBytes || frames_.size() >= kMaxFrames)) {
+ frames_.pop_front();
+ }
+ // Pins must not freeze capture when the remaining pool cannot fit
</code_context>
<issue_to_address>
**Memory-drop count is inaccurate**
When memory pressure evicts retained frames or a pin is reset while its frame remains in history, eviction removes frames without incrementing `dropped_`, while resetting a pin increments it even when the frame remains retained. The exported `dropped_for_memory` therefore does not report the number of frames actually lost.
Update `dropped_` only when a frame is actually removed from history.
Also at `source/MaaFramework/Task/FailureFrames.cpp:112-113`, `source/MaaFramework/Task/FailureFrames.cpp:260`.
</issue_to_address>
### Comment 7
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="102-103" />
<code_context>
+ ++dropped_;
+ return;
+ }
+ while (frames_.size() > 1 && (bytes_->load() + length > kMaxBytes || frames_.size() >= kMaxFrames)) {
+ frames_.pop_front();
+ }
+ // Pins must not freeze capture when the remaining pool cannot fit
</code_context>
<issue_to_address>
**History collapses after 64 frames**
When the history reaches 64 distinct frames and another distinct image is observed, `observe` keeps evicting while the frame-count limit is reached, not just until one slot is free. The next distinct frame therefore leaves only two frames in the history, so the export loses most of the retained context.
Handle the frame-count limit separately from byte pressure and evict only the oldest frame needed to make room.
</issue_to_address>
### Comment 8
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="173" />
<code_context>
+ job.frames.push_back(**pinned);
+ }
+ }
+ std::ranges::sort(job.frames, {}, &Frame::first_ms);
+ pending_ = std::move(job);
+ if (!writer_.joinable()) {
</code_context>
<issue_to_address>
**Exported frames can be out of order**
When distinct frames are captured within the same millisecond, `save()` sorts only by millisecond-resolution `first_ms`, so frames with equal timestamps can be reordered. The exported sequence and `change_bbox` values then no longer reliably describe capture order.
Use a tie-breaker that preserves capture order when sorting frames with equal `first_ms`.
Also at `source/MaaFramework/Task/FailureFrames.cpp:92`.
</issue_to_address>
### Comment 9
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="95" />
<code_context>
+ const auto now = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
+ const bool equal = !frames_.empty() && image.size() == frames_.back().pixels->image.size()
+ && image.type() == frames_.back().pixels->image.type()
+ && cv::norm(image, frames_.back().pixels->image, cv::NORM_INF) == 0;
+ if (!equal) {
+ const size_t length = image.total() * image.elemSize();
</code_context>
<issue_to_address>
**Repeated full-frame comparisons slow recognition**
When a captured image is tested against multiple candidates before one matches, `Recognizer::recognize` observes the image for every candidate node, even though `PipelineTask::recognize_list` passes the same captured image through its candidate loop. With frame history enabled by default, each such observation runs a full-image `cv::norm` against the retained frame, so fallback-heavy pipelines repeatedly scan the same high-resolution image and take longer to recognize.
Deduplicate observations by captured-image identity or generation so each recognition input is compared and recorded at most once.
Also at `source/MaaFramework/Task/Component/Recognizer.cpp:119`.
</issue_to_address>
### Comment 10
<location path="include/MaaFramework/MaaDef.h" line_range="134" />
<code_context>
+ ///
+ /// value: bool, eg: true; val_size: sizeof(bool)
+ /// default value is true, independent of SaveOnError and DebugMode
+ MaaGlobalOption_SaveOnErrorFrames = 10,
};
</code_context>
<issue_to_address>
**C++ module omits new option**
`MaaFramework.cppm` does not export `MaaGlobalOption_SaveOnErrorFrames`, so C++20 module consumers cannot use the new option through the module interface.
Export the new option from `source/modules/MaaFramework.cppm`.
</issue_to_address>
### Comment 11
<location path="source/binding/Python/maa/tasker.py" line_range="736" />
<code_context>
)
)
+ @staticmethod
+ def set_save_on_error_frames(save_on_error_frames: bool) -> bool:
+ """设置是否保存任务失败关键帧，默认启用 / Set whether to save task failure keyframes, enabled by default
</code_context>
<issue_to_address>
**Python option lacks test coverage**
The new Python setter has no corresponding test in `test/python/`, so regressions in configuring this option are not caught by the required Python unit tests.
Add a Python unit test that calls `set_save_on_error_frames` and verifies the option-setting result.
</issue_to_address>Original comment in English
Hey - I've found 11 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="214" />
<code_context>
+ temporary += ".partial";
+ try {
+ std::filesystem::create_directories(temporary / "frames");
+ json::array manifest;
+ cv::Mat previous;
+ for (size_t i = 0; i < job.frames.size(); ++i) {
</code_context>
<issue_to_address>
**Export follows planted symlinks**
When a local attacker can modify the configured `on_error` directory while a task failure export is being written, `create_directories` and the `ofstream` calls follow symlinks under the export path, so an attacker who can modify the shared log directory can race the writer with a symlink and make the application overwrite a file outside that directory.
Create the temporary export directory with restrictive permissions and write files using symlink-resistant, exclusive file creation, rejecting any pre-existing symlink or unexpected path component.
Also at `source/MaaFramework/Task/FailureFrames.cpp:224-226`, `source/MaaFramework/Task/FailureFrames.cpp:262-264`.
</issue_to_address>
### Comment 2
<location path="source/MaaFramework/Global/OptionMgr.cpp" line_range="124" />
<code_context>
+ return false;
+ }
+
+ save_on_error_frames_ = *reinterpret_cast<const bool*>(value);
+
+ LogInfo << "Set save on error frames" << VAR(save_on_error_frames_);
</code_context>
<issue_to_address>
**Changing the setting races tasks**
When a binding or native caller changes the option while a task is running, `set_save_on_error_frames` writes a plain `bool` while the task runner reads it in recognition and action paths, causing a C++ data race and undefined behavior; callers changing the setting during an active task can corrupt that task's execution.
Make the setting atomic or synchronize reads and writes.
Also at `source/MaaFramework/Global/OptionMgr.h:31`.
</issue_to_address>
### Comment 3
<location path="source/MaaFramework/Global/OptionMgr.h" line_range="55" />
<code_context>
bool save_draw_ = false;
bool debug_mode_ = false;
bool save_on_error_ = false;
+ bool save_on_error_frames_ = true;
int draw_quality_ = 85;
size_t reco_image_cache_limit_ = 4096;
</code_context>
<issue_to_address>
**Sensitive screens are retained by default**
When an application processes a screen containing credentials or personal data, its log directory is nonempty, and a Pipeline error occurs, `Recognizer::recognize` captures the unredacted recognition input, and `FailureFrames::write` encodes it as PNG in the configured log directory; credentials or personal data therefore remain in error artifacts accessible to anyone who can access or receives those logs.
Default failure-frame capture to off and require explicit opt-in, with clear notice that unredacted screen images will be written to the log directory.
Also at `source/MaaFramework/Task/FailureFrames.cpp:220-226`.
</issue_to_address>
### Comment 4
<location path="source/MaaFramework/Task/PipelineTask.cpp" line_range="498" />
<code_context>
+ if (option.save_on_error_frames() && tasker()) {
+ auto& frames = tasker()->failure_frames();
+ frames.observe(image, MaaInvalidId, node_name, true);
+ frames.save(directory / path(std::format("{}_{}", stamp, task_id_)), node_name);
}
-
</code_context>
<issue_to_address>
**Recovered tasks still leave failure exports**
When a node failure is handled by an error path that later recovers, or the task is stopped, `PipelineTask::save_on_error` queues the export before the final task outcome is known, and nothing cancels it after recovery or stop. The writer therefore publishes failure-frame files for tasks that succeed or stop.
Queue the export only after the task outcome is final, or cancel it when the task recovers or stops.
Also at `source/MaaFramework/Task/FailureFrames.cpp:163-164`.
</issue_to_address>
### Comment 5
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="163" />
<code_context>
+{
+ try {
+ std::scoped_lock lock(mutex_);
+ if (!active_ || pending_ || (frames_.empty() && !before_action_ && !after_action_)) {
+ LogWarn << "failure frame export unavailable or busy" << directory;
+ return;
</code_context>
<issue_to_address>
**One task produces duplicate exports**
When a later Pipeline error occurs after the first export has been accepted, `save()` checks `pending_`, but `work()` clears it when writing starts, so the later request is accepted instead of being dropped. Multiple exports for one task can create duplicate bundles or target the same timestamp-and-task-ID paths, causing a rename failure and a missing export.
Track that an export has already been accepted for the task and reject all subsequent save requests.
Also at `source/MaaFramework/Task/PipelineTask.cpp:488-498`, `source/MaaFramework/Task/FailureFrames.cpp:210-211`, `source/MaaFramework/Task/FailureFrames.cpp:266`.
</issue_to_address>
### Comment 6
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="103" />
<code_context>
+ return;
+ }
+ while (frames_.size() > 1 && (bytes_->load() + length > kMaxBytes || frames_.size() >= kMaxFrames)) {
+ frames_.pop_front();
+ }
+ // Pins must not freeze capture when the remaining pool cannot fit
</code_context>
<issue_to_address>
**Memory-drop count is inaccurate**
When memory pressure evicts retained frames or a pin is reset while its frame remains in history, eviction removes frames without incrementing `dropped_`, while resetting a pin increments it even when the frame remains retained. The exported `dropped_for_memory` therefore does not report the number of frames actually lost.
Update `dropped_` only when a frame is actually removed from history.
Also at `source/MaaFramework/Task/FailureFrames.cpp:112-113`, `source/MaaFramework/Task/FailureFrames.cpp:260`.
</issue_to_address>
### Comment 7
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="102-103" />
<code_context>
+ ++dropped_;
+ return;
+ }
+ while (frames_.size() > 1 && (bytes_->load() + length > kMaxBytes || frames_.size() >= kMaxFrames)) {
+ frames_.pop_front();
+ }
+ // Pins must not freeze capture when the remaining pool cannot fit
</code_context>
<issue_to_address>
**History collapses after 64 frames**
When the history reaches 64 distinct frames and another distinct image is observed, `observe` keeps evicting while the frame-count limit is reached, not just until one slot is free. The next distinct frame therefore leaves only two frames in the history, so the export loses most of the retained context.
Handle the frame-count limit separately from byte pressure and evict only the oldest frame needed to make room.
</issue_to_address>
### Comment 8
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="173" />
<code_context>
+ job.frames.push_back(**pinned);
+ }
+ }
+ std::ranges::sort(job.frames, {}, &Frame::first_ms);
+ pending_ = std::move(job);
+ if (!writer_.joinable()) {
</code_context>
<issue_to_address>
**Exported frames can be out of order**
When distinct frames are captured within the same millisecond, `save()` sorts only by millisecond-resolution `first_ms`, so frames with equal timestamps can be reordered. The exported sequence and `change_bbox` values then no longer reliably describe capture order.
Use a tie-breaker that preserves capture order when sorting frames with equal `first_ms`.
Also at `source/MaaFramework/Task/FailureFrames.cpp:92`.
</issue_to_address>
### Comment 9
<location path="source/MaaFramework/Task/FailureFrames.cpp" line_range="95" />
<code_context>
+ const auto now = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
+ const bool equal = !frames_.empty() && image.size() == frames_.back().pixels->image.size()
+ && image.type() == frames_.back().pixels->image.type()
+ && cv::norm(image, frames_.back().pixels->image, cv::NORM_INF) == 0;
+ if (!equal) {
+ const size_t length = image.total() * image.elemSize();
</code_context>
<issue_to_address>
**Repeated full-frame comparisons slow recognition**
When a captured image is tested against multiple candidates before one matches, `Recognizer::recognize` observes the image for every candidate node, even though `PipelineTask::recognize_list` passes the same captured image through its candidate loop. With frame history enabled by default, each such observation runs a full-image `cv::norm` against the retained frame, so fallback-heavy pipelines repeatedly scan the same high-resolution image and take longer to recognize.
Deduplicate observations by captured-image identity or generation so each recognition input is compared and recorded at most once.
Also at `source/MaaFramework/Task/Component/Recognizer.cpp:119`.
</issue_to_address>
### Comment 10
<location path="include/MaaFramework/MaaDef.h" line_range="134" />
<code_context>
+ ///
+ /// value: bool, eg: true; val_size: sizeof(bool)
+ /// default value is true, independent of SaveOnError and DebugMode
+ MaaGlobalOption_SaveOnErrorFrames = 10,
};
</code_context>
<issue_to_address>
**C++ module omits new option**
`MaaFramework.cppm` does not export `MaaGlobalOption_SaveOnErrorFrames`, so C++20 module consumers cannot use the new option through the module interface.
Export the new option from `source/modules/MaaFramework.cppm`.
</issue_to_address>
### Comment 11
<location path="source/binding/Python/maa/tasker.py" line_range="736" />
<code_context>
)
)
+ @staticmethod
+ def set_save_on_error_frames(save_on_error_frames: bool) -> bool:
+ """设置是否保存任务失败关键帧，默认启用 / Set whether to save task failure keyframes, enabled by default
</code_context>
<issue_to_address>
**Python option lacks test coverage**
The new Python setter has no corresponding test in `test/python/`, so regressions in configuring this option are not caught by the required Python unit tests.
Add a Python unit test that calls `set_save_on_error_frames` and verifies the option-setting result.
</issue_to_address>![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85b479412e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Retain parent inputs before custom callbacks and arm post-action capture without a preceding image. Order frames by capture sequence, count actual history losses, and synchronize option access. Refs MaaXYZ#1594
Keep the original export name until the destination or staging path is occupied. Create a fresh staging directory before writing so existing evidence stays intact. Refs MaaXYZ#1594
Compare continuous 8-bit images bytewise and retain the existing norm check for other layouts and depths. Keep observing each recognition input because shared buffers can change between callbacks.
Verify the history option independently of legacy screenshots, pixel-exact PNG exports, history eviction and cleanup after stopping an active task.
Disable native and Toolkit history capture by default while honoring existing config values. Document that opted-in exports contain unredacted recognition inputs and cover default and persisted configurations in the binding suite.
Create private staging and exclusive files relative to retained directory handles. Publish without replacing existing archives, and reject unsafe POSIX staging parents.
Prevent cached_image from cloning pixels while the screenshot worker updates the same buffer.
tianguzhang
marked this pull request as draft
November 13, 2026 13:58
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A single error screenshot can miss the transition that caused a Pipeline failure. This adds an opt-in, lossless history of the actual recognition inputs, following #1594.
SaveOnErrorFramesis disabled by default and independent ofSaveOnError,SaveDraw, and debug mode. Enabling it saves unredacted recognition images.frames.json.Validation at
1b328722: local Windows Framework/Toolkit builds and SDK regressions passed, including failure-frame history, concurrent cached-image reads, live-job cache clearing, and neural-network frame lifetime. Native Windows and WSL Linux tests cover occupied files, late publication collisions, directory replacement, and link handling. Removing the POSIX parent guard from a temporary build makes the new regression fail as expected. Temporary probes remain local; permanent coverage is included in PipelineTesting and the Python binding suite.Cross-platform builds and Windows, Ubuntu, and macOS tests passed.
Open design items remain in #1594. Task completion waits at most 15 seconds for export, so a timeout does not guarantee files are ready for client packaging. Final archive-size enforcement belongs to the client. Earlier intermittent pixel differences remain unresolved, with the original failed artifacts retained; later passing runs do not establish a fix.