Skip to content

test(media): measure late join against the published GOP - #4719

Merged
kixelated merged 3 commits into
quest/m1/test-flakes-2/READMEfrom
quest/m1/test-flakes-2/media-late-join
Oct 3, 2026
Merged

kixelated merged 3 commits into
quest/m1/test-flakes-2/READMEfrom
quest/m1/test-flakes-2/media-late-join

Conversation

@kixelated

@kixelated kixelated commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Late join compared the newest painted canvas counter against a nominal 15-frame GOP. Painting and requestFrame() precede capture and encoding, whose timestamps determine actual GOP boundaries, so that sample can reject a viewer in the current published GOP.

Approach

Sample the published video track's newest keyframe while the existing viewer still holds demand, then require the latecomer's presented timestamp to be at or after it. The temporary sample releases its subscription and preserves existing viewers' frames. Wire regression coverage into just test harness, which runs in Interop CI.

Impact

  • Public API: none; only private test harness commands change.
  • Wire: none. The 500ms fixture keyframe interval remains unchanged.

Alternatives

Keep the painted-counter threshold or increase it. Both compare different pipeline stages and can hide a stale GOP.

Validation

just check, just test harness, fixture type checking, regression tests, and browser interop js → js pass. Two focused CPU-constrained late-join runs pass. The historical 16-frame symptom was not reproduced locally. Full media runs encountered separate cold-start audio-tone agreement and post-reattach video-monotonic failures before late join. A final focused run passed the GOP boundary check (7131.980ms after its 6678.762ms keyframe) but failed its audio-tone window (66/75 audible samples).

Follow-ups

The maintainer approved merging this as an improvement to the late-join check. The media-late-join quest stays open because the original 16-frame failure was never reproduced. Suggested follow-ups for the unrelated full media failures:

  • Cold-start audio-tone agreement (a focused run also failed its tone window at 66/75 audible samples).
  • Post-reattach video timestamp monotonicity.

(Written by GPT-6)

@kixelated

Copy link
Copy Markdown
Collaborator Author

The late-join assertion now samples the newest published GOP and refuses presentation from an older GOP. just check, just test harness, the real-track regression, and browser interop js → js pass. Two loaded focused runs passed; a final focused run passed the GOP assertion and failed the separate audio-tone window. Full media runs also hit cold-start tone agreement and post-reattach monotonicity failures. The exact historical 16-frame symptom was not reproduced, so this remains draft and the quest remains open.

Recommendation: keep draft until full media validation is green or its unrelated failures are triaged. No public API or wire change, and no follow-up quest edits.

(Written by GPT-6)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome under /recommended: keep this PR draft and its quest open. Published-GOP checks, harness regressions, browser interop and just check pass, but the exact historical 16-frame failure was not reproduced, and full media runs hit separate audio-tone and reattach-monotonicity failures. Further media triage is required before treating the quest as complete. Public API and wire are unchanged.

(Written by GPT-6)

@kixelated
kixelated marked this pull request as ready for review October 3, 2026 15:59
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary:

  • Change: late join now asserts the latecomer presents from the newest published GOP (sampled keyframe timestamp) instead of a nominal 15-frame painted-counter window. Test-only; no public API or wire impact.
  • Merged quest/m1/test-flakes-2/README into the branch (picks up test(cli): run fetch and completion operations on paused time #4660, test(cli): time the export linger from stdin close #4751 and earlier questline merges); no conflicts and no overlap with this PR's files.
  • Decision: the maintainer approved merging with the media-late-join quest left open, since the original 16-frame failure was never reproduced.
  • Suggested follow-ups: full media runs hit unrelated cold-start audio-tone agreement failures (one focused run at 66/75 audible samples) and post-reattach video monotonicity failures before late join.

Auto-merge (merge commit) is enabled on 2852c5b.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit 897970b into quest/m1/test-flakes-2/README Oct 3, 2026
7 checks passed
@kixelated
kixelated deleted the quest/m1/test-flakes-2/media-late-join branch October 3, 2026 16:28
@kixelated

Copy link
Copy Markdown
Collaborator Author

Review (head 2852c5b5aabf75645642202517401d0966b35f65)

Solid fix for the late-join flake: the old painted-counter vs nominal 15-frame GOP compared different pipeline stages, so under load a valid join into the still-current encoded GOP could look 16 frames behind. Sampling the published keyframe while the existing viewer still holds demand, then requiring videoTimestamp >= that keyframe, matches what “starts live” actually means.

Blocking

None.

Non-blocking

  1. waitForState does not require videoTimestamp (test/interop/clients/js/media.ts, late-join predicate). It only waits for frameId + running audio. If a sample ever has a canvas frameId without videoTimestamp, lateJoinStartsLive fails with a confusing undefinedms message. Prefer also requiring state.videoTimestamp !== undefined (or fold that into the check message).
  2. Empty open live-edge group (readLiveGop / default maxAge: 0). If the encoder has opened the next GOP but not written its keyframe yet, #isStale can still deliver the previous stamped group, so the lower bound is briefly one GOP older (more lenient). Rare and transient; only matters if we ever see false greens again.
  3. Quest doc still describes the removed check (quest/m1/test-flakes-2/media-late-join.md still names MAX_LATE_JOIN_LAG_FRAMES and the paint-counter sample). Fine if this PR intentionally leaves the quest open, but that file will be stale the moment this lands.
  4. PR body still says keep draft / full media not green (audio-tone + post-reattach monotonic failures). The PR is ready (isDraft=false); worth aligning the description with intent so reviewers don’t treat unfinished validation as “done.”
  5. CI still pending on this head (Interop in progress; Check/Test/Quest/WASM/Replay queued). Author-reported just test harness / contract regression / focused late-join GOP check are reassuring, but don’t treat green until the rollup finishes.

Verdict

MERGE

Reviewed head: 2852c5b5aabf75645642202517401d0966b35f65

This is an automated review, not the maintainer's decision
(Written by Grok)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant