Repository navigation
feat(core): preview health names the media element that held play - #7159
Conversation
There was a problem hiding this comment.
Requesting changes at 6ae304f2. The code works, but two required checks fail on this head.
Blockers
- Comments ratchet fails (run 41833097818). The comment share went up in three files:
playbackHealth.ts(+3 lines),types.ts(+3),previewQualitySlice.ts(+2). The body ticks "Comments follow CONTRIBUTING.md", and that box is not true until this check passes. Cut the new doc comments or move what they say into names or types. For example,PlaybackHeldBy.fetchMs's "null = fetch not seen to start" could be a named type. TheheldByfield docs inPlaybackHealthInput,RuntimePreviewHealthMessageandPlaybackHealthrepeat each other. - PR captures fails (run 41833097688). The PR touches
packages/studio, and the body has no## Before/## After. The studio source change is 13 lines with no .tsx, .css or .html, so a## No visible changesection is enough.
Non-blocking
parseHeldBy: whenfetchMsis present but not a number (for example a string), it turns intonull, which means "fetch not seen to start". The body says a malformedheldByparses toundefined. ReturnundefinedwhenfetchMsis neither a number nornull, so bad input doesn't pass as a real reading.watchMediaFetchesonly seesloadstartevents that fire after runtime init, and only in the main document (media events aren't composed, so it misses shadow roots). Elements that started loading earlier reportfetchMs: null. That's acceptable as documented, but a choppy report on first load will usually shownullfor exactly the case this PR is meant to diagnose. Consider seeding fromperformance.getEntriesByName(el.currentSrc)or noting the limit.
Checked
bufferingForisHTMLMediaElement[], sobufferingFor[0] ?? find(...)gives the samewaitingvalue as the oldlength > 0 || some(...).heldByis the last non-null sample in the window. It resets on restart, and the test covers both cases.- Not a fix PR, so no
fails without the fix:line is needed.
Edit accuracy: accurate 267 (base branch 267), smooth 106 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
|
Fixed in 692f0e3.
Core 26 and studio 13 tests, both typechecks, oxfmt and oxlint pass on a Linux box. Removing the fallback or the |
There was a problem hiding this comment.
Approving at 692f0e3c. All the blockers from the review at 6ae304f2 are fixed:
- Comments ratchet: the new doc comments are gone, and the "not seen to start" meaning now lives in the
FetchNotSeenToStarttype. The Comments run is green on this head. - Captures: the body has a
## No visible changesection, and PR captures is green. parseHeldBy: afetchMsthat is neither a number nornullnow parses toundefined. A test covers it (fetchMs: "3500").- Early fetches:
heldByMediafalls back to the last resource-timing entry forcurrentSrc, and a test covers it.
The CI, preview-regression, Player perf and Player Safari audio runs are green on this head. Safari perf, Windows render, regression and CodeQL were still running when I approved.
Non-blocking: the timing fallback is best-effort. It can miss when the resource-timing buffer is full (250 entries by default), and with range requests .at(-1) measures the latest range, not the first byte. Both are fine for a diagnostic field.
692f0e3 to
8728883
Compare
There was a problem hiding this comment.
Approving at 87288832. This is a rebase of the head I approved at 692f0e3c, and the patch is unchanged.
What I ran on this head
- Patch content: the diff from the merge base is line-for-line identical to
692f0e3c's. The PR is one commit on top of main at26956ea1. - Main changes under the patch: main has moved under
init.ts,playbackHealth.tsandtypes.tssince the first review (#7373, #7378, #7379 and others). The tick block still reads the same:bufferingForis stillHTMLMediaElement[], andholderstill feedswaitingandheldByas before. - Tests: core
playbackHealth.test.tsandinit.buffering.test.tspass (26). StudiopreviewQualitySlice.test.tsandpreviewMessageRouter.test.tspass (13). - CI: CI, CodeQL, Comments, PR captures, preview-regression, Player perf and Player Safari audio are green on this head. Player Safari perf, regression and Windows render were still running when I approved.
What
A preview-health report now says what held play when the film waited:
heldBygives the media element's kind (audio or video), itsreadyStateandnetworkState, and the ms since that element's fetch started. It isnullwhen nothing held play in the window.parsePlaybackHealthin studio carries it through.Why
waitingSharesays how long play waited but not on what. When a film with no video sits at 0 fps waiting, the share alone cannot tell a slow audio fetch from a clip that never started loading, or an audio hold from a video one. Choppy-playback reports built on this health can then name the cause.Related work
None: a self-contained addition to the preview-health message.
How
init.ts: the transport tick already picked "waiting" as the first element the hold is for (bufferingFor[0]), else a playing video belowHAVE_FUTURE_DATA. It now keeps that element and passes it asheldBy, sowaitingis unchanged.playbackHealth.ts:watchMediaFetches(document)notes each element'sloadstart(capture phase, since media events do not bubble), installed and cleaned up besidewatchAudioLoadErrors.heldByMediaturns the element into numbers only. An element that started loading before the runtime did falls back to its resource-timing entry, and is null when it has none. The report carries the last holder in the window.types.ts:PlaybackHeldBy, exported from@hyperframes/coreand used by studio'sPlaybackHealth. A malformedheldBy, including afetchMsthat is neither a number nor null, parses to undefined, like the other optional fields.No visible change
No UI: the studio change is the health parser only (no .tsx, .css or .html).
Test plan
On a Linux box: core
playbackHealth.test.tsandinit.buffering.test.ts(26 tests), studiopreviewQualitySlice.test.tsandpreviewMessageRouter.test.ts(13),tsc --noEmitin core and studio, oxfmt and oxlint on the changed files: all pass.Each change is caught when broken: with
heldBy: nullat the tick,init.buffering's "a film held for data reads as choppy" fails. With the report'sheldBydropped, or theloadstartstamp removed,playbackHealthfails. With the parse line removed,previewQualitySlicefails.Not exercised: a real browser playing a slow-loading file. The tests use jsdom and stubbed ready states.