USB: Rewrite RingBuffer class

It was overflowing and corrupting the heap...

pcsx2/USB/shared/ringbuffer.cpp

@@ -1,175 +1,88 @@
-// SPDX-FileCopyrightText: 2002-2023 PCSX2 Dev Team
+// SPDX-FileCopyrightText: 2002-2024 PCSX2 Dev Team
 // SPDX-License-Identifier: LGPL-3.0+
 
 #include "ringbuffer.h"
 #include <cstring>
 #include <cassert>
+#include <algorithm>
 
-
+RingBuffer::RingBuffer() = default;
-RingBuffer::RingBuffer()
-	: m_begin(0)
-	, m_end(0)
-	, m_capacity(0)
-	, m_data(nullptr)
-	, m_overrun(false)
-{
-}
 
 RingBuffer::RingBuffer(size_t capacity)
 	: RingBuffer()
 {
-	reserve(capacity);
+	reset(capacity);
 }
 
-RingBuffer::~RingBuffer()
+RingBuffer::~RingBuffer() = default;
-{
-	delete[] m_data;
-}
 
-void RingBuffer::reserve(size_t capacity)
+void RingBuffer::reset(size_t capacity)
 {
-	delete[] m_data;
+	m_rpos = 0;
-	m_data = new char[capacity];
+	m_wpos = 0;
-	memset(m_data, 0, capacity);
+	m_full = false;
-	m_capacity = capacity;
+	m_data.reset();
+	if ((m_capacity = capacity) > 0)
+		m_data = std::make_unique<uint8_t[]>(capacity);
 }
 
 size_t RingBuffer::size() const
 {
-	size_t size = 0;
+	if (m_wpos == m_rpos)
-	if (m_begin == m_end)
+		return m_full ? m_capacity : 0;
-	{
+	else if (m_wpos > m_rpos)
-		if (m_overrun)
+		return m_wpos - m_rpos;
-			size = m_capacity;
 	else
-			size = 0;
+		return (m_capacity - m_rpos) + m_wpos;
-	}
-	else if (m_begin < m_end)
-		size = m_end - m_begin; // [   b...e   ]
-	else
-		size = m_capacity - m_begin + m_end; // [...e   b...]
-
-	return size;
 }
 
-size_t RingBuffer::read(uint8_t* dst, size_t nbytes)
+size_t RingBuffer::read(void* dst, size_t nbytes)
 {
+	uint8_t* bdst = static_cast<uint8_t*>(dst);
+
 	size_t to_read = nbytes;
-	while (to_read > 0 && size() > 0)
+	while (to_read > 0)
 	{
-		size_t bytes = std::min(to_read, peek_read());
+		size_t available;
-		memcpy(dst, front(), bytes);
+		if (m_wpos == m_rpos)
-		read(bytes);
+			available = m_full ? (m_capacity - m_rpos) : 0;
-		dst += bytes;
+		else if (m_wpos > m_rpos)
-		to_read -= bytes;
+			available = m_wpos - m_rpos;
+		else
+			available = m_capacity - m_rpos;
+
+		if (available == 0)
+			break;
+
+		const size_t copy = std::min(available, to_read);
+		std::memcpy(bdst, m_data.get() + m_rpos, copy);
+		bdst += copy;
+		to_read -= copy;
+
+		m_rpos = (m_rpos + copy) % m_capacity;
+		m_full = false;
 	}
+
 	return nbytes - to_read;
 }
 
-void RingBuffer::write(uint8_t* src, size_t nbytes)
+void RingBuffer::write(const void* src, size_t nbytes)
 {
+	const uint8_t* bsrc = static_cast<const uint8_t*>(src);
 	while (nbytes > 0)
 	{
-		size_t bytes = std::min(nbytes, m_capacity - m_end);
+		size_t free;
-		memcpy(back(), src, bytes);
+		if (m_wpos >= m_rpos)
-		write(bytes);
+			free = m_capacity - m_wpos;
-		src += bytes;
-		nbytes -= bytes;
-	}
-}
-
-size_t RingBuffer::peek_write(bool overwrite) const
-{
-	size_t peek = 0;
-
-	if (overwrite)
-		return m_capacity - m_end;
-
-	if (m_end < m_begin) // [...e   b...]
-		peek = m_begin - m_end;
-	else if (m_end < m_capacity) // [   b...e   ]
-		peek = m_capacity - m_end;
 		else
-		peek = m_begin; // [   b.......e]
+			free = m_rpos - m_wpos;
 
-	return peek;
+		const size_t copy = std::min(free, nbytes);
+		std::memcpy(m_data.get() + m_wpos, bsrc, copy);
+		bsrc += copy;
+		nbytes -= copy;
+
+		m_wpos = (m_wpos + copy) % m_capacity;
+		m_full = m_full || (m_wpos == m_rpos);
 	}
-
-size_t RingBuffer::peek_read() const
-{
-	size_t peek = 0;
-	if (m_begin == m_end)
-	{
-		if (m_overrun)
-			peek = m_capacity - m_begin;
-		else
-			peek = 0;
-	}
-	else if (m_begin < m_end) // [   b...e   ]
-		peek = m_end - m_begin;
-	else if (m_begin < m_capacity) // [...e   b...]
-		peek = m_capacity - m_begin;
-	else
-		peek = m_end; // [...e      b]
-
-	return peek;
-}
-
-/*size_t RingBuffer::write(const char *data, size_t bytes)
-{
-	size_t bytes_to_write;
-
-	if (m_end < m_begin)
-	{
-		bytes_to_write = std::min(m_begin - m_end, bytes);
-		memcpy(m_data + m_end, data, bytes_to_write);
-		m_end += bytes_to_write;
-		return bytes_to_write;
-	}
-	else
-	{
-		size_t in_bytes = bytes;
-		while (in_bytes > 0)
-		{
-			bytes_to_write = std::min(m_capacity - m_end, in_bytes);
-			if (m_end < m_begin && m_end + bytes_to_write > m_begin)
-				m_begin = (m_end + bytes_to_write + 1) % m_capacity;
-
-			memcpy(m_data + m_end, data, bytes_to_write);
-			in_bytes -= bytes_to_write;
-			m_end = (m_end + bytes_to_write) % m_capacity;
-		}
-		return bytes;
-	}
-}*/
-
-void RingBuffer::write(size_t bytes)
-{
-	//assert( bytes <= m_capacity - size() );
-
-	// push m_begin forward if m_end overlaps it
-	if ((m_end < m_begin && m_end + bytes > m_begin) ||
-		m_end + bytes >= m_begin + m_capacity)
-	{
-		m_overrun = true;
-		m_begin = (m_end + bytes) % m_capacity;
-		m_end = m_begin;
-	}
-	else
-		m_end = (m_end + bytes) % m_capacity;
-}
-
-void RingBuffer::read(size_t bytes)
-{
-	assert(bytes <= size());
-
-	m_overrun = false;
-	if ((m_begin < m_end && m_begin + bytes > m_end) ||
-		m_begin + bytes > m_end + m_capacity)
-	{
-		m_begin = m_end = 0;
-		return;
-	}
-
-	m_begin = (m_begin + bytes) % m_capacity;
 }

pcsx2/USB/shared/ringbuffer.h

@@ -1,89 +1,33 @@
-// SPDX-FileCopyrightText: 2002-2023 PCSX2 Dev Team
+// SPDX-FileCopyrightText: 2002-2024 PCSX2 Dev Team
 // SPDX-License-Identifier: LGPL-3.0+
 
 #pragma once
 
-#include <algorithm> // for std::min
 #include <cstdint>
+#include <memory>
 
 class RingBuffer
 {
-	RingBuffer(RingBuffer&) = delete;
+	RingBuffer(const RingBuffer&) = delete;
+	RingBuffer& operator=(const RingBuffer&) = delete;
 
 public:
 	RingBuffer();
 	RingBuffer(size_t capacity);
 	~RingBuffer();
 
-	//size_t write(const char *data, size_t bytes);
+	void reset(size_t size);
-	//size_t read(char *data, size_t bytes);
+	size_t capacity() const;
+	size_t size() const;
 
 	// Overwrites old data if nbytes > size()
-	void write(uint8_t* src, size_t nbytes);
+	void write(const void* src, size_t nbytes);
-	size_t read(uint8_t* dst, size_t nbytes);
+	size_t read(void* dst, size_t nbytes);
-
-	// just move pointers around
-	void write(size_t bytes);
-	void read(size_t bytes);
-
-	template <typename T>
-	void write(size_t samples)
-	{
-		write(samples * sizeof(T));
-	}
-
-	template <typename T>
-	void read(size_t samples)
-	{
-		read(samples * sizeof(T));
-	}
-
-	void reserve(size_t size);
-	// if you care about old data, check how much can be written
-	// may need to call available/write twice in case write pointer wraps
-	size_t peek_write(bool overwrite = false) const;
-	size_t peek_read() const;
-
-	template <typename T>
-	size_t peek_write(bool overwrite = false) const
-	{
-		return peek_write(overwrite) / sizeof(T);
-	}
-
-	template <typename T>
-	size_t peek_read() const
-	{
-		return peek_read() / sizeof(T);
-	}
-
-	// amount of valid data, may need to read twice
-	size_t size() const;
-	template <typename T>
-	size_t size() const
-	{
-		return size() / sizeof(T);
-	}
-	size_t capacity() const { return m_capacity; }
-	char* front() { return m_data + m_begin; }
-	char* back() { return m_data + m_end; }
-
-	template <typename T>
-	T* front()
-	{
-		return (T*)(m_data + m_begin);
-	}
-
-	template <typename T>
-	T* back()
-	{
-		return (T*)(m_data + m_end);
-	}
 
 private:
-	size_t m_begin;
+	std::unique_ptr<uint8_t[]> m_data;
-	size_t m_end;
+	size_t m_capacity = 0;
-	size_t m_capacity;
+	size_t m_rpos = 0;
-	char* m_data;
+	size_t m_wpos = 0;
-	bool m_overrun;
+	bool m_full = false;
 };
-

pcsx2/USB/usb-mic/audiodev-cubeb.cpp

@@ -182,24 +182,12 @@ namespace usb_mic
 		uint32_t CubebAudioDevice::GetBuffer(short* buff, uint32_t frames)
 		{
 			if (!mStream)
-				return frames;
+				return 0;
 
 			std::lock_guard<std::mutex> lk(mMutex);
-			u32 samples_to_read = frames * GetChannels();
+			const size_t read_size = frames * sizeof(buff[0]) * GetChannels();
-			short* pDst = (short*)buff;
+			const size_t bytes_read = mBuffer.read(buff, read_size);
-			pxAssert(samples_to_read <= mBuffer.size<short>());
+			return (bytes_read / sizeof(buff[0]) / GetChannels());
-
-			while (samples_to_read > 0)
-			{
-				u32 samples = std::min(samples_to_read, static_cast<u32>(mBuffer.peek_read<short>()));
-				if (!samples)
-					break;
-				memcpy(pDst, mBuffer.front(), samples * sizeof(short));
-				mBuffer.read<short>(samples);
-				pDst += samples;
-				samples_to_read -= samples;
-			}
-			return (frames - (samples_to_read / GetChannels()));
 		}
 
 		uint32_t CubebAudioDevice::SetBuffer(short* buff, uint32_t frames)
@@ -220,7 +208,7 @@ namespace usb_mic
 				return true;
 
 			std::lock_guard<std::mutex> lk(mMutex);
-			*size = mBuffer.size<short>() / GetChannels();
+			*size = mBuffer.size() / sizeof(short) / GetChannels();
 			return true;
 		}
 
@@ -242,7 +230,7 @@ namespace usb_mic
 		{
 			std::lock_guard<std::mutex> lk(mMutex);
 			const u32 samples = std::max(((mSampleRate * mChannels) * mLatency) / 1000u, mStreamLatency * mChannels);
-			mBuffer.reserve(sizeof(u16) * samples);
+			mBuffer.reset(sizeof(u16) * samples);
 		}
 
 		long CubebAudioDevice::DataCallback(
@@ -253,9 +241,15 @@ namespace usb_mic
 
 			std::lock_guard<std::mutex> lk(ad->mMutex);
 			if (ad->mAudioDir == AUDIODIR_SOURCE)
-				ad->mBuffer.write((u8*)input_buffer, bytes);
+			{
+				ad->mBuffer.write(input_buffer, bytes);
+			}
 			else
-				ad->mBuffer.read((u8*)output_buffer, bytes);
+			{
+				const size_t written = ad->mBuffer.read(output_buffer, bytes);
+				if (written < bytes)
+					std::memset(static_cast<u8*>(output_buffer) + written, 0, bytes - written);
+			}
 
 			return nframes;
 		}