refactor(#299): スペクトラムのタップ実サンプルレートを解析器まで伝搬#303
Merged
Conversation
process tap のミックスダウンは出力デバイスのレートに追従するため、44.1kHz 環境では 48kHz 固定前提だと Hz→FFT bin マッピングと帯域カットオフ (min_freq/max_freq) がズレる。#298 レビュー (CodeRabbit/Copilot) 指摘の うち HW 依存 1 件目 (課題1) を解消する。 - ProcessTapEngine が構築時に kAudioTapPropertyFormat からタップの実レート を読み取り公開 (読めなければ 48000 フォールバック)。 - AudioTapDataSourceImpl.latestSamples が各 StereoSamples 窓に実レートを付与 (StereoSamples に sampleRate フィールド追加、capture 無しのプレースホルダは 48000 だが解析器は空窓で走らない)。Repository は StereoSamples をそのまま 転送するので無改修。 - SpectrumUseCaseImpl が window.sampleRate を読み、メモ化 FrequencyAnalyzer の キーを (bars, sampleRate) に拡張 → デバイスのレート変更で再構築。 物理周波数が実レートに依らず同じバーに載り、帯域カットオフも正確になる。 FrequencyAnalyzer は既にレート引数を受け取る設計だったので、伝搬のみ。 サンプルレート不変性 (FrequencyAnalyzer) と伝搬 (SpectrumUseCase) を ユニットテストで検証。 課題2 (リフレッシュレート非依存化) は既存の見た目を変え実機再チューニングが 要るため別対応。 Refs #299
📝 WalkthroughWalkthroughThis PR propagates the audio process tap’s actual sample rate through the spectrum pipeline. The tap engine now resolves and exposes a real rate, samples carry it, and spectrum analyzer caching now keys on both bar count and sample rate. ChangesSample-rate propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
CoreAudio の process tap が出力デバイスの実サンプルレートに追従する前提に合わせて、キャプチャ経路からスペクトラム解析器までサンプルレートを伝搬し、Hz→FFT bin マッピングと min_freq / max_freq の解釈ズレを解消するリファクタです。
Changes:
ProcessTapEngineが tap のストリームフォーマットから実サンプルレートを読み取り、StereoSamplesにsampleRateを追加して窓ごとにタグ付けSpectrumUseCaseImplがwindow.sampleRateを用いてFrequencyAnalyzerを(bars, sampleRate)でメモ化- サンプルレート伝搬に関するユニットテストを追加し、バージョンを
2.20.1に更新
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/SpectrumUseCaseTests/SpectrumUseCaseImplTests.swift | サンプルレート伝搬により同一PCMでもバーが変わることを検証するテストを追加 |
| Tests/FrequencyAnalyzerTests/FrequencyAnalyzerTests.swift | 物理周波数がサンプルレート差に依らず同じバーにマップされることを検証 |
| Sources/VersionHandler/Resources/version.txt | バージョンを 2.20.1 に更新 |
| Sources/SpectrumUseCase/SpectrumUseCaseImpl.swift | window.sampleRate を解析器へ渡し、メモ化キーを (bars, sampleRate) に拡張 |
| Sources/Entity/StereoSamples.swift | StereoSamples に sampleRate フィールドを追加（既定 48000） |
| Sources/AudioTapDataSource/ProcessTapEngine.swift | tap のフォーマットから実サンプルレートを取得・公開 |
| Sources/AudioTapDataSource/AudioTapDataSourceImpl.swift | latestSamples が窓に tap の実サンプルレートを付与 |
| .claude/CLAUDE.md | #299 のサンプルレート伝搬方針を設計ドキュメントへ追記 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+20
to
+24
| /// Memoized analyzer plus the per-channel bar count and tap sample rate it | ||
| /// was built for. The displayed bar count is derived from the overlay width | ||
| /// (cava style), so it changes on resize, and the sample rate follows the | ||
| /// output device (#299) — rebuild when either changes. | ||
| private var analyzer: (bars: Int, sampleRate: Double, engine: FrequencyAnalyzer)? |
Owner
Author
There was a problem hiding this comment.
ご指摘の通りです。ProcessTapEngine.sampleRate は tap 構築時に確定する let で、稼働中は更新されません。コメントを、tap 再生成（再生/一時停止・ソース変更）時に新デバイスレートを拾って再構築される、という実挙動に合わせてしました。6881450
| **Spectrum cava-faithful smoothing + configurability (#297)**: `SpectrumPresenter.tick()` runs on the DisplayLink and applies a faithful port of cavacore.c (MIT): per-bar sensitivity scaling → gravity release → **non-normalized leaky integrator** (sustained energy compounds toward `1/(1-noise_reduction)` ≈ 4× at 0.77, so beats tower over one-frame transients — the reason cava's kick band reads prominent) → clamp at full height, with the gain auto-tuned from overshoot (cava's **autosens**, moved here from the UseCase). Three knob families are curated-default-but-configurable, following the philosophy of showing the good part without forcing setup: (1) **bar count is derived from the overlay size cava-style** (fixed `bar_width` + `bar_spacing`, count fills the track) — the View reports the track length via `SpectrumPresenter.updateBarTrackLength` (overlay width for vertical placements, height for horizontal), `targetBarCount` derives the count, and `SpectrumUseCase.magnitudes(style:barCount:)` rebuilds the `FrequencyAnalyzer` when the count changes; (2) **band cutoffs** `min_freq`/`max_freq` (default 40 Hz–14 kHz) — `FrequencyAnalyzer` maps Hz→FFT bin (at the 48 kHz tap rate) and log-divides that range; (3) **gradient direction** `frequency`/`amplitude`/`level` and **placement** `bottom`/`top`/`left`/`right`/`underlay` — `left`/`right` rotate the bars into horizontal columns growing inward, `spectrumBarRects` computing an edge-anchored growth axis vs. an edge-parallel track axis so one geometry covers all four edges (`SpectrumGradientDirection`, `SpectrumPlacement` in Entity). The growth-axis size is `height_ratio` (fraction of the axis) plus an optional absolute clamp `min_height`/`max_height` in points (CSS `min-height`/`max-height` semantics, min wins on conflict) — the pure testable `spectrumBarStripDepth` free function resolves it, so a ratio-based length stays sane across very different displays (e.g. capping a horizontal placement on an ultrawide). | ||
| **Spectrum cava-faithful smoothing + configurability (#297)**: `SpectrumPresenter.tick()` runs on the DisplayLink and applies a faithful port of cavacore.c (MIT): per-bar sensitivity scaling → gravity release → **non-normalized leaky integrator** (sustained energy compounds toward `1/(1-noise_reduction)` ≈ 4× at 0.77, so beats tower over one-frame transients — the reason cava's kick band reads prominent) → clamp at full height, with the gain auto-tuned from overshoot (cava's **autosens**, moved here from the UseCase). Three knob families are curated-default-but-configurable, following the philosophy of showing the good part without forcing setup: (1) **bar count is derived from the overlay size cava-style** (fixed `bar_width` + `bar_spacing`, count fills the track) — the View reports the track length via `SpectrumPresenter.updateBarTrackLength` (overlay width for vertical placements, height for horizontal), `targetBarCount` derives the count, and `SpectrumUseCase.magnitudes(style:barCount:)` rebuilds the `FrequencyAnalyzer` when the count changes; (2) **band cutoffs** `min_freq`/`max_freq` (default 40 Hz–14 kHz) — `FrequencyAnalyzer` maps Hz→FFT bin (at the tap's actual sample rate, propagated from the capture path — #299) and log-divides that range; (3) **gradient direction** `frequency`/`amplitude`/`level` and **placement** `bottom`/`top`/`left`/`right`/`underlay` — `left`/`right` rotate the bars into horizontal columns growing inward, `spectrumBarRects` computing an edge-anchored growth axis vs. an edge-parallel track axis so one geometry covers all four edges (`SpectrumGradientDirection`, `SpectrumPlacement` in Entity). The growth-axis size is `height_ratio` (fraction of the axis) plus an optional absolute clamp `min_height`/`max_height` in points (CSS `min-height`/`max-height` semantics, min wins on conflict) — the pure testable `spectrumBarStripDepth` free function resolves it, so a ratio-based length stays sane across very different displays (e.g. capping a horizontal placement on an ultrawide). | ||
|
|
||
| **Spectrum sample-rate propagation (#299)**: the process-tap mixdown follows the current output device, so its rate is 44.1 kHz, 48 kHz, or whatever the hardware runs at — not a fixed 48 kHz. `ProcessTapEngine` reads the tap's real rate from `kAudioTapPropertyFormat` at construction and exposes it; `AudioTapDataSourceImpl.latestSamples` tags each `StereoSamples` window with it (an Entity field, default 48000 for the no-capture placeholder that the analyzer never runs on); `SpectrumUseCaseImpl` reads `window.sampleRate` and keys the memoized `FrequencyAnalyzer` on `(bars, sampleRate)`, so a device rate change rebuilds it. This keeps a physical frequency on the same visual bar and the `min_freq`/`max_freq` cutoffs accurate regardless of the hardware rate. (#299 also tracks a second, decoupled item — refresh-rate-independent smoothing — which changes the tuned look and needs real-device re-tuning, so it ships separately.) |
Owner
Author
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
4 tasks
ProcessTapEngine.sampleRate は tap 構築時に一度だけ読み取る let であり、 稼働中のデバイス切替では更新されない。よって「device rate change rebuilds the analyzer」という表現は過大で、正確には「tap 再生成時に新レートを拾い 再構築される」が正しい。 SpectrumUseCaseImpl.swift のメモ化コメントと CLAUDE.md #299 ノートを いずれも実挙動（tap lifetime 中は固定、tap 再生成で更新）に合わせて修正。 Addresses Copilot review comments: #3523264777, #3523264792
…mSampleRate Pull the `format.mSampleRate > 0` judgment out of the CoreAudio call site in `streamSampleRate(of:)` into a standalone, dependency-free free function `tapSampleRate(from: AudioStreamBasicDescription) -> Double?`. `streamSampleRate` now delegates to it after a successful `AudioObjectGetPropertyData` call, keeping the OS-boundary crossing isolated while making the pure decision unit-testable. Tests added to ProcessTapEngineHelpersTests: - 44.1 kHz → 44100 - 48 kHz → 48000 - 0 Hz → nil (no audio stream) - -1 Hz → nil (malformed descriptor)
…e paths The existing tests exercised two distinct SpectrumUseCaseImpl instances, so the cache-hit branch of `resolvedAnalyzer` (return analyzer.engine when bars and sampleRate both match) was never reached. Two new tests on the *same* use-case instance: - analyzerCachedOnMatchingKey: identical bar count + sampleRate → first == second, proving the analyzer is reused (covers the cache-hit line). - analyzerRebuildsOnSampleRateChange: mutates FakeAudioCaptureRepository to yield a 44.1 kHz window after the initial 48 kHz call; first != second, proving the FrequencyAnalyzer is rebuilt when the tap rate changes on the same instance.
…mplerate # Conflicts: # Sources/VersionHandler/Resources/version.txt
latestSamples の sampleRate タグ付け(#299 の肝)と startTap/stopTap の tap 存在パスは、engine が ProcessTapEngine(ライブ CoreAudio タップ必須)に 直結していたためテスト不能で未カバーだった。ProcessTapEngine を AudioTapEngine プロトコルの背後にシーム化し、生成を factory 注入にすることで: - fake tap を注入してライブ音声デバイス無しで tap 存在パスを検証可能に - 散在していた #available / as? ProcessTapEngine のチェックが factory に集約 され production コードも簡潔化 FakeTapEngine で latestSamples のレートタグ付け・再起動時の旧タップ破棄・ factory nil 時の失敗・stopTap の破棄+クリアをカバー(4 テスト追加)。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/SpectrumUseCase/SpectrumUseCaseImpl.swift (1)
20-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCache key omits other analyzer-affecting
SpectrumStylefields.
resolvedAnalyzerrebuilds only onbars/sampleRatechanges, but the constructedFrequencyAnalyzeralso depends onstyle.fftSize,minDb,maxDb,scale,minFrequency, andmaxFrequency. If any of those change without a bar-count or sample-rate change, the stale cached engine is returned, silently mismatching the newly fetchedstyle.fftSize-sized window.This predates this PR (previously keyed by
barsalone), but since the cache key is being revisited here, consider broadening it.♻️ Possible direction: widen the cache key
- private var analyzer: (bars: Int, sampleRate: Double, engine: FrequencyAnalyzer)? + private var analyzer: (bars: Int, sampleRate: Double, style: SpectrumStyle, engine: FrequencyAnalyzer)? ... - if let analyzer, analyzer.bars == bars, analyzer.sampleRate == sampleRate { + if let analyzer, analyzer.bars == bars, analyzer.sampleRate == sampleRate, analyzer.style == style { return analyzer.engine }(Requires
SpectrumStyle: Equatable.)Can you confirm whether
SpectrumStyle's other fields (scale, frequency bounds, fftSize) are ever mutated at runtime on a live instance independent of a resize/rate change?
Also applies to: 65-86
🤖 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 `@Sources/SpectrumUseCase/SpectrumUseCaseImpl.swift` around lines 20 - 26, The analyzer cache key in SpectrumUseCaseImpl only tracks bars and sampleRate, but resolvedAnalyzer also depends on SpectrumStyle fields like fftSize, minDb, maxDb, scale, minFrequency, and maxFrequency. Update the memoization in SpectrumUseCaseImpl (especially the analyzer storage and resolvedAnalyzer logic) so any change to these style properties forces a rebuild, likely by widening the cached key and making SpectrumStyle Equatable if needed.
🤖 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 `@Sources/SpectrumUseCase/SpectrumUseCaseImpl.swift`:
- Around line 20-26: The analyzer cache key in SpectrumUseCaseImpl only tracks
bars and sampleRate, but resolvedAnalyzer also depends on SpectrumStyle fields
like fftSize, minDb, maxDb, scale, minFrequency, and maxFrequency. Update the
memoization in SpectrumUseCaseImpl (especially the analyzer storage and
resolvedAnalyzer logic) so any change to these style properties forces a
rebuild, likely by widening the cached key and making SpectrumStyle Equatable if
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72794a20-a212-443b-9526-ba5c3ec0051c
📒 Files selected for processing (10)
.claude/CLAUDE.mdSources/AudioTapDataSource/AudioTapDataSourceImpl.swiftSources/AudioTapDataSource/ProcessTapEngine.swiftSources/Entity/StereoSamples.swiftSources/SpectrumUseCase/SpectrumUseCaseImpl.swiftSources/VersionHandler/Resources/version.txtTests/AudioTapDataSourceTests/AudioTapDataSourceImplTests.swiftTests/AudioTapDataSourceTests/ProcessTapEngineHelpersTests.swiftTests/FrequencyAnalyzerTests/FrequencyAnalyzerTests.swiftTests/SpectrumUseCaseTests/SpectrumUseCaseImplTests.swift
streamSampleRate は「address 構築 + AudioObjectGetPropertyData + フォーマット 判定 + ?? 48000 フォールバック」を1つの private メソッドに束ねており、 ライブタップ成功後にしか呼ばれず未カバーだった。責務を分離: - resolvedTapSampleRate(from:) — フォールバック判定を純粋 free 関数に抽出。 nil/正/ゼロ/負の全パスをテスト(既存の tapSampleRate と同じパターン)。 - liveTapFormat(of:) — 唯一 irreducible な CoreAudio syscall を薄く隔離し internal 化。bogus object id で失敗パス(address 構築+syscall+guard)を テスト(processPid が既に AudioObjectGetPropertyData を bogus id で安全に 呼んでいる前例に倣う)。 結果、sampleRate 読み取り経路の未カバーは 2 行のみ(init 行 48 と liveTapFormat の成功 return)——どちらも実オーディオデバイス必須で証明可能に irreducible。残りは全て回収。
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/AudioTapDataSourceTests/ProcessTapEngineHelpersTests.swift (1)
9-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
format(rate:)helper across two test suites.Same private helper is redefined in
TapSampleRateTestsandResolvedTapSampleRateTests. Consider hoisting it to a shared file-private top-level function to avoid drift if the construction logic ever changes.♻️ Proposed dedup
+private func format(rate: Double) -> AudioStreamBasicDescription { + var f = AudioStreamBasicDescription() + f.mSampleRate = rate + return f +} + `@Suite`("tapSampleRate") struct TapSampleRateTests { - private func format(rate: Double) -> AudioStreamBasicDescription { - var f = AudioStreamBasicDescription() - f.mSampleRate = rate - return f - } ... } `@Suite`("resolvedTapSampleRate") struct ResolvedTapSampleRateTests { - private func format(rate: Double) -> AudioStreamBasicDescription { - var f = AudioStreamBasicDescription() - f.mSampleRate = rate - return f - } ... }Also applies to: 38-42
🤖 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 `@Tests/AudioTapDataSourceTests/ProcessTapEngineHelpersTests.swift` around lines 9 - 13, The private format(rate:) helper is duplicated in both TapSampleRateTests and ResolvedTapSampleRateTests, so move the shared AudioStreamBasicDescription निर्माण logic into a single file-private top-level helper in ProcessTapEngineHelpersTests.swift and update both suites to call it. Keep the helper name and behavior consistent so future changes only need to be made in one place.
🤖 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 `@Tests/AudioTapDataSourceTests/ProcessTapEngineHelpersTests.swift`:
- Around line 9-13: The private format(rate:) helper is duplicated in both
TapSampleRateTests and ResolvedTapSampleRateTests, so move the shared
AudioStreamBasicDescription निर्माण logic into a single file-private top-level
helper in ProcessTapEngineHelpersTests.swift and update both suites to call it.
Keep the helper name and behavior consistent so future changes only need to be
made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: faaee009-f173-4e6b-9346-5465c420f0ca
📒 Files selected for processing (2)
Sources/AudioTapDataSource/ProcessTapEngine.swiftTests/AudioTapDataSourceTests/ProcessTapEngineHelpersTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
#299 のうち 課題1（サンプルレートが 48kHz 固定） を解消する。CoreAudio process tap のミックスダウンは出力デバイスの実レートに追従するため、44.1kHz 環境では Hz→FFT bin のマッピングと帯域カットオフ（
min_freq/max_freq）がズレていた。タップの実サンプルレートを capture 経路から解析器まで伝搬させる。変更内容
ProcessTapEngine: 構築時にkAudioTapPropertyFormatからタップの実レートを読み取り公開（読めなければ 48000 フォールバック）。StereoSamples（Entity）:sampleRateフィールドを追加（既定 48000。capture 無しのプレースホルダ用で、解析器は空窓では走らない）。AudioTapDataSourceImpl.latestSamples: エンジンからレートを読み、各窓に付与。AudioCaptureRepositoryはStereoSamplesをそのまま転送するので無改修。SpectrumUseCaseImpl:window.sampleRateを読み、メモ化FrequencyAnalyzerのキーを(bars, sampleRate)に拡張 → デバイスのレート変更で再構築。FrequencyAnalyzerは既にsampleRate引数を受け取る設計だったので、変更は伝搬のみ。背景・動機
#298 のレビュー（CodeRabbit / Copilot）で挙がったスペクトラム DSP のハードウェア依存 2 件のうち 1 件目。これにより物理周波数が実レートに依らず同じバーに載り、帯域カットオフも正確になる。
課題2（平滑化定数が 60fps 前提） は既存の調整済みの見た目を変え、実機 120Hz での再チューニングが必須なため、本 PR には含めず別対応とする（#299 に残す）。
テスト計画
FrequencyAnalyzerTests: 物理周波数のサンプルレート不変性（44.1kHz と 48kHz で 2kHz が同じバーに載る）SpectrumUseCaseImplTests: 伝搬検証（同一 PCM・異なるレート → 帯域マッピングが変わる）＋ Fake repository がレートを保持make lintクリーン備考
2.20.1に更新。Refs #299, #298
Summary by CodeRabbit