Skip to content

fix(windows): 时间轴截图预览稳定性加固,修复开启开关即卡死 - #947

Closed
xb503 wants to merge 18 commits into
AimesSoft:mainfrom
xb503:main
Closed

xb503 wants to merge 18 commits into
AimesSoft:mainfrom
xb503:main

Conversation

@xb503

@xb503 xb503 commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

问题

Windows 上在设置中点击「时间轴截图预览」开关,窗口立即「未响应」只能强制结束。日志无任何异常输出。

根因(两个叠加)

  1. 开启时弹出的全屏确认对话框,其半透明遮罩在部分 Windows 机器的合成/显卡层渲染挂死,对话框尚未画出即冻结(点开关瞬间卡死、且无任何弹窗出现)。
  2. 该功能会创建第二个后台 MDK 播放器抽帧,其 updateTexture() 在 platform 线程再建一套 D3D11 共享纹理设备(CreateRT),与主播放器硬解实例存在驱动层争用风险;snapshot() 依赖渲染回调完成,挂住时无超时兜底。

修改

  1. 移除开启确认对话框,开关行为与其他普通开关一致(此为直接卡死点);
  2. 预览播放器在 Windows 强制软件解码(video.hwdec=no),并跳过 updateTexture()——fvp 的 snapshot 走 mdk 原生软渲染回传 RGBA,不依赖纹理挂载(见 third_party/fvp/lib/src/callbacks.cpp MdkSnapshot);
  3. prepare 加 5 秒超时、三处 snapshot 各加 2 秒超时,任何挂住只损失单张缩略图,不会卡死串行队列;
  4. 播放器释放改为先停止再延迟 150ms 异步 dispose,避免 UI 线程同步 join 原生线程;
  5. 非 Windows 平台(macOS)的 MDK 抽帧路径未做任何改动。

测试

  • 点击开关开/关不卡死
  • 悬停进度条正常出缩略图
  • 播放中切集无卡死

测试环境:Windows 11,MDK 内核(MFT:d3d=11 硬解)。

@xb503
xb503 requested a review from a team as a code owner September 26, 2026 07:16

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the timeout/hang hardening — the direction is reasonable and the D3D11 contention analysis is useful. Two blockers before this can be considered, plus one likely no-op.

1. Unrelated removal (please restore)

The first hunk deletes the 播放器菜单快捷调节 / "Player Menu Quick Controls" settings tile. That setting is still fully implemented on main — showPlayerMenuQuickControls is defined in lib/utils/video_player_state/video_player_state_preferences.dart, declared in lib/constants/settings_keys.dart, consumed by lib/player_menu/player_quick_controls.dart, and covered by test/player_quick_controls_visibility_test.dart. Removing only the settings tile makes the option unreachable from the UI with no replacement, which looks like a rebase/merge artefact. Please drop it so the PR stays scoped to the timeline-preview fix.

2. setProperty('video.hwdec', 'no') is probably a silent no-op on MDK

video.hwdec does not appear anywhere else in this codebase. MDK/fvp decoder selection in this project goes through previewPlayer.setDecoders(PlayerMediaType.video, [...]) (see lib/utils/decoder_manager.dart, where the selected list is applied and read back via video.decoder / decoder.video), and the software decoder names recognised by _isSoftwareDecoderName() are FFmpeg and dav1d. Since the call is wrapped in try/catch, an unsupported key is swallowed and the "force software decoding on Windows" guard would not actually take effect. Could you either confirm on Windows that the preview player really ends up on a software decoder, or set it explicitly with setDecoders(PlayerMediaType.video, ['FFmpeg'])? Ideally also confirm prepare() still succeeds with hardware decoding disabled for the main player's codecs.

3. The new timeout paths still dispose synchronously on the platform thread

_ensureTimelinePreviewPlayer()'s catch (e) { ... previewPlayer.dispose(); return null; } is unchanged, so the new 5s prepare() timeout lands in a synchronous dispose() on the UI thread — the exact synchronous native join that the deferred dispose in _disposeTimelinePreviewPlayer() was added to avoid. Please route this path through the same deferred disposal.

4. Minor

  • _disposeTimelinePreviewPlayer() now clears _timelinePreviewPlayer immediately and disposes 150 ms later, so _ensureTimelinePreviewPlayer() can construct the next native player while the previous one is still alive — a brief two-instance window, i.e. the contention this PR is trying to remove. At minimum worth a comment; a "disposing" latch would be safer.
  • The test checklist is still unchecked. This changes Windows-only behaviour that CI cannot exercise, and the crash is hardware/driver dependent, so please state explicitly what was verified locally (toggle on/off, thumbnail hover, switching episodes mid-playback) and on which build and GPU.

Note on CI: Analyze and Test is red on this head SHA, but the single failure is test/television_media_library_test.dart: media collection restores each source sort after switching sections, which also fails on unrelated PRs — not attributable to this diff.

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at head 917816a. All four points from the previous review are handled, and the diff is now scoped to the two files it should touch:

  • The settings change is only the removal of the 开启警告 dialog now — the unrelated quick-controls tile removal is gone.
  • setProperty('video.hwdec', 'no') is replaced by setDecoders(PlayerMediaType.video, ['FFmpeg']), with the correct note that fvp does not read that property key, so the software-decode guard now actually takes effect.
  • The prepare() timeout path goes through the deferred disposal instead of a synchronous dispose() on the UI thread.
  • The disposal latch closes the two-instance window: the previous player is disposed before the next one is created.

Skipping updateTexture() on Windows at both call sites, and the 2 s snapshot() timeouts, are consistent with the root cause you described.

Two things left before this can be approved:

  1. The PR description still documents video.hwdec = no under 修改 item 2, which is no longer what the code does. Worth updating so the rationale and the diff agree.
  2. The test checklist is still unchecked. CI cannot exercise this — the hang is Windows/driver dependent, and the three scenarios (toggle on/off, thumbnail hover, switching episodes mid-playback) are exactly what needs to be recorded, together with the build and the GPU. The environment line is there, the results are not.

Approval can follow that verification note, or a maintainer with a Windows machine can confirm it in the meantime. Nothing else in the diff blocks.

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at bde10eeb. The updateTexture() reversal is fine and the claim behind it holds up: MDKPlayerAdapter.updateTexture() already wraps _mdkPlayer.updateTexture() in a 10 s timeout (lib/player_abstraction/mdk_player_adapter_io.dart:434-444), so calling it again on Windows cannot hang the serial queue indefinitely, and registering the render target is indeed required for the snapshot Completer to complete. Forcing software decoding via setDecoders(...['FFmpeg']) is still in place.

The PR description now contradicts the code in two places, and both matter for whoever reads this next:

  1. 修改 item 2 still says the preview player skips updateTexture() and that snapshots do not depend on the texture being mounted. The code deliberately registers the render target again, so either drop the sentence or replace it with the reason it was restored (snapshot needs the render callback; only the software decoder is left as the contention mitigation).
  2. 根因 item 2 still names updateTexture()'s CreateRT as one of the two root causes while the fix now keeps that call. A sentence on why it is kept — bounded by the 10 s adapter timeout, with software decoding removing the hardware-decoder contention — would stop a future reader from removing it again as a cleanup.
  3. 修改 item 2 also still quotes video.hwdec=no, which the code no longer uses.

The test checklist is still unchecked: the three Windows scenarios plus build and GPU are what approval is waiting on. Nothing else in the diff is outstanding.

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 34a25822. The snapshot reordering itself looks right: requesting the snapshot while the player is still producing frames, then pausing in finally, addresses exactly the path that used to block the serial queue forever, and keeping the 2 s timeout plus one retry means a slow first frame costs a thumbnail, not the UI thread.

Two points, both about the diff now being wider than the PR describes:

  1. The change is no longer Windows-scoped. The new if (kernel == PlayerKernelType.mdk) block in _captureTimelineFrame is platform-independent, so the play → snapshot → pause ordering, the retry and the new timeouts also apply to macOS MDK. 修改 item 5 ("非 Windows 平台(macOS)的 MDK 抽帧路径未做任何改动") is no longer true, and Windows-only testing cannot cover that risk. Either gate the new ordering to Windows and keep the old sequence elsewhere, or update item 5 and say how the macOS path was reasoned about.

  2. 修改 item 2 is stale in two ways. The code no longer skips updateTexture() on Windows — it is called at both MDK sites again (the one in _captureTimelineFrame has no platform guard left), and the decoder selection goes through setDecoders(PlayerMediaType.video, ['FFmpeg']), not video.hwdec = no. Since 根因 item 2 names updateTexture()'s CreateRT as one of the two root causes, item 2 should also say explicitly why that call is kept: the adapter already bounds it with a 10 s timeout (lib/player_abstraction/mdk_player_adapter_io.dart:434-444) and the forced software decoder removes the hardware-decoder contention. Without that sentence the next reader will remove the call again as a cleanup — which is what the last two revisions went back and forth on.

The test checklist is still unchecked, and it is the only thing left before this can be approved: the three scenarios, the build, and the GPU. If the new ordering does stay platform-independent, please add at least a macOS smoke result (thumbnails still appear, playback unaffected) to that list.

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 2d7faf7b. Gating the new snapshot ordering to Windows resolves the scope point: I diffed the non-Windows branch against main and it is unchanged, so 修改 item 5 holds again and macOS keeps the sequence it shipped with.

That leaves the description:

  • 修改 item 2 is still stale — it says the preview player skips updateTexture() on Windows and quotes video.hwdec = no. The code calls updateTexture() at both MDK sites and selects the decoder with setDecoders(PlayerMediaType.video, ['FFmpeg']). Worth also stating there why the call is kept, since 根因 item 2 names its CreateRT as one of the causes (bounded by the adapter's 10 s timeout, hardware-decoder contention removed by the software decoder).
  • The test checklist is still empty. That is the last item: the three scenarios, the build and the GPU.

No further code points from me.

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 832537b7. The Windows/MDK restructure from the previous head is unchanged; what this push adds is the temporary diagnostic logging, and that one does block:

  • _tlLog() writes synchronously (writeAsStringSync, FileMode.append) to %USERPROFILE%\Documents\nipaplay\timeline_diag.log for every timeline-preview request, unconditionally on Windows. It has no debug/verbose switch, no size bound, and it creates a file in the user's own Documents folder. _ensureTimelinePreviewPlayer also logs the media source path at line 262, so the file ends up containing paths of the user's own media. This is fine as a local investigation, but it cannot ship — please remove the helper and all 20 call sites before this is mergeable, or gate it behind an existing debug setting if you think it is worth keeping.
  • The synchronous file write also happens on the hover path, so it is I/O on a UI-adjacent request; another reason not to leave it in.

If the local run is what produced that log, the evidence belongs in this PR rather than in it: paste the relevant lines (or a short summary of what each stage reported) under 测试 and tick the three boxes. That is the only thing left before approval — the code itself, including the Windows-gated snapshot ordering from the previous head, is fine from my side.

Still outstanding from earlier rounds: 修改 item 2 describes the preview player as skipping updateTexture() and quotes video.hwdec = no; the code calls updateTexture() at both MDK sites and selects software decoding with setDecoders(PlayerMediaType.video, ['FFmpeg']).

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at cd402fca. The diagnostics are gone — _tlLog, the 20 call sites and the Documents log file are all out of the diff, so that blocker is cleared. The _videoSize finding behind the new wait loop is a good one: keeping the player playing until mediaInfo reports real dimensions, retrying updateTexture(), and only pausing afterwards is consistent with fvp returning -1 silently when _setVideoSize has nothing to work with, and it explains the empty snapshots that the earlier heads produced.

Three things on the new loop:

  1. It is not platform-scoped, only the retry is. The for (var i = 0; i < 50; i++) dimension wait runs before the if (kernel == PlayerKernelType.mdk) block, so it also applies to macOS MDK and to MediaKit. That makes 修改 item 5 inaccurate again, and it changes behaviour on paths the PR is not about.
  2. The timeout throws, which is a harder failure than before. Failing the wait disposes the preview player and returns null, so on any kernel that never reports dimensions the feature loses every thumbnail, where previously it produced no frame for that bucket only. If the wait is really needed for Windows MDK, scope it there; if it stays general, treat "dimensions not ready" as a soft signal and attempt the snapshot anyway rather than discarding the player.
  3. Worst case is roughly 10 s inside the serial queue (5 s prepare + 5 s here, plus 3 × 150 ms of retries), and every later hover queues behind it. The loop also only checks after the first 100 ms sleep, so a ready player pays that delay on every creation. Worth a comment on the bound, and an immediate check before entering the loop.

The PR description needs one pass to match the code now: 修改 item 2 still describes skipping updateTexture() and video.hwdec = no, and the list does not mention the dimension wait, the updateTexture() retry or the deferred pause. The test checklist is still the last gate.

@xb503
xb503 marked this pull request as draft September 27, 2026 15:36

@FurudeRika123 FurudeRika123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 9772bda5. Two points from my previous review are unchanged in this head — the dimension wait still sits ahead of the if (kernel == PlayerKernelType.mdk) block, so it still runs on every platform and on MediaKit, and it still throws on timeout, which discards the preview player rather than degrading to "no frame for this bucket"; the worst case is still about 10 s inside the serial queue. I will not repeat them again, but they are open.

What this push changes:

  1. The temporary diagnostics are back in the diff — _tlLog, 6 call sites, appending to %USERPROFILE%\Documents\nipaplay\tl2.log, with "合并前移除" in the comment. The PR is not mergeable while that is in it: it writes into a user-visible folder on the hover path, and _ensureTimelinePreviewPlayer logs the media source path there. Debug logs are for the branch, not for a PR that a maintainer may merge.

  2. The forced software decode is gone entirely — no setDecoders, no hwdec anywhere in the file now. The explanation in the comment (an exclusive ['FFmpeg'] candidate that fails to initialise makes mdk declare decode failure, which nulls _videoSize) is plausible and may well be the right call, but it means 修改 item 2 and 根因 item 2 no longer describe this PR at all: nothing in the final diff forces software decoding, so the D3D11-contention mitigation that the description is built around has been replaced by "one instance at a time, timeouts, ordered disposal". The description needs to say that, and say why the decoder experiment was dropped — otherwise the next reviewer reads 根因 item 2 and asks for the software decode back.

Still required before merge: a description pass that matches the code, the test checklist, the platform scope of the dimension wait, and removal of _tlLog. I am not going to keep reviewing each push line by line — the last three heads have changed decoder selection and added/removed diagnostics without moving any of these four items, so the useful next step is the description and the verification, not another revision.

@xb503 xb503 closed this Sep 28, 2026
@xb503

xb503 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

感谢这几轮非常细致的 review——平台 scope、同步 dispose、描述与代码一致性这些意见都很到位。

说明一下关闭这个 PR 的原因:

按你的建议先做了本地诊断(临时日志,仅本地排查用),结果证实了一个比前几轮所有讨论更底层的问题:fvp 的第二个播放器在 Windows 上根本无法创建渲染目标——

原因是 fvp 内部的 _videoSize 在 loaded → 非 loaded 转换时被 complete(null)(third_party/fvp/lib/src/player.dart L139-145),导致 createTexture 静默返回 -1。fvp 的渲染管线设计上只服务挂载在 widget tree 上的播放器,第二播放器没有 widget 挂载,纹理创建必然失败。

这意味着在这个架构下,无论软解/硬解怎么选、时序怎么调、updateTexture 调不调,缩略图都不可能出图。前几轮反复修改却始终没有实质进展,根源就在这里——方向对,但天花板在 fvp。

因此我依然建议 Windows 上改用 ffmpeg.exe 独立子进程抽帧

再次感谢,这些 review 意见对后续实现很有帮助。

@FurudeRika123

Copy link
Copy Markdown
Member

Thank you for coming back with the diagnosis — that comment is the actual result of this thread, and it is worth more than the fix would have been.

I verified both halves of your claim in the tree so the record is not just prose: third_party/fvp/lib/src/player.dart completes _videoSize with null on the loading → invalid|stalled path (L138) and on loaded → 非 loaded (L144, the one you cited), and textureSize => _videoSize.future (L263) is what createTexture waits on — so with no widget mount there is no size, no texture, no render callback, and snapshot() never completes. That is a ceiling, not a tuning problem, and it explains every dead end in the previous revisions: the force-software-decode experiment, the updateTexture() skip and its revert, the play-then-snapshot ordering and the timeouts could all shift when we hit the ceiling, never whether we hit it.

For the record on my side: the loop we went through did not help you get closer to a fix, and I should have recognised earlier that the blocker was a platform capability rather than a code path — one review saying "this needs a sandboxed/off-screen render target or a different capture mechanism, verified on Windows" would have been the useful contribution, and the rest of the rounds were cost.

So we do not lose it: I have filed #967 as a tracking issue with the root cause, the code references above, the list of directions already ruled out, and the ffmpeg.exe subprocess route from #946 as the recommended one — credited to you and linked to both PRs, so the next person does not re-walk them.

No expectation that you reopen anything; if you ever want to take the subprocess version forward, that is a maintainer call, and this issue is where it starts.

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