HDR功能初步实现#1300
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并留下了一些总体反馈:
- HDR 色调映射工具(HdrSceneStats、曝光计算、tone_map_filmic、linear_to_srgb 等)在 FramePoolScreencap.cpp 和 ControllerAgent.cpp 中基本是重复的;建议把它们抽取到一个共享的辅助模块中,以避免两边逻辑偏移,并让将来的调优更轻松。
- 在 FramePoolScreencap::screencap() 中,switch 的 default 分支记录的是
texture_desc_.Format,而不是实际查询到的current_desc.Format;在重新创建纹理之后,前者可能已经过期。在日志中使用当前的描述符会让调试不支持的格式时更清晰。 - 多处使用了函数静态变量
std::chrono::steady_clock::time_point(例如 ControllerAgent 中的 HDR 日志节流以及 HDR 显示补偿路径)却没有做同步;如果截图处理可能在多个线程中进行,这会引入数据竞争。建议用互斥量进行保护,或者把节流状态移动到线程隔离的上下文中。
给 AI Agents 的提示
Please address the comments from this code review:
## Overall Comments
- The HDR tone mapping utilities (HdrSceneStats, exposure calculation, tone_map_filmic, linear_to_srgb, etc.) are essentially duplicated between FramePoolScreencap.cpp and ControllerAgent.cpp; consider extracting these into a shared helper to avoid drift and make future tuning easier.
- In FramePoolScreencap::screencap() the default case of the switch logs `texture_desc_.Format` instead of the actually queried `current_desc.Format`, which can be stale after recreating textures; using the current descriptor in the log will make debugging unsupported formats much clearer.
- Several places use function‑static `std::chrono::steady_clock::time_point` (e.g., HDR logging throttles in ControllerAgent and the HDR display compensation paths) without synchronization; if screenshots can be processed from multiple threads this introduces data races, so consider guarding these with a mutex or moving the throttling state to a thread‑confined context.
## Individual Comments
### Comment 1
<location path="source/MaaWin32ControlUnit/Screencap/FramePoolScreencap.cpp" line_range="667-668" />
<code_context>
+ case ScreencapMethod::ScreenDC:
+ return "ScreenDC";
+ case ScreencapMethod::UnknownYet:
+ default:
+ return "UnknownYet";
+ }
</code_context>
<issue_to_address>
**suggestion:** Log uses `texture_desc_` instead of the current frame’s `current_desc` when reporting an unsupported DXGI format.
This branch switches on `current_desc.Format`, but the log prints `texture_desc_.Format`. If the staging texture isn’t created yet or uses a different format, the log will show the wrong value and hinder debugging. Please log `current_desc.Format` here (or both, if you want to expose mismatches).
Suggested implementation:
```cpp
LOG_ERROR(
"Unsupported DXGI format: current_desc.Format={}, texture_desc_.Format={}",
static_cast<int>(current_desc.Format),
static_cast<int>(texture_desc_.Format));
```
If the logging macro or function name differs (e.g. `spdlog::error`, `LOGE`, `Logger::Error`, etc.), adjust the above replacement to match the existing logging style while keeping the two-format logging and the `static_cast<int>` conversions (or equivalent) to format the enum values.
</issue_to_address>请帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The HDR tone mapping utilities (HdrSceneStats, exposure calculation, tone_map_filmic, linear_to_srgb, etc.) are essentially duplicated between FramePoolScreencap.cpp and ControllerAgent.cpp; consider extracting these into a shared helper to avoid drift and make future tuning easier.
- In FramePoolScreencap::screencap() the default case of the switch logs
texture_desc_.Formatinstead of the actually queriedcurrent_desc.Format, which can be stale after recreating textures; using the current descriptor in the log will make debugging unsupported formats much clearer. - Several places use function‑static
std::chrono::steady_clock::time_point(e.g., HDR logging throttles in ControllerAgent and the HDR display compensation paths) without synchronization; if screenshots can be processed from multiple threads this introduces data races, so consider guarding these with a mutex or moving the throttling state to a thread‑confined context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The HDR tone mapping utilities (HdrSceneStats, exposure calculation, tone_map_filmic, linear_to_srgb, etc.) are essentially duplicated between FramePoolScreencap.cpp and ControllerAgent.cpp; consider extracting these into a shared helper to avoid drift and make future tuning easier.
- In FramePoolScreencap::screencap() the default case of the switch logs `texture_desc_.Format` instead of the actually queried `current_desc.Format`, which can be stale after recreating textures; using the current descriptor in the log will make debugging unsupported formats much clearer.
- Several places use function‑static `std::chrono::steady_clock::time_point` (e.g., HDR logging throttles in ControllerAgent and the HDR display compensation paths) without synchronization; if screenshots can be processed from multiple threads this introduces data races, so consider guarding these with a mutex or moving the throttling state to a thread‑confined context.
## Individual Comments
### Comment 1
<location path="source/MaaWin32ControlUnit/Screencap/FramePoolScreencap.cpp" line_range="667-668" />
<code_context>
+ case ScreencapMethod::ScreenDC:
+ return "ScreenDC";
+ case ScreencapMethod::UnknownYet:
+ default:
+ return "UnknownYet";
+ }
</code_context>
<issue_to_address>
**suggestion:** Log uses `texture_desc_` instead of the current frame’s `current_desc` when reporting an unsupported DXGI format.
This branch switches on `current_desc.Format`, but the log prints `texture_desc_.Format`. If the staging texture isn’t created yet or uses a different format, the log will show the wrong value and hinder debugging. Please log `current_desc.Format` here (or both, if you want to expose mismatches).
Suggested implementation:
```cpp
LOG_ERROR(
"Unsupported DXGI format: current_desc.Format={}, texture_desc_.Format={}",
static_cast<int>(current_desc.Format),
static_cast<int>(texture_desc_.Format));
```
If the logging macro or function name differs (e.g. `spdlog::error`, `LOGE`, `Logger::Error`, etc.), adjust the above replacement to match the existing logging style while keeping the two-format logging and the `static_cast<int>` conversions (or equivalent) to format the enum values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| default: | ||
| LogError << "Unsupported frame format" << VAR(texture_desc_.Format); |
There was a problem hiding this comment.
suggestion: 日志在报告不支持的 DXGI 格式时,使用了 texture_desc_,而不是当前帧的 current_desc。
这个分支是根据 current_desc.Format 进行 switch 的,但日志打印的是 texture_desc_.Format。如果暂存纹理尚未创建或使用了不同的格式,日志中显示的值就会不正确,从而妨碍调试。请在这里记录 current_desc.Format(或者两者都记录,如果你希望暴露格式不匹配的问题)。
建议实现:
LOG_ERROR(
"Unsupported DXGI format: current_desc.Format={}, texture_desc_.Format={}",
static_cast<int>(current_desc.Format),
static_cast<int>(texture_desc_.Format));
如果日志宏或函数名不同(例如 spdlog::error、LOGE、Logger::Error 等),请根据现有的日志风格调整上述替换代码,同时保留对两种格式的记录以及用于格式化枚举值的 static_cast<int> 转换(或等价方式)。
Original comment in English
suggestion: Log uses texture_desc_ instead of the current frame’s current_desc when reporting an unsupported DXGI format.
This branch switches on current_desc.Format, but the log prints texture_desc_.Format. If the staging texture isn’t created yet or uses a different format, the log will show the wrong value and hinder debugging. Please log current_desc.Format here (or both, if you want to expose mismatches).
Suggested implementation:
LOG_ERROR(
"Unsupported DXGI format: current_desc.Format={}, texture_desc_.Format={}",
static_cast<int>(current_desc.Format),
static_cast<int>(texture_desc_.Format));
If the logging macro or function name differs (e.g. spdlog::error, LOGE, Logger::Error, etc.), adjust the above replacement to match the existing logging style while keeping the two-format logging and the static_cast<int> conversions (or equivalent) to format the enum values.
There was a problem hiding this comment.
Pull request overview
该 PR 为 Win32 截图链路加入 HDR 相关能力:在检测到显示器开启 HDR 时,对传统 GDI/PrintWindow/ScreenDC 截图做 SDR 白点补偿;同时扩展 FramePool (WGC) 支持 HDR 浮点帧并提供 GPU 端预处理/色调映射,最终将 HDR/补偿状态上报到 Controller 层并输出调试探针图。
Changes:
- 新增
HdrDisplayUtils:查询 Windows 高级色彩/HDR 状态与 SDR 白点,并提供 HDR 显示补偿算法(CPU)。 - 传统截图方法(GDI/PrintWindow/ScreenDC)在 HDR 显示开启时对截图做补偿,并记录
ScreencapInfo。 - FramePool 截图支持 HDR 像素格式探测与 GPU 预处理管线;Win32ControlUnitMgr/ControllerAgent 增加 HDR 状态上报、通知与探针图保存。
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| source/MaaWin32ControlUnit/Screencap/ScreenDCScreencap.h | 增加 last_screencap_info() 与状态缓存字段。 |
| source/MaaWin32ControlUnit/Screencap/ScreenDCScreencap.cpp | HDR 显示开启时对 ScreenDC 截图做补偿并限频日志。 |
| source/MaaWin32ControlUnit/Screencap/PrintWindowWithPseudoMinimizeScreencap.h | 透传 last_screencap_info()。 |
| source/MaaWin32ControlUnit/Screencap/PrintWindowScreencap.h | 增加 last_screencap_info() 与状态缓存字段。 |
| source/MaaWin32ControlUnit/Screencap/PrintWindowScreencap.cpp | HDR 显示开启时对 PrintWindow 截图做补偿并限频日志。 |
| source/MaaWin32ControlUnit/Screencap/HwndUtils.hpp | 全屏/无边框时使用 rcMonitor 以允许覆盖任务栏区域。 |
| source/MaaWin32ControlUnit/Screencap/HdrDisplayUtils.hpp | 新增 HDR 查询/补偿接口与 HdrDisplayState 定义。 |
| source/MaaWin32ControlUnit/Screencap/HdrDisplayUtils.cpp | 实现 per-monitor HDR 状态查询、SDR 白点读取与补偿算法。 |
| source/MaaWin32ControlUnit/Screencap/GdiScreencap.h | 增加 last_screencap_info() 与状态缓存字段。 |
| source/MaaWin32ControlUnit/Screencap/GdiScreencap.cpp | HDR 显示开启时对 GDI 截图做补偿并限频日志。 |
| source/MaaWin32ControlUnit/Screencap/FramePoolWithPseudoMinimizeScreencap.h | 透传 last_screencap_info()。 |
| source/MaaWin32ControlUnit/Screencap/FramePoolScreencap.h | 增加 HDR GPU 处理相关成员与 last_screencap_info()。 |
| source/MaaWin32ControlUnit/Screencap/FramePoolScreencap.cpp | HDR/SDR 帧池探测、GPU 预处理/色调映射、缓存回退与包含从属窗口等增强。 |
| source/MaaWin32ControlUnit/Screencap/DesktopDupWindowScreencap.h | 增加 ScreenDC 回退与 last_screencap_info()。 |
| source/MaaWin32ControlUnit/Screencap/DesktopDupWindowScreencap.cpp | 多显示器跨度/越界等情况下回退到 ScreenDC。 |
| source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.h | 记录活动截图方法与最近截图信息并提供方法名。 |
| source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp | 截图失败时重初始化截图方式;get_info() 上报 HDR/方法信息。 |
| source/MaaWin32ControlUnit/Base/UnitBase.h | 新增 ScreencapInfo 与 ScreencapBase::last_screencap_info() 默认实现。 |
| source/MaaFramework/Controller/ControllerAgent.h | 增加 HDR 模式通知/探针字段与若干互斥锁。 |
| source/MaaFramework/Controller/ControllerAgent.cpp | HDR 帧色调映射与探针图保存;改进通知构建与若干线程安全/边界处理。 |
| rgb *= scale; | ||
| } | ||
|
|
||
| return float4(linear_to_srgb(rgb.r), linear_to_srgb(rgb.g), linear_to_srgb(rgb.b), 1.0); |
There was a problem hiding this comment.
The tonemap shader writes float4(R,G,B,A) but the render target is created as DXGI_FORMAT_B8G8R8A8_UNORM. For BGRA render targets, the first component maps to B, so this will swap red/blue in GPU HDR output (and bgra_to_bgr() will preserve the swap). Output BGRA order (B,G,R,A) here, or change the output texture format to an RGBA render target format.
| return float4(linear_to_srgb(rgb.r), linear_to_srgb(rgb.g), linear_to_srgb(rgb.b), 1.0); | |
| return float4(linear_to_srgb(rgb.b), linear_to_srgb(rgb.g), linear_to_srgb(rgb.r), 1.0); |
| constexpr LONG kDisplayConfigErrorInsufficientBuffer = 122; | ||
| constexpr int kMaxDisplayConfigRetries = 3; | ||
|
|
There was a problem hiding this comment.
kDisplayConfigErrorInsufficientBuffer and kMaxDisplayConfigRetries are declared here but never used in this file, which can trigger unused-variable warnings. Remove them or use the corresponding constants from the HDR query helper if needed.
| constexpr LONG kDisplayConfigErrorInsufficientBuffer = 122; | |
| constexpr int kMaxDisplayConfigRetries = 3; |
| return cached_image_; | ||
| } | ||
| default: | ||
| LogError << "Unsupported frame format" << VAR(texture_desc_.Format); |
There was a problem hiding this comment.
This error log reports texture_desc_.Format, but the switch is on current_desc.Format. If the staging texture desc differs (or texture_desc_ was reset), the log can be misleading—log current_desc.Format instead.
| LogError << "Unsupported frame format" << VAR(texture_desc_.Format); | |
| LogError << "Unsupported frame format" << VAR(current_desc.Format); |
| readable_texture_ = nullptr; | ||
| texture_desc_ = { 0 }; | ||
| if (!init_texture(texture)) { | ||
| LogError << "falied to init_texture"; |
There was a problem hiding this comment.
Typo in log message: "falied" should be "failed".
| LogError << "falied to init_texture"; | |
| LogError << "failed to init_texture"; |
| bool device_name_equals(const wchar_t (&lhs)[CCHDEVICENAME], const DeviceNameBuffer& rhs) | ||
| { | ||
| for (size_t i = 0; i < CCHDEVICENAME; ++i) { | ||
| if (lhs[i] != rhs[i]) { | ||
| return false; | ||
| } | ||
| if (lhs[i] == L'\0') { | ||
| return true; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This overload of device_name_equals() is not referenced in this translation unit. Consider removing it to avoid unused-function warnings and reduce duplicate logic.
| processed = std::move(hdr_bgr8); | ||
| } | ||
| else { | ||
| cv::resize(hdr_bgr8, processed, { target_width, target_height }, 0, 0, resize_method); |
| resized = raw; | ||
| } | ||
| else { | ||
| cv::resize(raw, resized, { target_width, target_height }, 0, 0, resize_method); |
| } | ||
|
|
||
| if (resized.type() == CV_8UC3) { | ||
| processed = resized.clone(); |
| processed = resized.clone(); | ||
| } | ||
| else if (resized.type() == CV_8UC4) { | ||
| cv::cvtColor(resized, processed, cv::COLOR_BGRA2BGR); |
|
|
||
| cv::Mat rgba32f; | ||
| if (hdr_rgba.type() == CV_32FC4) { | ||
| rgba32f = hdr_rgba; |
| rgba32f = hdr_rgba; | ||
| } | ||
| else { | ||
| hdr_rgba.convertTo(rgba32f, CV_32FC4); |
| stats.max_linear = std::numeric_limits<float>::lowest(); | ||
|
|
||
| std::vector<float> luminances; | ||
| luminances.reserve(static_cast<size_t>((rgba32f.rows / stride + 1) * (rgba32f.cols / stride + 1))); |
| double log_sum = 0.0; | ||
| size_t sample_count = 0; | ||
|
|
||
| for (int y = 0; y < rgba32f.rows; y += stride) { |
| } | ||
| } | ||
|
|
||
| cv::Mat bgr8(rgba32f.rows, rgba32f.cols, CV_8UC3); |
| cv::Mat bgr8(rgba32f.rows, rgba32f.cols, CV_8UC3); | ||
|
|
||
| cv::parallel_for_(cv::Range(0, rgba32f.rows), [&](const cv::Range& range) { | ||
| for (int y = range.start; y < range.end; ++y) { |
| rgba32f = hdr_rgba; | ||
| } | ||
| else { | ||
| hdr_rgba.convertTo(rgba32f, CV_32FC4); |
| stats.max_linear = std::numeric_limits<float>::lowest(); | ||
|
|
||
| std::vector<float> luminances; | ||
| luminances.reserve(static_cast<size_t>(rgba32f.rows * rgba32f.cols)); |
| size_t sample_count = 0; | ||
|
|
||
| for (int y = 0; y < rgba32f.rows; ++y) { | ||
| const auto* row = rgba32f.ptr<cv::Vec4f>(y); |
| } | ||
| } | ||
|
|
||
| d3d_context_->CopyResource(readable_texture_.get(), texture.get()); |
| d3d_context_->CopyResource(readable_texture_.get(), texture.get()); | ||
|
|
||
| D3D11_MAPPED_SUBRESOURCE mapped { 0 }; | ||
| ret = d3d_context_->Map(readable_texture_.get(), 0, D3D11_MAP_READ, 0, &mapped); |
| case DXGI_FORMAT_B8G8R8A8_UNORM: { | ||
| cv::Mat raw(raw_height, raw_width, CV_8UC4, mapped.pData, mapped.RowPitch); | ||
| last_screencap_info_ = {}; | ||
| cached_image_ = bgra_to_bgr(raw(client_roi)); |
| .display_hdr_active = true, | ||
| .display_hdr_compensated = false, | ||
| }; | ||
| cached_image_ = raw(client_roi).clone(); |
|
我测了反正自动战斗可以用 |
|
没办法全部做进显存里面的 |
| d3d_context_->CopyResource(hdr_stats_staging_.get(), hdr_stats_texture_.get()); | ||
|
|
||
| D3D11_MAPPED_SUBRESOURCE mapped_stats {}; | ||
| HRESULT ret = d3d_context_->Map(hdr_stats_staging_.get(), 0, D3D11_MAP_READ, 0, &mapped_stats); |
| } | ||
| OnScopeLeave([&]() { d3d_context_->Unmap(hdr_stats_staging_.get(), 0); }); | ||
|
|
||
| cv::Mat hdr_stats_mat(hdr_gpu_stats_size_.height, hdr_gpu_stats_size_.width, CV_16FC4, mapped_stats.pData, mapped_stats.RowPitch); |
|
没事不行就关了 |
| d3d_context_->OMSetRenderTargets(1, stats_rtv, nullptr); | ||
| d3d_context_->PSSetShader(hdr_copy_pixel_shader_.get(), nullptr, 0); | ||
| d3d_context_->Draw(3, 0); | ||
| d3d_context_->CopyResource(hdr_stats_staging_.get(), hdr_stats_texture_.get()); |
| ID3D11ShaderResourceView* null_shader_resources[] = { nullptr }; | ||
| d3d_context_->PSSetShaderResources(0, 1, null_shader_resources); | ||
|
|
||
| d3d_context_->CopyResource(hdr_output_staging_.get(), hdr_output_texture_.get()); |
| d3d_context_->CopyResource(hdr_output_staging_.get(), hdr_output_texture_.get()); | ||
|
|
||
| D3D11_MAPPED_SUBRESOURCE mapped_output {}; | ||
| ret = d3d_context_->Map(hdr_output_staging_.get(), 0, D3D11_MAP_READ, 0, &mapped_output); |
|
主要是想找点人试试有,看转换能不能行 |
| OnScopeLeave([&]() { d3d_context_->Unmap(hdr_output_staging_.get(), 0); }); | ||
|
|
||
| cv::Mat gpu_bgra(hdr_gpu_output_size_.height, hdr_gpu_output_size_.width, CV_8UC4, mapped_output.pData, mapped_output.RowPitch); | ||
| output = bgra_to_bgr(gpu_bgra); |
|
因为每个人颜色管理不一样的 |
| return false; | ||
| } | ||
|
|
||
| d3d_context_->CopyResource(hdr_shader_input_texture_.get(), texture.get()); |
| d3d_context_->RSSetViewports(1, &output_viewport); | ||
| d3d_context_->OMSetRenderTargets(1, output_rtv, nullptr); | ||
| d3d_context_->PSSetShader(hdr_tonemap_pixel_shader_.get(), nullptr, 0); | ||
| d3d_context_->Draw(3, 0); |
|
在视频转换项目里,做多次内存拷贝、显存拷贝,大不了时间长一点无所谓,时长x2 x3用户感知也不明显,多等一会就是了。 |
|
其实没有这个的原生Maaend 147kf的CPU占用也有4%( |
|
🤖 附带的线程安全修复、swipe 插值、null guard 这些都不错,但 HDR 核心路径有几个问题:
如果核心问题(1)能有数据佐证的话,后面几个改一下我们可以继续谈 |
|
🤖 MistEO 那边有个 #1105 在做类似的事,等他回来一起看看方向 |
是Add,不影响现在非HDR的功能实现。
采用了转换算法,延迟和CPU占用控制在合理范围啦!
实测整个Maaend 5%CPU占用(147kf)
延迟6~20ms吧
Summary by Sourcery
为 Win32 控制单元和控制器新增支持 HDR 的截图管线,包括色调映射、HDR 显示补偿以及按显示器检测 HDR,同时保持现有 SDR 路径不变。
New Features:
Bug Fixes:
Enhancements:
Original summary in English
Summary by Sourcery
Add HDR-aware screenshot pipeline for Win32 control unit and controller, including tone mapping, HDR display compensation, and per-monitor HDR detection while keeping existing SDR paths intact.
New Features:
Bug Fixes:
Enhancements: