spec: MIDI-voiced sound sources — soundfont engine, source role, GM floor, tones binding (1.18.0) — FEP #61 - #62
Conversation
📝 WalkthroughWalkthroughFeedpak v1.18.0 adds MIDI-voiced source rigs with soundfont realizations and General MIDI metadata, introduces arrangement and drum tone bindings, updates schemas and examples, and synchronizes specification version references. ChangesFeedpak v1.18.0 specification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Reader
participant Manifest
participant RigsJSON
participant Soundfont
Reader->>Manifest: Read tones or drum_tones binding
Manifest->>RigsJSON: Resolve base_rig
RigsJSON->>Soundfont: Load selected sf2 realization
Soundfont-->>Reader: Voice MIDI events
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Editor side — compatibility confirmed ✅ (co-owner note from the Checked #62 against the open editor PRs — no editor changes needed, and the design choices reinforce each other:
Two forward-looking coordination notes (not blockers):
Nice bonus: the drum GM floor ( |
topkoa
left a comment
There was a problem hiding this comment.
Reviewed against latest main. Nothing blocking — the cross-references check out (the §7.9 control-events bullet's spd/sph/spu and hi-hat/choke citations are accurate), all five check_versions.py locations are updated, the example placeholders follow the existing nam/ir conventions, and the wholesale-precedence rule is cleanly stated from both ends. Three small inline comments below, plus one non-blocking observation:
Underspecified corners (arguably fine under the format's permissive style — noting them so leaving them open is a choice, and a future clarification FEP can pick them up):
intent.gmcarrying bothprogramandpercussion/kit— undefined which wins.percussion: truewithkitabsent — presumably kit0(Standard), by analogy with thebank/programabsent-⇒-0 rule, but unstated.- A
sourceblock that is not at the head of the chain: the spec defines the head-position case and scopes out parallel audio input, but never says how a Reader should treat a nonconforming mid-chain source (skip it? treat the rig as invalid?).
|
Thanks — all three were right, all three fixed in 73b8da6.
Full local CI mirror re-run after the changes: |
|
Thanks for the compatibility pass — good to have it confirmed from the editor side rather than assumed. Two things in there are especially useful to have on record:
On your two forward-looking notes, both recorded on our side:
Nothing here needs a change to #62 — flagging both as follow-ups rather than folding them in. |
topkoa
left a comment
There was a problem hiding this comment.
All three addressed exactly in 73b8da6 — header date now matches the CHANGELOG, the realizations row and the mirrored schema \ now agree with the §7.9 bullet (good catch extending it to rigs.schema.json), and the Power-kit example is coherent end to end. CI green on the new head. The underspecified corners from the review body are fine as recorded follow-ups. LGTM — approving.
|
Heads-up: this PR went Textual conflicts (4 files). Both PRs edit the same spots:
( Version collision — One deliberate edit, not just conflict resolution: the §5.1 rationale for top-level
Happy to re-review promptly once it's rebased so this doesn't stall. |
…loor, tones binding (1.17.0) — FEP #61 A chart can now declare what a MIDI part (piano, keys, drums) should sound like, with a guaranteed General-MIDI floor: - reserved realization engine 'soundfont' (source blocks only; sf2 reserved, format open), addressed as ref + bank/program - reserved block role 'source' — generator at the head of the chain, input is the Reader-supplied MIDI event stream - OPTIONAL intent.gm — normative-when-present GM floor; kit numbers are 0-based wire/SF2 values - binding: arrangement-entry tones (manifest, wholesale precedence over in-JSON tones) + top-level drum_tones for the drum part - conformance: engine+format gate selection (general clarification); source blocks SHOULD voice from intent.gm rather than skip; control events honoured - prose: rigs are instrument-neutral signal chains (5.1/6.9/7.9) Spec, schemas, extended example, CHANGELOG, and version strings updated together; check_versions, validate, and the test suite pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
… row, GM Power kit in the example - header Date: 2026-07-19 -> 2026-07-20, matching the CHANGELOG's 1.17.0 date (repo convention per c536d76; check_versions.py does not guard this line) - the §7.9 realizations table row now mirrors the conformance bullet's engine-and-format gate; same wording applied to the matching $comment in rigs.schema.json - the example 'Rock Kit' declares the GM Power kit (16, 0-based) rather than Standard (0), and its sf2 program matches Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
…e the drum sound-binding precedence Reconciles this PR with #63, which landed while in review and took 1.17.0: - version retarget 1.17.0 -> 1.18.0 everywhere check_versions.py guards - drum sound binding composed with drums-as-arrangements, stated not implied: a type:drums arrangement binds sound through its entry tones (precedence); top-level drum_tones binds the song-level primary part and is the fallback for packs without drum arrangements; a Reader MUST NOT apply both to the same part, and a Writer SHOULD keep drum_tones mirroring the primary — the exact alias pattern #63 established for drum_tab - intent.gm kit absent => 0 (Standard), closing the asymmetry with the bank/program absent-=>-0 rule (review follow-up) - example: the primary drums arrangement carries entry tones; drum_tones mirrors it as the alias Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
73b8da6 to
a4f7657
Compare
|
Rebased onto #63 (drums as arrangements) merged while this was in review and took 1.17.0, which made this branch conflict. Rather than only resolving text, the rebase reconciles the two designs —
Also folded in from @topkoa's review-body corners (the cheap one): @topkoa: the force-push may have dismissed the approval — the delta since what you approved is exactly Full local check suite passes on the new head: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/feedpak-v1.md (2)
1416-1417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
sourcerig to the example.Since this release introduces
sourceblocks andsoundfontrealizations (and the extendedmanifest.yamlexample referencesgrand-pianoandrock-kitrigs), adding a simplesourcerig to this JSON block would clearly illustrate the new schema in action.💡 Proposed addition to the example
"graph": { // OPTIONAL; non-serial topology (parallel/stereo/wet-dry) "nodes": ["input", "amp", "output"], "edges": [["input", "amp"], ["amp", "output"]] } - } + }, + { + "id": "grand-piano", + "name": "Concert Grand", + "instrument": "keys", + "blocks": [ + { + "role": "source", + "intent": { + "kind": "instrument", + "gm": { "program": 0 } + }, + "realizations": [ + { + "engine": "soundfont", + "format": "sf2", + "ref": "sounds/piano.sf2", + "bank": 0, + "program": 0 + } + ] + } + ] + } ] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/feedpak-v1.md` around lines 1416 - 1417, Extend the JSON example block in spec/feedpak-v1.md with a simple source rig, demonstrating the newly introduced source block and soundfont realization schema. Use the existing grand-piano or rock-kit rig references from the manifest example so the example remains consistent with the documented configuration.
1400-1400: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the inline comment with the updated engine-and-format rules.
The conformance text and realization table now explicitly state that a Reader gates on
engineandformatwhen matching realizations. Updating this inline comment reflects that alignment.💡 Proposed fix
- "realizations": [ // ordered; reader renders the FIRST engine it supports + "realizations": [ // ordered; reader renders the FIRST realization whose engine (and format) it supports🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/feedpak-v1.md` at line 1400, Update the inline comment on the realizations field to state that readers render the first realization whose engine and format they support, aligning it with the conformance and realization-table rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/feedpak-v1.md`:
- Around line 1416-1417: Extend the JSON example block in spec/feedpak-v1.md
with a simple source rig, demonstrating the newly introduced source block and
soundfont realization schema. Use the existing grand-piano or rock-kit rig
references from the manifest example so the example remains consistent with the
documented configuration.
- Line 1400: Update the inline comment on the realizations field to state that
readers render the first realization whose engine and format they support,
aligning it with the conformance and realization-table rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 502e1d92-3240-4259-8e47-e2b4d5e65e72
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdexamples/extended.feedpak/manifest.yamlexamples/extended.feedpak/rigs.jsonschemas/manifest.schema.jsonschemas/rigs.schema.jsonspec/feedpak-v1.md
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- examples/extended.feedpak/rigs.json
- schemas/manifest.schema.json
- schemas/rigs.schema.json
topkoa
left a comment
There was a problem hiding this comment.
Re-approving after the rebase. Verified the delta claim with a range-diff: the first two commits carry the previously-approved content faithfully onto the new main (only #63's own changes appear), and the reconciliation is exactly a4f7657. That commit covers the full checklist — 1.18.0 consistent across every check_versions.py location, the drum sound-binding precedence stated normatively in §7.5 and mirrored in the §5.1 drum_tones row (entry tones win, drum_tones is the primary-part fallback, MUST NOT apply both — the alias pattern matching #63's drum_tab rule), and the extended example exercising both paths. Closing the kit-absent⇒0 asymmetry while in there was a nice extra. CI green on the new head. LGTM.
Implements FEP #61 (closes #61): a chart can now declare what a MIDI part — piano, keys, drums — should sound like, with a guaranteed General-MIDI floor. It extends the existing rig model (#44) rather than adding a parallel format.
What lands
soundfontrealization engine (sourceblocks only;sf2reserved,formatopen), addressed as libraryref+ voicebank/program(both OPTIONAL, absent ⇒0).sourceblock role — a generator at the head of the chain; its input is the Reader-supplied MIDI event stream (pack note data during playback or live controller input alike). Source-plus-effects composition falls out of the existing graph model (input(MIDI) → source → amp → reverb → output).intent.gm— a normative-when-present General-MIDI floor so a GM-only Reader still voices the part ({program}melodic;{percussion, kit}kits). Kit numbers are the 0-based wire/SF2 values (0 = Standard, 8 = Room, …), matchinggm.program.tones(§5.2), available whether or not the arrangement has afile, with manifest-wins-wholesale precedence over in-JSONtones(Writers SHOULD NOT emit both); new top-leveldrum_tonesanchors the drum part's binding next todrum_tab.engineandformat(a general clarification — already implicitly true forplugin); source blocks SHOULD fall back tointent.gmrather than skip; control events are honoured by whatever voices the part.Compatibility
Additive MINOR (1.17.0 → 1.18.0; retargeted after #63 took 1.17.0) per §4.2 — no required field removed, renamed, or repurposed. Unknown
engine/rolevalues are MUST-preserve (§7.9), and an older reader — never having heard ofintent.gm— voices nothing for such a part, exactly the silence it produces today. Every schema staysadditionalProperties: true, so a 1.18 pack validates under earlier schemas.Composed with #63 (drums as arrangements, 1.17.0): a
type: drumsarrangement binds sound through its entrytones(precedence); the top-leveldrum_tonesbinds the song-level primary part and is the fallback for packs without drum arrangements; a Reader MUST NOT apply both to the same part — the same alias pattern #63 uses fordrum_tab. Stated in §7.5 "Multiple drum parts".Two small normative additions beyond the FEP's literal text
Flagging these so they're reviewed as choices, not missed:
soundfontis scoped "sourceblocks only" in the reserved-engine list (a generator engine on an effect block is meaningless).bank/programare stated OPTIONAL, absent ⇒0(the SF2 default), closing an otherwise-undefined case.intent.gm.kitlikewise absent ⇒0(Standard) — from the review's underspecified-corners list.Open items intentionally left for follow-ups
None block this PR. Future MINORs may add non-
sf2soundfont formats (sfz,ds) —formatis left open for exactly that — as renderers appear.Checks
tools/check_versions.py,tools/validate.py(both example packs, dir + zip form), the pytest suite (57),mkdocs build --strict(link/anchor check), the Draft 2020-12 schema self-check, andruffall pass locally. The extended example pack exercises a source rig for both a keys grand piano and a drum kit, plus both binding keys.Spec side of the instrument-pathways program, co-owned with the editor team (see the FEP #61 thread); discussion is on Discord.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
drum_tonesfor the primary drum part and per-arrangementtonesbindings with wholesale override precedence.grand-pianoandrock-kitrig examples.engine/format) and tightened voiced-source skipping/control-event behavior.Documentation