Skip to content

docs: precise Ooura licensing statement, stale-name fixes, and the FFT design-note skeleton - #18

Merged
tap merged 2 commits into
mainfrom
claude/wave1-docs-provenance
Sep 17, 2026
Merged

tap merged 2 commits into
mainfrom
claude/wave1-docs-provenance

Conversation

@tap

@tap tap commented Sep 17, 2026

Copy link
Copy Markdown
Owner

What this changes

Wave 1 of the FFT plan, item "licensing/NOTICE + docs/fft-design.md skeleton" (audit Part 5, Part 8 item 2, F10, Part 6 P3, Part 13). Markdown plus one license text file — no header, CMake or CI file is touched.

  • NOTICE.md (canonical): fixes the stale names (include/dsptap/include/tap/dsp/, DSPTAP_FFT_CMSISTAP_DSP_FFT_CMSIS); replaces "permissive/public" with Ooura's notice quoted verbatim from readme.txt (the only upstream license text — the banner on the vendored fftsg.c is Tap's copy of it) and a precise statement of what it grants (use, copy, modify; distribution of the original package); what DspTap ships today (one file of the package plus its readme, textually identical to the 2006-12-28 fft.tgz except for the Tap-added banner and stripped trailing whitespace — the maintainer's reading is that this is within the intent of the distribution grant); what the forthcoming C++ port relies on (the modification grant — the port is a derivative work, not the original package); WebRTC's precedent described as what it is (a broader notice in their LICENSE that is not in fft.tgz and that DspTap does not rely on); the SPDX plan (LicenseRef-Ooura AND MIT for the port header, MIT elsewhere); readme.txt stays at third_party/ooura/readme.txt permanently (D6 as amended); the judgement-call caveat.
  • LICENSES/LicenseRef-Ooura.txt (new): readme.txt lines 140–145 verbatim, so the SPDX reference resolves in the REUSE layout.
  • docs/fft-design.md (new): the design-note skeleton with every section present and each measurement-dependent entry marked TODO(stage 1|1b|2a|2b|2c|3b|3c|4): purpose/scope (two profiles today, Q15/Q31 planned); per-profile contract table (double/float from fft.h and test_fft.cpp; Q15/Q31 from the Part 7 design; the audit's Part 6 N2 probe values labelled as such, pinned at 2a); split-radix vs radix-4-int32 rationale with full Welch 1969, Oppenheim & Weinstein 1972 and Oppenheim & Schafer references; fp-contraction policy; transliteration rules; empty per-target .text and icount tables; provenance, consumer follow-ups (MuTap THIRD_PARTY_NOTICES.md at 2c, itu-compliance.md glossary at 2b, ci.yml header-only at 2c, MuTap-Max transitive), the reasoning behind NOTICE's statement, the port header's banner ready to paste; a draft email to the author marked for the maintainer to send — nothing has been sent; an empty contract-changelog table.
  • README.md: Provenance and License sections only, aligned with NOTICE and pointing at it. The Build paragraph and the FIR-substrate table are untouched (other agents own them).
  • third_party/cmsis-dsp/VENDOR.md: the one stale MUTAP_FFT_CMSISTAP_DSP_FFT_CMSIS (Tap-authored file; its own rule protects vendored sources).

Why

The old NOTICE overstated Ooura's grant ("permissive/public") and named paths and options that do not exist. The port (Stage 2a–2c) will ship a derivative of fftsg.c, so the exact wording of what the notice grants, what DspTap ships today, and what the port relies on has to be written down before that lands — and the design note needs a home for the numbers each stage will produce.

Verification

  • cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DTAP_DSP_WERROR=ON && cmake --build build -j8 && ctest --test-dir build --output-on-failure at 03c397f: 160/160 pass, zero warnings (proves nothing but prose moved).
  • Relative links checked: all resolve except docs/fft-design.mdaudit-fft-and-code-smells.md, which lands with the plan PR docs: FFT audit and staged plan (revision 3.1) #25. docs: FFT audit and staged plan (revision 3.1) #25 merges before this PR, so the link resolves on main; the design note and README say so.
  • No performance claims are made; the design note marks the existing "~3×" backend numbers as to-be-re-measured.
  • Line lengths: prose ≤ 80 characters in the new/changed files; tables exempt.

Notes for the reviewer

  • Licensing wording is a maintainer judgement call, not legal advice; NOTICE says so and is canonical. The statement is deliberately narrow: it says what the derivative's distribution relies on, not that it is granted. The draft email is in the design note for the maintainer to send or not; it now asks the author whether WebRTC's broader wording is his and applies to fftsg.c.
  • Documented exception (forward-declared). The paste-ready banner in the design note carries a // prose block after the four house banner lines (Ooura's notice verbatim plus the derivative statement). The wave-2 port PR should tag that as a documented exception under this heading.
  • Not in this PR's ownership — LICENSE footer. LICENSE still says "this license covers DspTap's own wrapper code"; NOTICE now says "DspTap's own code". Whoever owns LICENSE should drop "wrapper" (one word) in a follow-up.
  • Stale names found in files this PR does not own (left for their owners): cmake/arm-cortex-m55-mps3.cmake lines 12 and 47 (MUTAP_BARE_METAL, Stage 1 legs owner, Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17); include/tap/dsp/fft.h:25 ("DspTap::fft static library" — the target is tap_dsp_fft / tap::dsp_fft; header owner). DSPTAP_API in tools/capi/dsptap_capi.h is the C ABI's own export macro, not stale.
  • Merge order: after docs: FFT audit and staged plan (revision 3.1) #25, then late in wave 1 per Part 12. Stage 1b: FFT benchmark scenarios and the instruction-count ratchet scaffold #21's tools/qemu_insn_plugin/ (MIT, MuTap-copied) needs no NOTICE entry per its reviewer; NOTICE's owned-path sentence covers it.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

…T design-note skeleton

NOTICE.md: fix include/dsptap/ -> include/tap/dsp/ and DSPTAP_FFT_CMSIS ->
TAP_DSP_FFT_CMSIS; replace "permissive/public" with Ooura's notice quoted
verbatim and a statement of what it grants (use, copy, modify; distribution of
the ORIGINAL package), what DspTap ships today (the original), and what the
forthcoming C++ port relies on (the modification grant; it is a derivative
work, not the original package); SPDX plan LicenseRef-Ooura AND MIT for the
port header, MIT elsewhere; readme.txt stays in-tree permanently.

docs/fft-design.md: the design-note skeleton per audit Part 8 item 2, with
every measurement-dependent entry marked TODO(stage N): per-profile contract
table (double/float filled from fft.h and its tests, Q15/Q31 from the Part 7
design), the split-radix vs radix-4-int32 rationale with the Welch 1969 and
Oppenheim & Weinstein 1972 references, the fp-contraction policy, the
transliteration rules, empty per-target size and icount tables, the
provenance and licensing statement with the port header's banner ready to
paste, a draft email to the address in Ooura's notice marked for the
maintainer to send (nothing sent), and an empty contract-changelog table.

README.md: Provenance and License sections aligned with NOTICE; one sentence
pointing at docs/ for the port plan and design note. Nothing is claimed to
have landed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

@tap tap left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hostile review B (process / downstream)

Reviewed f76abce against origin/main (5ca3b1c), the plan (docs/audit-fft-and-code-smells.md, Parts 3 rev 3 / 5 / 8 / 12), CLAUDE.md, STYLE.md and the PR template. Hosted suite rebuilt in a private scratch dir with -DTAP_DSP_WERROR=ON: configure/build clean, 0 warnings, 160/160 pass. CI: 12/12 check runs green on both workflows for this SHA.

Findings

1. Blocker (sequencing, not content) — the design note's first link is dangling and the branch it depends on has no PR.
docs/fft-design.md line 8 links audit-fft-and-code-smells.md. That file is not on main (git cat-file -e origin/main:docs/audit-fft-and-code-smells.md fails; docs/ does not exist on main at all), and list_pull_requests state=all head=tap:claude/fft-code-smells-audit-xk4a2g returns nothing — the plan branch has never been PR'd. After this PR merges, docs/ will contain one file whose plan-of-record link 404s. The PR body admits the gap but leaves the sequencing to chance.
Fix: open the plan PR from claude/fft-code-smells-audit-xk4a2g (it is rebased on main, one file, and merge-tree against this branch is clean) and merge it before this one; or, until then, point the link at the branch blob URL and switch it back in the plan PR.

2. Should-fix — readme.txt's permanent path contradicts itself inside this PR, and the paste-ready banner hardcodes the side that the plan says goes away.
Plan Stage 2c and D6: "move fftsg.c and readme.txt to tests/reference/ooura/". The design note repeats that ("where fftsg.c and readme.txt move at Stage 2c", Provenance section). Three paragraphs later the banner says "the original package's readme.txt is kept in-tree at third_party/ooura/readme.txt", NOTICE's last Ooura bullet says "third_party/ooura/readme.txt stays in-tree permanently", and README says "the readme stays in-tree permanently". The wave-2 port agent pastes the banner at 2a (path correct), and at 2c it becomes wrong, or 2c has to edit the banner, NOTICE and README — exactly the edit-free paste the brief wanted to avoid.
Fix: decide the permanent home now. Simplest: readme.txt stays at third_party/ooura/readme.txt and only fftsg.c moves; change the design-note sentence to say so, add one line noting the deviation from the Stage 2c/D6 wording (or amend the plan in the plan PR), and make the banner path-agnostic ("kept in-tree; NOTICE.md gives its path") so it never needs editing.

3. Should-fix — "distributes the original fftsg.c unmodified … which the distribution grant covers" overstates the facts.
third_party/ooura/fftsg.c lines 8-10 carry a DspTap-added provenance paragraph inside the copyright comment, so the file is not byte-identical to upstream; and the tree ships one source file plus readme.txt, not the "ORIGINAL package" (whose readme.txt lists nine sources plus two sample directories). NOTICE, README ("DspTap ships that original today") and the design note all repeat the claim, and the note's whole argument rests on "today is covered; the port is the new exposure".
Fix: state it as it is — "a single source file from the original package, unmodified apart from a three-line provenance comment appended to its banner, plus its readme.txt" — and either drop "which the distribution grant covers" or put it under the same judgement-call caveat as the derivative sentence.

4. Should-fix — the email commitment differs between the two texts the note says must be identical, and each names a different "here".
NOTICE: "the maintainer will attempt to reach the address … and record the outcome here" (NOTICE.md). Design note: "Whether to send it is the maintainer's decision … records the outcome here" (fft-design.md). The note's own rule: "NOTICE.md carries the same statement; when one changes, the other changes in the same PR." Plan Part 5 makes the attempt a maintainer judgement call, not a promise in a shipped notice.
Fix: one wording, one record. Suggest: NOTICE says "the maintainer decides whether to ask the address in the notice for an explicit statement; the decision and any outcome are recorded here at Stage 2c", and the design note's Outcome line points at NOTICE.

5. Should-fix (downstream) — the PR scopes itself to DspTap correctly but records only part of the consumer follow-up.
Plan Part 5 (last bullet) and Part 8 item 4 list MuTap's THIRD_PARTY_NOTICES.md rewrite in the 2c bump as a provenance surface; the diff never mentions THIRD_PARTY_NOTICES or MuTap-Max (grep of the diff: no hits). The note's "Provenance stays visible … through four things" lists the banner, NOTICE, the parity test and the itu-compliance.md glossary line and omits the consumer notice file. Anyone doing the 2c bump from this note will miss it.
Fix: add a short "Consumer follow-ups" list under Provenance: (a) MuTap THIRD_PARTY_NOTICES.md rewritten in the 2c bump with this statement (the notice will then live in a header compiled into every external); (b) MuTap docs/itu-compliance.md glossary line at the 2b bump; (c) MuTap ci.yml:319-324 header-only rewrite at 2c; (d) MuTap-Max: picks it up transitively, nothing to write unless it redistributes NOTICE separately.

6. Nit — the paste-ready banner diverges from STYLE.md §3 without saying so.
House banner is /// @file, /// @brief, // SPDX-License-Identifier, // Copyright … (fft.h and kaiser.h follow it exactly). The pasted banner interleaves Ooura's five-line notice between the SPDX line and DspTap's copyright, and the DspTap copyright is buried in prose ("Modifications Copyright 2026 … ; this is a derivative work …"). Part 5's requirements (notice verbatim, SPDX LicenseRef-Ooura AND MIT, a derivative-work line with modifications copyright and date) are met.
Fix: keep the four house lines first (// Copyright(C) 1996-2001 Takuya OOURA and // Copyright 2026 Timothy Place and the DspTap contributors (modifications). as lines 4-5), then a // prose block with the notice verbatim and the derivative statement; and tag it as a "Documented exception" per the PR template so the wave-2 porter and its tidy reviewer expect it.

7. Nit — one stage tag is stale under revision 3. "a platform-independent table is a Stage 3a candidate" copies Part 4's sentence, whose rationale ("because the fixed-point twiddles are generated there") is true of Stage 3b (fft/fixed_point.h, full Q1.30 table at construction), not 3a (traits substrate). Every other tag (1, 1b, 2a, 2b, 2c, 3b, 3c, 4, 5) checks out against Part 3 rev 3, and the note correctly says "Stage 5 view" where Part 5 still says "Stage 4 view". Fix: "Stage 3b candidate" here; the two stale audit sentences go to the plan PR.

8. Nit — the Welch-model-vs-measured-floors bullet sits under ### Derivations (TODO(stage 3b)) but its numbers come from the executed notebooks/fft.ipynb at Stage 3c (Part 8 item 6; Stage 3c). Tag the derivation 3b and the comparison 3c so the 3b PR is not held for a notebook it does not own.

9. Nit — "NOTICE.md carries the same statement" is not true today. The design note adds the WebRTC/Chromium sentence, the "permissive/public" retraction, and the judgement-call caveat, and names a different record location (finding 4). Pick one: NOTICE canonical (it is what ships) and the note says "NOTICE.md is canonical; this section gives the reasoning", or make the two blocks identical.

10. Nit — commit and PR hygiene. Trailers present and correct (Co-Authored-By, Claude-Session). Subject is 91 characters (STYLE.md sets no limit; conventional 72). PR body carries two footers: the template's "🤖 Generated with … / session URL" and an extra --- / _Generated by [Claude Code](…)_; keep one. Template sections are filled and the Verification section is truthful for a docs PR: 160/160 reproduced here, "warning-free" reproduced, link check accurate (only the audit link dangles; [docs/](docs/) resolves once this PR creates the directory), and the stale-name claims in Notes for the reviewer verified (arm-cortex-m55-mps3.cmake lines 12 and 47, VENDOR.md:5, fft.h:25; tap_dsp_fft is the real target at CMakeLists.txt:21). "Part 6 P3" exists (line 532). Contract-table numbers spot-checked against source: N = 1024, 1e-12 / 2e-5 (test_fft.cpp:35-37), < 1e-6 (:209), 62-line fftsg_float.c that #include "fftsg.c" at line 62, wrapper-side vDSP alignment (fft.h:128-137), 1.105e-7 / 1.19e-7 / 15.7 / 19.9 / 39.9 KB / 25 dB / 22 bits / 4√2 / << 14 / ~2,580 lines all match the audit.

Ownership and cross-PR hazards (checked, no findings)

  • Files touched: NOTICE.md, README.md, docs/fft-design.md — exactly the Part 12 allotment. third_party/ooura/readme.txt untouched. Nothing sent; the email is a fenced draft with "Nothing has been sent".
  • README hunks confined to lines 364-383 (Provenance / License). Sibling README hunks: #17 legs @329 (Build), #21 bench @333 (Build), #22 traits @248-293 (FIR table), #23 capi @308-323 (Notebooks) — all disjoint.
  • git merge-tree --write-tree of this branch against each of wave1-stage0-counter, stage1-legs, stage1b-bench, stage2a-tests, stage3a-traits, stage5-spectrum, capi-fft and against claude/fft-code-smells-audit-xk4a2g: clean (exit 0) in every case. No other wave-1 branch touches NOTICE.md.
  • Interaction to record for the bench reviewer: #21 adds tools/qemu_insn_plugin/insn_count.c ("copied from MuTap … Copyright 2026 MuTap contributors", MIT) and MuTap-adapted scripts/icount.py / bench/icount/. This PR's new NOTICE opening sentence declares tools/ "DspTap's own code". Both are MIT and the in-file attribution satisfies MIT, so whether a NOTICE line is warranted is #21's call; if #21 merges after #18, its owner should either add the line or accept the sentence as-is. Not a conflict.
  • Merge position: after the plan PR (finding 1), then late in wave 1 per the Part 12 left-to-right order (position 8 of 9, before capi #23). Nothing depends on this PR; merging it last means its README/NOTICE prose describes the tree the other wave-1 PRs actually produced.

Verdict: hold — one sequencing blocker (the plan PR does not exist) and four should-fix text corrections confined to the three owned files; no cross-PR conflicts; merge late in wave 1 once those land.


Generated by Claude Code

@tap tap left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hostile review A (accuracy)

Scope: every factual claim in NOTICE.md, README.md (Provenance/License hunks) and docs/fft-design.md, checked against third_party/ooura/readme.txt, the vendored fftsg.c, Ooura's current release (fft.tgz, kurims.kyoto-u.ac.jp, 2006-12-28, fetched today), fft.h, test_fft.cpp, the plan (revision 3), and WebRTC's tree for the precedent the note cites. Built and tested the branch in a private scratch dir (-DTAP_DSP_WERROR=ON): 160/160, zero warnings; the diff touches only the three Markdown files.

Blockers

1. "unmodified" is false, in four places.
Evidence — NOTICE: "Today DspTap distributes the original fftsg.c unmodified (…), which the distribution grant covers." README: "DspTap ships that original today". Design note: same sentence as NOTICE. Draft email: "It has shipped your fftsg.c unmodified for some time, with your readme and notice intact."
Fact: diff of Ooura's fft/fftsg.c (fft.tgz, 3314 lines) against the vendored file (3325 lines) gives ~60 hunks: an 11-line banner prepended at the top (the copyright notice plus the "This file … is vendored from …" paragraph), trailing whitespace stripped throughout, and the final blank line removed. readme.txt is likewise whitespace-stripped. More important: the upstream fftsg.c carries no copyright notice at all — the notice exists only in readme.txt; the banner at the top of the vendored file was added by Tap (MuTap's THIRD_PARTY_NOTICES.md says so: "The copyright/permission notice is retained at the top of fftsg.c"). DspTap also ships 2 of the package's 14 files, so it is not distributing "this ORIGINAL package" in any literal reading either.
Consequences: "Ooura's terms, stated in full in third_party/ooura/readme.txt and in the fftsg.c banner" (design note; NOTICE says the same) presents a Tap-authored banner as an upstream statement, and the email tells the author something untrue.
Fix (NOTICE and design note, replace the "Today DspTap distributes…" sentence): "Today DspTap ships one source file of the package, fftsg.c, plus its readme.txt. The vendored fftsg.c is textually identical to the 2006-12-28 fft.tgz except for a provenance banner Tap added at the top (the upstream file carries no notice of its own; the notice lives in readme.txt) and stripped trailing whitespace. That is a partial copy of the original package with the notice attached, not a modified transform; the maintainer's reading is that it is within the intent of the distribution grant, and the draft email puts the question to the author." Replace "stated in full in readme.txt and in the fftsg.c banner" with "stated in readme.txt, the only upstream license text (the banner at the top of the vendored fftsg.c is Tap's copy of it)". README: "DspTap ships that file today with the notice attached". Email: "It has carried your fftsg.c for some time, textually unchanged apart from a provenance comment we added at the top, with your readme.txt alongside."

2. A legal conclusion is stated as fact where there is no caveat.
Evidence — NOTICE and README: "…which the distribution grant covers." The "maintainer judgement call, not legal advice" sentence exists only in docs/fft-design.md (last line of "The licensing statement"); NOTICE.md and README.md have no such sentence. The design note also says "NOTICE.md carries the same statement; when one changes, the other changes in the same PR" — they already differ (the note has the WebRTC sentence, the "permissive/public" retraction and the caveat; NOTICE has none of the three).
Fix: add "This is a maintainer judgement call, not legal advice." to NOTICE's Ooura section (README can point at NOTICE); change "covers" per finding 1; change "carries the same statement" to "carries the short form of this statement" or make them identical.

3. The WebRTC precedent is mischaracterised, and it changes the rationale.
Evidence — design note: "the widespread practice of shipping modified Ooura code with the notice retained (WebRTC / Chromium under common_audio/third_party/ooura/, with a local-modifications README) — practice plus the author's informally reported lack of objection, not license text".
Fact (fetched today from webrtc.googlesource.com, refs/heads/main): common_audio/third_party/ooura/README.chromium is 15 lines, has no local-modifications section, says "Short Name: fft4g" and "License: LicenseRef-takuya-ooura", and the adjacent LICENSE file carries a different, broader notice: "Copyright Takuya OOURA, 1996-2001 / You may use, copy, modify and distribute this code for any purpose (include commercial use) and without fee. Please refer to this package when you modify this code." That text appears in neither fft.tgz nor fft2d.tgz (both carry only the narrow "ORIGINAL package" wording). So WebRTC is not "practice, not license text": it rests on license text that expressly grants modify-and-distribute, of provenance this PR has not traced. Separately, "the author's informally reported lack of objection" is uncited hearsay inside a licensing statement.
Fix: replace the parenthetical with: "(WebRTC/Chromium ship a modified fft4g.c under common_audio/third_party/ooura/; their LICENSE quotes a broader notice — 'You may use, copy, modify and distribute this code for any purpose (include commercial use) and without fee. Please refer to this package when you modify this code.' — that is not in the fft.tgz readme. DspTap has not traced the origin of that text and does not rely on it.)" Delete "the author's informally reported lack of objection" unless a citation is added. Add one sentence to the email asking whether that broader wording is the author's and applies to fftsg.c — it is the single most useful question to ask.

Should-fix

4. 1.105e-7 sits in a table that claims to be pinned.
Evidence — table preamble: "The header owns these numbers and the tests pin them"; Noise-floor row: "measured 1.105e-7 at N = 512 (audit Part 4)"; rationale: "was measured and is a wash (table max absolute error 1.19e-7 either way; … 1.105e-7 … 1.114e-7)". Source is the audit's Part 6 N2 reviewer probe; nothing in tests/, notebooks/ or fft.h produces or states these numbers.
Fix: "(audit Part 6 N2 probe; not reproduced by any committed test — re-measured and pinned at Stage 2a)" in both places.

5. readme.txt's future location contradicts itself.
Evidence — NOTICE: "third_party/ooura/readme.txt stays in-tree permanently … after fftsg.c itself has moved to the test reference tree"; banner: "kept in-tree at third_party/ooura/readme.txt"; design note, same file: "tests/reference/ooura/ (where fftsg.c and readme.txt move at Stage 2c)" — which is also what plan 2c and D6 say.
Fix: pick one. A license record does not belong under tests/; say "readme.txt stays at third_party/ooura/readme.txt; only fftsg.c moves at 2c (supersedes the plan's 2c/D6 wording on this point)", or the reverse, in all three places.

6. LicenseRef-Ooura has no license text file.
Evidence — "LicenseRef-Ooura is a project-local SPDX license reference whose text is third_party/ooura/readme.txt's copyright section". SPDX LicenseRef- is valid syntax, but the identifier has to resolve to a text; the REUSE convention is LICENSES/LicenseRef-Ooura.txt. The repo has no LICENSES/ directory, and a section of a 166-line readme is not a license file.
Fix: add LICENSES/LicenseRef-Ooura.txt (the Copyright: block of readme.txt, lines 139–145, verbatim) in this PR, or state in NOTICE "REUSE layout not adopted; LicenseRef-Ooura denotes the Copyright: block of third_party/ooura/readme.txt, lines 139–145" — one or the other, written down.

7. The email targets the wrong address first and carries an unverified title.
Evidence — "The address in the notice is ooura@mmm.t.u-tokyo.ac.jp and dates from 1996–2001, so a bounce is a likely outcome and is itself the record."; salutation "Dear Professor Ooura".
Fact: Ooura's current homepage (kurims.kyoto-u.ac.jp/~ooura/, "Last Updated: Dec 28, 2006", the site that serves the current fft.tgz) publishes ooura@kurims.kyoto-u.ac.jp (obfuscated in an HTML table), and fft2d's readme uses that same address. The title is unverified.
Fix: "Addresses: ooura@kurims.kyoto-u.ac.jp (current homepage and the fft2d readme) first, ooura@mmm.t.u-tokyo.ac.jp (the fft.tgz notice) second"; drop "a bounce is a likely outcome"; salutation "Dear Dr. Ooura" or "Dear Takuya Ooura". Otherwise the draft is brief, correctly marked as unsent and for the maintainer, and commits to nothing NOTICE does not already state.

8. Scope paragraph describes Q15/Q31 as existing.
Evidence — "tap::dsp::basic_real_fft<Sample> (…) is one real-FFT contract over four sample profiles — double …, float …, and the Q15 / Q31 fixed-point profiles from sample_traits.h." Today the class static_asserts float/double; real_fft_q15 / real_fft_q31 (table headers) do not exist.
Fix: "is today a two-profile contract (double, float); the plan (Stage 3) extends the same contract to Q15 / Q31" and mark the two alias names "(planned)" in the table header.

9. The dangling link is admitted in the PR body, not in the documents.
Evidence — README: "The plan … and its design note live in docs/"; design note: "the plan of record is audit-fft-and-code-smells.md". On main after this PR alone, docs/ contains only fft-design.md.
Fix: README: "the design note is docs/fft-design.md; the plan of record, docs/audit-fft-and-code-smells.md, lands with the audit branch"; design note: append "(lands with the audit PR; a forward link until then)".

10. VENDOR.md is misclassified as untouchable.
Evidence — PR body: "third_party/cmsis-dsp/VENDOR.md:5 (MUTAP_FFT_CMSIS — vendored, do not hand-edit; NOTICE now notes the discrepancy)". VENDOR.md is Tap-authored (the pinned-commit table, the refresh procedure); its own rule reads "Do not hand-edit vendored sources". The result is a workaround parenthetical in NOTICE for a one-word fix. (VENDOR.md also points at bench/README.md and docs/optimization.md, neither of which exists in this repo.)
Fix: change MUTAP_FFT_CMSIS to TAP_DSP_FFT_CMSIS in VENDOR.md in this PR and delete the parenthetical from NOTICE.

11. "wrapper" removed from NOTICE/README but not from LICENSE; owned-path list incomplete.
Evidence — LICENSE footer: "this license covers DspTap's own wrapper code"; NOTICE now says "DspTap's own code (include/tap/dsp/, tests/, tools/, notebooks/, the build files)" — scripts/, cmake/, docs/ are omitted.
Fix: "DspTap's own code" in LICENSE too; list cmake/, scripts/, docs/ or say "everything outside third_party/".

Nits

12. "The vendored fftsg.c was carried byte-for-byte inside MuTap and AmbiTap" — MuTap's tree no longer has its own copy (submodule), so this is unverifiable from disk, and "byte-for-byte" is the wrong phrase for a file that differs from upstream (finding 1). Say "textually identical between the two consumers".
13. Oppenheim & Schafer is cited without edition or section; give both. Welch 1969 (IEEE Trans. Audio Electroacoust., AU-17(2):151–157, June 1969) and Oppenheim & Weinstein 1972 (Proc. IEEE 60(8):957–976, Aug. 1972) check out against my knowledge; I could not verify them against the publishers through the proxy.
14. Line length: the repo's prose wraps at 80 (NOTICE.md max 80); docs/fft-design.md lines 122 (81) and 198 (92) exceed it. Tables are exempt.
15. NOTICE author line "Copyright (C) 1996-2001" vs the notice's "Copyright(C)"; the blockquotes and the banner are character-identical to readme.txt apart from re-wrapping.

Verified, no finding

Stale names on the branch: include/dsptap, DSPTAP_FFT_CMSIS, MuTap::fft — zero hits; MUTAP_ — only cmake/arm-cortex-m55-mps3.cmake:12,47 and VENDOR.md:5; fft.h:25 "DspTap::fft static library" is stale (target is tap_dsp_fft / tap::dsp_fft). The PR's list of three is accurate. Every TODO(stage N) (1, 1b, 2a, 2b, 2c, 3b, 3c, 4) and the "Stage 5 view" / "Stage 3a candidate" references map to revision 3's numbering. 1e-12 / 2e-5 at N = 1024 (RoundTripReproducesInput), FloatTracksDouble rms < 1e-6 at N = 1024, lazy table build (fft.h:349), 5ca3b1c = current main, fftsg_float.c = 62 lines, 38 globals × 2 = 76, nwh > 4, cftrec4 non-recursive, the 15.7 / 19.9 / 39.9 KB figures and the fp-contraction measurements match fft.h, test_fft.cpp and the plan. The Q15/Q31 rows contain only Part 7 design statements and TODO(stage 3b) tags, no invented measurements.

Verdict: not mergeable as-is — three blockers (a false "unmodified" claim repeated in NOTICE, README, the design note and the email; a legal conclusion stated as fact in the two files that carry no caveat; a WebRTC precedent that in fact rests on a broader notice this PR has not seen), all fixable in prose; the rest are consistency and labelling fixes.


Generated by Claude Code

…ews (#18)

NOTICE.md (canonical): the vendored fftsg.c is not "unmodified" — upstream
carries no notice (it lives only in readme.txt), the banner on the vendored
copy is Tap's, trailing whitespace is stripped, and DspTap ships 2 of the
package's files; state that, soften "which the distribution grant covers" to
the maintainer's reading, add the judgement-call caveat, drop the uncited
"informally reported lack of objection", and describe WebRTC's precedent as
what it is: a broader notice in their LICENSE that is not in fft.tgz and
that DspTap does not rely on. Owned-path list now covers everything outside
third_party/. LicenseRef-Ooura resolves to LICENSES/LicenseRef-Ooura.txt
(readme.txt lines 140-145 verbatim). readme.txt stays at third_party/ooura/
permanently; only fftsg.c moves at 2c (D6 as amended).

docs/fft-design.md: points at NOTICE as canonical and gives the reasoning;
same fact corrections; two profiles today, Q15/Q31 planned; the 1.105e-7 /
1.19e-7 / 1.114e-7 values labelled as the audit's Part 6 N2 probe, pinned at
Stage 2a; Oppenheim & Schafer cited with edition and section; Stage 3a -> 3b
for the platform-independent table; Welch derivation (3b) split from the
notebook numbers (3c); consumer follow-ups list (MuTap THIRD_PARTY_NOTICES at
2c, itu-compliance glossary at 2b, ci.yml header-only at 2c, MuTap-Max
transitive); banner restructured with the four house lines first and tagged
as a documented exception; email addressed to the author's current Kyoto
address first, "Dr.", no bounce prediction, corrected description of what
DspTap ships, and the question about WebRTC's broader wording; forward-link
note for the plan PR (#25).

README.md: Provenance/License aligned with NOTICE; VENDOR.md: the one stale
MUTAP_FFT_CMSIS -> TAP_DSP_FFT_CMSIS (Tap-authored file, not vendored source).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

tap commented Sep 17, 2026

Copy link
Copy Markdown
Owner Author

Fix pass — 03c397f

Both reviews addressed; reviewer A's fetched facts (fft.tgz, WebRTC LICENSE, the author's current homepage) were taken as overriding. Hosted build with -DTAP_DSP_WERROR=ON: 160/160, zero warnings. Files touched: NOTICE.md, README.md (Provenance/License), docs/fft-design.md, new LICENSES/LicenseRef-Ooura.txt, third_party/cmsis-dsp/VENDOR.md (one word).

Review A (accuracy)

# Finding Disposition
1 "unmodified" false in four places; banner presented as upstream Applied. NOTICE, design note, README and the email now say what ships: one file plus its readme, textually identical to the 2006-12-28 fft.tgz except for the Tap-added banner (upstream carries no notice; it lives in readme.txt) and stripped trailing whitespace. "stated in full in readme.txt and in the fftsg.c banner" → "stated in readme.txt, the only upstream license text (the banner is Tap's copy of it)". Email: "textually unchanged apart from a provenance comment we added at the top".
2 Legal conclusion as fact; caveat only in the design note Applied. "which the distribution grant covers" → "the maintainer's reading is that it is within the intent of the distribution grant, and the draft email puts the question to the author"; "This is a maintainer judgement call, not legal advice" added to NOTICE; NOTICE is canonical, README and the note point at it (the note "gives the reasoning").
3 WebRTC precedent mischaracterised; uncited "lack of objection" Applied. Parenthetical replaced with your wording (broader notice quoted, "not in the fft.tgz readme", "DspTap has not traced the origin of that text and does not rely on it"); "informally reported lack of objection" deleted everywhere; the email asks whether that wording is the author's and applies to fftsg.c.
4 1.105e-7 in a table that claims to be pinned Applied. Table preamble distinguishes test-pinned entries from sourced ones; the noise-floor row and the rationale paragraph label 1.105e-7 / 1.19e-7 / 1.114e-7 as the audit's Part 6 N2 reviewer-probe values, not reproduced by any committed test, pinned at Stage 2a.
5 readme.txt location contradicts itself Applied per D6 as amended in #25: readme.txt stays at third_party/ooura/readme.txt permanently; only fftsg.c moves at 2c. One statement in NOTICE, README and the note; the banner cites the fixed path.
6 LicenseRef-Ooura has no text file Applied. LICENSES/LicenseRef-Ooura.txt = readme.txt lines 140–145 verbatim (the Copyright: block; line 139 is blank); NOTICE says what the ref denotes and links the file.
7 Email address and title Applied. ooura@kurims.kyoto-u.ac.jp first, ooura@mmm.t.u-tokyo.ac.jp secondary; "bounce likely" dropped; "Dear Dr. Ooura".
8 Scope says Q15/Q31 exist Applied. "is today a two-profile contract … the plan's Stage 3 extends the same contract to Q15 / Q31"; table headers say "(planned)".
9 Dangling link admitted only in the PR body Applied. Design note: lands with the plan PR #25, which merges before this note. README: the plan of record lands with #25.
10 VENDOR.md misclassified as untouchable Applied. MUTAP_FFT_CMSISTAP_DSP_FFT_CMSIS in VENDOR.md; the workaround parenthetical removed from NOTICE. Its dangling bench/README.md / docs/optimization.md pointers are noted but left: they belong to whoever refreshes the vendored subset.
11 LICENSE "wrapper"; owned-path list incomplete Partly applied. NOTICE now says "everything outside third_party/" and lists bench/, scripts/, cmake/, docs/ too. LICENSE is outside this PR's ownership; flagged in the PR body for a one-word follow-up.
12 "byte-for-byte" Applied. "textually identical between the two consumers" in the note; README Provenance likewise.
13 Oppenheim & Schafer edition/section Applied. 3rd ed., Prentice Hall, 2010, Section 9.7 "Effects of Finite Register Length".
14 Line lengths 122, 198 Applied for 122 (prose). 198 is a table header (exempt by your rule) but shortened anyway ("double (host only)"). Whole-file check: prose ≤ 80 chars.
15 "Copyright (C)" vs "Copyright(C)" Applied. Copyright(C) as in the readme, everywhere.

Review B (process / downstream)

# Finding Disposition
1 Plan branch has no PR (blocker) Resolved upstream. The plan PR is #25 and merges before this one; the design note and README say so.
2 readme.txt path contradiction; banner hardcodes it Applied per D6 as amended: fixed path third_party/ooura/readme.txt, one statement everywhere, banner cites that fixed path (it never changes, so the paste needs no edit at 2c). Path-agnostic banner wording declined in favour of the fixed path, since D6 now fixes it.
3 "unmodified … which the distribution grant covers" Applied (see A-1, A-2).
4 Email commitment differs between the two texts Applied. One wording: NOTICE — "the maintainer decides whether to ask the author …; the decision and any outcome are recorded here at Stage 2c"; the note's Outcome line points at NOTICE.
5 Consumer follow-ups missing Applied. "Consumer follow-ups" list under Provenance: MuTap THIRD_PARTY_NOTICES.md at 2c, itu-compliance.md glossary at 2b, ci.yml:319-324 header-only at 2c, MuTap-Max transitive.
6 Banner diverges from STYLE.md §3 silently Applied. Four house lines first (@file, @brief, SPDX, Copyright(C) 1996-2001 Takuya OOURA, Copyright 2026 Timothy Place and the DspTap contributors (modifications).), then the // prose block with the notice verbatim; tagged as a documented exception in the note and forward-declared in the PR body's Notes for the reviewer.
7 "Stage 3a candidate" stale Applied. → Stage 3b ("the fixed-point twiddle generator lands there").
8 Welch bullet under 3b but numbers from 3c Applied. Split: derivation TODO(stage 3b), notebook comparison TODO(stage 3c).
9 "carries the same statement" untrue Applied. NOTICE canonical; the note gives the reasoning.
10 Commit subject length; double footer Partly applied. This commit's subject is 76 characters; the PR body carries one footer (the second _Generated by …_ line is appended by the tooling, not by the body).

Not done: nothing sent to the author (maintainer's call); LICENSE footer (not owned).


🤖 Generated with Claude Code

https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy


Generated by Claude Code

tap added a commit that referenced this pull request Sep 17, 2026
…size probe

Fix pass for the two hostile reviews on #21 (plan Part 13).

- Checksum: an integer FNV-1a-64 fold over the memcpy'd bit pattern of
  every output word replaces the float running sum. Exact and
  order-sensitive: a 1-ulp nudge of one spectrum bin at one iteration
  changes rfft_f32_512 from 0x662dd085b5b88325 to 0x259510dc8721cba8
  (reviewer A's perturbation, reproduced); a float sum absorbed it.
  Printed as 16 hex digits (%016llx) with a backend=cmsis|accelerate|ooura
  field from the visible macros; ok= is a round-trip sanity check.
- Seeding is not a gate bypass: the Seed mode runs only on push to main
  or workflow_dispatch; a pull request with an empty key fails with the
  seeding instructions, and one that empties a key its base branch had
  seeded fails against the base ref's baselines.json. Every run records
  the measured counts (icount.py --record) and uploads measured-<key>;
  a baseline entry with no binary fails (STALE BASELINE); per-scenario
  lines go to the step summary.
- seed-summary fails when any icount job failed and is documented as
  never a required check; concurrency group; push: branches [main];
  permissions: contents: read.
- Plugin API version: the pinned v8.2.2 header defines
  QEMU_PLUGIN_VERSION 1; bench.yml, README and insn_count.c now say v1.
- Sizes: tap_dsp_size_probe_rfft_f32_512 (bench/size_probe.cpp, one
  profile, no stdio) built MinSizeRel like the legs, measured with
  arm-none-eabi-size -A (.text row); per-key text_ceiling in bench.yml,
  0 = report only; ceilings are set by the wave-2 seeding commit.
- Host smoke tests under ctest: each icount binary runs twice, ok=1 and
  identical DONE lines (bench/icount/smoke.cmake).
- CMAKE_SYSTEM_NAME STREQUAL Generic decides host-class everywhere;
  fill() comment states the closed interval; k_engine_name is a
  const char*; SPDX/copyright on scripts/icount.py.
- Provenance: qemu-plugin.h is GPL-2.0-or-later, fetched at CI time,
  used only to build a CI tool, never shipped (README and insn_count.c);
  NOTICE.md untouched (#18 owns it).
- README: seeding records the main push run's URL and SHA; fixed table
  shape for updates; measured dilution (16.1/14.0/16.6 % of the count is
  copy + scale + fold) next to the 109/125/108 M host figures.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
tap added a commit that referenced this pull request Sep 17, 2026
…size probe

Fix pass for the two hostile reviews on #21 (plan Part 13).

- Checksum: an integer FNV-1a-64 fold over the memcpy'd bit pattern of
  every output word replaces the float running sum. Exact and
  order-sensitive: a 1-ulp nudge of one spectrum bin at one iteration
  changes rfft_f32_512 from 0x662dd085b5b88325 to 0x259510dc8721cba8
  (reviewer A's perturbation, reproduced); a float sum absorbed it.
  Printed as 16 hex digits (%016llx) with a backend=cmsis|accelerate|ooura
  field from the visible macros; ok= is a round-trip sanity check.
- Seeding is not a gate bypass: the Seed mode runs only on push to main
  or workflow_dispatch; a pull request with an empty key fails with the
  seeding instructions, and one that empties a key its base branch had
  seeded fails against the base ref's baselines.json. Every run records
  the measured counts (icount.py --record) and uploads measured-<key>;
  a baseline entry with no binary fails (STALE BASELINE); per-scenario
  lines go to the step summary.
- seed-summary fails when any icount job failed and is documented as
  never a required check; concurrency group; push: branches [main];
  permissions: contents: read.
- Plugin API version: the pinned v8.2.2 header defines
  QEMU_PLUGIN_VERSION 1; bench.yml, README and insn_count.c now say v1.
- Sizes: tap_dsp_size_probe_rfft_f32_512 (bench/size_probe.cpp, one
  profile, no stdio) built MinSizeRel like the legs, measured with
  arm-none-eabi-size -A (.text row); per-key text_ceiling in bench.yml,
  0 = report only; ceilings are set by the wave-2 seeding commit.
- Host smoke tests under ctest: each icount binary runs twice, ok=1 and
  identical DONE lines (bench/icount/smoke.cmake).
- CMAKE_SYSTEM_NAME STREQUAL Generic decides host-class everywhere;
  fill() comment states the closed interval; k_engine_name is a
  const char*; SPDX/copyright on scripts/icount.py.
- Provenance: qemu-plugin.h is GPL-2.0-or-later, fetched at CI time,
  used only to build a CI tool, never shipped (README and insn_count.c);
  NOTICE.md untouched (#18 owns it).
- README: seeding records the main push run's URL and SHA; fixed table
  shape for updates; measured dilution (16.1/14.0/16.6 % of the count is
  copy + scale + fold) next to the 109/125/108 M host figures.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy
@tap
tap merged commit 9c25237 into main Sep 17, 2026
12 checks passed
tap added a commit that referenced this pull request Sep 17, 2026
)

* Add the FFT benchmark scenarios and the instruction-count ratchet scaffold

Stage 1b of docs/audit-fft-and-code-smells.md: the measurement
infrastructure lands before the C++20 port so Stage 2b is ratcheted
against the vendored C rather than against nothing.

- bench/icount: one deterministic binary per scenario (rfft_f32_512,
  rfft_f32_2048, rfft_f64_512 on host-class targets only), compile-time
  selected, xorshift input, a checksum over every output printed as its
  bit pattern; 2^20 samples per direction so construction is <0.2% of
  the count (measured with callgrind on the host: 98M/114M/102M).
  TAP_DSP_BENCH_ENGINE selects the engine: reference_c today, with the
  split_radix hook documented for Stage 2a.
- bench/bench_fft.cpp: host wall-clock min-of-N microbenchmark,
  informational only.
- scripts/icount.py and tools/qemu_insn_plugin/insn_count.c adapted
  from MuTap (MIT): targets m4-softfp, m4f (mps2-an386), m33
  (mps2-an505), m55, m55-ooura (mps3-an547); +-3% two-sided gate;
  --merge folds per-key seeding files.
- bench/baselines.json: empty per-key skeleton. Numbers are measured on
  CI and committed with SHA, GCC and QEMU versions in bench/README.md,
  never typed in.
- .github/workflows/bench.yml: icount-ratchet matrix over the five
  keys, assuming the Stage 1 legs' toolchain files; an unseeded key
  runs --update and uploads its baselines as an artifact, a seeded key
  compares. arm-none-eabi-size reported per binary as the placeholder
  for the .text ceilings.
- Root CMakeLists: option(TAP_DSP_BUILD_BENCH) + add_subdirectory(bench).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

* bench: print checksums without the PRI macros

newlib's <inttypes.h> still hides PRIx32/PRIx64 from C++ behind
__STDC_FORMAT_MACROS, so the icount binaries failed to compile on the
M55 leg while building cleanly on glibc. Print through unsigned long /
unsigned long long with explicit casts instead. Host checksums are
unchanged (0x446b9819, 0xc7aa23fa, 0x408d7334f8ae008e).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

* bench: bit-exact checksum, no seeding from pull requests, MinSizeRel size probe

Fix pass for the two hostile reviews on #21 (plan Part 13).

- Checksum: an integer FNV-1a-64 fold over the memcpy'd bit pattern of
  every output word replaces the float running sum. Exact and
  order-sensitive: a 1-ulp nudge of one spectrum bin at one iteration
  changes rfft_f32_512 from 0x662dd085b5b88325 to 0x259510dc8721cba8
  (reviewer A's perturbation, reproduced); a float sum absorbed it.
  Printed as 16 hex digits (%016llx) with a backend=cmsis|accelerate|ooura
  field from the visible macros; ok= is a round-trip sanity check.
- Seeding is not a gate bypass: the Seed mode runs only on push to main
  or workflow_dispatch; a pull request with an empty key fails with the
  seeding instructions, and one that empties a key its base branch had
  seeded fails against the base ref's baselines.json. Every run records
  the measured counts (icount.py --record) and uploads measured-<key>;
  a baseline entry with no binary fails (STALE BASELINE); per-scenario
  lines go to the step summary.
- seed-summary fails when any icount job failed and is documented as
  never a required check; concurrency group; push: branches [main];
  permissions: contents: read.
- Plugin API version: the pinned v8.2.2 header defines
  QEMU_PLUGIN_VERSION 1; bench.yml, README and insn_count.c now say v1.
- Sizes: tap_dsp_size_probe_rfft_f32_512 (bench/size_probe.cpp, one
  profile, no stdio) built MinSizeRel like the legs, measured with
  arm-none-eabi-size -A (.text row); per-key text_ceiling in bench.yml,
  0 = report only; ceilings are set by the wave-2 seeding commit.
- Host smoke tests under ctest: each icount binary runs twice, ok=1 and
  identical DONE lines (bench/icount/smoke.cmake).
- CMAKE_SYSTEM_NAME STREQUAL Generic decides host-class everywhere;
  fill() comment states the closed interval; k_engine_name is a
  const char*; SPDX/copyright on scripts/icount.py.
- Provenance: qemu-plugin.h is GPL-2.0-or-later, fetched at CI time,
  used only to build a CI tool, never shipped (README and insn_count.c);
  NOTICE.md untouched (#18 owns it).
- README: seeding records the main push run's URL and SHA; fixed table
  shape for updates; measured dilution (16.1/14.0/16.6 % of the count is
  copy + scale + fold) next to the 109/125/108 M host figures.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.

1 participant