From f15e2cc9891c6c8007b65a64755d58a655f42052 Mon Sep 17 00:00:00 2001 From: tkrmagid Date: Sun, 14 Jun 2026 02:57:09 +0900 Subject: [PATCH] v0.4.40: make video playback survive shader-pack multi-pass rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under Iris/shader packs the world renders in several passes per frame, so the block-entity renderer's extractRenderState (and thus getOrStart) runs many times per frame and tick() can transiently miss the BE. The old design recreated the backend + a fresh per-instance texture Identifier on every miss and tore the entry down on the first absent-BE tick, producing a runaway: hundreds of decoder threads/sec for one screen, the texture id changing every pass ("Missing resource .../dynamic/"), and a panel that never stabilised — invisible for shader users while non-shader users saw it. Fixes: - Deterministic texture Identifier derived from BlockPos, so a recreate reuses the same id and the renderer never binds a released id. - Serialise create/url-change under a lock + ConcurrentHashMap so concurrent passes can't double-spawn a decoder per anchor. - Tolerate a missing BE for MISSING_GRACE_TICKS (~2s) before teardown instead of nuking on the first miss; real deletes still stop immediately via BLOCK_ENTITY_UNLOAD. - Transient texture-upload errors no longer kill the decoder (logged once). Co-Authored-By: Claude Opus 4.7 --- gradle.properties | 2 +- .../client/playback/VideoPlayback.java | 175 ++++++++++++------ 2 files changed, 121 insertions(+), 56 deletions(-) diff --git a/gradle.properties b/gradle.properties index 68bbb2a..9890a63 100644 --- a/gradle.properties +++ b/gradle.properties @@ -5,7 +5,7 @@ org.gradle.configuration-cache=false # Mod mod_id=video_player -mod_version=0.4.39 +mod_version=0.4.40 maven_group=com.ejclaw.videoplayer archives_base_name=video_player diff --git a/src/main/java/com/ejclaw/videoplayer/client/playback/VideoPlayback.java b/src/main/java/com/ejclaw/videoplayer/client/playback/VideoPlayback.java index 8ab512b..a5deebf 100644 --- a/src/main/java/com/ejclaw/videoplayer/client/playback/VideoPlayback.java +++ b/src/main/java/com/ejclaw/videoplayer/client/playback/VideoPlayback.java @@ -11,11 +11,11 @@ import net.minecraft.core.BlockPos; import net.minecraft.resources.Identifier; import java.nio.file.Path; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; /** * SPEC §5 — per-anchor playback registry. Maps {@link BlockPos} → ({@link VideoBackend} + @@ -26,67 +26,103 @@ import java.util.Set; *

When no backend is available on the classpath (e.g. JavaCV jar not installed by the * user), the texture stays at its initial placeholder pattern, so the anchor's screen quad * still renders as a visible surface. + * + *

Shader-pack robustness. Under Iris/OptiFine-style shader packs the world is + * rendered in several passes per frame (shadow map, deferred, translucent, …), so the block + * entity renderer's {@code extractRenderState} — and therefore {@link #getOrStart} — runs many + * times per frame, sometimes off the main render thread. The earlier design re-created the + * backend + a fresh per-instance texture {@link Identifier} whenever it momentarily failed to + * find the existing entry, and {@link #tick()} tore the whole entry down on the first frame the + * anchor's BE wasn't found. With shaders that produced a runaway loop: hundreds of decoder + * threads spawned per second for a single screen, the texture id changed every pass (renderer + * logged "Missing resource …/dynamic/"), and the panel never stabilised — i.e. the video + * was invisible for shader users while non-shader users saw it fine. This class is now hardened + * against that: the texture {@link Identifier} is derived deterministically from the + * {@link BlockPos} (so a recreate reuses the same id and the renderer never dangles), creation + * is serialised so concurrent passes can't spawn duplicate decoders, a missing BE is tolerated + * for {@link #MISSING_GRACE_TICKS} before teardown, and a transient upload error no longer kills + * the decoder. */ @Environment(EnvType.CLIENT) public final class VideoPlayback { private VideoPlayback() {} private static final int PLACEHOLDER_SIZE = 32; - private static final Map ENTRIES = new HashMap<>(); + /** ConcurrentHashMap: render-state extraction can run off-thread under shader packs. */ + private static final Map ENTRIES = new ConcurrentHashMap<>(); + /** Serialises the create / url-change path so multiple render passes can't double-spawn. */ + private static final Object LOCK = new Object(); + /** + * Consecutive {@link #tick()} calls an anchor's BE may be absent before we tear its playback + * down. Under shader packs {@code mc.level.getBlockEntity(pos)} can transiently miss during a + * question/anchor swap or an extra render pass; tearing down on the first miss is exactly what + * made the renderer recreate the decoder every pass. Tolerating a short absence (~2 s at 20 + * tps) keeps the audio-on-delete safety net without the churn — a real delete still fires + * {@code BLOCK_ENTITY_UNLOAD} and {@link #stop(BlockPos)} immediately. + */ + private static final int MISSING_GRACE_TICKS = 40; /** * Ensure a playback entry exists for this anchor and return its texture identifier. * Creates a backend + dynamic texture on first call. Returns {@code null} only if the - * URL is empty or autoplay is off. + * URL is empty or autoplay is off. Safe to call many times per frame / from multiple + * render passes: it reuses the existing entry and only ever creates one decoder per + * (pos, url). */ public static Identifier getOrStart(VideoAnchorBlockEntity be) { BlockPos pos = be.getBlockPos(); - Entry e = ENTRIES.get(pos); + String url = be.getUrl(); - if (be.getUrl().isEmpty() || !be.isAutoplay()) { - if (e != null) { - stop(pos); - } + if (url.isEmpty() || !be.isAutoplay()) { + if (ENTRIES.containsKey(pos)) stop(pos); return null; } - if (e != null && e.url.equals(be.getUrl())) { - return e.id; - } - if (e != null) { - stop(pos); + // Fast path: existing entry for the same URL — just reuse it (and clear any pending + // missing-BE grace count, since the renderer clearly still sees this anchor). + Entry existing = ENTRIES.get(pos); + if (existing != null && existing.url.equals(url)) { + existing.missingTicks = 0; + return existing.id; } - VideoBackend backend = WatermediaProbe.isAvailable() ? new WatermediaBackend() : new JavaCvBackend(); - // If /videopreload already cached the URL to disk, hand the local file path to FFmpeg - // instead of the HTTP URL — eliminates the network read entirely. Falls back to the - // live URL when the cache miss or the download hasn't finished yet. The lookup key stays - // the anchor's raw URL (matching how the download published it); only the live URL handed - // to FFmpeg on a miss is percent-encoded, so a non-ASCII path (e.g. ".../음악퀴즈/...") - // streams correctly instead of relying on FFmpeg's lenient raw handling. - Path cached = VideoCache.lookup(be.getUrl()); - String source = cached != null - ? cached.toAbsolutePath().toString() - : VideoCache.encodeUrl(be.getUrl()); - // Diagnostic: record, per anchor, whether this play resolved to the local cache file or - // fell back to a live network stream. A LIVE STREAM line on a video that doesn't show - // means a cache miss (download hadn't finished / failed) and the failure is on the - // network/decoder-open path; a CACHE line that still doesn't show means the local file - // decoded badly. This is the single fact that splits "cache problem" from "decode - // problem" when comparing two clients' logs. - if (cached != null) { - VideoPlayerMod.LOG.info("[{}] play {} at {} -> CACHE {}", - VideoPlayerMod.MOD_ID, be.getUrl(), pos.toShortString(), cached.getFileName()); - } else { - VideoPlayerMod.LOG.info("[{}] play {} at {} -> LIVE STREAM (not cached yet)", - VideoPlayerMod.MOD_ID, be.getUrl(), pos.toShortString()); - } - backend.play(source, be.isLoop()); - backend.setVolume(be.isMuted() ? 0F : be.getVolume()); + // Create / url-change path. Serialise so two render passes (or an off-thread extraction) + // can't both pass the check above and each spawn a decoder + texture for the same anchor. + synchronized (LOCK) { + Entry e = ENTRIES.get(pos); + if (e != null && e.url.equals(url)) { + e.missingTicks = 0; + return e.id; + } + if (e != null) { + stop(pos); // URL genuinely changed (e.g. next quiz video) — replace. + } - Entry created = new Entry(be.getUrl(), backend); - ENTRIES.put(pos, created); - return created.id; + VideoBackend backend = WatermediaProbe.isAvailable() ? new WatermediaBackend() : new JavaCvBackend(); + // If the URL was preloaded to disk, hand the local file path to FFmpeg instead of the + // HTTP URL — eliminates the network read entirely. Falls back to the live URL on a + // cache miss. The lookup key stays the anchor's raw URL (matching how the download + // published it); only the live URL handed to FFmpeg on a miss is percent-encoded, so a + // non-ASCII path (e.g. ".../음악퀴즈/...") streams instead of relying on FFmpeg's lenient + // raw handling. + Path cached = VideoCache.lookup(url); + String source = cached != null + ? cached.toAbsolutePath().toString() + : VideoCache.encodeUrl(url); + if (cached != null) { + VideoPlayerMod.LOG.info("[{}] play {} at {} -> CACHE {}", + VideoPlayerMod.MOD_ID, url, pos.toShortString(), cached.getFileName()); + } else { + VideoPlayerMod.LOG.info("[{}] play {} at {} -> LIVE STREAM (not cached yet)", + VideoPlayerMod.MOD_ID, url, pos.toShortString()); + } + backend.play(source, be.isLoop()); + backend.setVolume(be.isMuted() ? 0F : be.getVolume()); + + Entry created = new Entry(pos, url, backend); + ENTRIES.put(pos, created); + return created.id; + } } public static Identifier currentTexture(BlockPos pos) { @@ -110,7 +146,7 @@ public final class VideoPlayback { e.backend.setVolume(be.isMuted() ? 0F : be.getVolume()); } - /** Called every client tick to upload new frames into the GPU texture. */ + /** Called every client tick / render frame to upload new frames into the GPU texture. */ public static void tick() { Minecraft mc = Minecraft.getInstance(); if (mc == null) return; @@ -120,21 +156,30 @@ public final class VideoPlayback { BlockPos pos = me.getKey(); Entry e = me.getValue(); // Belt-and-suspenders for the audio-on-delete bug: if BLOCK_ENTITY_UNLOAD didn't - // fire for some edge case (dimension change, chunk torn down before event runs, - // etc.), the BE will be gone from the level but our Entry still holds an open - // audio line. Catch it here and stop next tick. + // fire for some edge case the BE may be gone from the level while our Entry still + // holds an open audio line. But under shader packs getBlockEntity can transiently + // miss for a frame or two, so we only tear down after MISSING_GRACE_TICKS consecutive + // misses — otherwise we'd nuke and respawn the decoder every render pass. if (mc.level == null || !(mc.level.getBlockEntity(pos) instanceof VideoAnchorBlockEntity)) { - e.close(); - it.remove(); + if (++e.missingTicks > MISSING_GRACE_TICKS) { + e.close(); + it.remove(); + } continue; } + e.missingTicks = 0; if (!e.backend.isReady()) continue; try { e.tryUpload(); } catch (Throwable t) { - VideoPlayerMod.LOG.warn("[{}] texture upload failed: {}", VideoPlayerMod.MOD_ID, t.toString()); - e.close(); - it.remove(); + // Do NOT tear the decoder down on a transient upload error — that was the trigger + // for the shader-pack recreate storm. Keep playing; log the real exception once so + // a genuinely fatal upload problem is still diagnosable. + if (!e.uploadErrorLogged) { + VideoPlayerMod.LOG.warn("[{}] texture upload error (decoder kept alive): {}", + VideoPlayerMod.MOD_ID, t.toString()); + e.uploadErrorLogged = true; + } } } } @@ -161,18 +206,38 @@ public final class VideoPlayback { DynamicTexture texture; int texW = 0, texH = 0; boolean registered = false; + /** Consecutive ticks the BE has been missing (see {@link #MISSING_GRACE_TICKS}). */ + volatile int missingTicks = 0; + /** One-shot guard so a per-frame upload failure logs once, not every frame. */ + boolean uploadErrorLogged = false; - Entry(String url, VideoBackend backend) { + Entry(BlockPos pos, String url, VideoBackend backend) { this.url = url; this.backend = backend; - String tag = Integer.toHexString(System.identityHashCode(this)); - this.id = Identifier.fromNamespaceAndPath(VideoPlayerMod.MOD_ID, "dynamic/" + tag); + // Deterministic id from the block position (not System.identityHashCode of this + // object). If the entry is ever recreated for the same anchor, the renderer's cached + // texture Identifier stays valid and TextureManager keeps a texture registered under + // it — so a recreate can never leave the renderer binding a released id ("Missing + // resource …/dynamic/"), which was the visible shader-pack breakage. + this.id = Identifier.fromNamespaceAndPath(VideoPlayerMod.MOD_ID, + "dynamic/" + Long.toHexString(pos.asLong())); ensureTexture(PLACEHOLDER_SIZE, PLACEHOLDER_SIZE, true); } /** Allocate or resize the dynamic texture, registering it on first allocation. */ private void ensureTexture(int w, int h, boolean fillPlaceholder) { - if (texture != null && w == texW && h == texH) return; + if (texture != null && w == texW && h == texH) { + // Re-register defensively in case the id was released by a prior teardown for this + // same position (deterministic ids are shared across an anchor's lifetime). + if (!registered) { + Minecraft mc = Minecraft.getInstance(); + if (mc != null) { + mc.getTextureManager().register(id, texture); + registered = true; + } + } + return; + } if (texture != null) texture.close(); NativeImage img = new NativeImage(w, h, false); if (fillPlaceholder) {