Skip to content

fix(lint): flag media clips whose window has no out-point - #2972

Closed
xiayewang-heygen wants to merge 1 commit into
mainfrom
fix/lint-unbounded-media-window
Closed

fix(lint): flag media clips whose window has no out-point#2972
xiayewang-heygen wants to merge 1 commit into
mainfrom
fix/lint-unbounded-media-window

Conversation

@xiayewang-heygen

Copy link
Copy Markdown
Contributor

Why

Two HyperFrames design-element renders failed in prod with PLAN_TOO_LARGE: a 7.0s element produced a 5.41 GiB planDir and an 8.0s element produced 15.11 GiB, against the 2 GiB v1 ceiling.

Neither is about composition length. Both compositions contain one clip shaped like this:

<video id="five9_video" src=".../training.mp4"
       data-start="0" data-media-start="2280" muted playsinline>

An in-point, no out-point. data-media-start says where the media window opens; nothing closes it, so resolveSegmentDuration treats the window as unbounded and extracts durationSeconds - mediaStart — everything from the in-point to EOF. The source is a 54-minute upload (duration=3267.26s, 250.9 MiB — exactly the source-media figure in the error), so:

element in-point extracted frames @30fps reported
7.0s 2280s 987.3s 29,617 29,622 files / 5.16 GiB video-frames
8.0s 1089s 2178.3s 65,348 15.11 GiB

The render only ever samples 210 frames. Everything else is waste — a 141× overshoot.

Nothing flagged it. media_missing_data_start hard-errors on a missing data-start but mentions data-duration only in a fixHint, and the authoring contract explicitly blessed the omission: "Video/audio can default to media duration when known." Harmless in preview and in-process render, where the composition's 7s window just clips playback; catastrophic in the distributed plan path, where "media duration" silently becomes the frame-extraction budget.

Plan v2 (#43811) disabled the planDir cap, so the error stopped on 2026-07-29 — but the 141× extraction still runs on every such render.

What

New rule media_unbounded_media_window on <video>/<audio> that have data-start and a source but neither data-end nor data-duration:

  • error when data-media-start > 0 — an in-point proves a slice was intended, so a missing out-point is a contradiction, not a style choice.
  • warning otherwise — "play the whole clip" is a legitimate pattern (full-length A-roll), so this only surfaces the plan cost.

Untimed and sourceless media are left to the rules that already own those shapes.

Docs updated so the guidance no longer authorizes the pattern: data-duration is now listed as required for video/audio that set data-media-start, plus a note in both data-attributes.md and tracks-and-clips.md that a clip's media window is its extraction budget, not just its playback range.

Verification

  • Run against the two real failing compositions pulled from S3: both flagged error, on exactly the offending element (five9_video, bg_video).
  • Run across the repo's 636 HTML files: 0 errors, 22 warnings. All 22 are the legitimate full-clip A-roll pattern in packages/producer/tests/*-prod/ fixtures — none carries an in-point, so nothing existing is blocked.
  • @hyperframes/lint: 515 tests pass (4 new). CLI consumers of lint output (validate, check, lintFormat): 79 pass.

Follow-ups (not in this PR)

  1. Producer clamp — extract only [max(start,0), min(end, compositionEnd)], using compositionEnd when end is non-finite. Lint can't reach hand-authored or external-API compositions; this bounds the planDir regardless of what was authored. buildPlanVideosJson already threads compositionEnd.
  2. Fail fast — the only pre-extraction check measures the compiled tree, so a doomed plan burns full ffmpeg extraction (45s in the observed failure) before pre-freeze rejects it. Estimate Σ(window × fps × bytes/frame) after probe, and drop "shorten the composition" from the error — it is a dead end when the element is already 7 seconds.

🤖 Generated with Claude Code

A <video>/<audio> with data-start but no data-end/data-duration has a
media window that runs to the end of its source file. Distributed plans
pre-extract one image per frame of that window, so the window — not the
composition duration — is the planDir budget.

Errors when data-media-start > 0 (an in-point proves a slice was
intended); warns otherwise, since playing a whole clip is legitimate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head 7d33a53. Correctly scoped, well-verified fix for the exact root cause behind the McKesson PLAN_TOO_LARGE incident (FB-3653/VA-1973):

  • New rule media_unbounded_media_window correctly gates on timed (data-start) + sourced (src/data-var-src) media missing BOTH data-end and data-duration, with severity keyed on whether data-media-start > 0 proves an intended slice (error) vs. legitimate full-clip playback (warning). Logic handles the missing-attribute-as-0-vs-NaN edge case correctly either way.
  • Untimed/sourceless media correctly left to the rules that already own those shapes (media_missing_data_start/media_missing_src) — verified by a dedicated test.
  • Verified against both real failing compositions (both flag error on the exact offending element) and the full 636-file repo sweep (0 errors, 22 warnings, all legitimate full-clip A-roll — nothing existing breaks).
  • Docs (data-attributes.md, tracks-and-clips.md) correctly updated to stop blessing the omission that caused this.

This is the authoring-time half (lint) — the two follow-ups (producer clamp, fail-fast estimate) are correctly scoped out as separate work, and James notes in-thread the distributed-renderer side is now fixed too, so this closes the loop from both ends.

— Jerrai

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid fix. The severity split — error when data-media-start > 0 (an in-point without an out-point is a contradiction), warning otherwise — is well-argued, and skipping tags that miss data-start / src cleanly delegates to the rules that already own those shapes (media_missing_data_start, media_missing_src, media_variable_src_no_fallback). Tests cover all four branches (error / warning / valid-with-out-point / delegate-to-other-rules) and pin the 2280 interpolation. Docs additions in both data-attributes.md and tracks-and-clips.md reframe the media window as an extraction budget, which is the right mental model to leave in the AI-authoring skill. The deferred producer-side clamp + fail-fast follow-ups in the description are correctly out of scope for a lint fix.

Nits

  • skills/hyperframes-core/references/data-attributes.md:31 says data-duration is "required for video/audio that set data-media-start". Read literally, that covers data-media-start="0" too — but the lint rule only errors when data-media-start > 0 (the hasInPoint = Number.isFinite(mediaStart) && mediaStart > 0 gate at packages/lint/src/rules/media.ts:550), so data-media-start="0" yields the same warning as omitting the attribute entirely. Doc language of "required for video/audio that set data-media-start to a positive value" would match the enforced contract exactly. Very minor — data-media-start="0" is functionally equivalent to omitting it, so no real author is threading this needle.
  • The error/fixHint messages interpolate the Number-coerced mediaStart, so data-media-start="2.28e3" reports back as data-media-start="2280". Fine, semantically identical, but the raw string would echo author intent more literally. Skip if not worth the churn.

What I didn't verify

  • The "0 errors, 22 warnings across 636 HTML files" scan (accept on trust — the surface is consistent with the fixture layout).
  • Producer-side names cited in the PR body (resolveSegmentDuration, buildPlanVideosJson, compositionEnd) — the lint rule's correctness doesn't depend on them, and the debugging narrative is supporting context, not a code claim to pin.

Review by Rames D Jusso

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 — media_unbounded_media_window lint rule (head 7d33a53)

Verdict: Approve (one non-blocking nit)

Well-motivated rule that catches a real prod failure pattern. Two compositions blew past the 2 GiB plan ceiling because a 7s clip cut from a 54-minute source extracted ~29,600 frames instead of ~210. The rule is correctly scoped, the severity split is sound, and the docs update closes the authoring loophole.

What was verified

Rule logic — all edge cases clean:

  • data-media-start="0"0 > 0hasInPoint = false → warning. Correct: "start from beginning" is semantically identical to omitting the attribute.
  • data-media-start absent → readAttr returns null, Number(null) = 0, 0 > 0 = false → warning. Correct.
  • Negative data-media-start → warning. Correct: invalid offset is a separate concern; runtime clamps to 0 (init.ts:755).
  • data-end="" or data-duration=""readAttr regex [^"']+ requires 1+ chars → returns null → rule fires. Correct: empty attribute is effectively unset.
  • data-duration="0"readAttr returns "0" (truthy string) → rule skips. Correct: zero-duration is bounded (zero frames extracted), not the unbounded problem.
  • data-var-src without src → rule proceeds. Correct: clip has a source at render time.
  • readAttr boundary safety: (?<!\w-) lookbehind prevents data-start from matching inside data-media-start. Verified.

Rule boundaries — no overlaps:

  • Untimed media (no data-start) → deferred to media_missing_data_start. Mutually exclusive gates.
  • Sourceless media → deferred to media_missing_src. Mutually exclusive.
  • Out-point present (data-end or data-duration) → rule skips. Correct.

Tests: 4 tests covering error (in-point, no out-point), warning (no in-point, no out-point), acceptance (out-point present via data-duration or data-end), and boundary delegation (untimed/sourceless → other rules fire, this one doesn't).

Docs: data-attributes.md table updated: data-duration now required for video/audio with data-media-start. New paragraph explains extraction budget concept. tracks-and-clips.md gains a matching paragraph. Both close the "Video/audio can default to media duration when known" loophole that authorized the pattern.

Nit (non-blocking)

data-playback-start alias not checked for severity classification. The runtime resolves the media offset as data-playback-start ?? data-media-start (init.ts:751), and the attribute is documented in 21 files across docs, runtime, and lint fix hints. A composition with data-playback-start="120" (no data-media-start, no bounds) would get a warning instead of an error, even though it has a clear non-zero in-point — the exact 54-minute-source disaster scenario the error severity is designed to catch.

Fix would be:

const mediaStart = Number(
  readAttr(tag.raw, "data-playback-start") ?? readAttr(tag.raw, "data-media-start")
);

Mirroring the runtime's resolution order. The error message would also want to name whichever attribute was actually found. This is severity-only (the rule fires either way), so non-blocking for merge — but worth a follow-up if data-playback-start appears in real compositions.


Review by Miga

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive runtime-contract review at head 7d33a53. The existing reviews cover the rule predicate and test branches; this checks the premise against the renderer behavior already present in this PR's base.

The new tests are internally consistent with the proposed rule (packages/lint/src/rules/media.test.ts:235-292), but the rule no longer matches production video extraction.

Blocker

  • [blocker] packages/lint/src/rules/media.ts:533-560 — the exact video failure described by this rule is already bounded by #2955, so this creates a false hard error. This PR already contains packages/producer/src/services/render/stages/extractVideosStage.ts:379, which passes the finite composition duration as timelineEnd; packages/engine/src/services/videoFrameExtractor.ts:951-956 intersects the source window with that timeline. Both in-process and distributed-plan callers go through this shared stage, with regression coverage at packages/producer/src/services/render/stages/extractVideosStage.timelineBound.test.ts:99-109. The reported 7-second composition therefore extracts seven seconds, not the remaining 987 seconds, even with data-media-start="2280" and no authored duration. Lines 557-560 nevertheless make this supported shape an error and claim it still creates a multi-GiB plan / PLAN_TOO_LARGE. I recommend closing this PR as superseded by #2955 unless an uncovered production video path can be demonstrated.

Important if the rule is repurposed

  • packages/lint/src/rules/media.ts:540-564 also applies the video-frame/plan-size diagnostic to <audio>, which does not publish pre-extracted video frames into the plan. There is a real but different audio-preprocessing concern: the current mixer can prepare the full remaining source before the final mix trims to composition duration (packages/engine/src/services/audioMixer.ts:736-760,783-788). If that is worth linting, scope and describe it as temporary audio CPU/disk work—or preferably bound the audio stage—rather than preserving this obsolete Plan-too-large rule.
  • packages/lint/src/rules/media.ts:563 suggests data-end, but packages/lint/src/rules/composition.ts:466-485 emits deprecated_data_end as an error when it appears without data-duration. The acceptance test at packages/lint/src/rules/media.test.ts:266-276 checks only that this new finding is absent, not that the full lint result is green.
  • skills/hyperframes-core/references/data-attributes.md:31-37 and skills/hyperframes-core/references/tracks-and-clips.md:46 would publish the now-false extraction/PLAN_TOO_LARGE claim while packages/core/docs/core.md:260-261,285-286 still documents omitted duration as supported natural-source behavior.

Miga's data-playback-start alias gap also remains valid if any narrower rule is retained.

— Codex

Verdict: REQUEST CHANGES
Reasoning: #2955 already fixes the cited video over-extraction on every production video-render path, so the PR now blocks a safe supported pattern and documents behavior that is no longer true.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive exact-head review at 7d33a53. The predicate and branch tests in packages/lint/src/rules/media.test.ts:235-292 are clear, but I independently confirmed the runtime-contract blocker raised by jrusso1020.

Blocker

  • [blocker] packages/lint/src/rules/media.ts:533-560 — this hard error and its PLAN_TOO_LARGE explanation are obsolete on this PR base. The head already contains #2955. There is exactly one production call to extractAllVideoFrames, at packages/producer/src/services/render/stages/extractVideosStage.ts:368-386, and it always passes the finite composition.duration as timelineEnd. The only two production callers of that shared stage are the in-process renderer (packages/producer/src/services/renderOrchestrator.ts:2325-2346) and distributed planner (packages/producer/src/services/distributed/plan.ts:1033-1045); both validate duration before extraction. The engine then intersects the natural source range with that timeline at packages/engine/src/services/videoFrameExtractor.ts:928-956 and :815-889. The regression at packages/producer/src/services/render/stages/extractVideosStage.timelineBound.test.ts:99-119 explicitly covers both materialization modes. A 7-second composition with an open 55-minute video now extracts at most its visible 7-second timeline window, so this PR would reject a supported shape and publish a false multi-GiB/PLAN_TOO_LARGE claim. Close as superseded, or demonstrate a remaining production video path and scope the rule to it.

If repurposed

  • packages/lint/src/rules/media.ts:540-564 applies video-frame/plan-size language to audio, although audio does not create the plan video-frame tree. A narrower audio preprocessing rule needs its own accurate cost/failure contract.
  • Miga’s alias finding is real, but secondary: packages/core/src/runtime/init.ts:750-752 and Studio trimming use data-playback-start before data-media-start, while the rule reads only data-media-start at packages/lint/src/rules/media.ts:549. Because hyperframes lint exits zero on warnings (packages/cli/src/commands/lint.ts:50-62), that is a genuine blocking-severity bypass if a narrower rule remains.

Exact-head CI is terminal-green (37 success, 8 intentional skips); the objection is semantic, not CI.

Verdict: REQUEST CHANGES
Reasoning: #2955 already bounds every production video-extraction path by the finite composition timeline, so the proposed video error and documentation describe a failure that this head can no longer produce.

— Magi

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseding my own APPROVE above at the same head (7d33a53) — I got this wrong.

I independently re-verified the blocker jrusso1020 and Magi raised, not just taking their word: #2955 ("fix: bound HDR and video extraction resources") merged 2026-08-03T03:21:55Z, and #2972's base (bc6dbc7) was branched from main at 2026-08-03T19:05:41Z — i.e. after #2955 landed. Reading the actual source at this PR's own head confirms it:

  • packages/producer/src/services/render/stages/extractVideosStage.ts:379 — the single production call to extractAllVideoFrames — passes timelineEnd: composition.duration.
  • packages/engine/src/services/videoFrameExtractor.tsresolveTimelineExtractionWindow computes visibleDuration = Math.max(0, Math.min(resolvedVisibleDuration, timelineDuration)) where timelineDuration = timelineEnd - compositionStart. This clamps extraction to the composition's own duration regardless of how long the source file is.
  • extractVideosStage.timelineBound.test.ts has regression coverage for exactly this.

So the premise I approved against — "an in-point with no out-point extracts to EOF of a 54-minute source" — is no longer true on this base. A 7s composition with data-media-start="2280" and no data-duration now extracts ~7s, not ~987s. The hard error severity and the docs updates (data-attributes.md, tracks-and-clips.md) describe a PLAN_TOO_LARGE failure mode this head can no longer produce.

I missed this because my original verification confirmed the lint rule fires correctly on the two known-bad HTML snippets, but I never re-ran the actual extraction/render pipeline at this head to check whether the underlying runtime bug this PR cites was still live — I took the PR description's incident narrative at face value instead of checking it against a same-day sibling PR that had just landed. That's on me.

Agree with jrusso1020's recommendation: close as superseded by #2955, or narrow this to the audio-preprocessing concern he identified (with its own accurate cost/failure contract) if that's still worth linting. Also agree Miga's data-playback-start alias gap is real and would need fixing if any narrower rule survives.

— Jerrai (correcting my own review)

@xiayewang-heygen

Copy link
Copy Markdown
Contributor Author

#2955 has done the job

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.

6 participants