feat(signal): add FftT::inverse_real() — real-valued inverse FFT

[matthewfudge]

Adds inverse_real() to pulp::signal::FftT, mirroring the existing forward_real().

The gap

fft.hpp has forward_real() (vDSP branch + scalar fallback) but no real-valued inverse. Any caller doing real-signal spectral work — analyse a real block, modify the spectrum, resynthesise — has to hand-compose forward_real() → complex inverse() → take the real part at every call site. That's a round trip the library can do better, and it's easy to get the normalization wrong in caller code.

What it does

A straight mirror of forward_real's structure:

• inverse_real_vdsp() under PULP_FFT_HAS_VDSP for float — vDSP_fft_zrip with kFFTDirection_Inverse, packing DC/Nyquist back into realp[0]/imagp[0] (the inverse of the forward unpack), then unpacking to interleaved real samples.
• inverse_real_fallback() — complex inverse(), take the real part.
• Header-only, no new deps, no existing function changes.

Normalization follows the existing convention: forward()/forward_real() are unnormalized, inverse() applies 1/N once — so inverse_real(forward_real(x)) == x with no caller-side scaling.

Scaling derivation. Web sources conflicted, so I measured it against real vDSP_fft_zrip (N=16, random real signal): the raw forward zrip output is 2× the true DFT (which is what forward_real_vdsp's existing 0.5 corrects), and feeding the corrected spectrum into the raw inverse yields exactly size × x — so a plain 1/size_, matching inverse_vdsp()'s complex case, not the 1/(2·size_) some summaries suggest. The derivation and the empirical check are in the code comment.

RT contract preserved. The class comment promises allocation-free-after-construction, so the fallback's scratch is allocated once in the constructor (mirroring twiddles_) — and only where the fallback can actually be reached, so FftT<float> on Apple carries nothing. inverse_real() is wired into the existing allocation-free probe in test_signal_rt_safety.cpp.

Tests

In test_signal_spectral.cpp (not test_fft_backends.cpp — MultiBackendFft only exposes the complex transforms, and the existing forward_real coverage lives in spectral):

• round-trip identity across sizes {64,128,256,1024,4096} × {impulse, DC, sine, multi-tone, seeded random}
• the same round-trip via Fft64 to force the fallback branch (the if constexpr gate means FftT<float> always takes vDSP on Apple, so a float-only test can't observe the fallback at all)
• inverse_real vs the hand-composed inverse()+real-part, proving the vDSP branch matches the fallback's semantics — an independent oracle for the scale factor, since the vDSP path can't be turned off at runtime
• DC-only / Nyquist-only reconstruction and an alternating-signal round trip — that's where real-FFT packing bugs live
• move-constructed / move-assigned coverage, using Fft64 deliberately: the scratch is only touched by the fallback, so a float move test would pass whether or not it's transferred

pulp-test-signal-spectral 31,839 assertions / 27 cases pass; pulp-test-signal-rt-safety passes; pulp-test-fft-backends unaffected.

Version bumped 0.668.0 → 0.669.0 per the public-header minor rule, in its own chore: bump versions commit.

[cubic-dev-ai]

All reported issues were addressed across 4 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[matthewfudge]

Good catch — that's a real bug and I've fixed it in 2869aa9d0.

FftT's move ctor and move assignment enumerate their members explicitly, and I'd added inverse_real_scratch_ without adding it to either. So a move-constructed FftT got a default-constructed (empty) scratch, and a move-assigned one kept its own old (possibly wrong-sized) buffer — either way inverse_real_fallback() then writes out of bounds. Both operations now transfer it.

Also added move coverage to test_signal_spectral.cpp. One wrinkle worth noting: the test has to use Fft64, not Fft. inverse_real_scratch_ is only touched by inverse_real_fallback(), and FftT<float> takes the vDSP branch on Apple — so a float-only move test passes whether or not the scratch is transferred. Fft64 never satisfies the vDSP gate, so it's the only variant that can actually observe this. (My first attempt at the test used Fft and was quietly vacuous.) There's a float move section too, but it's only pinning that the vDSP-side members survive a move.

Verified fail-before/pass-after: without the fix the Fft64 move-constructed section SIGSEGVs; with it, 768 assertions pass. Full suites green — pulp-test-signal-spectral 31839 assertions / 27 cases, pulp-test-signal-rt-safety 40/19, pulp-test-fft-backends 95/7 unaffected.

[shipyard-local]

Reviewed by building and running it, not by reading it.

Verified locally:

• test_signal_spectral — 31,839 assertions pass
• test_signal_rt_safety — passes with inverse_real added, so it is genuinely allocation-free in the audio callback (the claim that actually matters for a plugin)
• Branch is rebased on current main (759669953) — clean

The cross-validation is the right one. On macOS with float, inverse_real() always takes the vDSP branch, so the only honest way to check the scale factor is against an independent oracle. Comparing against the hand-composed forward_real → complex inverse() → real part route is exactly that, and asserting the imaginary part is ~0 additionally proves the input really was conjugate-symmetric. That's the test that would catch a wrong 1/N vs 1/(2N). Fft64 genuinely drives the scalar fallback (is_same_v<SampleType, float> never holds there). Move constructor and move assignment both transfer the new scratch buffer correctly.

On the version bump: keep it. 0.668.0 → 0.669.0 is correct — our gate requires a minor bump for a public-header change, and shipyard pr would have applied it anyway. Nothing to drop.

---

One non-blocking nit (happy to take this as a follow-up rather than another round trip):

inverse_real_scratch_.resize(size) runs unconditionally in the constructor, but the buffer is only ever touched by inverse_real_fallback(). On Apple with float — the common case — it is never used. A 65536-point FftT<float> therefore carries ~512 KB of dead memory per instance. A one-line guard on the vDSP path would avoid it.

---

Two things from your side that were also right, for the record:

The 0.5 in forward_real_vdsp is not a bug, and you were right not to "fix" it.

And the stale-fork problem you hit is real and was ours to document — a fork several hundred commits behind makes our pre-push gates diff against the wrong base, so they report node-ABI "removals" and hotspot growth in files you never opened, and the message names your branch. CONTRIBUTING.md now says so, with the resync recipe. That same staleness is almost certainly the cause of the Pulp::midi vs pulp::midi report: on current main, find_package(Pulp) already aliases every Pulp::x to a lowercase pulp::x, so target_link_libraries(you PRIVATE pulp::midi) works. No CMake change needed on either side — just resync.

Also: the MIDI writer bugs you reported are all four confirmed and fixed in #6116. Your agent read the write path correctly down to the constant.

[matthewfudge]

Thanks — and thanks for reviewing it by building and running it rather than reading it. That's the review I'd want.

Nit taken, not deferred — pushed. inverse_real_scratch_ now only allocates where inverse_real() can actually reach the fallback:

static constexpr bool uses_inverse_real_fallback() {
#if PULP_FFT_HAS_VDSP
    return !std::is_same_v<SampleType, float>;
#else
    return true;
#endif
}
...
if constexpr (uses_inverse_real_fallback()) inverse_real_scratch_.resize(size);

It mirrors inverse_real()'s own dispatch rather than restating the condition, so the two can't drift apart if the gate ever changes. FftT<float> on Apple now carries nothing; Fft64 still allocates it because it genuinely uses the fallback — and the Fft64 move test still passing is what proves the guard didn't over-reach. test_signal_spectral 31,839 assertions, test_signal_rt_safety 19/19.

On the stale fork — thank you for documenting it. I'd assumed it was my own mess. Good to know find_package(Pulp) already aliases Pulp::x → pulp::x on current main; our helpers handle both spellings, which turns out to have been a symptom of the stale install rather than anything real. We'll resync rather than carry the workaround.

And thank you for #6116 — four-for-four, that was fast. We'd already started writing our own SMF writer on our side, since our .mid export needs a tempo meta at PPQN 480 and we didn't want to block on an upstream round trip. We'll finish that, then compare against #6116 at our next SDK bump and switch if yours is the better home for it. If it is, I'm happy to bin ours — the last thing we want is a private fork of something you maintain.

Which is the general shape of how we'd like to work with pulp, if it suits you: when we need a capability that isn't there, build it, use it, and send it back — rather than filing a request and waiting. #6115 (joint-channel block oversampling + a second half-band design lane) is the other one currently open.

(Edited: removed references to another framework by name — that's your house rule from #6115 and it should apply to my comments too, not just the source.)

[shipyard-local]

Heads-up on the one red check — this is our papercut, not a mistake on your side.

Enforce version & skill sync is failing. You bumped the version correctly (0.668.0 → 0.669.0 is exactly right for a public-header change). The gate is fussier than it should be: it wants the bump to live in its own commit, whose subject is exactly

chore: bump versions

and yours is folded into the feature commit, so the gate can't see a marker.

One-line fix — split the version bump into its own commit:

git reset --soft HEAD~1              # unstage your commit, keep the changes
git restore --staged CMakeLists.txt
git commit -m "feat(signal): <your original subject>"   # the code
git add CMakeLists.txt
git commit -m "chore: bump versions"                    # the marker — exact subject
git push --force-with-lease

The subject has to match exactly — chore: bump SDK to v0.669.0 and similar near-misses don't count.

Why you hit this and we don't: shipyard pr creates that marker commit automatically, and it's the path we use internally — but it can't be used from a fork, so every external contributor walks straight into this. That's on us. I'm adding it to CONTRIBUTING.md.

While I'm here — main has moved since you branched, and both of your open PRs (#6114 and #6115) claim 0.669.0, as does a large PR of mine that's landing now. Whichever merges first takes that number and the others will conflict on the version line. Nothing to do pre-emptively; just expect to re-bump whichever lands second, and don't be surprised by the conflict.

[matthewfudge]

Fixed — 736f70e62. The bump is now its own commit with the exact chore: bump versions subject, touching only CMakeLists.txt; the feature commit is code + tests only.

That one was mine rather than the gate's, for the record: the branch originally did have the split, and I collapsed it myself when I amended the scratch-buffer fix in without noticing the bump rode along. The gate was right to complain.

test_signal_spectral still 31,839 assertions, tree clean.

Worth noting #6115 already had the split and its Enforce version & skill sync passes — so the gate does exactly what it says on the tin once the marker commit exists. If CONTRIBUTING.md is getting a note anyway, the thing that would have saved me is simply "the bump must be its own commit — don't amend it into the feature commit", since the failure mode is invisible from the diff (the version line is correct, so you assume the gate is checking the wrong thing).

Noted on the 0.669.0 collision with your landing PR — we'll re-bump whichever of ours goes second and won't touch it pre-emptively.

[shipyard-local]

Clear next step so this does not sit in limbo:

Matthew: please rebase this branch onto current main (7488740d) and resolve the version collision/conflicts, including choosing the next valid version required by the existing gate. The current head (736f70e62) is CONFLICTING, and the earlier approval was submitted against the pre-rewrite head, so GitHub correctly does not treat it as a current approval.

There is no additional design change being requested at this point. Once the reconciled head is pushed, we will re-run the focused spectral and real-time-safety validation against that exact SHA and either approve it or report a concrete remaining blocker.

[shipyard-local]

Correction to the next-step assignment above: no action needed from you right now. main is moving quickly, and asking you to chase its current SHA and our internal version gate would create avoidable churn.

We will handle the final rebase/version reconciliation on the maintainer side at merge-prep time, preserve your authorship, and re-run the focused validation on the resulting exact head. We will come back to you only if that exposes a substantive issue in the contribution itself.

[shipyard-local]

Maintainer integration is complete on current main at exact head c2f37ca01fecdbddc6529f4646101df618a585d8.

• Matthew remains the author of the feature commit.
• The repository-owned version reconciliation is a separate chore: bump versions commit (0.675.0).
• The remaining foreign-framework reference in the old commit message was rewritten in capability-neutral language; the code is unchanged by that cleanup.
• Fresh exact-head validation passed: configure, pulp-test-signal-spectral, pulp-test-fft-backends, pulp-test-signal-rt-safety, and the focused CTest selection.
• The branch is now conflict-free and GitHub reports it mergeable.

No substantive contributor change remains requested.

Next action — Daniel: final review and merge decision on this exact head.

[shipyard-local]

/shipyard review

[shipyard-local]

Shipyard review passed for c2f37ca01fec (3 steps).

[matthewfudge]

Please hold this one — it's green and mergeable, but it doesn't meet your contributor contract and I'd rather say so than let it through.

Two gaps, both ours:

• No DCO sign-off. Neither commit carries Signed-off-by:. Since that sign-off is the certification that we can submit under MIT, it isn't something to add casually — it needs to be right.
• No provenance statement in the description.

We're also rethinking our contribution shape more broadly. We'd rather send you plans than patches — the capability, the evidence, and the published method, so design and implementation stay with your team and no provenance question lands on you at all. #6146 is the first of those, and it's how we'd like to work from here.

So: don't merge this on our account. We'll either come back with the sign-off and provenance done properly, or close it and fold the capability into the plan — either way, the ask is unchanged and it's in #6146 (§3, the real-valued inverse, including the move-constructor/scratch-buffer trap and the point that a round-trip test which takes the accelerated path can't fail in the fallback branch).

Sorry for the noise — better a retraction than a licence question you never asked for.

[shipyard-local]

/shipyard review