mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 22:03:13 -06:00
fix(#146): web player — no-change refresh loses video (keeps audio); re-attach idempotently
ROOT CAUSE (hypothesis A, pre-existing — NOT a beta7 regression; server/player/index.html is untouched since v1.9.2-beta6): handlePlaylistUpdate's "Playlist unchanged" branch blindly returned. The media re-attach (renderContent) lives ONLY in the content-changed branch, so if the <video> surface was lost (element detached from the DOM while still decoding — video gone, audio still playing) a no-new-content refresh never re-attached it. New-content refreshes were fine because they re-render. FIX (make the refresh idempotent for the media surface, no flicker on the healthy path): - server/lib/player-media-health.js (new, UMD + unit-testable, mirrors schedule-eval.js): needsReattach(state) — re-attach ONLY when playback should be happening but the current item's surface is actually lost (video null / detached / ended / errored; non-video: no mounted surface). A healthy attached+live video returns false, so a routine poll stays a no-op (no re-render, no flicker). Served at /player/player-media-health.js from the single source; loaded by the player. - index.html no-change branch: extract the current item's DOM facts and, iff PlayerMediaHealth.needsReattach, call playCurrentItem() to re-render the current item. Wrapped so the health check can never break a refresh. - teardownCurrentMedia: also release currentVideoEl even when it was DETACHED from the container — a detached-but-playing <video> keeps emitting audio and the container-scoped querySelectorAll can't find it. This kills the "ghost audio" on re-attach. - sw.js cache bumped v9 -> v10 so players pick up the new index.html + module. Tests: test/player-media-health.test.js (6) exercises the branch selection — healthy video -> no re-attach; detached/null/ended/errored -> re-attach; idle -> never; non-video by surface presence. Inline player JS syntax-checked; module served + referenced verified on a booted server. Suite 316/316. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
385eda3cb1
commit
26c72d62bf
48
server/lib/player-media-health.js
Normal file
48
server/lib/player-media-health.js
Normal file
|
|
@ -0,0 +1,48 @@
|
|||
// Player media-surface health decision (#146 web-player fix).
|
||||
//
|
||||
// THE BUG (hypothesis A): a NO-NEW-CONTENT refresh in handlePlaylistUpdate returned early
|
||||
// ("Playlist unchanged") without verifying the media surface is still attached. If the
|
||||
// <video> element had been detached from the DOM while still decoding (audio keeps playing,
|
||||
// video surface gone), the re-attach — which lived ONLY in the content-changed branch —
|
||||
// never ran, so the video never came back. This module is the branch-selection decision the
|
||||
// no-change path now consults: re-attach ONLY when playback should be happening but the
|
||||
// surface is actually lost, so a healthy poll stays a no-op (no flicker every refresh).
|
||||
//
|
||||
// Pure + dependency-free so it is unit-testable without a DOM: the caller extracts the DOM
|
||||
// facts (is the <video> in the document? ended? errored?) into a plain state object.
|
||||
//
|
||||
// Dependency-free UMD: Node (require) + browser/Tizen (window.PlayerMediaHealth).
|
||||
(function (root, factory) {
|
||||
if (typeof module === 'object' && module.exports) module.exports = factory();
|
||||
else root.PlayerMediaHealth = factory();
|
||||
})(typeof self !== 'undefined' ? self : this, function () {
|
||||
'use strict';
|
||||
|
||||
// state = {
|
||||
// isPlaying: boolean // the player believes an item is playing
|
||||
// hasCurrentItem: boolean // playlist[currentIndex] exists
|
||||
// itemKind: 'video' | 'youtube' | 'image' | 'widget' | 'other'
|
||||
// videoEl: { attached, ended, errored } | null // for a plain <video> item
|
||||
// surfaceAttached:boolean // for non-video: a rendered surface is present in the DOM
|
||||
// }
|
||||
// Returns true iff the no-change refresh must re-render/re-attach the current item.
|
||||
function needsReattach(state) {
|
||||
var s = state || {};
|
||||
// Idle or no content: nothing to re-attach — leave the idle/waiting screen alone.
|
||||
if (!s.isPlaying || !s.hasCurrentItem) return false;
|
||||
|
||||
if (s.itemKind === 'video') {
|
||||
// The exact bug: a <video> that is gone or detached from the DOM (its element may
|
||||
// still be emitting audio) — or one that ended/errored — must be re-attached.
|
||||
if (!s.videoEl) return true;
|
||||
if (!s.videoEl.attached) return true;
|
||||
if (s.videoEl.ended || s.videoEl.errored) return true;
|
||||
return false; // attached + live -> healthy, do NOT re-render (avoids flicker)
|
||||
}
|
||||
|
||||
// Non-video surfaces (image / youtube iframe / widget): healthy iff a surface is mounted.
|
||||
return !s.surfaceAttached;
|
||||
}
|
||||
|
||||
return { needsReattach: needsReattach };
|
||||
});
|
||||
|
|
@ -232,6 +232,7 @@
|
|||
|
||||
<script src="/socket.io/socket.io.js"></script>
|
||||
<script src="/player/schedule-eval.js"></script>
|
||||
<script src="/player/player-media-health.js"></script>
|
||||
<script>
|
||||
// ==================== i18n ====================
|
||||
// Lightweight inline i18n for the player. The player is a standalone page
|
||||
|
|
@ -1307,6 +1308,33 @@
|
|||
|
||||
if (newFp === oldFp && playlist.length > 0 && !wallChanged) {
|
||||
console.log('Playlist unchanged');
|
||||
// #146 fix: a no-change refresh used to blindly return — so if the <video> surface
|
||||
// had been lost (detached from the DOM while its element kept decoding audio: video
|
||||
// gone, audio still playing), the re-attach (which lives ONLY in the content-changed
|
||||
// branch below) never ran. Re-render the CURRENT item, but ONLY when the surface is
|
||||
// actually unhealthy, so a healthy poll stays a no-op (no flicker on every refresh).
|
||||
try {
|
||||
const item = playlist[currentIndex];
|
||||
const kind = !item ? 'other'
|
||||
: item.widget_id ? 'widget'
|
||||
: item.mime_type === 'video/youtube' ? 'youtube'
|
||||
: (typeof item.mime_type === 'string' && item.mime_type.startsWith('video/')) ? 'video'
|
||||
: (typeof item.mime_type === 'string' && item.mime_type.startsWith('image/')) ? 'image' : 'other';
|
||||
const container = document.getElementById('playerContainer');
|
||||
const state = {
|
||||
isPlaying: isPlaying,
|
||||
hasCurrentItem: !!item,
|
||||
itemKind: kind,
|
||||
videoEl: currentVideoEl
|
||||
? { attached: document.contains(currentVideoEl), ended: !!currentVideoEl.ended, errored: !!currentVideoEl.error }
|
||||
: null,
|
||||
surfaceAttached: !!(container && container.querySelector('video,img,iframe,.wall-stage')),
|
||||
};
|
||||
if (window.PlayerMediaHealth && PlayerMediaHealth.needsReattach(state)) {
|
||||
console.log('[refresh] media surface lost on no-change refresh — re-attaching current item');
|
||||
playCurrentItem();
|
||||
}
|
||||
} catch (e) { /* never let the health check break a refresh */ }
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -1607,6 +1635,17 @@
|
|||
});
|
||||
container.innerHTML = '';
|
||||
}
|
||||
// #146 fix: also release currentVideoEl even if it was DETACHED from the container —
|
||||
// a detached-but-playing <video> keeps emitting audio and the querySelectorAll above
|
||||
// (scoped to the container) can't find it. This is what kills the "ghost audio".
|
||||
if (currentVideoEl) {
|
||||
try {
|
||||
currentVideoEl.onended = null; currentVideoEl.onerror = null; currentVideoEl.onloadeddata = null;
|
||||
currentVideoEl.pause();
|
||||
currentVideoEl.removeAttribute('src');
|
||||
currentVideoEl.load();
|
||||
} catch (e) { /* element may already be gone */ }
|
||||
}
|
||||
currentVideoEl = null;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
const CACHE_NAME = 'rd-player-v9';
|
||||
const CACHE_NAME = 'rd-player-v10';
|
||||
|
||||
// Install: skip waiting to activate immediately
|
||||
self.addEventListener('install', (event) => {
|
||||
|
|
|
|||
|
|
@ -276,6 +276,13 @@ app.get('/player/schedule-eval.js', (req, res) => {
|
|||
res.sendFile(path.join(__dirname, 'lib', 'schedule-eval.js'));
|
||||
});
|
||||
|
||||
// #146 web-player fix: serve the media-surface health decision from its single source
|
||||
// (server/lib/player-media-health.js) so the player and the Node test can't drift.
|
||||
app.get('/player/player-media-health.js', (req, res) => {
|
||||
res.type('application/javascript').setHeader('Cache-Control', 'no-cache');
|
||||
res.sendFile(path.join(__dirname, 'lib', 'player-media-health.js'));
|
||||
});
|
||||
|
||||
// Serve web player at /player (same no-cache for JS/HTML). The index.html
|
||||
// route above intercepts the HTML requests; everything else still falls
|
||||
// through to this static handler (debug-overlay.js, sw.js, manifest, etc).
|
||||
|
|
|
|||
42
server/test/player-media-health.test.js
Normal file
42
server/test/player-media-health.test.js
Normal file
|
|
@ -0,0 +1,42 @@
|
|||
'use strict';
|
||||
|
||||
// #146 web-player fix — the no-change-refresh branch-selection decision. This is the unit
|
||||
// the refresh handler consults to decide whether to re-attach the media surface. It is the
|
||||
// smallest testable piece of the "no new content lost the video but not the audio" fix.
|
||||
|
||||
const { test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { needsReattach } = require('../lib/player-media-health');
|
||||
|
||||
const video = (o) => ({ isPlaying: true, hasCurrentItem: true, itemKind: 'video', videoEl: o, surfaceAttached: true });
|
||||
|
||||
test('healthy attached+live video on a no-change refresh -> NO re-attach (no flicker)', () => {
|
||||
assert.equal(needsReattach(video({ attached: true, ended: false, errored: false })), false);
|
||||
});
|
||||
|
||||
test('THE BUG: a detached-but-playing <video> (audio persists, surface gone) -> re-attach', () => {
|
||||
assert.equal(needsReattach(video({ attached: false, ended: false, errored: false })), true);
|
||||
});
|
||||
|
||||
test('video element gone entirely -> re-attach', () => {
|
||||
assert.equal(needsReattach({ isPlaying: true, hasCurrentItem: true, itemKind: 'video', videoEl: null }), true);
|
||||
});
|
||||
|
||||
test('ended or errored video -> re-attach', () => {
|
||||
assert.equal(needsReattach(video({ attached: true, ended: true, errored: false })), true);
|
||||
assert.equal(needsReattach(video({ attached: true, ended: false, errored: true })), true);
|
||||
});
|
||||
|
||||
test('idle / no current item -> never re-attach (leave the waiting screen)', () => {
|
||||
assert.equal(needsReattach({ isPlaying: false, hasCurrentItem: true, itemKind: 'video', videoEl: null }), false);
|
||||
assert.equal(needsReattach({ isPlaying: true, hasCurrentItem: false, itemKind: 'video', videoEl: null }), false);
|
||||
assert.equal(needsReattach(undefined), false);
|
||||
});
|
||||
|
||||
test('non-video surface (image/youtube/widget): re-attach only when the surface is missing', () => {
|
||||
const base = { isPlaying: true, hasCurrentItem: true };
|
||||
assert.equal(needsReattach({ ...base, itemKind: 'image', surfaceAttached: true }), false);
|
||||
assert.equal(needsReattach({ ...base, itemKind: 'image', surfaceAttached: false }), true);
|
||||
assert.equal(needsReattach({ ...base, itemKind: 'youtube', surfaceAttached: false }), true);
|
||||
assert.equal(needsReattach({ ...base, itemKind: 'widget', surfaceAttached: true }), false);
|
||||
});
|
||||
Loading…
Reference in a new issue