amadan
room.js: a tile follows the role's own track, not the stream's first
Codex S4-9 (P2), and the leading explanation of the receiving-side
failure `laterolls` reproduces.
`adoptTrack` pruned only tracks marked `ended`. A receiver can be
superseded while its track is still `live` — it simply never carries
another frame — so a camera rebuilt by a rollback, arriving on the mid
the camera is already known by and often with no stream of its own,
left the stream holding [old, new]. A media element renders the first
track of its kind, `tile()` saw neither a changed stream nor a changed
first video track, and the face never moved again.
Three changes, one rule: the role owns what it shows.
- Each role — camera, screen — keeps a stream of this file's own for
the life of the entry, and the remote stream is read for its id
alone. A remote stream is a thing the far side composes; it can carry
the old track and the new one at once, in whatever order.
- `adoptTrack` makes the arriving track *the* track of its kind in that
stream: every other one of that kind comes out, whatever its
readyState, and this one goes in. Audio is a kind of its own, so a
camera replacing a camera leaves the microphone alone.
- `renderTiles` names the track each tile is meant to show
(`entry.cameraTrack`, `entry.share.track`) and `tile()` binds to it,
falling back to the stream's first video track only for a tile whose
role has never named one. No element follows an array position now.
Four tests, all four seen failing against the shipped source first —
the same-mid replacement with the live predecessor reproduced Codex's
`cameraTracks: [old, new]`, `shownTrack: old` exactly. The unit test
that used to assert the old rule ("a live track already there is left
alone") asserted the defect, and says the opposite now.
node --check, 197 node tests, make ci.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzvnuKhLzudvq7jZ7SvVt2
2 files changed,
+182
−31
internal/meet/static/room.js+60 −24test/browser/room-share.test.mjs+122 −7
diff --git a/internal/meet/static/room.js b/internal/meet/static/room.js| index 5b5b48d..288b662 100644 |
| --- a/internal/meet/static/room.js |
| +++ b/internal/meet/static/room.js |
| @@ -3056,9 +3056,18 @@ |
| if (what === 'share') { |
| entry.sharing = true; |
| entry.shareMid = mid || entry.shareMid; |
| - const held = (entry.share && entry.share.stream) || null; |
| - entry.share = { stream: stream || held || new MediaStream(), track }; |
| - adoptTrack(entry.share.stream, track); |
| + // The stream a tile shows is this file's own, kept for the life |
| + // of the entry, and what goes in it is decided here (Codex S4-9). |
| + // A remote stream is a thing the far side composes: it can carry |
| + // the old track and the new one at once, in whatever order, and |
| + // binding a tile to "its first video track" then shows whichever |
| + // happens to be first. So the role owns a stream, the role's |
| + // authoritative track goes in it, and nothing else of that kind |
| + // stays behind. |
| + const held = (entry.share && entry.share.stream) || new MediaStream(); |
| + entry.share = { stream: held, track }; |
| + entry.shareTrack = track; |
| + adoptTrack(held, track); |
| // §5.4's first ending: the sender stopped the transceiver, so |
| // this track ends rather than freezing. The listener is dropped |
| // with the connection, and dropShare is idempotent, so the `share |
| @@ -3075,35 +3084,51 @@ |
| // what every later video track is judged against. |
| if (mid) entry.cameraMid = mid; |
| if (stream) entry.cameraStream = stream.id; |
| + // The authoritative camera track, which is what the tile binds |
| + // to. A replacement arriving on the same mid supersedes its |
| + // predecessor whether or not the predecessor has ended. |
| + entry.cameraTrack = track; |
| } |
| // Recorded from the first track of any kind, camera or microphone: |
| // it is what places a video track for a peer that has no camera. |
| if (stream && !entry.peerStream) entry.peerStream = stream.id; |
| - entry.stream = stream || entry.stream || new MediaStream(); |
| + // This tile's own stream, again: the remote one is only ever read |
| + // for its id. Audio and video are kept apart by `adoptTrack`, so a |
| + // camera replacing a camera leaves the microphone alone. |
| + entry.stream = entry.stream || new MediaStream(); |
| adoptTrack(entry.stream, track); |
| renderTiles(); |
| } |
| - // adoptTrack puts a newly arrived track into the stream a tile shows, |
| - // and takes out any ended track of the same kind that was there |
| - // before. Pure enough to lift by name, and gated in the node test. |
| + // adoptTrack makes one arriving track *the* track of its kind in the |
| + // stream a tile shows: every other track of that kind comes out, |
| + // whatever its readyState, and this one goes in. Pure enough to lift |
| + // by name, and gated in the node test. |
| // |
| - // This is the receiving half of the rollback damage (Codex S4-5's |
| - // other side, found by the gate's `laterolls`). Chromium rebuilds |
| - // every stream on a connection when it rolls back an offer that added |
| - // an m-line — receive streams included — so the far side's camera |
| - // arrives again as a *new* track, sometimes carrying no stream of its |
| - // own and sometimes carried inside the very stream that still holds |
| - // the old, ended one. A media element renders the first track of its |
| - // kind, so an ended track left in place is a face that never moves |
| - // again on a connection that is otherwise perfect: connected, stable, |
| - // sendrecv, and decoding hundreds of frames nobody can see. |
| + // This is the receiving half of the rollback damage, and its first |
| + // draft got the rule wrong (Codex S4-9). Chromium rebuilds every |
| + // stream on a connection when it rolls back an offer that added an |
| + // m-line — receive streams included — so the far side's camera |
| + // arrives again as a *new* track, sometimes with no stream of its own |
| + // and sometimes inside the very stream that still holds the old one. |
| + // The old one is not necessarily `ended`: a receiver can be |
| + // superseded while its track is still live and simply never carries |
| + // another frame. Pruning only ended tracks therefore left the stream |
| + // holding [old, new], and a media element renders the first track of |
| + // its kind — a face that never moves again on a connection that is |
| + // otherwise perfect: connected, stable, sendrecv, and decoding |
| + // hundreds of frames nobody can see. |
| + // |
| + // Which track is authoritative is `onRemoteTrack`'s judgement, not |
| + // this function's: classifyTrack has already said whether this is the |
| + // camera or the screen, and each role owns a stream of its own. So |
| + // there is no ordering question left here, and none of "is the old |
| + // one finished with" either. Audio is a kind of its own and is |
| + // untouched by a video track arriving. |
| function adoptTrack(stream, track) { |
| if (!stream || !track) return stream; |
| for (const old of stream.getTracks()) { |
| - if (old !== track && old.kind === track.kind && old.readyState === 'ended') { |
| - stream.removeTrack(old); |
| - } |
| + if (old !== track && old.kind === track.kind) stream.removeTrack(old); |
| } |
| if (!stream.getTracks().includes(track)) stream.addTrack(track); |
| return stream; |
| @@ -3433,12 +3458,19 @@ |
| if (local) tile('self', selfName, local, true, ''); |
| for (const [id, entry] of pcs) { |
| const name = names.get(id) || 'Someone'; |
| - tile(id, name, entry.stream, false, tileNote(id, entry)); |
| + // The track as well as the stream: which video track a tile is |
| + // meant to be showing is `onRemoteTrack`'s judgement, and passing |
| + // it here is what keeps the element off a superseded one (Codex |
| + // S4-9). Undefined for a tile whose role has never named one, in |
| + // which case tile() falls back to the stream's own first video |
| + // track, which is unambiguous by then. |
| + tile(id, name, entry.stream, false, tileNote(id, entry), entry.cameraTrack); |
| // The screen's own tile carries no note: §5.3's and §5.5's |
| // sentences are about a connection, and this is the same |
| // connection as the camera tile above, which is already saying it. |
| if (hasShare(entry)) { |
| - tile(shareKey(id), shareCaption(name), entry.share && entry.share.stream, false, ''); |
| + tile(shareKey(id), shareCaption(name), entry.share && entry.share.stream, false, '', |
| + entry.share && entry.share.track); |
| } |
| } |
| renderStatus(); |
| @@ -3467,7 +3499,7 @@ |
| // uses innerHTML, and every name is written with textContent: names |
| // come from the server's roster, peer and knock events and are other |
| // people's text. |
| - function tile(key, name, stream, isSelf, noteText) { |
| + function tile(key, name, stream, isSelf, noteText, want) { |
| let t = tiles.get(key); |
| if (!t) { |
| const figure = document.createElement('figure'); |
| @@ -3534,7 +3566,11 @@ |
| // watched only the object would keep an element pointed at a track |
| // that has ended. Nulling srcObject first is what makes the element |
| // let go of it — assigning the same object again is a no-op. |
| - const shown = shownTrack(stream); |
| + // `want` is the track this tile is meant to be showing, named by |
| + // whoever put it in the stream; the stream's own first video track |
| + // is the fallback for a tile nobody has named one for. Either way |
| + // the element follows a track identity and not an array position. |
| + const shown = want || shownTrack(stream); |
| if (t.stream !== stream || t.track !== shown) { |
| // srcObject only: media-src in the page's CSP would block a blob: |
| // URL, and createObjectURL is the wrong tool here anyway. |
diff --git a/test/browser/room-share.test.mjs b/test/browser/room-share.test.mjs| index d737678..67c5055 100644 |
| --- a/test/browser/room-share.test.mjs |
| +++ b/test/browser/room-share.test.mjs |
| @@ -406,7 +406,21 @@ function mediaStream(tracks) { |
| }; |
| } |
| -const videoTrack = (id, readyState) => ({ id, kind: 'video', readyState: readyState || 'live' }); |
| +// A track as room.js handles one: the share path listens for its |
| +// ending, so the fake has to be able to be listened to. |
| +const videoTrack = (id, readyState) => ({ |
| + id, |
| + kind: 'video', |
| + readyState: readyState || 'live', |
| + listeners: {}, |
| + addEventListener(type, fn) { |
| + (this.listeners[type] = this.listeners[type] || []).push(fn); |
| + }, |
| + end() { |
| + this.readyState = 'ended'; |
| + for (const fn of this.listeners.ended || []) fn(); |
| + }, |
| +}); |
| test('a rebuilt camera replaces the ended one it arrives beside', () => { |
| const dead = videoTrack('v1', 'ended'); |
| @@ -419,16 +433,21 @@ test('a rebuilt camera replaces the ended one it arrives beside', () => { |
| assert.equal(shownTrack(stream), live); |
| }); |
| -test('a live track already there is left alone', () => { |
| - // Two live video tracks on one stream is not this function's business |
| - // to resolve: a camera and something else are told apart by |
| - // classifyTrack long before this. |
| +test('a replacement supersedes a live predecessor, not only an ended one', () => { |
| + // The rule this function used to have, and the defect under it |
| + // (Codex S4-9). A receiver can be superseded while its track is still |
| + // `live`: it simply never carries another frame. Pruning only ended |
| + // tracks left the stream holding [old, new], and an element renders |
| + // the first track of its kind — the old one, for the rest of the |
| + // call. Which track is authoritative is settled before this is |
| + // called; there is no ordering question left for it to get wrong. |
| const first = videoTrack('v1'); |
| const second = videoTrack('v2'); |
| const stream = mediaStream([first]); |
| adoptTrack(stream, second); |
| - assert.deepEqual(stream.getVideoTracks(), [first, second]); |
| - assert.equal(shownTrack(stream), first); |
| + assert.deepEqual(stream.getVideoTracks(), [second], |
| + 'the superseded track is still in the stream the tile shows'); |
| + assert.equal(shownTrack(stream), second); |
| }); |
| test('adopting a track twice adds it once', () => { |
| @@ -439,6 +458,102 @@ test('adopting a track twice adds it once', () => { |
| assert.deepEqual(stream.getTracks(), [track]); |
| }); |
| +// onRemoteTrack, driven whole: the same-mid replacement regression |
| +// (Codex S4-9). A camera rebuilt by a rollback arrives as a new track |
| +// on the mid the camera is already known by, often with no stream of |
| +// its own, and the old track can still be `live`. What the tile ends up |
| +// bound to is the whole question. |
| + |
| +const remoteBox = new Function( |
| + 'pcs', 'renderTiles', 'dropShare', 'MediaStream', 'SHARE_SUFFIX', |
| + [lift(src, 'onRemoteTrack'), lift(src, 'classifyTrack'), lift(src, 'adoptTrack'), |
| + lift(src, 'shownTrack'), lift(src, 'dropShare')].join('\n') + |
| + '\nreturn { onRemoteTrack, shownTrack };', |
| +); |
| + |
| +// arriving builds the `track` event as onRemoteTrack reads one. |
| +function arriving(track, opts) { |
| + const o = opts || {}; |
| + return { |
| + track, |
| + streams: o.stream ? [o.stream] : [], |
| + transceiver: { mid: o.mid === undefined ? '1' : o.mid, currentDirection: o.direction || 'sendrecv' }, |
| + }; |
| +} |
| + |
| +function receiving() { |
| + const renders = []; |
| + const pcs = new Map(); |
| + const entry = { |
| + sentVideo: true, |
| + stream: null, |
| + share: null, |
| + sharing: false, |
| + cameraMid: '', |
| + cameraStream: '', |
| + peerStream: '', |
| + shareMid: '', |
| + }; |
| + pcs.set('p1', entry); |
| + const box = remoteBox( |
| + pcs, |
| + () => renders.push('tiles'), |
| + () => {}, |
| + function FakeMediaStream() { return mediaStream([]); }, |
| + '#share', |
| + ); |
| + return { entry, pcs, renders, onRemoteTrack: box.onRemoteTrack, shownTrack: box.shownTrack }; |
| +} |
| + |
| +test('a camera rebuilt on the same mid replaces the one before it', () => { |
| + const r = receiving(); |
| + const first = videoTrack('cam1'); |
| + r.onRemoteTrack('p1', r.entry, arriving(first, { stream: mediaStream([first]) })); |
| + assert.equal(r.entry.cameraMid, '1'); |
| + assert.equal(r.shownTrack(r.entry.stream), first); |
| + |
| + // The rollback's rebuild: same mid, new track, no stream of its own — |
| + // and the old track has not ended, it has simply stopped carrying |
| + // frames. |
| + const second = videoTrack('cam2'); |
| + assert.equal(first.readyState, 'live', 'the predecessor is still live, which is the whole point'); |
| + r.onRemoteTrack('p1', r.entry, arriving(second, { mid: '1' })); |
| + |
| + assert.deepEqual(r.entry.stream.getVideoTracks(), [second], |
| + 'the tile stream still holds the superseded camera'); |
| + assert.equal(r.shownTrack(r.entry.stream), second); |
| + assert.equal(r.entry.cameraTrack, second, 'the authoritative track was not recorded'); |
| +}); |
| + |
| +test('a rebuilt camera does not disturb the microphone', () => { |
| + const r = receiving(); |
| + const mic = { id: 'mic1', kind: 'audio', readyState: 'live' }; |
| + const first = videoTrack('cam1'); |
| + r.onRemoteTrack('p1', r.entry, arriving(mic, { mid: '0' })); |
| + r.onRemoteTrack('p1', r.entry, arriving(first, { mid: '1' })); |
| + r.onRemoteTrack('p1', r.entry, arriving(videoTrack('cam2'), { mid: '1' })); |
| + assert.deepEqual(r.entry.stream.getTracks().map((t) => t.id), ['mic1', 'cam2']); |
| +}); |
| + |
| +test("a peer's screen is bound to its own track, on its own stream", () => { |
| + const r = receiving(); |
| + const camera = videoTrack('cam1'); |
| + r.onRemoteTrack('p1', r.entry, arriving(camera, { mid: '1' })); |
| + const screen = videoTrack('scr1'); |
| + r.onRemoteTrack('p1', r.entry, arriving(screen, { mid: '2' })); |
| + assert.equal(r.entry.sharing, true); |
| + assert.equal(r.entry.share.track, screen); |
| + assert.deepEqual(r.entry.share.stream.getVideoTracks(), [screen]); |
| + assert.notEqual(r.entry.share.stream, r.entry.stream, 'the two tiles share one stream'); |
| + assert.deepEqual(r.entry.stream.getVideoTracks(), [camera], 'the screen displaced the camera'); |
| + |
| + // And a rebuilt screen replaces the screen, not the camera. |
| + const screen2 = videoTrack('scr2'); |
| + r.onRemoteTrack('p1', r.entry, arriving(screen2, { mid: '2' })); |
| + assert.deepEqual(r.entry.share.stream.getVideoTracks(), [screen2]); |
| + assert.deepEqual(r.entry.stream.getVideoTracks(), [camera]); |
| +}); |
| + |
| test('adoptTrack survives what a browser can hand it', () => { |
| assert.equal(adoptTrack(null, videoTrack('v1')), null); |
| const stream = mediaStream([]); |