On Thu, Jul 29, 2021 at 03:01:33PM -0400, Kurt Mosiejczuk wrote:
> On Thu, Jul 29, 2021 at 02:46:57PM -0400, Bryan Steele wrote:
>
> > > > It would be nice to commit his diff below, or perhaps backout the 1.21.1
> > > > update in ports until the bug can be fixed upstream..
>
> > > It fixes audio for me. ok kmos on using this diff.
>
> > ok brynet@ too.
>
> To be clear, I _prefer_ keeping the update with the diff since it has cured
> the clipping I experienced all the time with minecraft (and sometimes with
> other things). I just played for a few minutes during work time because I so
> enjoyed the clipping being gone. :D
>
> --Kurt
Can you guys try out the following diff?
After bringing up the breakage upstream it looks like the author
fixed whatever the root issue was.
Index: Makefile
===================================================================
RCS file: /home/cvs/ports/audio/openal/Makefile,v
retrieving revision 1.56
diff -u -p -u -p -r1.56 Makefile
--- Makefile 30 Jul 2021 12:47:53 -0000 1.56
+++ Makefile 31 Jul 2021 03:50:08 -0000
@@ -6,7 +6,7 @@ V = 1.21.1
EPOCH = 0
DISTNAME = openal-soft-$V
PKGNAME = openal-$V
-REVISION = 0
+REVISION = 1
CATEGORIES = audio
SHARED_LIBS = openal 4.1
Index: patches/patch-alc_backends_sndio_cpp
===================================================================
RCS file: /home/cvs/ports/audio/openal/patches/patch-alc_backends_sndio_cpp,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 patch-alc_backends_sndio_cpp
--- patches/patch-alc_backends_sndio_cpp 31 Jul 2021 00:27:59 -0000
1.2
+++ patches/patch-alc_backends_sndio_cpp 31 Jul 2021 04:02:08 -0000
@@ -1,30 +1,45 @@
$OpenBSD: patch-alc_backends_sndio_cpp,v 1.2 2021/07/31 00:27:59 brynet Exp $
-Revert..
+Simplify channel handling in the sndio backend
+620836f173ae6fc4505d0634984e0f2c46166367
-sndio: Support more than 2 channels
-5cffe7e50a2885d9b392c15f2cd3941cac49692c
+Use non-block mode for sndio capture
+1fd4c865fc084f134363db5155361d5483679235
-and
-
-Use a safer layout if sndio changes the channel count
-b4a52321c4ad8e8bc0eb29d2cdae2c043fc405e1
-
-which break the backend.
Index: alc/backends/sndio.cpp
--- alc/backends/sndio.cpp.orig
+++ alc/backends/sndio.cpp
-@@ -57,7 +57,7 @@ struct SndioPlayback final : public BackendBase {
+@@ -22,6 +22,7 @@
+
+ #include "backends/sndio.h"
+
++#include <poll.h>
+ #include <stdio.h>
+ #include <stdlib.h>
+ #include <string.h>
+@@ -43,7 +44,12 @@ namespace {
+
+ static const char sndio_device[] = "SndIO Default";
+
++struct SioPar : public sio_par {
++ SioPar() { sio_initpar(this); }
+
++ void clear() { sio_initpar(this); }
++};
++
+ struct SndioPlayback final : public BackendBase {
+ SndioPlayback(ALCdevice *device) noexcept : BackendBase{device} { }
+ ~SndioPlayback() override;
+@@ -56,6 +62,7 @@ struct SndioPlayback final : public BackendBase {
+ void stop() override;
sio_hdl *mSndHandle{nullptr};
++ uint mFrameStep{};
-- al::vector<al::byte> mBuffer;
-+ al::vector<ALubyte> mBuffer;
+ al::vector<al::byte> mBuffer;
- std::atomic<bool> mKillNow{true};
- std::thread mThread;
-@@ -74,24 +74,16 @@ SndioPlayback::~SndioPlayback()
+@@ -74,39 +81,29 @@ SndioPlayback::~SndioPlayback()
int SndioPlayback::mixerProc()
{
@@ -35,106 +50,133 @@ Index: alc/backends/sndio.cpp
- mDevice->handleDisconnect("Failed to get device parameters");
- return 1;
- }
--
++ const size_t frameStep{mFrameStep};
++ const size_t frameSize{frameStep * mDevice->bytesFromFmt()};
+
- const size_t frameStep{par.pchan};
- const size_t frameSize{frameStep * par.bps};
-
SetRTPriority();
althrd_setname(MIXER_THREAD_NAME);
-- while(!mKillNow.load(std::memory_order_acquire)
-- && mDevice->Connected.load(std::memory_order_acquire))
-+ const size_t frameStep{mDevice->channelsFromFmt()};
-+ const uint frameSize{mDevice->frameSizeFromFmt()};
-+
-+ while(!mKillNow.load(std::memory_order_acquire) &&
-+ mDevice->Connected.load(std::memory_order_acquire))
+ while(!mKillNow.load(std::memory_order_acquire)
+ && mDevice->Connected.load(std::memory_order_acquire))
{
- al::byte *WritePtr{mBuffer.data()};
-+ ALubyte *WritePtr{mBuffer.data()};
- size_t len{mBuffer.size()};
+- size_t len{mBuffer.size()};
++ al::span<al::byte> buffer{mBuffer};
- mDevice->renderSamples(WritePtr, static_cast<uint>(len/frameSize),
frameStep);
-@@ -135,47 +127,35 @@ bool SndioPlayback::reset()
- sio_initpar(&par);
+- mDevice->renderSamples(WritePtr, static_cast<uint>(len/frameSize),
frameStep);
+- while(len > 0 && !mKillNow.load(std::memory_order_acquire))
++ mDevice->renderSamples(buffer.data(), static_cast<uint>(buffer.size()
/ frameSize),
++ frameStep);
++ while(!buffer.empty() && !mKillNow.load(std::memory_order_acquire))
+ {
+- size_t wrote{sio_write(mSndHandle, WritePtr, len)};
++ size_t wrote{sio_write(mSndHandle, buffer.data(), buffer.size())};
+ if(wrote == 0)
+ {
+ ERR("sio_write failed\n");
+ mDevice->handleDisconnect("Failed to write playback samples");
+ break;
+ }
+-
+- len -= wrote;
+- WritePtr += wrote;
++ buffer = buffer.subspan(wrote);
+ }
+ }
- par.rate = mDevice->Frequency;
+@@ -131,26 +128,12 @@ void SndioPlayback::open(const char *name)
+
+ bool SndioPlayback::reset()
+ {
+- sio_par par;
+- sio_initpar(&par);
++ SioPar par;
+
+- par.rate = mDevice->Frequency;
- switch(mDevice->FmtChans)
-- {
++ auto tryfmt = mDevice->FmtType;
++retry_params:
++ switch(tryfmt)
+ {
- case DevFmtMono : par.pchan = 1; break;
- case DevFmtQuad : par.pchan = 4; break;
- case DevFmtX51Rear: // fall-through - "Similar to 5.1, except using rear
channels instead of sides"
- case DevFmtX51 : par.pchan = 6; break;
- case DevFmtX61 : par.pchan = 7; break;
- case DevFmtX71 : par.pchan = 8; break;
-+ par.pchan = ((mDevice->FmtChans != DevFmtMono) ? 2 : 1);
-
+-
- // fall back to stereo for Ambi3D
- case DevFmtAmbi3D : // fall-through
- case DevFmtStereo : par.pchan = 2; break;
- }
-
- switch(mDevice->FmtType)
- {
-- case DevFmtByte:
-- par.bits = 8;
-- par.sig = 1;
-- break;
-- case DevFmtUByte:
-- par.bits = 8;
-- par.sig = 0;
-- break;
+- switch(mDevice->FmtType)
+- {
+ case DevFmtByte:
+ par.bits = 8;
+ par.sig = 1;
+@@ -159,7 +142,6 @@ bool SndioPlayback::reset()
+ par.bits = 8;
+ par.sig = 0;
+ break;
- case DevFmtFloat:
-- case DevFmtShort:
-- par.bits = 16;
-- par.sig = 1;
-- break;
-- case DevFmtUShort:
-- par.bits = 16;
-- par.sig = 0;
-- break;
-- case DevFmtInt:
-- par.bits = 32;
-- par.sig = 1;
-- break;
-- case DevFmtUInt:
-- par.bits = 32;
-- par.sig = 0;
-- break;
-+ case DevFmtByte:
-+ par.bits = 8;
-+ par.sig = 1;
-+ break;
-+ case DevFmtUByte:
-+ par.bits = 8;
-+ par.sig = 0;
-+ break;
-+ case DevFmtFloat:
-+ case DevFmtShort:
-+ par.bits = 16;
-+ par.sig = 1;
-+ break;
-+ case DevFmtUShort:
-+ par.bits = 16;
-+ par.sig = 0;
-+ break;
-+ case DevFmtInt:
-+ par.bits = 32;
-+ par.sig = 1;
-+ break;
-+ case DevFmtUInt:
-+ par.bits = 32;
-+ par.sig = 0;
-+ break;
+ case DevFmtShort:
+ par.bits = 16;
+ par.sig = 1;
+@@ -168,6 +150,7 @@ bool SndioPlayback::reset()
+ par.bits = 16;
+ par.sig = 0;
+ break;
++ case DevFmtFloat:
+ case DevFmtInt:
+ par.bits = 32;
+ par.sig = 1;
+@@ -177,70 +160,64 @@ bool SndioPlayback::reset()
+ par.sig = 0;
+ break;
}
++ par.bps = SIO_BPS(par.bits);
par.le = SIO_LE_NATIVE;
++ par.msb = 1;
-@@ -202,28 +182,8 @@ bool SndioPlayback::reset()
- }
++ par.rate = mDevice->Frequency;
++ par.pchan = mDevice->channelsFromFmt();
++
+ par.round = mDevice->UpdateSize;
+ par.appbufsz = mDevice->BufferSize - mDevice->UpdateSize;
+ if(!par.appbufsz) par.appbufsz = mDevice->UpdateSize;
- mDevice->Frequency = par.rate;
-+ mDevice->FmtChans = ((par.pchan==1) ? DevFmtMono : DevFmtStereo);
+- if(!sio_setpar(mSndHandle, &par) || !sio_getpar(mSndHandle, &par))
+- {
+- ERR("Failed to set device parameters\n");
+- return false;
+- }
++ try {
++ if(!sio_setpar(mSndHandle, &par))
++ throw al::backend_exception{al::backend_error::DeviceError,
++ "Failed to set device parameters"};
+- if(par.bits != par.bps*8)
+- {
+- ERR("Padded samples not supported (%u of %u bits)\n", par.bits,
par.bps*8);
+- return false;
+- }
+- if(par.le != SIO_LE_NATIVE)
+- {
+- ERR("Non-native-endian samples not supported (got %s-endian)\n",
+- par.le ? "little" : "big");
+- return false;
+- }
++ par.clear();
++ if(!sio_getpar(mSndHandle, &par))
++ throw al::backend_exception{al::backend_error::DeviceError,
++ "Failed to get device parameters"};
+
+- mDevice->Frequency = par.rate;
+-
- if(par.pchan < 2)
- {
- if(mDevice->FmtChans != DevFmtMono)
@@ -142,7 +184,16 @@ Index: alc/backends/sndio.cpp
- WARN("Got %u channel for %s\n", par.pchan,
DevFmtChannelsString(mDevice->FmtChans));
- mDevice->FmtChans = DevFmtMono;
- }
-- }
++ if(par.bps > 1 && par.le != SIO_LE_NATIVE)
++ throw al::backend_exception{al::backend_error::DeviceError,
++ "%s-endian samples not supported", par.le ? "Little" : "Big"};
++ if(par.bits < par.bps*8 && !par.msb)
++ throw al::backend_exception{al::backend_error::DeviceError,
++ "MSB-padded samples not supported (%u of %u bits)", par.bits,
par.bps*8};
++ if(par.pchan < 1)
++ throw al::backend_exception{al::backend_error::DeviceError,
++ "No playback channels on device"};
+ }
- else if((par.pchan == 2 && mDevice->FmtChans != DevFmtStereo)
- || par.pchan == 3
- || (par.pchan == 4 && mDevice->FmtChans != DevFmtQuad)
@@ -154,37 +205,249 @@ Index: alc/backends/sndio.cpp
- {
- WARN("Got %u channels for %s\n", par.pchan,
DevFmtChannelsString(mDevice->FmtChans));
- mDevice->FmtChans = DevFmtStereo;
-- }
--
- if(par.bits == 8 && par.sig == 1)
- mDevice->FmtType = DevFmtByte;
- else if(par.bits == 8 && par.sig == 0)
-@@ -247,15 +207,8 @@ bool SndioPlayback::reset()
- mDevice->UpdateSize = par.round;
- mDevice->BufferSize = par.bufsz + par.round;
-
-- mBuffer.resize(mDevice->UpdateSize * par.pchan*par.bps);
-- if(par.sig == 1)
-- std::fill(mBuffer.begin(), mBuffer.end(), al::byte{});
-- else if(par.bits == 8)
-- std::fill_n(mBuffer.data(), mBuffer.size(), al::byte(0x80));
-- else if(par.bits == 16)
-- std::fill_n(reinterpret_cast<uint16_t*>(mBuffer.data()),
mBuffer.size()/2, 0x8000);
-- else if(par.bits == 32)
-- std::fill_n(reinterpret_cast<uint32_t*>(mBuffer.data()),
mBuffer.size()/4, 0x80000000u);
-+ mBuffer.resize(mDevice->UpdateSize * mDevice->frameSizeFromFmt());
-+ std::fill(mBuffer.begin(), mBuffer.end(), 0);
++ catch(al::backend_exception &e) {
++ if(tryfmt == DevFmtShort)
++ throw;
++ par.clear();
++ tryfmt = DevFmtShort;
++ goto retry_params;
+ }
+
+- if(par.bits == 8 && par.sig == 1)
+- mDevice->FmtType = DevFmtByte;
+- else if(par.bits == 8 && par.sig == 0)
+- mDevice->FmtType = DevFmtUByte;
+- else if(par.bits == 16 && par.sig == 1)
+- mDevice->FmtType = DevFmtShort;
+- else if(par.bits == 16 && par.sig == 0)
+- mDevice->FmtType = DevFmtUShort;
+- else if(par.bits == 32 && par.sig == 1)
+- mDevice->FmtType = DevFmtInt;
+- else if(par.bits == 32 && par.sig == 0)
+- mDevice->FmtType = DevFmtUInt;
++ if(par.bps == 1)
++ mDevice->FmtType = (par.sig==1) ? DevFmtByte : DevFmtUByte;
++ else if(par.bps == 2)
++ mDevice->FmtType = (par.sig==1) ? DevFmtShort : DevFmtUShort;
++ else if(par.bps == 4)
++ mDevice->FmtType = (par.sig==1) ? DevFmtInt : DevFmtUInt;
+ else
++ throw al::backend_exception{al::backend_error::DeviceError,
++ "Unhandled sample format: %s %u-bit",
(par.sig?"signed":"unsigned"), par.bps*8};
++
++ mFrameStep = par.pchan;
++ if(par.pchan != mDevice->channelsFromFmt())
+ {
+- ERR("Unhandled sample format: %s %u-bit\n",
(par.sig?"signed":"unsigned"), par.bits);
+- return false;
++ WARN("Got %u channel%s for %s\n", par.pchan, (par.pchan==1)?"":"s",
++ DevFmtChannelsString(mDevice->FmtChans));
++ if(par.pchan < 2) mDevice->FmtChans = DevFmtMono;
++ else mDevice->FmtChans = DevFmtStereo;
+ }
++ mDevice->Frequency = par.rate;
+
+ setDefaultChannelOrder();
- return true;
+@@ -287,6 +264,11 @@ void SndioPlayback::stop()
}
-@@ -323,8 +276,8 @@ int SndioCapture::recordProc()
- const uint frameSize{mDevice->frameSizeFromFmt()};
-- while(!mKillNow.load(std::memory_order_acquire)
-- && mDevice->Connected.load(std::memory_order_acquire))
-+ while(!mKillNow.load(std::memory_order_acquire) &&
-+ mDevice->Connected.load(std::memory_order_acquire))
++/* TODO: This could be improved by avoiding the ring buffer and record thread,
++ * counting the available samples with the sio_onmove callback and reading
++ * directly from the device. However, this depends on reasonable support for
++ * capture buffer sizes apps may request.
++ */
+ struct SndioCapture final : public BackendBase {
+ SndioCapture(ALCdevice *device) noexcept : BackendBase{device} { }
+ ~SndioCapture() override;
+@@ -301,6 +283,7 @@ struct SndioCapture final : public BackendBase {
+
+ sio_hdl *mSndHandle{nullptr};
+
++ al::vector<struct pollfd> mFds;
+ RingBufferPtr mRing;
+
+ std::atomic<bool> mKillNow{true};
+@@ -326,37 +309,53 @@ int SndioCapture::recordProc()
+ while(!mKillNow.load(std::memory_order_acquire)
+ && mDevice->Connected.load(std::memory_order_acquire))
{
- auto data = mRing->getWriteVector();
- size_t todo{data.first.len + data.second.len};
+- auto data = mRing->getWriteVector();
+- size_t todo{data.first.len + data.second.len};
+- if(todo == 0)
++ /* Wait until there's some samples to read. */
++ const int nfds{sio_pollfd(mSndHandle, mFds.data(), POLLIN)};
++ if(nfds <= 0)
+ {
+- static char junk[4096];
+- sio_read(mSndHandle, junk,
+- minz(sizeof(junk)/frameSize, mDevice->UpdateSize)*frameSize);
++ mDevice->handleDisconnect("Failed to get polling fds: %d", nfds);
++ break;
++ }
++ int pollres{::poll(mFds.data(), static_cast<uint>(nfds), 2000)};
++ if(pollres < 0)
++ {
++ if(errno == EINTR) continue;
++ mDevice->handleDisconnect("Poll error: %s", strerror(errno));
++ break;
++ }
++ if(pollres == 0)
+ continue;
++
++ const int revents{sio_revents(mSndHandle, mFds.data())};
++ if((revents&POLLHUP))
++ {
++ mDevice->handleDisconnect("Got POLLHUP from poll events");
++ break;
+ }
++ if(!(revents&POLLIN))
++ continue;
+
+- size_t total{0u};
+- data.first.len *= frameSize;
+- data.second.len *= frameSize;
+- todo = minz(todo, mDevice->UpdateSize) * frameSize;
+- while(total < todo)
++ auto data = mRing->getWriteVector();
++ al::span<al::byte> buffer{data.first.buf, data.first.len*frameSize};
++ while(!buffer.empty())
+ {
+- if(!data.first.len)
+- data.first = data.second;
++ size_t got{sio_read(mSndHandle, buffer.data(), buffer.size())};
++ if(got == 0) break;
+
+- size_t got{sio_read(mSndHandle, data.first.buf, minz(todo-total,
data.first.len))};
+- if(!got)
++ mRing->writeAdvance(got / frameSize);
++ buffer = buffer.subspan(got);
++ if(buffer.empty())
+ {
+- mDevice->handleDisconnect("Failed to read capture samples");
+- break;
++ data = mRing->getWriteVector();
++ buffer = {data.first.buf, data.first.len*frameSize};
+ }
+-
+- data.first.buf += got;
+- data.first.len -= got;
+- total += got;
+ }
+- mRing->writeAdvance(total / frameSize);
++ if(buffer.empty())
++ {
++ /* Got samples to read, but no place to store it. Drop it. */
++ static char junk[4096];
++ sio_read(mSndHandle, junk, sizeof(junk) -
(sizeof(junk)%frameSize));
++ }
+ }
+
+ return 0;
+@@ -371,76 +370,80 @@ void SndioCapture::open(const char *name)
+ throw al::backend_exception{al::backend_error::NoDevice, "Device name
\"%s\" not found",
+ name};
+
+- mSndHandle = sio_open(nullptr, SIO_REC, 0);
++ mSndHandle = sio_open(nullptr, SIO_REC, true);
+ if(mSndHandle == nullptr)
+ throw al::backend_exception{al::backend_error::NoDevice, "Could not
open backend device"};
+
+- sio_par par;
+- sio_initpar(&par);
+-
++ SioPar par;
+ switch(mDevice->FmtType)
+ {
+ case DevFmtByte:
+- par.bps = 1;
++ par.bits = 8;
+ par.sig = 1;
+ break;
+ case DevFmtUByte:
+- par.bps = 1;
++ par.bits = 8;
+ par.sig = 0;
+ break;
+ case DevFmtShort:
+- par.bps = 2;
++ par.bits = 16;
+ par.sig = 1;
+ break;
+ case DevFmtUShort:
+- par.bps = 2;
++ par.bits = 16;
+ par.sig = 0;
+ break;
+ case DevFmtInt:
+- par.bps = 4;
++ par.bits = 32;
+ par.sig = 1;
+ break;
+ case DevFmtUInt:
+- par.bps = 4;
++ par.bits = 32;
+ par.sig = 0;
+ break;
+ case DevFmtFloat:
+ throw al::backend_exception{al::backend_error::DeviceError,
+ "%s capture samples not supported",
DevFmtTypeString(mDevice->FmtType)};
+ }
+- par.bits = par.bps * 8;
++ par.bps = SIO_BPS(par.bits);
+ par.le = SIO_LE_NATIVE;
+- par.msb = SIO_LE_NATIVE ? 0 : 1;
++ par.msb = 1;
+ par.rchan = mDevice->channelsFromFmt();
+ par.rate = mDevice->Frequency;
+
+ par.appbufsz = maxu(mDevice->BufferSize, mDevice->Frequency/10);
+- par.round = minu(par.appbufsz, mDevice->Frequency/40);
++ par.round = minu(par.appbufsz/2, mDevice->Frequency/40);
+
+- mDevice->UpdateSize = par.round;
+- mDevice->BufferSize = par.appbufsz;
+-
+ if(!sio_setpar(mSndHandle, &par) || !sio_getpar(mSndHandle, &par))
+ throw al::backend_exception{al::backend_error::DeviceError,
+ "Failed to set device praameters"};
+
+- if(par.bits != par.bps*8)
++ if(par.bps > 1 && par.le != SIO_LE_NATIVE)
+ throw al::backend_exception{al::backend_error::DeviceError,
++ "%s-endian samples not supported", par.le ? "Little" : "Big"};
++ if(par.bits < par.bps*8 && !par.msb)
++ throw al::backend_exception{al::backend_error::DeviceError,
+ "Padded samples not supported (got %u of %u bits)", par.bits,
par.bps*8};
+
+- if(!((mDevice->FmtType == DevFmtByte && par.bits == 8 && par.sig != 0)
+- || (mDevice->FmtType == DevFmtUByte && par.bits == 8 && par.sig == 0)
+- || (mDevice->FmtType == DevFmtShort && par.bits == 16 && par.sig != 0)
+- || (mDevice->FmtType == DevFmtUShort && par.bits == 16 && par.sig ==
0)
+- || (mDevice->FmtType == DevFmtInt && par.bits == 32 && par.sig != 0)
+- || (mDevice->FmtType == DevFmtUInt && par.bits == 32 && par.sig == 0))
+- || mDevice->channelsFromFmt() != par.rchan || mDevice->Frequency !=
par.rate)
++ auto match_fmt = [](DevFmtType fmttype, const sio_par &par) -> bool
++ {
++ return (fmttype == DevFmtByte && par.bps == 1 && par.sig != 0)
++ || (fmttype == DevFmtUByte && par.bps == 1 && par.sig == 0)
++ || (fmttype == DevFmtShort && par.bps == 2 && par.sig != 0)
++ || (fmttype == DevFmtUShort && par.bps == 2 && par.sig == 0)
++ || (fmttype == DevFmtInt && par.bps == 4 && par.sig != 0)
++ || (fmttype == DevFmtUInt && par.bps == 4 && par.sig == 0);
++ };
++ if(!match_fmt(mDevice->FmtType, par) || mDevice->channelsFromFmt() !=
par.rchan
++ || mDevice->Frequency != par.rate)
+ throw al::backend_exception{al::backend_error::DeviceError,
+ "Failed to set format %s %s %uhz, got %c%u %u-channel %uhz
instead",
+ DevFmtTypeString(mDevice->FmtType),
DevFmtChannelsString(mDevice->FmtChans),
+- mDevice->Frequency, par.sig?'s':'u', par.bits, par.rchan,
par.rate};
++ mDevice->Frequency, par.sig?'s':'u', par.bps*8, par.rchan,
par.rate};
+
+ mRing = RingBuffer::Create(mDevice->BufferSize, par.bps*par.rchan, false);
++ mDevice->BufferSize = static_cast<uint>(mRing->writeSpace());
++ mDevice->UpdateSize = par.round;
+
+ setDefaultChannelOrder();
+
