feat(audio): design the equalizer contract (tr-sbp) - #183
Conversation
|
Claude finished @jm2's task in 2m 56s —— View job Review completeTodo list
SummaryThis PR is a documentation-only design record ( The I found three remaining consistency/accuracy issues worth fixing before this is treated as frozen (posted as inline comments):
Everything else in the contract — the GStreamer element properties ( Not reviewed: I did not attempt to build/lint/test anything, since this PR touches only Markdown and there's no code to execute. |
Up to standards ✅🟢 Issues
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| Pre-LP clip behavior is what clip protection actually guards against: the EQ can elevate peaks | ||
| above 0 dBFS even with a sane-looking preamp, especially on already-mastered pop/rock material. | ||
| When `Clip protection = Off`, the contract explicitly permits clipping and the application must | ||
| not pretend it was prevented. The `Soft` option is therefore the recommended default for fresh | ||
| installs starting with enabled EQ. |
There was a problem hiding this comment.
This paragraph contradicts the contract's own stated defaults. It says:
The
Softoption is therefore the recommended default for fresh installs starting with enabled EQ.
But the "Bounded user surface" table (line 42) fixes Clip protection default to Off, and the "Fresh-install default state" block (lines 286–297) explicitly persists clip_protect=off. As written, an implementer following the acceptance matrix/persistence sections literally would ship Off as the default, directly conflicting with this sentence.
Given this document states "reviewers must reject any PR that introduces a new band value without the matching entry in the appendix" (line 66-68) for a similar kind of self-consistency requirement, this default-value contradiction should be resolved one way or the other before the doc is treated as the frozen contract.
Separately, "Pre-LP clip behavior" (start of this paragraph) isn't a term defined anywhere else in the document — worth spelling out what "LP" stands for or rewording, since every other technical term in this doc (rglimiter, equalizer-10bands, etc.) is precisely defined.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR establishes a comprehensive equalizer design, but several critical issues must be addressed before the contract is finalized. Most importantly, there is a fundamental discrepancy between the PR description and the documentation text regarding the limiter behavior (soft-knee vs. brick-wall). Additionally, technical errors in the GStreamer element properties and pipeline management strategy pose risks to runtime stability and audio quality.
The persistence logic also contains a flaw that prevents users from reverting to default settings. Finally, while the PR is up to standards according to Codacy, an acceptance criterion mentioned in the PR notes (regarding the threshold/ceiling matrix) is missing from the actual documentation.
About this PR
- There is a significant discrepancy regarding the clipping protection behavior. The PR description notes a shift to a soft-knee compressor model, but the design text still refers to a brick-wall limiter. This needs to be unified to ensure the implementation matches the intended hardware behavior.
- The proposed manual insertion and removal of elements during playback is high-risk and prone to deadlocks. Utilizing the 'audio-filter' property of 'playbin3' is the idiomatic approach to minimize audio gaps and state transition errors.
Test suggestions
- Verify fresh install defaults: EQ disabled, Flat preset, 0.0 dB gains/preamp, clip protection off.
- Enable EQ on local output and verify elements are inserted via pause/relink within 100ms.
- Update single band gain mid-playback and verify use of property writes without audio gaps.
- Select Pop preset and verify atomic update of all ten bands and the preamp value.
- Toggle clip protection and verify rglimiter insertion/removal via the specified pause/relink seam.
- Verify EQ UI controls are disabled with an appropriate tooltip when the active output is AirPlay.
- Recover from a malformed 'equalizer.cfg' file by resetting to defaults and logging a warning.
- Validate that out-of-bounds preamp or band values in the config file are clamped to the [-24, +12] dB range.
- Confirm EQ settings are correctly re-applied across gapless track transitions.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify fresh install defaults: EQ disabled, Flat preset, 0.0 dB gains/preamp, clip protection off.
2. Enable EQ on local output and verify elements are inserted via pause/relink within 100ms.
3. Update single band gain mid-playback and verify use of property writes without audio gaps.
4. Select Pop preset and verify atomic update of all ten bands and the preamp value.
5. Toggle clip protection and verify rglimiter insertion/removal via the specified pause/relink seam.
6. Verify EQ UI controls are disabled with an appropriate tooltip when the active output is AirPlay.
7. Recover from a malformed 'equalizer.cfg' file by resetting to defaults and logging a warning.
8. Validate that out-of-bounds preamp or band values in the config file are clamped to the [-24, +12] dB range.
9. Confirm EQ settings are correctly re-applied across gapless track transitions.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
|
||
| Persistence uses the same debounced single-writer pattern already used by `Player::save_volume`. | ||
| A 750 ms idle interval coalesces slider-drag changes into one write per change-spell. The save | ||
| runs on the GTK main loop and is suppressed entirely when the `Enabled = false` state matches |
There was a problem hiding this comment.
🔴 HIGH RISK
The suppression logic for saving defaults will cause user settings to revert if they manually return all values to their default states. If a user has custom EQ settings saved and then moves the sliders back to zero (the 'Flat' baseline), the write will be suppressed, leaving the old custom settings on disk to be reloaded on the next application start.
| volume name=eq-preamp volume=<factor> ! | ||
| equalizer-10bands name=eq | ||
| band0=<gain> band1=<gain> ... band9=<gain> ! | ||
| rglimiter name=clipper enabled=true ! |
There was a problem hiding this comment.
🔴 HIGH RISK
The 'rglimiter' element does not have an 'enabled' property. It uses the 'peak' property (double) to set the ceiling. To achieve the specified -1 dBFS brick-wall limit, the 'peak' property should be set to '0.891' (the linear equivalent of -1 dBFS).
| rglimiter name=clipper enabled=true ! | |
| rglimiter name=clipper peak=0.891 ! |
| Implementation acceptance requires the exact conditions listed below. The matrix is exhaustive | ||
| for this contract; new conditions require a new revision. | ||
|
|
||
| | Scenario | Expected outcome | |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The acceptance matrix is missing the row 'asserting the real ceiling and threshold' mentioned in the PR implementation notes.
| −1 dBFS". The element takes no other tuning; attack, release, and ceiling are fixed by the | ||
| element. Clip protection is the second of the five deliverables named by the bead; the | ||
| contract pins it to `rglimiter` rather than to `audioamplify`, which is not a limiter (it is a | ||
| static amplifier with hard-clip / wrap / none options, no envelope follower, and no |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The text still describes a 'fixed brick-wall ceiling at approximately -1 dBFS', which conflicts with the PR description's claim of a 'soft-knee compressor with -6 dBFS threshold and asymptotic 0 dBFS ceiling'.
| | Preset | enum | `Flat` / `Pop` / `Rock` / `Jazz` / `Classical` | — | `Flat` | | ||
| | Preamp | linear dB | `−24.0` … `0.0` … `+12.0` dB; integer or half-step | 0.5 dB | `0.0` dB | | ||
| | Bands 1..10 | linear dB | `−24.0` … `0.0` … `+12.0` dB | 0.5 dB | `0.0` dB | | ||
| | Clip protection | enum | `Off` / `Soft` (transparent limiter at −1 dBFS) | — | `Off` | |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This value contradicts the PR's stated intent to reflect measured behavior (-6 dBFS threshold and 0 dBFS ceiling).
|
|
||
| ## Filter graph | ||
|
|
||
| All equalizer DSP runs in the local-output `playbin3` pipeline by inserting a chain of three |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Manually relinking the internal graph of a 'playbin3' pipeline between the decoder and sink is complex and can lead to deadlocks. The recommended GStreamer pattern for this is to wrap the preamp, equalizer, and limiter into a single 'GstBin' and assign it to the 'audio-filter' property of 'playbin3'.
| `volume`. The on-disk format is one line per key, comments are not permitted, key order is | ||
| stable (the keys above, in that order), and values are quoted with `"…"` so that whitespace, | ||
| quotes, or unicode values cannot break parsing. | ||
|
|
||
| Persistence uses the same debounced single-writer pattern already used by `Player::save_volume`. | ||
| A 750 ms idle interval coalesces slider-drag changes into one write per change-spell. The save | ||
| runs on the GTK main loop and is suppressed entirely when the `Enabled = false` state matches | ||
| the default, the preset is `Flat`, all bands are zero, preamp is zero, and clip protection is | ||
| `Off`. | ||
|
|
||
| Fresh-install default state is exactly: | ||
|
|
||
| ```ini | ||
| schema_version=1 | ||
| enabled=false | ||
| preset=flat | ||
| preamp_db=0.0 | ||
| band0_db=0.0 | ||
| … | ||
| band9_db=0.0 | ||
| clip_protect=off | ||
| ``` |
There was a problem hiding this comment.
The stated on-disk format and the worked example disagree. Line 278-279 says:
values are quoted with
"…"so that whitespace, quotes, or unicode values cannot break parsing.
But the "Fresh-install default state" example right below (lines 288-297) shows unquoted values (enabled=false, preamp_db=0.0, clip_protect=off, …), not enabled="false" etc. Since this file format is itself part of the frozen contract (an implementer would presumably copy the example verbatim), this ambiguity should be fixed — either add quotes to the example or drop the quoting requirement from the prose.
| | Local | supported | Pipeline owns the decoder-to-sink chain; the equalizer chain runs in process. | | ||
| | AirPlay (RAOP) | unsupported | The receiving speaker renders audio; in-band equalizer protocol is proprietary and not exposed by the deployed receiver APIs. | | ||
| | Chromecast | unsupported | The receiving speaker renders audio; the Cast V2 protocol does not expose a public equalizer channel. | | ||
| | MPD | unsupported | MPD exposes server-side EQ commands (`eq`, `setvol`) that require server cooperation and vary by `libmpdclient` build; the canonical contracted behavior is host-side rendering, so host EQ does not reach the receiver. | |
There was a problem hiding this comment.
I'm not confident this claim is accurate: "MPD exposes server-side EQ commands (eq, setvol) that require server cooperation and vary by libmpdclient build". setvol is a real MPD protocol command, but it sets playback volume, not an equalizer — it isn't an "EQ command". I could not find an eq command in the MPD protocol reference or in libmpdclient's API; I wasn't able to verify this via web search in this environment, so flagging it rather than asserting it's wrong.
Given this PR's whole purpose is correcting a previously-rejected factual claim about a GStreamer element (rglimiter, in the prior two commits), it seems worth holding this MPD claim to the same bar — please double check against the actual MPD protocol docs (or drop eq/setvol from the reasoning and just state that MPD has no native EQ capability, if that's the real justification for "unsupported").
📝 WalkthroughWalkthroughThe pull request adds a design contract for Tributary’s ten-band equalizer, covering controls, DSP topology, persistence, output support, UI behavior, diagnostics, accessibility, localization, presets, and acceptance criteria. It also links the contract from the P2.4 backlog item. ChangesEqualizer contract
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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.
Inline comments:
In `@docs/equalizer.md`:
- Around line 330-334: Update the user-visible settings UI behavior for each
unsupported output so the equalizer limitation is communicated through visible
explanatory text or screen-reader-associated text in addition to the disabled
controls' tooltip. Keep the controls disabled and preserve the existing local
equalizer behavior and persisted bands.
- Around line 92-105: Update the equalizer pipeline contract to use playbin3’s
supported audio-filter/audio-sink integration seam instead of requiring direct
decoder-to-playsink relinking. Define the filter as a GstBin containing the
preamp, equalizer-10bands, and optional rglimiter chain, and specify how
enable/disable or clip-protection toggles replace or reconfigure that bin;
alternatively document the supported internal API if direct relinking is
intentional.
- Around line 275-297: Update the equalizer.cfg serialization example to use one
canonical format consistent with the stated contract: quote every value and
order keys according to the preceding table, including placing schema_version
last if that is the table’s order. Ensure the documented fresh-install defaults
and parser/writer expectations use the same ordering and quoting conventions.
- Around line 280-284: Update the persistence suppression logic described around
Player::save_volume so resetting the equalizer to its default state is still
written to disk when older custom settings may exist. Preserve debouncing and
single-writer behavior, but only suppress writes when it is safe to do so
without leaving stale persisted settings that can reappear after restart.
- Around line 99-105: Add a language identifier, such as text or gstreamer, to
the opening fence of each pipeline example in docs/equalizer.md, including the
blocks around the referenced sections, so all fenced code blocks satisfy
markdownlint MD040.
- Around line 303-314: Clarify the validation rules in the “Validation rules on
read” section by defining whether missing keys use per-key defaults while
preserving valid fields or trigger wholesale replacement like malformed values.
Align the malformed-file diagnostic and acceptance behavior with that
precedence, including the affected key and replacement scope.
- Around line 163-170: Clarify the gapless-transition requirement consistently:
the new-generation path must reapply the equalizer-10bands and preamp volume
settings when the gapless event occurs. Update the documentation around the
gapless event hook to state this behavior and add or adjust coverage for that
exact transition, while preserving the existing volume and event-generation
handling.
- Around line 135-138: Update the equalizer pipeline description around the
preamp and post-limiter audioconvert instances to state that audioconvert
handles format and channel conversion only; explicitly account for audioresample
for sample-rate conversion and caps or a mix matrix for stereo output, or adjust
the graph accordingly.
- Around line 183-186: Replace “atomic” wording in the equalizer documentation,
including the descriptions at the referenced preset-update sections, with
“logical” or equivalent language. Clarify that band gains and preamp are applied
through separate property writes and may briefly be inconsistent unless an
explicit snapshot or buffering boundary is implemented; do not claim atomic
behavior without such a mechanism.
- Around line 323-328: Update the MPD row in the equalizer support table to
remove the inaccurate `eq` protocol claim or replace it with a specific
supported server-side equalizer/filter, including its exact plugin or
configuration and supported version. Preserve the documented conclusion that
host-side equalizer processing does not reach the receiver.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 373a9f7c-9a74-4a16-8817-86e6b9ee6e26
📒 Files selected for processing (2)
docs/equalizer.mddocs/task.md
| Persistence uses the same debounced single-writer pattern already used by `Player::save_volume`. | ||
| A 750 ms idle interval coalesces slider-drag changes into one write per change-spell. The save | ||
| runs on the GTK main loop and is suppressed entirely when the `Enabled = false` state matches | ||
| the default, the preset is `Flat`, all bands are zero, preamp is zero, and clip protection is | ||
| `Off`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix default-state suppression so reset settings are persisted.
This is the same unresolved issue from the previous review: suppressing the write when the state equals defaults leaves older custom settings on disk, so resetting to Flat can resurrect those settings after restart.
🤖 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 `@docs/equalizer.md` around lines 280 - 284, Update the persistence suppression
logic described around Player::save_volume so resetting the equalizer to its
default state is still written to disk when older custom settings may exist.
Preserve debouncing and single-writer behavior, but only suppress writes when it
is safe to do so without leaving stale persisted settings that can reappear
after restart.
| | Output | Equalizer DSP | Reasoning | | ||
| |------------------|---------------|----------------------------------------------------------------------------------------------------------------------------| | ||
| | Local | supported | Pipeline owns the decoder-to-sink chain; the equalizer chain runs in process. | | ||
| | AirPlay (RAOP) | unsupported | The receiving speaker renders audio; in-band equalizer protocol is proprietary and not exposed by the deployed receiver APIs. | | ||
| | Chromecast | unsupported | The receiving speaker renders audio; the Cast V2 protocol does not expose a public equalizer channel. | | ||
| | MPD | unsupported | MPD exposes server-side EQ commands (`eq`, `setvol`) that require server cooperation and vary by `libmpdclient` build; the canonical contracted behavior is host-side rendering, so host EQ does not reach the receiver. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
MPD protocol eq command equalizer docs setvol ReplayGain official protocol equalizer plugin version
💡 Result:
The Music Player Daemon (MPD) protocol does not contain a native "eq" or "equalizer" command [1][2][3]. Equalization in MPD is typically handled through external system-level plugins or configuration-based filters, rather than through direct protocol commands sent by clients [4][5][6]. 1. Equalization Methods: - Native MPD Filters: Since MPD version 0.22, you can use the ffmpeg filter plugin to apply equalization natively within the mpd.conf file [4]. This involves defining a filter with plugin "ffmpeg" and a graph string (e.g., "anequalizer") [4]. Once configured, these filters are applied to specific audio_output sections [4]. - ALSA Equalizer Plugin: Historically and commonly, users apply equalization by using the ALSA "equal" plugin [5][6][7]. This is configured in the system's ALSA settings (e.g.,.asoundrc) and accessed as an audio output in MPD [5][7]. Control of this equalizer is generally performed via external shell scripts using amixer or alsamixer, rather than through the MPD protocol itself [5][6][7]. 2. Protocol Commands: - setvol: The setvol {VOL} command is the standard MPD protocol command to set the playback volume (range 0-100) [2][3][8]. It is independent of equalization settings [9]. - ReplayGain: The protocol includes specific commands for ReplayGain management, such as replay_gain_mode {MODE} (options: off, track, album, auto) and replay_gain_status [2][3][8]. These commands control how MPD applies gain metadata to normalize volume, but they do not function as a graphical or parametric equalizer [9]. There is no version-specific "equalizer plugin version" command in the official MPD protocol [1][2][3]. If you require real-time equalization control, it must be implemented at the audio output or filter chain level, which generally falls outside the scope of standard MPD client-to-server protocol commands [4][5][7].
Citations:
- 1: https://mpd.readthedocs.io/en/latest/protocol.html
- 2: https://github.com/MusicPlayerDaemon/MPD/blob/master/doc/protocol.rst
- 3: https://mpd.readthedocs.io/en/stable/protocol.html?highlight=albumart
- 4: https://www.hackitu.de/mpd_docker/
- 5: https://github.com/GioF71/mpd-alsa-docker/blob/main/doc/alsa-eq.md
- 6: https://linuxaudiofoundation.org/category/alsa-eq-plugin/
- 7: https://kb.gtkc.net/mpd-with-the-alsa-equalizer-plugin
- 8: https://python-mpd2.readthedocs.io/en/latest/topics/commands.html
- 9: Unexpected behaviour using replay_gain_handler "mixer" and mixer_type "null" MusicPlayerDaemon/MPD#1306
Remove the eq claim or replace it with the concrete server-side equalizer/filter. MPD’s protocol has no native eq command; if this row refers to equalization, name the exact plugin/configuration and supported version (for example, an MPD 0.22+ ffmpeg filter or an ALSA equalizer setup).
🤖 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 `@docs/equalizer.md` around lines 323 - 328, Update the MPD row in the
equalizer support table to remove the inaccurate `eq` protocol claim or replace
it with a specific supported server-side equalizer/filter, including its exact
plugin or configuration and supported version. Preserve the documented
conclusion that host-side equalizer processing does not reach the receiver.
| For each `unsupported` output, the user-visible settings UI renders the equalizer controls as | ||
| disabled with a tool-tip explaining the limitation (e.g. "AirPlay receivers render audio | ||
| end-to-end, so Tributary's equalizer cannot reach the speaker.") Disabled controls preserve the | ||
| last-saved values locally; the equalizer does run for the local output even while the user's | ||
| active output is unsupported, so a later switch back to local reflects the same persisted bands. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provide an accessible explanation beyond a tooltip on disabled controls.
Disabled controls may not receive focus, so the limitation must also be exposed as visible or screen-reader-associated text. A tooltip alone can make the unsupported-output state undiscoverable by keyboard and assistive-technology users.
🤖 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 `@docs/equalizer.md` around lines 330 - 334, Update the user-visible settings
UI behavior for each unsupported output so the equalizer limitation is
communicated through visible explanatory text or screen-reader-associated text
in addition to the disabled controls' tooltip. Keep the controls disabled and
preserve the existing local equalizer behavior and persisted bands.
|
Claude finished @jm2's task in 1m 45s —— View job Review completeTodo list
SummaryThis is a synchronize event — new commits ( What the latest commits fixed well: Issues from the previous review round that are still open (unaddressed by these two commits):
Design questions raised by CodeRabbit that are legitimate but not factual errors (worth a decision before implementation, not blocking as design-doc issues): using Not reviewed: no build/lint/test run, since this PR touches only Markdown. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@docs/equalizer.md`:
- Around line 94-95: Update the rglimiter description in the equalizer
documentation to call it a “soft-knee compressor/limiter” instead of a “peak
limiter,” and use that terminology consistently without changing the surrounding
contract language.
- Around line 313-314: Resolve the contradiction between the malformed-file read
rules and the UI reload behavior in docs/equalizer.md: choose whether malformed
files are repaired automatically or only when `Reload defaults from disk` is
invoked, then update both sections consistently. Preserve the documented
diagnostic details and ensure the chosen behavior is explicit.
- Around line 333-338: Clarify the equalizer documentation around the
unsupported active output to state that the local DSP chain is configured but
dormant, not running in a background local pipeline; explain that processing
resumes when local output becomes active while persisted bands remain intact.
- Around line 238-239: Update the EOS-resending property-write wording in the
equalizer documentation to remove the implication that g_object_set() causes a
bus flush or Buffering event. State that spinner behavior depends on actual
pipeline messages, and describe any flushing only as a separate operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abdca1ca-2f24-4422-ba4f-5bcb3df6a578
📒 Files selected for processing (2)
docs/equalizer.mddocs/task.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/task.md
| A malformed file is replaced with the default state, the user's prior preferences are recorded | ||
| in a typed diagnostic with file path, byte count, and the bad key, and the change is not silent. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the malformed-file reload contradiction.
The read rules say malformed files are automatically replaced with defaults, but the UI section says Reload defaults from disk is the only way to remove one. Define whether repair happens automatically or only after explicit reload, then align both sections.
Also applies to: 358-360
🤖 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 `@docs/equalizer.md` around lines 313 - 314, Resolve the contradiction between
the malformed-file read rules and the UI reload behavior in docs/equalizer.md:
choose whether malformed files are repaired automatically or only when `Reload
defaults from disk` is invoked, then update both sections consistently. Preserve
the documented diagnostic details and ensure the chosen behavior is explicit.
Adds docs/equalizer.md fixing the equalizer filter graph, ten fixed ISO bands 31 Hz..16 kHz with five named presets, linear preamp range -24..+12 dB, optional clip-protection limiter, mid-playback live reconfiguration boundary, persisted equalizer.cfg with schema_version 1, and the per-output capability matrix (local supported; AirPlay, Chromecast, MPD unsupported with disabled controls and honest explanations). Mirrors task.md entry for P2.4 to cite the in-progress contract. Implementation record remains pending.
…nt properties Fixes the four review-blocking defects identified by the refinery against polecat/tr-sbp (commit 32c5394): 1. equalizer-10bands has no 'preamp' property and no bypass flag. Verified: gst-inspect-1.0 shows band0..band9, name, parent, qos only. Contract now pins the preamp to a dedicated 'volume' element ahead of the equalizer (factor = 10^(dB/20), range 0.0631..3.9811 inside the element's 0..10 window), and treats Enabled=false as chain insert/remove (no bypass property to flip). 2. audioamplify is not a limiter and has no clip-mode / max-amplitude / envelope follower. Verified: gst-inspect-1.0 shows amplification, clipping-method (enum 0=clip, 1=wrap-negative, 2=wrap-positive, 3=none), name, parent, qos only. The doc's claim that 1 is 'soft-knee' and 0 is 'off' is inverted: 1 is gross distortion and 0 is hard clip. Replaced with rglimiter (gst-plugins-good), whose 'enabled' boolean is its only behavioural knob and whose ceiling is fixed at approximately -1 dBFS -- exactly the contract's 'transparent limiter at -1 dBFS' semantics. 3. Band centres stated as 'fixed by the spec' were wrong. The element's centres are 29, 59, 119, 237, 474, 947, 1889, 3770, 7523, 15011 Hz. Contract now uses these canonical centres and documents the GstChildProxy mechanism (band0::freq..band9::freq) for any future custom-centre revision; appendix preset vectors relabelled to match. 4. Live-reconfiguration boundary contradicted itself on Enabled=false (bypass property vs chain removal vs insert/remove-without-pause) and on Clip protection (pause/relink seam). Unified to a single pause/insert/resume seam for both Enabled and Clip protection, with a per-toggle 100 ms swap budget; band/preamp/preset remain property-write (no state transition). Also widened the band dB range from [-12, +12] to [-24, +12] in the bounded surface, persistence validation, and accessibility copy to match the equalizer-10bands element's actual range. Refinery note from the rejection: persistence, capability matrix, diagnostics, accessibility, acceptance matrix, and the rest of the document were considered complete and implementable; this commit corrects only the four named defects and leaves everything else untouched. Refs: tr-sbp, gate 1 review 32c5394.
The clip-protection contract described rglimiter as a transparent
brick-wall limiter at −1 dBFS. Measurement shows the element is a
soft-knee signal compressor ('Apply signal compression to raw audio
data' per gst-inspect), with a fixed −6 dBFS threshold and an
asymptotic 0 dBFS output ceiling; a 0 dBFS input is compressed by
approximately 1.1 dB.
Update the control table label, the Clip-protection paragraph, the
rglimiter element description in Filter graph, the Band-and-preamp-
mechanics paragraph, and the Scope statement to describe what the
element actually does. Drop the stale locale enumeration (main ships
13 locales; the doc named 10) and reference locales/ instead. Add an
acceptance-matrix row asserting the measured threshold and ceiling.
Refs: refinery rejection against d021603 (Gate 1 fail on clip
protection).
Resolves the refinery's semantic review of d5d0b4f (sha 722b8d73) which cited four internally-contradictory sections in docs/equalizer.md: 1. Filter graph (formerly lines 135-170). The previous chain `uridecodebin ! audioconvert ! volume ! equalizer-10bands ! rglimiter ! audioconvert ! playsink` promised sample-rate/channel normalisation and F32LE negotiation, but the graph defined neither audioresample nor capsfilter and assigned format negotiation to an audioconvert placed after rglimiter (audioconvert alone does not change sample rate and cannot pin F32LE). Rework the chain as a playbin3 audio-filter bin with two audioresample ! audioconvert ! capsfilter caps=<...> wrappers around the EQ/limiter stage. The pre-EQ capsfilter pins audio/x-raw, format=F32LE, channels=2, layout=interleaved so both the biquad and rglimiter see the format they require. The post-EQ capsfilter pins <sink-caps> filled in at chain-construction time. Failure during audio-filter-caps negotiation is now the spec's only rollback path: the bin does not insert, no chain is half-installed. 2. Live-reconfiguration (formerly lines 183-239). The previous text claimed g_object_set produces a bus-flush Buffering event and described multi- property writes as 'atomic' without defining the mechanism. Rework the mechanism as a three-step buffer-boundary transaction: capture into EqSettings, wrap writes in g_object_freeze_notify / g_object_thaw_notify per affected element, then wait for the next GST_EVENT_CAPS / SEGMENT on the bus as the buffer-boundary marker. State explicitly that g_object_set produces no Buffering event (GObject state mutation, not a pipeline event) so the UI does not confuse decoder-side Buffering with EQ writes. Add the write-only Custom preset name: manual edits move the persisted preset field from any named preset to 'custom', and the UI displays 'Custom' to signal that the active vector no longer matches a named response. Loading a named preset from Custom re-sets the persisted name. 3. Persistence (formerly lines 275-314). The previous text said values are quoted with double quotes, but the normative example showed unquoted values, and the 750 ms debounce promised no lost writes without any atomic-replace or shutdown-flush guarantee. Make the grammar canonical: every value is double-quoted UTF-8, in fixed order, with quote and backslash escaping. Update the example to match. Add an atomic-replace protocol: write to equalizer.cfg.tmp with O_EXCL + fsync, then rename to the destination, then fsync the directory. Add a shutdown-flush hook on gtk::main_quit and SIGTERM/SIGINT so quitting while the debounce is pending still produces a complete file. Update the validation rules and preset coercion ('flat' / 'custom' included in the named set). 4. Capability matrix (formerly lines 330-338). The previous text said both 'the equalizer does run for the local output even while the user's active output is unsupported' and 'the equalizer is not applied to anything' - a direct contradiction. Collapse to one rule: when the active output is unsupported, the audio-filter bin is not installed in any pipeline (playbin3.audio-filter is NULL); no DSP runs; the persisted state remains on disk and re-attaches when output switches back to a supported output. Update the acceptance matrix and implementation-boundary section to match the new graph and the new persistence grammar. Validation: docs/equalizer.md is now self-consistent against the four reviewer concerns; cross-references between the Filter graph, Live-reconfiguration, Persistence, Capability matrix, and Acceptance matrix sections now agree on the bin layout, the buffer-boundary transaction, the quoted grammar, the Custom state, and the one output-activation rule. No implementation files were touched (this is a design-only contract document per the header status line); the next bead on the implementation backlog references this contract verbatim.
|
Claude finished @jm2's task in 3m 53s —— View job Review completeTodo list
Summary
What this rewrite fixed well (resolving issues flagged in earlier review rounds):
Issues from earlier rounds still open (unaddressed by this rewrite — posted as inline comments):
New/leftover issue found in this round: Not reviewed: no build/lint/test run, since this PR touches only Markdown. |
| Pre-LP clip behavior is what clip protection actually guards against: the EQ can elevate peaks | ||
| above 0 dBFS even with a sane-looking preamp, especially on already-mastered pop/rock material. | ||
| When `Clip protection = Off`, the contract explicitly permits clipping and the application must | ||
| not pretend it was prevented. The `Soft` option is therefore the recommended default for fresh | ||
| installs starting with enabled EQ. |
There was a problem hiding this comment.
This contradiction from the first two review rounds is still unresolved by the latest rewrite (b8b87a9): "The Soft option is therefore the recommended default for fresh installs starting with enabled EQ" directly conflicts with the Bounded-user-surface table (line 43, Clip protection default Off) and the Fresh-install default state block (lines 364-375, clip_protect="off"). The rewrite fixed the Filter graph, Live-reconfiguration, Persistence, and Capability-matrix sections, but this Band-and-preamp-mechanics paragraph wasn't touched. "Pre-LP" (line 220) is also still an undefined term.
| | Local | supported | Pipeline owns the decoder-to-sink chain; the equalizer chain runs in process. | | ||
| | AirPlay (RAOP) | unsupported | The receiving speaker renders audio; in-band equalizer protocol is proprietary and not exposed by the deployed receiver APIs. | | ||
| | Chromecast | unsupported | The receiving speaker renders audio; the Cast V2 protocol does not expose a public equalizer channel. | | ||
| | MPD | unsupported | MPD exposes server-side EQ commands (`eq`, `setvol`) that require server cooperation and vary by `libmpdclient` build; the canonical contracted behavior is host-side rendering, so host EQ does not reach the receiver. | |
There was a problem hiding this comment.
Still unresolved across three review rounds: "MPD exposes server-side EQ commands (eq, setvol)". A prior review round in this same thread already ran the verification and found no native eq command in the MPD protocol or libmpdclient — setvol is real but controls volume, not equalization. The doc's own stated purpose (correcting the rglimiter misdescription) makes this the same class of unverified/incorrect technical claim it was written to fix. Suggest replacing the reasoning with something verifiable, e.g. "MPD has no native equalizer command; equalization would require an output-side ALSA/ffmpeg filter configured outside the MPD protocol, which this contract does not attempt to control."
| For each `unsupported` output, the user-visible settings UI renders the equalizer controls as | ||
| disabled with a tool-tip explaining the limitation (e.g. "AirPlay receivers render audio | ||
| end-to-end, so Tributary's equalizer cannot reach the speaker.") Disabled controls preserve the | ||
| last-saved values locally. |
There was a problem hiding this comment.
Still tooltip-only, unaddressed since the first review round: "the user-visible settings UI renders the equalizer controls as disabled with a tool-tip explaining the limitation." Disabled controls are frequently unreachable by keyboard focus and screen readers, so a tooltip alone can make this limitation undiscoverable to assistive-technology users. The Accessibility and localization section (lines 465-478) doesn't add any accessible-text requirement for this case either. Needs visible or screen-reader-associated explanatory text alongside the disabled controls, not just a tooltip.
| Persistence uses a debounced single-writer pattern: a 750 ms idle interval coalesces slider-drag | ||
| changes into one write per change-spell. The save runs on the GTK main loop and is suppressed | ||
| entirely when the state matches the fresh-install default (Enabled `false`, preset `Flat`, all | ||
| bands zero, preamp zero, clip protection `Off`). The debounce timer is reset on every change so | ||
| drag-induced writes are flushed on the trailing edge of the gesture. |
There was a problem hiding this comment.
The persistence-suppression bug flagged in earlier rounds is narrower now but still reproducible under the new grammar. The save is "suppressed entirely when the state matches the fresh-install default." Concrete repro: disk holds (enabled="false", preset="flat", bands=0, preamp="0.0", clip_protect="soft") — not a default match, so it was written normally. User then toggles Clip protection from Soft to Off. New state is now the exact default vector, so the debounced write is suppressed — the on-disk file keeps clip_protect="soft" even though the live/last-chosen state is Off. The shutdown-flush hook (lines 357-362) will correct this on a clean quit, but nothing here says the shutdown flush bypasses this same "matches default" suppression, and an unclean exit (crash, SIGKILL, power loss) leaves the stale clip_protect="soft" on disk to be reloaded next launch. Worth either dropping the default-state suppression (writes are cheap and atomic already) or explicitly stating the shutdown flush always writes unconditionally regardless of this match.
| | Preamp outside bounds in saved file | Value clamped to range; preset and bands remain valid | | ||
| | Band value outside bounds in saved file | Value clamped to range; other bands remain valid | | ||
| | Preset name not in the named set on disk | Coerced to `flat`; band vector remains as written on disk | | ||
| | Hardware sink with 8-channel layout (macOS) | Pre-EQ `audioconvert` caps remain `[1, 2]`; EQ runs in stereo; same cap fix as existing module | |
There was a problem hiding this comment.
This row wasn't updated for the Filter-graph rewrite and now contradicts it: "Pre-EQ audioconvert caps remain [1, 2]". Per the rewritten Filter graph section (lines 137-147), audioconvert explicitly does not own or negotiate caps — that's the job of the pre/post-EQ capsfilter elements ("This is the only element in the chain that negotiates a format; audioconvert does conversion work, capsfilter enforces the boundary"). This acceptance-matrix row should say capsfilter caps stay channels=2, not audioconvert caps.
|
|
||
| The chain layout for the *enabled, clip-protection-on* state is: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Minor/nit, carried over from earlier rounds: this fence (and the one at line 311) still has no language identifier, so markdownlint MD040 still fires on these two blocks. The persistence example fence at line 366 was already fixed with ini; these two could use text or similar.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/equalizer.md`:
- Around line 260-276: Replace the buffer-boundary acknowledgement described in
the documentation around the “buffer-boundary transaction” with a real
observable mechanism: use an explicit GstMessage, pad probe, or
sequence-number/generation acknowledgement instead of relying on
g_object_freeze_notify or GST_EVENT_CAPS/GST_EVENT_SEGMENT bus messages. Update
the three-step sequence and multi-property transaction guidance to describe the
selected acknowledgement accurately.
- Around line 101-110: Revise the equalizer documentation to describe
negotiation through the internal capsfilter and ghost-pad contract, not a caps
property on the GstBin or sink-pad activate behavior. State that setup failures
are link/negotiation failures, the bin is not inserted, and the pipeline falls
back to the existing passthrough layout without element-by-element fallback or
partial insertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21948466-b175-43d7-a6b0-ab1e9a0393a1
📒 Files selected for processing (2)
docs/equalizer.mddocs/task.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/task.md
| The bin carries a `caps` property pinned to | ||
| `audio/x-raw, format=F32LE, channels=2, layout=interleaved, rate=<samplerate>` (where | ||
| `<samplerate>` is the rate `playbin3` negotiated with the decoder on the bin's sink pad at | ||
| chain-construction time). `playbin3` uses this caps property to negotiate the upstream format; | ||
| if the upstream decoder cannot deliver that caps — typically only on a malformed or non-PCM | ||
| source — the bin's sink pad `activate` mode returns `FALSE`, `playbin3` propagates the error to | ||
| the bus, the implementation does **not** insert the bin, and the pipeline falls back to the | ||
| existing passthrough layout (a single info-level diagnostic names the source URI). This is the | ||
| spec's only rollback path: there is no element-by-element fallback inside the bin, and a failed | ||
| `audio-filter-caps` negotiation does not leave the chain half-inserted. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
gst-inspect-1.0 playbin3
gst-inspect-1.0 rglimiterRepository: jm2/tributary
Length of output: 204
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== docs/equalizer.md excerpt ==\n'
sed -n '90,125p' docs/equalizer.md
printf '\n== search for caps property / activate mode / playbin3 references ==\n'
rg -n 'caps property|activate mode|playbin3|audio-filter-caps|GstBin|ghost pad|capsfilter' docs/equalizer.mdRepository: jm2/tributary
Length of output: 5327
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== docs/equalizer.md lines 128-150 ==\n'
sed -n '128,150p' docs/equalizer.md
printf '\n== docs/equalizer.md lines 236-250 ==\n'
sed -n '236,250p' docs/equalizer.md
printf '\n== docs/equalizer.md lines 510-522 ==\n'
sed -n '510,522p' docs/equalizer.mdRepository: jm2/tributary
Length of output: 3551
🌐 Web query:
GStreamer playbin3 audio-filter property GstBin caps property ghost pads capsfilter
💡 Result:
In GStreamer's playbin3, the "audio-filter" property allows you to inject one or more custom audio processing elements into the audio sink chain [1][2]. If you need to apply multiple elements (such as a capsfilter combined with other processing elements) via this property, you should wrap those elements within a GstBin [3][4]. To achieve this: 1. Create a GstBin: Assemble your desired processing chain, such as "capsfilter caps=audio/x-raw,...! your-element", using gst_parse_bin_from_description or manual element construction [3]. 2. Manage Pads with Ghost Pads: Because a bin does not have pads of its own, you use ghost pads to expose the sink and source pads of the internal elements to the bin level [5][6]. When you build a bin manually, you add ghost pads to the bin that target the internal elements' pads so that playbin3 can transparently link to them [5][6]. 3. Apply the Bin: Set the completed GstBin as the value for the "audio-filter" property of the playbin3 instance using g_object_set [3][4]. Using this approach, you treat the entire collection of elements as a single GstElement, which satisfies the "audio-filter" property requirement while allowing complex configurations like forced caps or specific filter sequences [3][4].
Citations:
- 1: https://gstreamer.freedesktop.org/documentation/playback/playbin3.html
- 2: https://github.com/GStreamer/gstreamer/blob/c0647404/subprojects/gst-plugins-base/gst/playback/gstplaybin3.c
- 3: https://discourse.gstreamer.org/t/adding-two-audio-filter-plug-ins-to-playbin3-in-c/3668
- 4: https://gstreamer.freedesktop.org/documentation/tutorials/playback/custom-playbin-sinks.html
- 5: https://gstreamer.freedesktop.org/documentation/gstreamer/gstghostpad.html
- 6: https://gstreamer.freedesktop.org/documentation/application-development/basics/pads.html
🌐 Web query:
GStreamer playbin3 negotiation pad activate FALSE capsfilter ghost pad error bus
💡 Result:
In GStreamer, encountering "pad activate FALSE" errors related to playbin3, capsfilter, or ghost pads typically indicates a failure during the pipeline's negotiation or state-change process [1][2]. When a GStreamer pipeline changes state (e.g., READY to PAUSED), it activates pads to prepare for data flow [1]. If playbin3 attempts to activate a path that fails—often due to incompatible caps between a source (like decodebin) and a sink (via a capsfilter or ghost pad)—the negotiation fails [3][2]. Common causes and debugging steps include: 1. Negotiation Mismatches: The most frequent cause is that downstream elements (sinks) cannot accept the caps proposed by upstream elements [3][2]. If you are using a capsfilter, ensure its caps are compatible with the actual stream [2]. If the filter is too restrictive or specifies formats the sink cannot handle, negotiation will fail [2]. 2. Ghost Pad Issues: playbin3 heavily relies on ghost pads to route data to sinks [4][5]. If a ghost pad is created but not properly linked to a target, or if the target sink is not ready to receive data, the pipeline may stall or report activation errors [6]. Ensure that any custom bin or sink structure correctly links internal pads to the ghost pad before the pipeline enters the PAUSED state [4][6]. 3. Debugging: - Use the GST_DEBUG environment variable to isolate the failure. Setting GST_DEBUG=*:3,negotiation:5,pad:5 often reveals exactly which pad fails to negotiate and why [2]. - Verify if the capsfilter is truly necessary. Sometimes replacing it with a capability-aware element or adjusting the video-sink / audio-sink property of playbin3 is safer than forcing caps manually [5]. - Check if your sink supports the format by testing the pipeline segment with gst-launch-1.0 to ensure the sink is not rejecting the caps even without the capsfilter [3]. If you see "pad activate FALSE," it signifies that the core is deactivating a pad after a failed initialization or state transition attempt [1][7]. Focus your investigation on the caps query logs (using GST_DEBUG) immediately preceding this error to identify which element is rejecting the proposed format [2][8].
Citations:
- 1: https://gstreamer.freedesktop.org/documentation/additional/design/activation.html
- 2: https://gstreamer.freedesktop.org/documentation/additional/design/negotiation.html
- 3: https://lists.freedesktop.org/archives/gstreamer-devel/2013-August/042391.html
- 4: https://github.com/GStreamer/gst-plugins-base/blob/master/gst/playback/gstplaybin3.c
- 5: https://gstreamer.freedesktop.org/documentation/playback/playbin3.html
- 6: https://lists.freedesktop.org/archives/gstreamer-devel/2017-May/063832.html
- 7: https://gstreamer.freedesktop.org/documentation/gstreamer/gstpad.html
- 8: https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/negotiation.html
🌐 Web query:
site:gstreamer.freedesktop.org playbin3 audio-filter property GstElement GstBin ghost pads
💡 Result:
In GStreamer, playbin3 uses the audio-filter property to allow applications to insert custom audio processing elements or bins into the audio pipeline [1][2]. The audio-filter property accepts a GstElement [1][3]. If you wish to insert a complex chain of elements rather than a single element, you can create a GstBin that encapsulates those elements [4][5]. To make this GstBin act like a single element that playbin3 can utilize, you use GstGhostPads [5][6]. A GhostPad acts as a proxy for a pad on one of the internal elements within the bin, effectively exposing it on the surface of the bin itself [7][6]. Key implementation steps for this approach: 1. Create a GstBin container [5]. 2. Add your desired audio processing elements into the bin and link them [4][5]. 3. Identify the sink pad of the first element and the source pad of the last element in your chain [4]. 4. Create sink and source GstGhostPads for the bin, pointing them to the corresponding internal sink and source pads, and add these ghost pads to the bin [4][6]. 5. Assign this custom GstBin to the audio-filter property of playbin3 [1][2]. By doing this, the bin appears to playbin3 as a standard element with valid input and output pads, allowing the playback pipeline to integrate your custom processing chain seamlessly [4][6].
Citations:
- 1: https://gstreamer.freedesktop.org/documentation/playback/playbin3.html
- 2: https://gstreamer.freedesktop.org/documentation/playback/playsink.html
- 3: https://gstreamer.freedesktop.org/documentation/playback/playbin3.html?gi-language=c
- 4: https://gstreamer.freedesktop.org/documentation/tutorials/playback/custom-playbin-sinks.html
- 5: https://gstreamer.freedesktop.org/documentation/gstreamer/gstbin.html
- 6: https://gstreamer.freedesktop.org/documentation/application-development/basics/pads.html
- 7: https://gstreamer.freedesktop.org/documentation/gstreamer/gstghostpad.html
Use the internal capsfilter/ghost-pad contract here. Don’t describe the GstBin itself as carrying a caps property or negotiating through sink-pad activate; if setup fails, frame it as a link/negotiation failure that falls back to passthrough.
🤖 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 `@docs/equalizer.md` around lines 101 - 110, Revise the equalizer documentation
to describe negotiation through the internal capsfilter and ghost-pad contract,
not a caps property on the GstBin or sink-pad activate behavior. State that
setup failures are link/negotiation failures, the bin is not inserted, and the
pipeline falls back to the existing passthrough layout without
element-by-element fallback or partial insertion.
| The *buffer-boundary transaction* the spec requires is a three-step sequence on the application | ||
| side: | ||
|
|
||
| 1. Capture the new band vector and preamp into a single typed struct (`EqSettings`). | ||
| 2. Wrap the property writes in `g_object_freeze_notify` / `g_object_thaw_notify` on each | ||
| affected element (`equalizer-10bands` for the ten bands, `volume` for the preamp). Inside | ||
| the freeze, each `g_object_set` only mutates the element's internal state; the | ||
| `properties-changed` notification is suppressed until `thaw_notify` returns. The bus sees | ||
| **one** `properties-changed` notification per element per transaction, not eleven. | ||
| 3. Wait for the next `GST_MESSAGE_ELEMENT` carrying a `GST_EVENT_CAPS` or `GST_EVENT_SEGMENT` | ||
| on the bus from `equalizer-10bands` or `volume`. That message marks the buffer boundary at | ||
| which the new coefficients are picked up by the audio thread; `thaw_notify` returning | ||
| *before* the buffer-boundary message is published does not mean the new coefficients have | ||
| yet been read by the audio thread — it only means the property state is now visible to | ||
| readers. Single-band writes skip the freeze/thaw wrapper and skip the boundary wait; only | ||
| multi-property transactions (preamp changes, preset loads, multi-band batched edits) | ||
| require it. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n 'GST_MESSAGE_ELEMENT|GST_EVENT_CAPS|GST_EVENT_SEGMENT|properties-changed' .Repository: jm2/tributary
Length of output: 834
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== docs/equalizer.md lines 240-290 ==\n'
sed -n '240,290p' docs/equalizer.md
printf '\n== docs/equalizer.md lines 470-510 ==\n'
sed -n '470,510p' docs/equalizer.md
printf '\n== search for boundary / acknowledgement terminology ==\n'
rg -n 'boundary|acknowledg|properties-changed|freeze_notify|thaw_notify|GST_MESSAGE_ELEMENT|GST_EVENT_CAPS|GST_EVENT_SEGMENT' docs .Repository: jm2/tributary
Length of output: 50370
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== search repo for custom bus messages / app messages around equalizer ==\n'
rg -n 'new_custom|gst_message_new|gst_bus_post|GstMessage|message.*equalizer|equalizer.*message|properties-changed' .
printf '\n== search for seqno / generation / ack terminology ==\n'
rg -n 'seqno|generation|acknowledg|probe|pad probe' docs .Repository: jm2/tributary
Length of output: 50370
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== files mentioning equalizer-10bands / volume ==\n'
rg -n 'equalizer-10bands|volume' .
printf '\n== list docs/equalizer.md around the table entry at line 490 ==\n'
sed -n '486,494p' docs/equalizer.mdRepository: jm2/tributary
Length of output: 50369
🏁 Script executed:
#!/bin/sh
set -eu
printf '\n== search for equalizer runtime / implementation files ==\n'
git ls-files | rg 'equalizer|gstreamer|gst|audio|volume'
printf '\n== search for property-change notifications or bus posting code ==\n'
rg -n 'notify::|g_object_freeze_notify|g_object_thaw_notify|gst_bus_post|gst_message_new|properties-changed|GST_MESSAGE_ELEMENT|GST_EVENT_CAPS|GST_EVENT_SEGMENT|g_signal_emit' .Repository: jm2/tributary
Length of output: 8876
🌐 Web query:
GStreamer GST_EVENT_CAPS GST_EVENT_SEGMENT are pad events not GstMessage on the bus properties-changed notify freeze_notify
💡 Result:
You are correct: GST_EVENT_CAPS and GST_EVENT_SEGMENT are GstEvent types, not GstMessage types [1][2]. In GStreamer, these two mechanisms serve distinct purposes: 1. GstEvent (GST_EVENT_CAPS, GST_EVENT_SEGMENT): These are pad events that travel through the pipeline along with the data flow [3][2]. - They are processed by elements via a sink-event function [3][4]. - They are serialized with the buffer flow, meaning they arrive at the element in the exact order relative to the buffers they describe [2][4]. - GST_EVENT_CAPS informs elements about the format of subsequent buffers [1][2]. - GST_EVENT_SEGMENT provides timing information for subsequent buffers [2][4]. 2. GstMessage: These are messages posted by elements to the GstBus to communicate with the application [5]. - They are not part of the data flow and are used for asynchronous notifications (e.g., state changes, errors, EOS, or specific application-requested information) [5]. Regarding properties-changed, notify, and freeze_notify: - Notify and Deep-Notify: These are GObject signals (inherited by GstObject) used to indicate that an object's property has changed [6][7]. They are not GStreamer events or messages in the same sense as the pad events above, though you can configure an element to post a message to the bus when a property changes using specialized API calls like addPropertyNotifyWatch [8][9]. - Freeze_notify: This is a mechanism to temporarily suppress the emission of notify signals for an object, which is useful when updating multiple properties at once to avoid unnecessary overhead or intermediate state notifications [6][10]. In summary, GST_EVENT_CAPS and GST_EVENT_SEGMENT are serialized pad events for in-band control of the data stream, while the property-related signals are GObject-level notifications for tracking object state changes [3][2][6].
Citations:
- 1: https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html
- 2: https://gstreamer.freedesktop.org/documentation/additional/design/events.html
- 3: https://gstreamer.freedesktop.org/documentation/plugin-development/basics/eventfn.html
- 4: https://gstreamer.freedesktop.org/documentation/plugin-development/advanced/events.html
- 5: https://gstreamer.freedesktop.org/documentation/gstreamer/gstmessage.html
- 6: https://gstreamer.freedesktop.org/documentation/rust/stable/latest/docs/gstreamer/struct.Object.html
- 7: https://www.manpagez.com/html/gstreamer-1.0/gstreamer-1.0-1.12.1/GstObject.php
- 8: https://api.gtkd.org/gstreamer.Message.Message.parsePropertyNotify.html
- 9: https://api.gtkd.org/gstreamer.Element.Element.addPropertyNotifyWatch.html
- 10: https://gstreamer.pages.freedesktop.org/gstreamer-rs/stable/latest/docs/gstreamer/struct.Bus.html
Define a real buffer-boundary ack here. g_object_freeze_notify() only coalesces notify signals, and GST_EVENT_CAPS / GST_EVENT_SEGMENT are pad events, not bus messages. If this boundary must be observable on the bus, use an explicit GstMessage, pad probe, or seqno/generation ack instead.
🤖 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 `@docs/equalizer.md` around lines 260 - 276, Replace the buffer-boundary
acknowledgement described in the documentation around the “buffer-boundary
transaction” with a real observable mechanism: use an explicit GstMessage, pad
probe, or sequence-number/generation acknowledgement instead of relying on
g_object_freeze_notify or GST_EVENT_CAPS/GST_EVENT_SEGMENT bus messages. Update
the three-step sequence and multi-property transaction guidance to describe the
selected acknowledgement accurately.
1 participant
Summary
docs/task.md:1003-1005. Issue #49.
DESIGN-FIRST record (task.md:21-22 requires a design doc or refined issue before implementing cross-output work).
Specify: filter graph, band/preset/preamp/clipping contract, live-reconfiguration boundary, persistence, and a capability matrix across local, AirPlay, Chromecast and MPD outputs.
Blocks the equalizer implementation. Fleet-completable as a design artifact.
Implementation notes
Implemented: corrected rglimiter clip-protection description in docs/equalizer.md to match measured GStreamer 1.28.5 behaviour (soft-knee compressor with −6 dBFS threshold and asymptotic 0 dBFS ceiling; a 0 dBFS input is compressed by ~1.1 dB). Dropped the stale locale enumeration. Added an acceptance-matrix row asserting the real ceiling and threshold. Resolves refinery rejection of d021603 (Gate 1: clip protection).
Refinery handoff
tr-sbp(task, P2)polecat/tr-sbpmainmainvia Gastown Refinery.Summary by CodeRabbit