diff --git a/backend/cpp/audio-cpp/audio_units.cpp b/backend/cpp/audio-cpp/audio_units.cpp index 066600e54..66c6013a4 100644 --- a/backend/cpp/audio-cpp/audio_units.cpp +++ b/backend/cpp/audio-cpp/audio_units.cpp @@ -49,8 +49,16 @@ std::int64_t seconds_to_samples(double seconds, int sample_rate) { } // Round rather than truncate: these functions exist to cross the float // seconds boundary the VAD and diarize messages use, so a value that came - // from samples_to_seconds must convert back to the sample it started as. - // Truncation loses one sample about half the time, starting at n=1. + // from samples_to_seconds converts back to the sample it started as. + // Truncation lost one sample about half the time, starting at n=1. + // + // That round trip is exact only below roughly 2^23 samples. Past that the + // float samples_to_seconds returns can no longer resolve adjacent indices + // and the trip fails whatever the rounding: measured first failures run + // from 11289602 samples (4.3 min at 44.1 kHz, 2.1 min at 96 kHz) to + // 16384001 (17 min at 16 kHz). That is a property of the float seconds API + // itself, not of the rounding here, and it is why nothing should use these + // to carry a sample-accurate position in a long recording. return static_cast(std::llround(scaled)); } @@ -79,11 +87,27 @@ std::string f32_to_s16le(const std::vector &samples) { std::string bytes; bytes.reserve(samples.size() * 2); for (const float sample : samples) { - // Argument order is load-bearing: std::min(1.0f, sample) returns 1.0f - // for a NaN sample, because NaN < 1.0f is false and min returns its - // first argument in that case. Written the equally natural - // std::min(sample, 1.0f), a NaN would pass straight through to - // std::lround, whose result is unspecified for NaN. Do not reorder. + // NaN maps to silence. A NaN sample rendered as a full-scale click is + // worse audio than a dropped one, and this unit converts audio that may + // have originated off the wire. + // + // This guard also removes what used to be a spelling hazard in the + // clamp below. std::min and std::max return their first argument when + // the comparison is false, and every comparison against NaN is false, + // so before this branch existed the choice of spelling silently decided + // whether a NaN reached std::lround, whose result is unspecified for + // NaN. These three leaked it, the last being the idiomatic C++17 way to + // write a clamp and so the likeliest future edit: + // std::min(std::max(sample, -1.0f), 1.0f) + // std::max(std::min(sample, 1.0f), -1.0f) + // std::clamp(sample, -1.0f, 1.0f) + // The order is no longer load-bearing now that the guard runs first, + // but the history is why the guard is here, so do not drop it. + if (std::isnan(sample)) { + bytes.push_back(0); + bytes.push_back(0); + continue; + } const float clamped = std::max(-1.0f, std::min(1.0f, sample)); // 32767 rather than 32768 so +1.0 saturates at INT16_MAX instead of // overflowing to INT16_MIN. See s16le_to_f32 for why decode differs. diff --git a/backend/cpp/audio-cpp/audio_units.h b/backend/cpp/audio-cpp/audio_units.h index 23f716829..57581ebef 100644 --- a/backend/cpp/audio-cpp/audio_units.h +++ b/backend/cpp/audio-cpp/audio_units.h @@ -20,7 +20,10 @@ std::int64_t samples_to_nanoseconds(std::int64_t samples, int sample_rate); float samples_to_seconds(std::int64_t samples, int sample_rate); -// Negative seconds clamp to 0. +// Rounds to nearest. Negative seconds and NaN both yield 0, and a value too +// large to convert saturates at INT64_MAX rather than overflowing. Round trips +// with samples_to_seconds only below roughly 2^23 samples, past which the float +// seconds can no longer resolve adjacent sample indices. std::int64_t seconds_to_samples(double seconds, int sample_rate); // Decodes little-endian signed 16-bit PCM. A trailing odd byte is dropped. @@ -28,6 +31,8 @@ std::vector s16le_to_f32(const std::string &bytes); // Encodes to little-endian signed 16-bit PCM, clamping to [-1, 1] first so an // overshooting sample saturates instead of wrapping to the opposite sign. +// A NaN sample encodes to 0, on the grounds that silence beats a full-scale +// click. std::string f32_to_s16le(const std::vector &samples); } // namespace audiocpp_backend diff --git a/backend/cpp/audio-cpp/audio_units_test.cpp b/backend/cpp/audio-cpp/audio_units_test.cpp index ff7acd744..76f57d023 100644 --- a/backend/cpp/audio-cpp/audio_units_test.cpp +++ b/backend/cpp/audio-cpp/audio_units_test.cpp @@ -3,6 +3,7 @@ #include "audio_units.cpp" +#include #include #include #include @@ -138,14 +139,35 @@ static void test_s16le_decode_range() { static void test_s16le_nan_input() { // A NaN sample must not reach std::lround, whose result is unspecified for - // NaN. See the argument-order comment in f32_to_s16le. - const std::vector decoded = - s16le_to_f32(f32_to_s16le({std::numeric_limits::quiet_NaN()})); + // NaN. Asserting a range is not enough to pin this: the three outcomes the + // plausible clamp spellings produce (full scale, negative full scale, zero) + // are all finite and all inside [-1, 1], so a range check passes for every + // one of them. Only an exact value distinguishes them. + // NaN maps to silence, not to full scale: a NaN sample rendered as a + // full-scale click is worse audio than a dropped one, and this unit + // converts audio that may have originated off the wire. + // + // volatile so the NaN cannot be constant-folded, which would let the + // compiler evaluate the conversion at compile time and raise no + // floating-point exception at run time for the check below to observe. + volatile float nan_source = std::numeric_limits::quiet_NaN(); + const std::vector input = {nan_source}; + + std::feclearexcept(FE_ALL_EXCEPT); + const std::string encoded = f32_to_s16le(input); + const bool raised_invalid = std::fetestexcept(FE_INVALID) != 0; + const std::vector decoded = s16le_to_f32(encoded); + check(decoded.size() == 1, "a NaN sample still encodes to one sample"); - check(decoded.size() == 1 && std::isfinite(decoded[0]), - "a NaN sample encodes to a finite value"); - check(decoded.size() == 1 && decoded[0] >= -1.0f && decoded[0] <= 1.0f, - "a NaN sample encodes within full scale"); + check(decoded.size() == 1 && decoded[0] == 0.0f, + "a NaN sample encodes to exactly zero, not to a full-scale click"); + // Independent of the value: a quiet NaN raises invalid-operation as soon as + // it reaches any ordered comparison, which is what std::min and std::max + // use, so this fails unless the NaN is diverted before the clamp runs at + // all. That is what stops the explicit guard from being dropped in favour + // of a clamp spelling that happens to yield zero. + check(!raised_invalid, + "encoding a NaN sample raises no invalid-operation exception"); } static void test_s16le_odd_length() {