Conversation
FurudeRika123
left a comment
There was a problem hiding this comment.
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_timelinePreviewPlayerimmediately 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
left a comment
There was a problem hiding this comment.
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 bysetDecoders(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 synchronousdispose()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:
- The PR description still documents
video.hwdec = nounder 修改 item 2, which is no longer what the code does. Worth updating so the rationale and the diff agree. - 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
left a comment
There was a problem hiding this comment.
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:
- 修改 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). - 根因 item 2 still names
updateTexture()'sCreateRTas 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. - 修改 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
left a comment
There was a problem hiding this comment.
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:
-
The change is no longer Windows-scoped. The new
if (kernel == PlayerKernelType.mdk)block in_captureTimelineFrameis 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. -
修改 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_captureTimelineFramehas no platform guard left), and the decoder selection goes throughsetDecoders(PlayerMediaType.video, ['FFmpeg']), notvideo.hwdec = no. Since 根因 item 2 namesupdateTexture()'sCreateRTas 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
left a comment
There was a problem hiding this comment.
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 quotesvideo.hwdec = no. The code callsupdateTexture()at both MDK sites and selects the decoder withsetDecoders(PlayerMediaType.video, ['FFmpeg']). Worth also stating there why the call is kept, since 根因 item 2 names itsCreateRTas 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
left a comment
There was a problem hiding this comment.
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.logfor 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._ensureTimelinePreviewPlayeralso logs the mediasourcepath 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
left a comment
There was a problem hiding this comment.
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:
- It is not platform-scoped, only the retry is. The
for (var i = 0; i < 50; i++)dimension wait runs before theif (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. - 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.
- 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.
FurudeRika123
left a comment
There was a problem hiding this comment.
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:
-
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_ensureTimelinePreviewPlayerlogs the mediasourcepath there. Debug logs are for the branch, not for a PR that a maintainer may merge. -
The forced software decode is gone entirely — no
setDecoders, nohwdecanywhere 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.
|
感谢这几轮非常细致的 review——平台 scope、同步 dispose、描述与代码一致性这些意见都很到位。 说明一下关闭这个 PR 的原因: 按你的建议先做了本地诊断(临时日志,仅本地排查用),结果证实了一个比前几轮所有讨论更底层的问题:fvp 的第二个播放器在 Windows 上根本无法创建渲染目标—— 原因是 fvp 内部的 这意味着在这个架构下,无论软解/硬解怎么选、时序怎么调、updateTexture 调不调,缩略图都不可能出图。前几轮反复修改却始终没有实质进展,根源就在这里——方向对,但天花板在 fvp。 因此我依然建议 Windows 上改用 ffmpeg.exe 独立子进程抽帧 再次感谢,这些 review 意见对后续实现很有帮助。 |
|
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: 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 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. |
问题
Windows 上在设置中点击「时间轴截图预览」开关,窗口立即「未响应」只能强制结束。日志无任何异常输出。
根因(两个叠加)
updateTexture()在 platform 线程再建一套 D3D11 共享纹理设备(CreateRT),与主播放器硬解实例存在驱动层争用风险;snapshot()依赖渲染回调完成,挂住时无超时兜底。修改
video.hwdec=no),并跳过updateTexture()——fvp 的 snapshot 走 mdk 原生软渲染回传 RGBA,不依赖纹理挂载(见 third_party/fvp/lib/src/callbacks.cpp MdkSnapshot);prepare加 5 秒超时、三处snapshot各加 2 秒超时,任何挂住只损失单张缩略图,不会卡死串行队列;测试