docs(skills): teach speed ramps as a rate lane in one reference page - #4171
miguel-heygen wants to merge 2 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
terencecho
left a comment
There was a problem hiding this comment.
COMMENT / HOLD at ce7d3151 — ordering dependency on unmerged #4153 + one stale recipe missed + a now-vacuous test assertion
Content is largely correct and well-organized — the new speed-ramps.mdx matches merged core/engine source (verified below). But three items should be addressed before this can safely auto-merge.
1. Ordering dependency (blocking stamp=MERGE) — #4153 is not merged
Your body says "merges after the Studio speed-ramp PR it describes." Confirmed merge state:
| PR | State | Note |
|---|---|---|
#4129 (speed-ramp core rate lane, MAX_PLAYBACK_RATE=10) |
MERGED 12:27 UTC | ✅ |
| #4143 (renderer speed-ramp, atempo slicing) | MERGED 14:01 UTC | ✅ |
#4153 (Studio Media Speed slider + A toggle + preset menu) |
OPEN | ❌ |
#4170 (CLI snapshot speed ramp + rate 10x) |
OPEN | not required by this PR's text |
The docs page's "Editing in Studio" section names the Speed slider, the A automation toggle, and the timeline rate lane — that UI ships in #4153. "Presets" names the Speed preset menu (Montage, Hero, Bullet, Jump cut, Flash in, Flash out) — also #4153. If #4171 auto-merges ahead of #4153, both sections describe UI that isn't there yet.
Holding the APPROVE for a live head after #4153 lands. If you'd prefer, I can APPROVE once #4153 shows state=MERGED; a nudge in this thread when it lands is enough.
2. Real doc miss — creator-editing-recipes.md:439 still contradicts the new rule
The PR body claim is "no page still says rates stop at 5x or that ramps are unsupported." One recipe still says both.
skills/hyperframes-core/references/creator-editing-recipes.md, in "Split a section and change its speed" (line 439 at ce7d3151):
Source math:
<video>and<audio>parts usedata-playback-rate(0.1 to 5, constant) per the constant-speed recipe above, with matching audio. Limit: a speed ramp (a rate that changes within one clip) is not in the format; approximate with several constant-rate parts or preprocess a derived asset through/media-use.
Both halves are wrong now: the range is 0.1 to 10, and a speed ramp is in the format as a rate lane. The other recipe in the same file (~line 227) was updated correctly; this one was missed. Small fix in the same PR unblocks the body's "repo-wide" framing.
3. Latent CI gap — assertions in scripts/creator-editing-capabilities.test.mjs no longer verify their stated intent
Two assertions still require the removed phrasing (line 99 and line 135):
assert.match(keyframes, /speed ramps?[\s\S]{0,300}(not supported|preprocess)/i);
assert.match(audio, /speed ramps?[\s\S]{0,220}(not supported|preprocess)/i);They pass on the new content only because [\s\S]{0,300} is wide enough to bridge across an intervening table row into the "mid-source freeze ... not supported; preprocess a still/derived segment" Freeze/hold row (keyframes SKILL.md), and similarly bridges to nearby /media-use "preprocessing" mentions (audio SKILL.md). Green today, but they no longer lock in what they were written to lock in — a future PR could re-introduce "speed ramps are not supported" verbatim and this suite wouldn't catch it. Consider inverting both to assert.doesNotMatch(...) for the old phrasing plus a positive assert.match(..., /rate lane/) for the new one — keeps the drift-lock while flipping the invariant.
Non-blocking on its own, but worth folding into the same push since you're already touching the skills.
Content verification — accurate against merged source
MAX_PLAYBACK_RATE = 10,MIN_PLAYBACK_RATE = 0.1inpackages/parsers/src/mediaDuration.tsat head; coreplaybackRateBounds.tsre-exports (verified in the #4166 clamp-unification arc). Docs "Clamped to0.1to10" ✅.sourceTimeAt(spec, t)inpackages/core/src/speedRamp.tsat head, with the docstring "the one rate-to-time mapping consumers share" — matches your "one function,sourceTimeAt, which the preview and the renderer both use" ✅.sampleAutomationLane(spec, t, "log")at head — matches your "logarithmic scale, so going from 0.5 to 2 passes through 1 at the midpoint" ✅.tin clip seconds (shiftRateLaneandrateAtboth accept clip-localt;shiftRateLanereindexes for the trim case) — matches "Seconds from the start of the clip, not from the start of the composition" ✅.- Renderer atempo slicing — I verified
RAMP_SLICE_SECONDS=0.25andMAX_RAMP_SLICES=240in my #4143 review at merged head3f13e1e6; matches your "short constant-speed slices of a quarter second, which approximates the curve while keeping pitch" ✅. - Lane-wins-over-constant,
data-durationfixes timeline hold, pitch preservation via browser default (HTMLMediaElement.preservesPitch=true, verified in my #4143 pitch-attr-removal review) — all consistent ✅.
Verified-clean negatives (peer-review style)
- New page cross-links to
/reference/audio-effects#the-four-attributes— the anchor is stable in existing docs (unchanged in this PR). docs.jsonaddsreference/speed-rampsonce, sits under the same reference nav block ashtml-schema/audio-effects— nav shape sensible.skills-manifest.jsonhashes updated for the four skills whose SKILL.md/references changed (hyperframes,hyperframes-audio,hyperframes-core,hyperframes-keyframes) — Skills manifest-in-sync CI check ✅.- Mintlify Deployment ✅ at head.
CI
Docs Validate docs still IN_PROGRESS at read-time; other required lanes SKIPPED per docs-only path filter (which also means Test (the broad suite) didn't run — that's how the test-file gap in §3 stayed green). Skills manifest + skills tests + format + lint + Mintlify all green at ce7d3151.
Stamp mechanics
hf-oss require_last_push_approval=true — I'll re-review at whichever head follows the #2/#3 fixes (or the current head if you push nothing and just wait for #4153 to merge; but this PR would then still need a re-APPROVE at that later head anyway per the setting).
— Review by tai (pr-review)
ce7d315 to
8e6879f
Compare
8e6879f to
0eb7ea9
Compare
terencecho
left a comment
There was a problem hiding this comment.
Re-review at 0eb7ea91 — HOLD stands, same reasons; recipe B in creator-editing-recipes.md still contradicts recipe A within the same file
Delta since my prior COMMENT at ce7d3151 (5 commits ahead / 2 behind; author-only commits: 55dfe335 "teach speed ramps as a rate lane in one reference page" + 0eb7ea91 "say what fixes a ramped clip's length and how the render approximates pitch"). The rebase pulled in #4165 (dock engine, merged), #4144 (docs, merged), #4169 (timeline, merged) — those aren't this PR's content.
Item 1 — Ordering dependency on #4153 (primary blocker, unchanged)
gh pr view 4153: still state=OPEN, no mergedAt. The docs page's "## Presets" (line 47) and "## Editing in Studio" (line 53) still name the Studio Speed preset menu, the A automation toggle, and the timeline rate lane — all UI that ships in #4153. Auto-merging this PR ahead of #4153 leaves those sections describing UI that isn't there yet.
Item 2 — Real doc miss: recipe B in creator-editing-recipes.md (line 491, was 439 at ce7d3151)
55dfe335 fixed recipe A at line 232 (constant-speed recipe): "Limit: normalized 0.1..10. For a speed ramp put a rate lane in data-automation, e.g. {...}; it wins over the constant." ✅
Recipe B is unchanged. At 0eb7ea91 line 491, the "Split a section and change its speed" recipe still reads:
Source math:
<video>and<audio>parts usedata-playback-rate(0.1 to 5, constant) per the constant-speed recipe above, with matching audio. Limit: a speed ramp (a rate that changes within one clip) is not in the format; approximate with several constant-rate parts or preprocess a derived asset through/media-use.
Both halves still contradict the merged core (MAX_PLAYBACK_RATE=10 in packages/parsers/src/mediaDuration.ts) and the new rate lane in data-automation. And "0.1 to 5" here is not the viewer transport-speed range Miguel added at speed-ramps.mdx:62 — this recipe is about the authored data-playback-rate attribute, which is 0.1..10. It's the same finding as before, just at a shifted line number.
Same-file inconsistency between recipe A (correct) and recipe B (still wrong) is the specific tell. Small fix in the same PR would close this cleanly.
Item 3 — Test-regex bridging (non-blocking, unchanged)
scripts/creator-editing-capabilities.test.mjs:99,135 still uses /speed ramps?[\s\S]{0,300}(not supported|preprocess)/i and /speed ramps?[\s\S]{0,220}(not supported|preprocess)/i — coincidentally pass by bridging into a neighboring row (Freeze/hold's "mid-source freeze is not supported; preprocess a still/derived segment"). The new assert.doesNotMatch(keyframes, /keyframe(?:d|ing)?\s+(?:the\s+)?data-playback-rate/i) at line 101 is a positive whitelist for a different phrasing — welcome, but it doesn't replace the vacuous ramps assertion. Same-PR fold if you're already touching skills.
What's actually cleaner at this head
- Recipe A at line 232 in
creator-editing-recipes.md: correctly says0.1..10+ rate-lane example ✅ skills/hyperframes-audio/SKILL.md: constant range 0.1..10 + "speed ramp is aratelane indata-automation" ✅speed-ramps.mdx: pitch-approximation quarter-second note, transport-speed cap explicit ✅docs.jsonnav + skills-manifest hashes coherent
Stamp mechanics
reviewDecision: REVIEW_REQUIRED, mergeStateStatus: BLOCKED. hf-oss require_last_push_approval=true — will APPROVE at the head that (a) has recipe B fixed and (b) either #4153 merged, or the Studio-UI sections in speed-ramps.mdx qualified as "coming in a follow-up." Item 3 remains non-blocking.
— Review by tai (pr-review)
Adds the speed-ramps reference, corrects the 0.1 to 5 rate range to 0.1 to 10 and the 'speed ramps are not supported' lines in the skills, and regenerates the skills manifest.
…r approximates pitch
0eb7ea9 to
bafe642
Compare
terencecho
left a comment
There was a problem hiding this comment.
Re-review at bafe6422 — HOLD stands; both prior blockers unchanged (content-identical rebase after #4174 confirms it)
Miguel's re-pin flags this as a rebase after #4174 (skills manifest regenerated) with "content identical". Verified: content IS identical for the two things I was holding on.
Item 1 — Ordering dependency on #4153 (still open)
gh pr view 4153: state: OPEN, mergedAt: null, reviewDecision: REVIEW_REQUIRED, head b7aa702c. The docs page's "## Presets" and "## Editing in Studio" still name the Studio Speed preset menu, the A automation toggle, and the timeline rate lane — UI that ships in #4153. Merging this ahead leaves those sections describing UI that isn't there yet.
Item 2 — Recipe B in creator-editing-recipes.md (line 491)
Fetched the file at bafe6422; recipe B is byte-identical to 0eb7ea91. Line 491:
Source math:
<video>and<audio>parts usedata-playback-rate(0.1 to 5, constant) per the constant-speed recipe above, with matching audio. Limit: a speed ramp (a rate that changes within one clip) is not in the format; approximate with several constant-rate parts or preprocess a derived asset through/media-use.
Both halves still contradict merged core (MAX_PLAYBACK_RATE=10 in packages/parsers/src/mediaDuration.ts) and the new rate lane in data-automation — which recipe A at line 232 in the same file describes correctly (Limit: normalized 0.1..10. For a speed ramp put a "rate" lane in "data-automation"…). Same-file A-vs-B inconsistency stands.
"0.1 to 5" here is not the viewer transport-speed range Miguel added at speed-ramps.mdx:62 — recipe B is about the authored data-playback-rate attribute, which is 0.1..10. Same finding as ce7d3151 and 0eb7ea91; the rebase moved nothing about it.
What will lift HOLD
- Either #4153 merged (Somu owns it at
b7aa702c), OR the Studio-UI sections inspeed-ramps.mdxre-scoped as "coming in a follow-up." AND - Recipe B updated to match recipe A (
0.1..10+ rate-lane pointer).
Item 3 (test regex bridging at creator-editing-capabilities.test.mjs:99,135) remains non-blocking.
— Review by tai (pr-review)
|
Folded into #4153 (one speed-ramp feature, one PR). |
What changes
Speed ramps are taught in one place. A new reference page,
speed-ramps, explains theratelane indata-automation: the lane shape, presets, what Studio shows, pitch handling, and limits.Three lines that were wrong now that ramps exist are fixed: the
data-playback-raterow in the HTML schema said 0.1 to 5 (the range is 0.1 to 10), and the audio, keyframes, main and editing-recipes skills said speed ramps are unsupported and to preprocess. The skills manifest is regenerated.Verification
data-duration; the render approximates the curve in quarter-second slices) are fixed here.hyperframes snapshotstill clamped the rate at 5 and ignored lanes; that is fixed separately in fix(cli): snapshot picks the frame through a rate lane and up to 10x #4170.gen-skills-manifestreports in sync; docs.json is valid and lists the page once.