From 45a064e545eeb6f66f494cbfd890d22a483906c4 Mon Sep 17 00:00:00 2001 From: yusufipk Date: Sat, 1 Aug 2026 20:34:15 +0700 Subject: [PATCH] Tell a recorder that died from one that was asked to stop The pump now says when the capture ended with nothing captured, which is worth saying: parec refusing the device looks like silence otherwise. But stop() ends it the same way, so a recording shorter than 0.3 s raised that alarm first and "Recording too short" second, sending the user after a sound server that is fine. It follows the flag MeetingRecorder already carries for this. test_desktop_compat.py moves into the files for the modules it covers, so a test is where the next person looking at that module will find it. --- audio.py | 28 ++++++++---- tests/test_audio.py | 106 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 115 insertions(+), 19 deletions(-) diff --git a/audio.py b/audio.py index 0200058..39bceb2 100644 --- a/audio.py +++ b/audio.py @@ -44,6 +44,7 @@ class Recorder(QObject): self._buffer = bytearray() self._rms = [] self._cancelled = False + self._stopping = False self._lock = threading.Lock() @property @@ -71,6 +72,7 @@ class Recorder(QObject): self._buffer = bytearray() self._rms = [] self._cancelled = False + self._stopping = False self._max_bytes = int(max_seconds * RATE * SAMPLE_WIDTH * CHANNELS) self._thread = threading.Thread(target=self._pump, daemon=True) self._thread.start() @@ -94,17 +96,25 @@ class Recorder(QObject): break except (OSError, ValueError): pass - if not self._cancelled and not self._buffer and proc.poll() is not None: - try: - detail = proc.stderr.read().decode("utf-8", "replace").strip() - except (AttributeError, OSError): - detail = "" - self.failed.emit(t( - "Audio recorder stopped before receiving sound: {error}", - error=detail or f"exit code {proc.returncode}", - )) + # Nobody asked it to end and it captured nothing: the recorder is not + # installed properly, or the device was refused. Said out loud here, + # because stop() would otherwise report it as a recording that was too + # short, which sends the user looking in the wrong place. + with self._lock: + captured = bool(self._buffer) + if self._stopping or self._cancelled or captured: + return + try: + detail = proc.stderr.read().decode("utf-8", "replace").strip() + except (AttributeError, OSError): + detail = "" + self.failed.emit(t( + "Audio recorder stopped before receiving sound: {error}", + error=detail or f"exit code {proc.returncode}", + )) def _terminate(self): + self._stopping = True proc = self._proc if proc and proc.poll() is None: try: diff --git a/tests/test_audio.py b/tests/test_audio.py index 9a8dbaa..52f2f54 100644 --- a/tests/test_audio.py +++ b/tests/test_audio.py @@ -7,6 +7,7 @@ speakers, and neither list may go missing when pactl is absent. import array import contextlib +import io import json import os import subprocess @@ -194,10 +195,10 @@ class FakeProcess: """A pw-record that hands over a fixed buffer and then ends.""" def __init__(self, data): - import io self.stdout = io.BytesIO(data) self.stderr = io.BytesIO(b"") self.signals = [] + self.returncode = 0 self._alive = True def poll(self): @@ -215,6 +216,48 @@ class FakeProcess: self._alive = False +@linux_only +class RecordingCommand(DikteTest): + """Which program captures the microphone, and how it is asked to.""" + + def test_parec_is_preferred(self): + """It speaks to PulseAudio and to PipeWire's compatibility service, so + it is the one that works on both desktops.""" + with only_these_tools("parec", "pw-record"): + self.assertEqual(audio.recording_command()[0], "parec") + + def test_pw_record_is_the_fallback(self): + with only_these_tools("pw-record"): + self.assertEqual(audio.recording_command()[0], "pw-record") + + def test_neither_is_installed(self): + with only_these_tools(): + self.assertEqual(audio.recording_command(), []) + + def test_both_capture_the_format_the_rest_of_the_code_expects(self): + for tool in ("parec", "pw-record"): + with self.subTest(tool=tool), only_these_tools(tool): + cmd = audio.recording_command() + joined = " ".join(cmd) + self.assertIn(str(audio.RATE), joined) + self.assertIn(str(audio.CHANNELS), joined) + self.assertIn("s16", joined) + + def test_a_chosen_microphone_reaches_either_one(self): + with only_these_tools("parec"): + self.assertIn("--device=alsa_input.usb", audio.recording_command( + "alsa_input.usb")) + with only_these_tools("pw-record"): + self.assertIn("--target=alsa_input.usb", audio.recording_command( + "alsa_input.usb")) + + def test_no_microphone_named_means_no_device_flag(self): + for tool, flag in (("parec", "--device="), ("pw-record", "--target=")): + with self.subTest(tool=tool), only_these_tools(tool): + self.assertFalse([arg for arg in audio.recording_command() + if arg.startswith(flag)]) + + @linux_only class RecorderChain(DikteTest): """Start to WAV, with pw-record faked out.""" @@ -233,15 +276,6 @@ class RecorderChain(DikteTest): recorder.stop() return recorder, results, failures, popen - def test_pw_record_is_not_installed(self): - recorder = audio.Recorder() - failures = [] - recorder.failed.connect(failures.append) - with only_these_tools(): - recorder.start() - self.assertEqual(len(failures), 1) - self.assertIn("pipewire", failures[0]) - def test_the_capture_format_is_what_the_rest_of_the_code_expects(self): _, _, _, popen = self.record(silence(1.0)) cmd = popen.call_args.args[0] @@ -295,6 +329,58 @@ class RecorderChain(DikteTest): self.addCleanup(os.unlink, path) self.assertLessEqual(duration, 1.1) + def test_a_recorder_that_is_not_installed_at_all(self): + recorder = audio.Recorder() + failures = [] + recorder.failed.connect(failures.append) + with only_these_tools(): + recorder.start() + self.assertEqual(len(failures), 1) + self.assertIn("pulseaudio-utils", failures[0]) + + def pump(self, data=b"", stderr=b"", stopping=False, cancelled=False): + """Run the pump in this thread, where a queued signal would need an + event loop nobody is running here.""" + recorder = audio.Recorder() + failures = [] + recorder.failed.connect(failures.append) + proc = FakeProcess(data) + proc.stderr = io.BytesIO(stderr) + proc._alive = False + recorder._proc = proc + recorder._max_bytes = 10 ** 9 + recorder._stopping = stopping + recorder._cancelled = cancelled + recorder._pump() + return failures + + def test_a_recorder_that_died_on_its_own_says_so(self): + """parec refused the device, or the sound server went away.""" + failures = self.pump(stderr=b"connection refused\n") + self.assertEqual(len(failures), 1) + self.assertIn("connection refused", failures[0]) + + def test_a_death_with_nothing_on_stderr_still_names_the_exit_code(self): + failures = self.pump() + self.assertIn("exit code", failures[0]) + + def test_a_recording_we_ended_ourselves_is_not_a_death(self): + """Otherwise a stray keypress produces two errors, and the first one + sends the user looking for a broken sound server.""" + self.assertEqual(self.pump(stopping=True), []) + + def test_a_cancelled_recording_is_not_a_death(self): + self.assertEqual(self.pump(cancelled=True), []) + + def test_a_recorder_that_captured_something_first_is_not_a_death(self): + self.assertEqual(self.pump(data=silence(0.5)), []) + + def test_a_short_recording_reports_only_that(self): + _, results, failures, _ = self.record(silence(0.1)) + self.assertEqual(results, []) + self.assertEqual(len(failures), 1) + self.assertIn("0.3", failures[0]) + def test_a_recorder_that_could_not_start(self): recorder = audio.Recorder() failures = []