detached 6e8cc4b

fix: address player review findings

commit
6e8cc4bc732fda0d3ca8f994075b96b6fd26c215
author
brice · · 2026-04-09 17:29 UTC
parent
755c788
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
5 files · +18 -29
Unified
UnifiedSplit
M cmd/pipewiregen/internal/parser/parser_test.go +3 -3
@@ -121,7 +121,7 @@ func TestLoadEmptyCallbackNameReturnsError(t *testing.T) {
121121 if !strings.Contains(err.Error(), "validation failed") {
122122 t.Fatalf("error message should contain 'validation failed', got: %v", err)
123123 }
124- if !strings.Contains(err.Error(), "callback") && !strings.Contains(err.Error(), "empty name") {
124+ if !strings.Contains(err.Error(), "callback") || !strings.Contains(err.Error(), "empty name") {
125125 t.Fatalf("error message should mention callback empty name, got: %v", err)
126126 }
127127 }
@@ -134,7 +134,7 @@ func TestLoadEmptyCallbackSignatureReturnsError(t *testing.T) {
134134 if !strings.Contains(err.Error(), "validation failed") {
135135 t.Fatalf("error message should contain 'validation failed', got: %v", err)
136136 }
137- if !strings.Contains(err.Error(), "callback") && !strings.Contains(err.Error(), "empty signature") {
137+ if !strings.Contains(err.Error(), "callback") || !strings.Contains(err.Error(), "empty signature") {
138138 t.Fatalf("error message should mention callback empty signature, got: %v", err)
139139 }
140140 }
@@ -147,7 +147,7 @@ func TestLoadEmptyEventStructNameReturnsError(t *testing.T) {
147147 if !strings.Contains(err.Error(), "validation failed") {
148148 t.Fatalf("error message should contain 'validation failed', got: %v", err)
149149 }
150- if !strings.Contains(err.Error(), "event struct") && !strings.Contains(err.Error(), "empty name") {
150+ if !strings.Contains(err.Error(), "event struct") || !strings.Contains(err.Error(), "empty name") {
151151 t.Fatalf("error message should mention event struct empty name, got: %v", err)
152152 }
153153 }
@@ -121,7 +121,7 @@ func TestLoadEmptyCallbackNameReturnsError(t *testing.T) {
121 if !strings.Contains(err.Error(), "validation failed") {121 if !strings.Contains(err.Error(), "validation failed") {
122 t.Fatalf("error message should contain 'validation failed', got: %v", err)122 t.Fatalf("error message should contain 'validation failed', got: %v", err)
123 }123 }
124- if !strings.Contains(err.Error(), "callback") && !strings.Contains(err.Error(), "empty name") {124+ if !strings.Contains(err.Error(), "callback") || !strings.Contains(err.Error(), "empty name") {
125 t.Fatalf("error message should mention callback empty name, got: %v", err)125 t.Fatalf("error message should mention callback empty name, got: %v", err)
126 }126 }
127 }127 }
@@ -134,7 +134,7 @@ func TestLoadEmptyCallbackSignatureReturnsError(t *testing.T) {
134 if !strings.Contains(err.Error(), "validation failed") {134 if !strings.Contains(err.Error(), "validation failed") {
135 t.Fatalf("error message should contain 'validation failed', got: %v", err)135 t.Fatalf("error message should contain 'validation failed', got: %v", err)
136 }136 }
137- if !strings.Contains(err.Error(), "callback") && !strings.Contains(err.Error(), "empty signature") {137+ if !strings.Contains(err.Error(), "callback") || !strings.Contains(err.Error(), "empty signature") {
138 t.Fatalf("error message should mention callback empty signature, got: %v", err)138 t.Fatalf("error message should mention callback empty signature, got: %v", err)
139 }139 }
140 }140 }
@@ -147,7 +147,7 @@ func TestLoadEmptyEventStructNameReturnsError(t *testing.T) {
147 if !strings.Contains(err.Error(), "validation failed") {147 if !strings.Contains(err.Error(), "validation failed") {
148 t.Fatalf("error message should contain 'validation failed', got: %v", err)148 t.Fatalf("error message should contain 'validation failed', got: %v", err)
149 }149 }
150- if !strings.Contains(err.Error(), "event struct") && !strings.Contains(err.Error(), "empty name") {150+ if !strings.Contains(err.Error(), "event struct") || !strings.Contains(err.Error(), "empty name") {
151 t.Fatalf("error message should mention event struct empty name, got: %v", err)151 t.Fatalf("error message should mention event struct empty name, got: %v", err)
152 }152 }
153 }153 }
M internal/core/player.go +3 -4
@@ -16,7 +16,6 @@ type player struct {
1616 config playerConfig
1717 callbacks playerCallbacks
1818 state atomic.Int32
19- terminal atomic.Bool
2019 paused atomic.Bool
2120 mu sync.Mutex
2221 }
@@ -139,15 +138,15 @@ func (p *player) Pause() error {
139138 return p.transition(PlayerStatePaused)
140139 }
141140
142-// Stop stops playback but allows restart
141+// Stop stops playback but allows restart.
143142 func (p *player) Stop() error {
144143 current := p.State()
145144 if current == PlayerStateClosed || current == PlayerStateClosing {
146145 return ErrPlayerClosed
147146 }
148147
149- // Valid from Playing, Paused, or Idle
150- if current != PlayerStatePlaying && current != PlayerStatePaused && current != PlayerStateIdle {
148+ // Valid from Playing or Paused.
149+ if current != PlayerStatePlaying && current != PlayerStatePaused {
151150 return ErrInvalidPlayerState
152151 }
153152
@@ -16,7 +16,6 @@ type player struct {
16 config playerConfig16 config playerConfig
17 callbacks playerCallbacks17 callbacks playerCallbacks
18 state atomic.Int3218 state atomic.Int32
19- terminal atomic.Bool
20 paused atomic.Bool19 paused atomic.Bool
21 mu sync.Mutex20 mu sync.Mutex
22 }21 }
@@ -139,15 +138,15 @@ func (p *player) Pause() error {
139 return p.transition(PlayerStatePaused)138 return p.transition(PlayerStatePaused)
140 }139 }
141 140
142-// Stop stops playback but allows restart141+// Stop stops playback but allows restart.
143 func (p *player) Stop() error {142 func (p *player) Stop() error {
144 current := p.State()143 current := p.State()
145 if current == PlayerStateClosed || current == PlayerStateClosing {144 if current == PlayerStateClosed || current == PlayerStateClosing {
146 return ErrPlayerClosed145 return ErrPlayerClosed
147 }146 }
148 147
149- // Valid from Playing, Paused, or Idle148+ // Valid from Playing or Paused.
150- if current != PlayerStatePlaying && current != PlayerStatePaused && current != PlayerStateIdle {149+ if current != PlayerStatePlaying && current != PlayerStatePaused {
151 return ErrInvalidPlayerState150 return ErrInvalidPlayerState
152 }151 }
153 152
M internal/core/player_rt.go +11 -5
@@ -24,9 +24,11 @@ type PCMBuffer struct {
2424
2525 // allocate allocates the Samples slice structure
2626 func (b *PCMBuffer) allocate() {
27- if b.Samples == nil {
27+ if len(b.Samples) != b.Channels {
2828 b.Samples = make([][]float32, b.Channels)
29- for ch := 0; ch < b.Channels; ch++ {
29+ }
30+ for ch := 0; ch < b.Channels; ch++ {
31+ if len(b.Samples[ch]) != b.Frames {
3032 b.Samples[ch] = make([]float32, b.Frames)
3133 }
3234 }
@@ -102,10 +104,14 @@ func (p *player) processPCM(buf *PCMBuffer) (int, error) {
102104
103105 // fillSilence fills the buffer with silence (0.0) from startFrame to endFrame
104106 func (p *player) fillSilence(buf *PCMBuffer, startFrame, endFrame int) {
107+ if startFrame < 0 {
108+ startFrame = 0
109+ }
110+ if endFrame > buf.Frames {
111+ endFrame = buf.Frames
112+ }
105113 for ch := 0; ch < buf.Channels; ch++ {
106- for frame := startFrame; frame < endFrame && frame < len(buf.Samples[ch]); frame++ {
107- buf.Samples[ch][frame] = 0.0
108- }
114+ clear(buf.Samples[ch][startFrame:endFrame])
109115 }
110116 }
111117
@@ -24,9 +24,11 @@ type PCMBuffer struct {
24 24
25 // allocate allocates the Samples slice structure25 // allocate allocates the Samples slice structure
26 func (b *PCMBuffer) allocate() {26 func (b *PCMBuffer) allocate() {
27- if b.Samples == nil {27+ if len(b.Samples) != b.Channels {
28 b.Samples = make([][]float32, b.Channels)28 b.Samples = make([][]float32, b.Channels)
29- for ch := 0; ch < b.Channels; ch++ {29+ }
30+ for ch := 0; ch < b.Channels; ch++ {
31+ if len(b.Samples[ch]) != b.Frames {
30 b.Samples[ch] = make([]float32, b.Frames)32 b.Samples[ch] = make([]float32, b.Frames)
31 }33 }
32 }34 }
@@ -102,10 +104,14 @@ func (p *player) processPCM(buf *PCMBuffer) (int, error) {
102 104
103 // fillSilence fills the buffer with silence (0.0) from startFrame to endFrame105 // fillSilence fills the buffer with silence (0.0) from startFrame to endFrame
104 func (p *player) fillSilence(buf *PCMBuffer, startFrame, endFrame int) {106 func (p *player) fillSilence(buf *PCMBuffer, startFrame, endFrame int) {
107+ if startFrame < 0 {
108+ startFrame = 0
109+ }
110+ if endFrame > buf.Frames {
111+ endFrame = buf.Frames
112+ }
105 for ch := 0; ch < buf.Channels; ch++ {113 for ch := 0; ch < buf.Channels; ch++ {
106- for frame := startFrame; frame < endFrame && frame < len(buf.Samples[ch]); frame++ {114+ clear(buf.Samples[ch][startFrame:endFrame])
107- buf.Samples[ch][frame] = 0.0
108- }
109 }115 }
110 }116 }
111 117
M pipewire/player.go +1 -1
@@ -179,7 +179,7 @@ func NewPlayer(config PlayerConfig, callbacks PlayerCallbacks) (Player, error) {
179179 },
180180 Fill: func(buf *core.PCMBuffer) (int, error) {
181181 if callbacks.Fill == nil {
182- return 0, nil
182+ return buf.Frames, nil
183183 }
184184 // Reuse scratch buffer by updating its fields
185185 p.scratchBuf.Frames = buf.Frames
@@ -179,7 +179,7 @@ func NewPlayer(config PlayerConfig, callbacks PlayerCallbacks) (Player, error) {
179 },179 },
180 Fill: func(buf *core.PCMBuffer) (int, error) {180 Fill: func(buf *core.PCMBuffer) (int, error) {
181 if callbacks.Fill == nil {181 if callbacks.Fill == nil {
182- return 0, nil182+ return buf.Frames, nil
183 }183 }
184 // Reuse scratch buffer by updating its fields184 // Reuse scratch buffer by updating its fields
185 p.scratchBuf.Frames = buf.Frames185 p.scratchBuf.Frames = buf.Frames
M pipewire/player_test.go +0 -16
@@ -47,22 +47,6 @@ func TestPlayerStateReflectsLifecycle(t *testing.T) {
4747 }
4848 }
4949
50-func TestPlayerStateReflectsInitialState(t *testing.T) {
51- config := PlayerConfig{
52- SampleRate: 44100,
53- Channels: 2,
54- FramesPerBuffer: 256,
55- SampleFormat: SampleFormatF32,
56- }
57- player, err := NewPlayer(config, PlayerCallbacks{})
58- if err != nil {
59- t.Fatalf("NewPlayer returned error: %v", err)
60- }
61- if player.State() != PlayerStateIdle {
62- t.Fatalf("expected initial state PlayerStateIdle, got %v", player.State())
63- }
64-}
65-
6650 func TestPlayerLifecycleStartStopClose(t *testing.T) {
6751 config := PlayerConfig{
6852 SampleRate: 44100,
@@ -47,22 +47,6 @@ func TestPlayerStateReflectsLifecycle(t *testing.T) {
47 }47 }
48 }48 }
49 49
50-func TestPlayerStateReflectsInitialState(t *testing.T) {
51- config := PlayerConfig{
52- SampleRate: 44100,
53- Channels: 2,
54- FramesPerBuffer: 256,
55- SampleFormat: SampleFormatF32,
56- }
57- player, err := NewPlayer(config, PlayerCallbacks{})
58- if err != nil {
59- t.Fatalf("NewPlayer returned error: %v", err)
60- }
61- if player.State() != PlayerStateIdle {
62- t.Fatalf("expected initial state PlayerStateIdle, got %v", player.State())
63- }
64-}
65-
66 func TestPlayerLifecycleStartStopClose(t *testing.T) {50 func TestPlayerLifecycleStartStopClose(t *testing.T) {
67 config := PlayerConfig{51 config := PlayerConfig{
68 SampleRate: 44100,52 SampleRate: 44100,

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 1

Tags 8

↑↓ moveenter openesc close