Skip to content

Fix pitch shift on mid-stream format change #180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
maximmaxim345 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix pitch shift on mid-stream format change #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions sendspin/audio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,17 @@ def set_volume(self, volume: int, *, muted: bool) -> None:
self._volume = max(0, min(100, volume))
self._muted = muted

def is_drained(self) -> bool:
"""Return True when the internal audio queue is empty.

Thread-safe: called from the worker thread while the PortAudio
callback thread updates ``_current_chunk``. Also returns True
when the stream is not actively playing (nothing to drain).
"""
if not self._stream_started:
return True
return self._queue.empty() and self._current_chunk is None

def stop(self) -> None:
"""Stop playback and release resources."""
self._closed = True
Expand Down
48 changes: 48 additions & 0 deletions sendspin/audio_connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import logging
import queue
import threading
import time
from collections.abc import Callable
from dataclasses import dataclass
from typing import TYPE_CHECKING, cast
Expand Down Expand Up @@ -192,6 +193,41 @@ def _run(
chunk_item = cast(_ChunkWorkItem, item)
fmt = chunk_item.fmt
if current_format != fmt:
# Format changed: drain old-format audio before switching
# to prevent pitch shift from old PCM played at new sample rate.
buffered_chunks: list[_ChunkWorkItem] = [chunk_item]
drained = player.is_drained()
Comment on lines 195 to +199
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

The new mid-stream format-switch behavior is fairly subtle (draining, buffering, handling stop/clear/volume during the drain). Since there are already unit tests around AudioStreamHandler in tests/test_audio_connector.py, it would be good to add a targeted test that simulates a format change with queued old-format chunks and verifies no old-format chunk is submitted after set_format(), and that clear during a pending switch drops buffered chunks.

Copilot uses AI. Check for mistakes.
deadline = time.monotonic() + 60.0
Comment on lines +198 to +200
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

buffered_chunks can grow without an explicit bound while waiting up to 60s for the player to drain, because the worker keeps consuming items from queue_obj and appending them to the list. If the server continues streaming during a long drain (or if draining stalls), this can create significant memory growth and latency spikes. Consider enforcing a maximum buffered duration/bytes/chunks and applying backpressure (e.g., stop consuming from queue_obj for a while, or drop/clear with a clear warning) once the cap is hit.

Copilot uses AI. Check for mistakes.

while not drained and time.monotonic() < deadline:
try:
drain_item = queue_obj.get(timeout=0.01)
except queue.Empty:
drained = player.is_drained()
continue

drain_type = type(drain_item)
if drain_type is _StopWorkItem:
player.stop()
return
if drain_type is _ClearWorkItem:
player.clear()
drained = True
break
Comment on lines +213 to +216
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

If a _ClearWorkItem arrives during the format-switch drain, player.clear() is called but buffered_chunks (which already contains at least the first new-format chunk) is still submitted after the format switch. That means a clear request won’t actually clear all queued/buffered audio. After processing a clear, consider also discarding buffered_chunks (and potentially skipping the immediate format switch) so the clear semantics remain consistent.

Copilot uses AI. Check for mistakes.
if drain_type is _SetVolumeWorkItem:
vol = cast(_SetVolumeWorkItem, drain_item)
software_volume = vol.volume
software_muted = vol.muted
player.set_volume(software_volume, muted=software_muted)
continue
# Buffer incoming new-format chunks during drain
buffered_chunks.append(cast(_ChunkWorkItem, drain_item))
Comment on lines +223 to +224
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

During the drain loop, every non-control item is blindly cast to _ChunkWorkItem and appended to buffered_chunks, regardless of that chunk’s format. If there are still old-format chunks sitting in the worker queue (not yet submitted to AudioPlayer) when the first new-format chunk arrives, they’ll be buffered and then submitted after set_format(), which reintroduces the pitch-shift problem and/or drops intended old-format playback ordering. Consider handling _ChunkWorkItem differently based on its fmt: submit old-format chunks to the player while draining, and only buffer chunks whose fmt matches the new target format (or handle multiple pending format changes explicitly).

Suggested change
# Buffer incoming new-format chunks during drain
buffered_chunks.append(cast(_ChunkWorkItem, drain_item))
# Handle incoming chunk work items during drain based on format
chunk = cast(_ChunkWorkItem, drain_item)
if chunk.fmt == current_format:
# Still receiving old-format audio: play it immediately
payload = chunk.audio_data
if current_format.codec == AudioCodec.FLAC and flac_decoder is not None:
payload = flac_decoder.decode(payload)
if not payload:
drained = player.is_drained()
continue
player.submit(chunk.server_timestamp_us, payload)
elif chunk.fmt == fmt:
# Buffer incoming new-format chunks during drain
buffered_chunks.append(chunk)
else:
# Unexpected third format during drain; buffer to preserve data
buffered_chunks.append(chunk)

Copilot uses AI. Check for mistakes.
drained = player.is_drained()

if not drained:
logger.warning("Drain timeout during format switch; forcing clear")
player.clear()

current_format = fmt
player.set_format(fmt, device=self._audio_device)

Expand All @@ -210,6 +246,18 @@ def _run(
if self._use_software_volume:
player.set_volume(software_volume, muted=software_muted)

# Process buffered new-format chunks
for buffered in buffered_chunks:
payload = buffered.audio_data
if fmt.codec == AudioCodec.FLAC:
if flac_decoder is None:
flac_decoder = FlacDecoder(fmt)
Comment on lines +253 to +254
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Inside the buffered chunk processing loop, the if flac_decoder is None: flac_decoder = FlacDecoder(fmt) branch is redundant because flac_decoder was just initialized immediately above when fmt.codec == AudioCodec.FLAC (and set to None otherwise). Consider removing the inner None-check to simplify control flow (or, if you intend to allow lazy init here, avoid eager init above).

Suggested change
if flac_decoder is None:
flac_decoder = FlacDecoder(fmt)

Copilot uses AI. Check for mistakes.
payload = flac_decoder.decode(payload)
if not payload:
continue
player.submit(buffered.server_timestamp_us, payload)
continue

payload = chunk_item.audio_data
if fmt.codec == AudioCodec.FLAC:
if flac_decoder is None:
Expand Down