amadan

oficina / meet Public

room.js: the frame counter survives a stream being replaced

The second thing the new gate found, and the more alarming of the two:
`laterolls` reported a camera that had stopped arriving, on a tile
whose own video element was playing perfectly — readyState 4, not
paused, currentTime advancing, 452 frames decoded and rising. What had
stopped was the counter.

Setting `srcObject` resets a media element and discards any pending
requestVideoFrameCallback. `watchFrames` kept a `WeakSet` of elements,
so a tile handed a *new* stream — a rebuilt pair, or a camera the far
side rebuilt after exactly the rollback this branch is about — lost its
chain and was refused another. The count then stood still for the rest
of the call, which every frames assertion in the browser gate reads as
dead media, and which `state().frames` reports to anything else that
asks.

Keyed by element *and* stream now, through a WeakMap: re-arm when
either changed, leave a live chain alone, and let the old chain retire
itself the moment it is no longer the current one so two counters never
race the same tile up. A chain that finds its tile detached forgets
itself, and `tile()` asks on every render rather than only when the
stream changes, so the next render puts it back.

Five tests in room-share.test.mjs, mutation-verified: restoring the
element-only guard fails the two that are about a replaced stream, and
only those.

node --check, 187 node tests, make ci.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
Paul Campbell pushed by paul@keymail.dev 3ecebb615a2729acfa4e652744cf4ff992490b35 parent d8316e8
2 files changed, +151 −15
  • internal/meet/static/room.js +38 −15
  • test/browser/room-share.test.mjs +113 −0
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js
index 2e87e07..9651507 100644
--- a/internal/meet/static/room.js
+++ b/internal/meet/static/room.js
@@ -3494,14 +3494,15 @@
// URL, and createObjectURL is the wrong tool here anyway.
t.video.srcObject = stream || null;
t.stream = stream || null;
- if (stream && !isSelf) {
- // The frame counter follows the stream, not the element: a
- // tile whose track arrives after it was drawn must still be
- // counted, and a rebuilt pair hands the same element a new one.
- watchFrames(key, t.video);
- startPlaying(key, t);
- }
- }
+ if (stream && !isSelf) startPlaying(key, t);
+ }
+ // The frame counter follows the stream, not the element: a tile
+ // whose track arrives after it was drawn must still be counted, and
+ // a rebuilt pair hands the same element a new one. Asked on every
+ // render rather than only when the stream changes, because a chain
+ // can also end on its own — an element detached while the grid was
+ // rebuilt — and this is what puts it back.
+ if (stream && !isSelf) watchFrames(key, t.video);
// One note element and one Retry beside it, whichever sentence the
// tile is carrying: `restartIce` is the handler for both, and it
// decides whether a retry means a bare ICE restart or a renewal.
@@ -3744,16 +3745,38 @@
const HAS_RVFC = typeof HTMLVideoElement !== 'undefined' &&
typeof HTMLVideoElement.prototype.requestVideoFrameCallback === 'function';
- // watchFrames counts rendered frames per peer. One chain per element:
- // a tile whose stream is replaced must not end up with two counters
- // racing each other up.
- const watched = new WeakSet();
+ // watchFrames counts rendered frames per peer. One chain per element
+ // *and* per stream, which the first draft got wrong and the browser
+ // gate's `laterolls` caught: setting `srcObject` resets the media
+ // element and discards any pending `requestVideoFrameCallback`, so a
+ // tile handed a new stream — a rebuilt pair, or a camera the far side
+ // rebuilt after a rollback — lost its chain, and a `WeakSet` of
+ // elements then refused to arm a new one. The tile went on playing
+ // perfectly and its count stood still for the rest of the call, which
+ // is a lie the gate reads as dead media.
+ //
+ // A WeakMap of what each element is currently being counted on says
+ // both things: re-arm when the stream (or the tile) changed, and
+ // leave a live chain alone. The old chain retires itself the moment
+ // it is no longer the current one, so two counters never race the
+ // same tile up. A chain that finds its element detached forgets
+ // itself, so the next render arms a fresh one rather than counting
+ // nothing for ever.
+ const watched = new WeakMap(); // video -> {id, stream}
function watchFrames(id, video) {
- if (!HAS_RVFC || watched.has(video)) return;
- watched.add(video);
+ if (!HAS_RVFC) return;
+ const stream = video.srcObject;
+ const current = watched.get(video);
+ if (current && current.id === id && current.stream === stream) return;
+ const mine = { id, stream };
+ watched.set(video, mine);
const step = () => {
- if (!video.isConnected) return;
+ if (watched.get(video) !== mine) return;
+ if (!video.isConnected) {
+ watched.delete(video);
+ return;
+ }
frames.set(id, (frames.get(id) || 0) + 1);
video.requestVideoFrameCallback(step);
};
diff --git a/test/browser/room-share.test.mjs b/test/browser/room-share.test.mjs
index 8efd41f..00de110 100644
--- a/test/browser/room-share.test.mjs
+++ b/test/browser/room-share.test.mjs
@@ -376,3 +376,116 @@ test('signed out is read before the status, because it can be true of a 200', ()
assert.equal(broadcastVerdict(answer(401, { signedOut: true })), 'signedout');
assert.equal(accepted('signedout'), false);
});
+
+// ---------------------------------------------------------------
+// watchFrames: the counter the whole browser gate stands on
+// ---------------------------------------------------------------
+//
+// Every frames assertion in call.mjs reads this counter, so a counter
+// that can die quietly makes the gate able to report dead media on a
+// tile that is playing perfectly — which is what `laterolls` found.
+// Setting `srcObject` resets the media element and discards any pending
+// requestVideoFrameCallback, so a tile handed a new stream (a rebuilt
+// pair, or a camera the far side rebuilt after a rollback) lost its
+// chain, and the WeakSet of elements it used to keep then refused to
+// arm another one.
+
+const framesBox = new Function(
+ 'frames', 'HAS_RVFC',
+ 'const watched = new WeakMap();\n' + lift(src, 'watchFrames') +
+ '\nreturn { watchFrames };',
+);
+
+// A video element as watchFrames sees one: a stream, a connectedness,
+// and one pending frame callback at a time.
+function fakeVideo(stream) {
+ return {
+ srcObject: stream,
+ isConnected: true,
+ pending: null,
+ requestVideoFrameCallback(fn) {
+ this.pending = fn;
+ },
+ // frame() delivers one rendered frame to whatever chain is armed.
+ frame() {
+ const fn = this.pending;
+ this.pending = null;
+ if (fn) fn();
+ return this;
+ },
+ };
+}
+
+function counter() {
+ const frames = new Map();
+ return { frames, watchFrames: framesBox(frames, true).watchFrames };
+}
+
+test('one chain per tile: arming twice does not count twice', () => {
+ const c = counter();
+ const video = fakeVideo({ id: 's1' });
+ c.watchFrames('p1', video);
+ c.watchFrames('p1', video);
+ video.frame().frame().frame();
+ assert.equal(c.frames.get('p1'), 3, 'two chains raced the same tile up');
+});
+
+test('a tile handed a new stream starts counting again', () => {
+ // The defect itself. The element is the same one; the stream is not.
+ const c = counter();
+ const video = fakeVideo({ id: 's1' });
+ c.watchFrames('p1', video);
+ video.frame().frame();
+ assert.equal(c.frames.get('p1'), 2);
+
+ // What `srcObject = stream` does: the element resets, and the pending
+ // frame callback goes with it.
+ video.srcObject = { id: 's2' };
+ video.pending = null;
+ c.watchFrames('p1', video);
+ video.frame().frame().frame();
+ assert.equal(c.frames.get('p1'), 5,
+ 'the tile stopped counting when its stream was replaced');
+});
+
+test('a chain whose tile was detached is armed again on the next render', () => {
+ const c = counter();
+ const video = fakeVideo({ id: 's1' });
+ c.watchFrames('p1', video);
+ video.frame();
+ assert.equal(c.frames.get('p1'), 1);
+
+ // The grid was rebuilt under it.
+ video.isConnected = false;
+ video.frame();
+ assert.equal(c.frames.get('p1'), 1, 'a detached tile went on counting');
+
+ video.isConnected = true;
+ c.watchFrames('p1', video);
+ video.frame().frame();
+ assert.equal(c.frames.get('p1'), 3, 'the tile never started counting again');
+});
+
+test('a tile that changed peer counts under the new key only', () => {
+ const c = counter();
+ const video = fakeVideo({ id: 's1' });
+ c.watchFrames('p1', video);
+ video.frame();
+ video.srcObject = { id: 's2' };
+ video.pending = null;
+ c.watchFrames('p2', video);
+ video.frame().frame();
+ assert.equal(c.frames.get('p1'), 1);
+ assert.equal(c.frames.get('p2'), 2);
+});
+
+test('a browser without requestVideoFrameCallback counts nothing here', () => {
+ // pollFrames is that browser's counter, and arming this one would be a
+ // second one racing it.
+ const frames = new Map();
+ const { watchFrames } = framesBox(frames, false);
+ const video = fakeVideo({ id: 's1' });
+ watchFrames('p1', video);
+ assert.equal(video.pending, null);
+ assert.equal(frames.size, 0);
+});