Seek after Reset might not work as expected

[hajimehoshi]

#171 (comment)

Oto internally reads immediately after Reset, so Seek after Reset might be risky.

/CC @bloeys

[bloeys]

I agree with your comments in the old issue, things will not be safe if you seek while Oto is playing and that is fine.
Either Oto or the user can be using the interface at one time, and this is understandable.

What is not expected though is that it is not safe to seek even when the player is paused and has all its buffers cleared.
This makes even simple use cases like pausing then playing from a different position unsafe.

//Assume a paused player
player.Pause()

//This is NOT safe because 'rseeker' is being used by player
player.Reset()
rseeker.Seek()

//The following is safe to do ONCE. But after that we don't know when we can do it again :)
//So this is 'safe'
rseeker.Seek()
player.Reset()

//This is NOT safe because the second seek will collide with the first reset
rseeker.Seek()
player.Reset()
rseeker.Seek()
player.Reset()

Because of the behavior I mentioned earlier we can't provide users with an API that (for example) controls audio position safely, because they will only be able to call it one time haha.

Some things we can do:

• Ability to disable pre-read (if this causes bad sounds might not be a good idea)
• Function similar to IsPlaying that says when things are being read, like IsReading

I prefer the second one (and I think its easy to implement? One mutex?), but I am worried about there being large latency before Oto stops pre-reading. All sound control APIs will have large latency each time they want to seek or do some simple processing.

So to fix the latency problem, along with point 2 I believe it would be good to be able to control how much pre-read is done. Perhaps a function similar to SetBufferSize, like SetPreReadBufferSize.

Hope this is a good summary of everything.
Would like to hear your thoughts :D

[hajimehoshi]

Another possibility is to let a player implement io.Seeker. This means you no longer have to (or cannot) manipulate the source stream that is passed to a player.

[bloeys]

So the player will handle synchronization internally right? That sounds nice.

Just make sure that the player handles seek calls as soon as possible. We don't want it to finish all reads first (e.g. to fill a buffer), that will be too much latency.

For the implementation, maybe something like the wrapper I made here is enough? When someone gives you a Reader/ReadSeeker wrap internally then let the player use the wrapped version.

[hajimehoshi]

Just make sure that the player handles seek calls as soon as possible. We don't want it to finish all reads first (e.g. to fill a buffer), that will be too much latency.

The player's Seek will invoke Reset internally so the seeking will happen immediately. Read will still happen, but does this cause so much latency?

For the implementation, maybe something like the wrapper I made #171 is enough? When someone gives you a Reader/ReadSeeker wrap internally then let the player use the wrapped version.

You will no longer need locks as you don't have to call Read and Seek on the source stream and instead Oto will call them.

[bloeys]

The player's Seek will invoke Reset internally so the seeking will happen immediately. Read will still happen, but does this cause so much latency?

So when I call player.Seek(), Oto will internally do Reset->pre-reading->source.seek?
If this is the setup then am a bit worried about how long pre-reading will take, especially for low-latency applications.

We both do video game stuff and so am sure you know how much a millisecond is worth ;)

Why I mentioned locks is because lets say pre-read will do 10 reads then stop. Without locks Oto will do reset->10 reads->seek, but if we lock we can do reset->1 read->seek->continue pre-reading. Basically as soon as seek has a chance we run it so the main app can continue its thing.

Sorry if you meant something else.
I just want us to minimize how long we block programs that use Oto, which will allow powerful, high performance audio tools to be built on top!

[hajimehoshi]

If this is the setup then am a bit worried about how long pre-reading will take, especially for low-latency applications.

If you really care about this latency, you should adjust the buffer size by SetBufferSize. Does this make sense?

[bloeys]

Maybe I missed something, but when I was testing before pre-reading was not following SetBufferSize.

For example, I set the buffer to a lot more than enough to read till EOF, but after Reset it read some random amount of data, not the full sound file.

Is this expected behavior?

Edit: Wanted to ask, what's the benefit of allowing pre-read to fully finish before doing a seek?
Like why not (for example) put a lock around this line in the player: https://github.com/hajimehoshi/oto/blob/65dbd988610c6fd92c2ec230aa807777e6d89d31/player.go#L412

[hajimehoshi]

Oto tries to fill the buffer with the buffer size, so the amount should depend on the buffer size.

[hajimehoshi]

Edit: Wanted to ask, what's the benefit of allowing pre-read to fully finish before doing a seek?

Prereading just after Seek might not be needed but this simplifies the implementation (the reading loop can restart immediately after the player can read the source again by resetting).

I'm not sure this is really a problem to you. Can we revisit this issue when you find an actual latency issue that cannot be solved even with SetBufferSize?

[bloeys]

Oto tries to fill the buffer with the buffer size, so the amount should depend on the buffer size.

That's what I expect, but its not what I see with Reset. I had the tada sound, which in total is 451584 bytes. I set the buffer to double that but still it didn't read till EOF.

You can reproduce with this:

        //Play tada
	player.Play()
	for player.IsPlaying() {
		time.Sleep(time.Millisecond)
	}

        // mp3.Length() reports 451584, so we use double that
	player.(oto.BufferSizeSetter).SetBufferSize(451584 * 2)
	fw.Seek(0, io.SeekStart)
	player.Reset()

        //Let Oto finish pre-read
        time.Sleep(5*time.Second)

        //Check how much was read. You will notice its a small amount, and the amount is different every run?!
	pos, _ := fw.Seek(0, io.SeekCurrent)
	println("Amount read:", pos)

Is this correct behavior?

I'm not sure this is really a problem to you. Can we revisit this issue when you find an actual latency issue that cannot be solved even with SetBufferSize?

Definitely!
Just wanted to put the consideration out there :)

[hajimehoshi]

Let me check the actual reading size later. Thanks!

[hajimehoshi]

Please take a look at 19c3101d30142518729150dc41b54752776b14f8. Thanks,

[bloeys]

Thank you! This looks like a great start!
The seek function looks good, locks ASAP without waiting for preload (low latency!) and we no longer have safety issues.

So the only (minor) thing we have left is this: #172 (comment)

Perhaps we should move this to a new issue? Something like "Pre-read loads undefined number of bytes and doesn't fill buffer"?

[hajimehoshi]

Perhaps we should move this to a new issue? Something like "Pre-read loads undefined number of bytes and doesn't fill buffer"?

Yes, please, thanks!