Player is locked too much

[MarkKremer]

Hey there 👋 ,

I was seeing if I could make Beep working with Oto v2.0. I ran into a problem where the sound stutters when playing from an mp3 file. I think I've narrowed it down to the following:

There are two "loops"; one fills the player's buffer and the other consumes the buffer. The filling loop uses readSourceToBuffer and the consuming loop uses readBufferAndAdd. For both functions, during the entire duration of the function, they lock a lock.

I don't think this is intentional. This defeats the purpose of the buffer. For example, if filling of the buffer takes longer than the time between consuming function calls, the consumer has to wait for the filling to be done.

Instead I would expect readSourceToBuffer to fill the player's p.tmpBuf, then lock and append the p.tmpBuf to p.buf. In this case it matters less that reading from src takes a bit long because the readBufferAndAdd can consume buffer still and appending p.tmpBuf to p.buf is fast.

The mp3 streaming example in the README doesn't have this problem. This is because the mp3 decoder reader returns less bytes than player requested. The requested number of bytes is about 0.5 sec worth of samples long by default. With Beep, the current implementation of the Mixer does try to fill the whole buffer by repeatedly calling the mp3 reader. This takes long enough that the above problem occurs on my laptop (about 60ms).

I've made an example so you can hear the problem yourself. The following code has a reader which will sleep for 100ms every 0.5sec of samples. On my machine, the sounds starts stuttering around 65ms of delay, 100ms is used to make it more obvious. It should take <200ms to read 0.5sec of samples. Which should be enough if the buffer worked correctly, but isn't.

Change the path in the example to an existing mp3 file on your computer.

package main

import (
	"io"
	"os"
	"time"

	"github.com/hajimehoshi/go-mp3"
	"github.com/hajimehoshi/oto/v2"
)

// Usually 44100 or 48000. Other values might cause distortions in Oto
const samplingRate = 44100

// Number of channels (aka locations) to play sounds from. Either 1 or 2.
// 1 is mono sound, and 2 is stereo (most speakers are stereo).
const numOfChannels = 2

// Bytes used by a channel to represent one sample. Either 1 or 2 (usually 2).
const audioBitDepth = 2

type EvilReader struct {
	src         io.Reader
	samplesRead int
}

func (e *EvilReader) Read(p []byte) (n int, err error) {
	if e.samplesRead > samplingRate/2 {
		time.Sleep(time.Millisecond * 100)
		e.samplesRead = 0
	}

	n, err = e.src.Read(p)

	e.samplesRead += n / numOfChannels / audioBitDepth

	return n, err
}

func main() {
	path := "./my-file.mp3"

	// Open the file for reading. Do NOT close before you finish playing!
	file, err := os.Open(path)
	if err != nil {
		panic("opening my-file.mp3 failed: " + err.Error())
	}
	defer file.Close()

	// Decode file
	decodedMp3, err := mp3.NewDecoder(file)
	if err != nil {
		panic("mp3.NewDecoder failed: " + err.Error())
	}

	// Insert an evil reader into the chain.
	evilReader := &EvilReader{
		src:         decodedMp3,
		samplesRead: 0,
	}

	// Prepare an Oto context (this will use your default audio device) that will
	// play all our sounds. Its configuration can't be changed later.

	// Remember that you should **not** create more than one context
	otoCtx, readyChan, err := oto.NewContext(samplingRate, numOfChannels, audioBitDepth)
	if err != nil {
		panic("oto.NewContext failed: " + err.Error())
	}
	// It might take a bit for the hardware audio devices to be ready, so we wait on the channel.
	<-readyChan

	// Create a new 'player' that will handle our sound. Paused by default.
	player := otoCtx.NewPlayer(evilReader)

	// Play starts playing the sound and returns without waiting for it (Play() is async).
	player.Play()

	// We can wait for the sound to finish playing using something like this
	for player.IsPlaying() {
		time.Sleep(time.Millisecond)
	}

	// If you don't want the player/sound anymore simply close
	err = player.Close()
	if err != nil {
		panic("player.Close failed: " + err.Error())
	}
}

Note that I'm not sure how much of this Beep will use in the future. But I hope this bug report helps.

---

Sidenote: readSourceToBuffer could be called in parallel for all players instead of sequentially. This would give even more wiggle room for readers to be slow. But that's a nice to have.

[hajimehoshi]

Thanks!

Instead I would expect readSourceToBuffer to fill the player's p.tmpBuf, then lock and append the p.tmpBuf to p.buf. In this case it matters less that reading from src takes a bit long because the readBufferAndAdd can consume buffer still and appending p.tmpBuf to p.buf is fast.

The current lock is used not only for buffers but also for other members like state. So the things are not so easy, but I admit the lock granurarity could be smaller.

I'll try the example later, thanks!

[hajimehoshi]

The core issue is that an interface call Read should not be in a critical section by a mutex.

[MarkKremer]

The core issue is that an interface call Read should not be in a critical section by a mutex.

Couldn't have summarized it better.

Thanks for the quick reply.