Fix mp3.decoder.Len() when file size is unknown

[duysqubix]

I'm trying to read an audio file through a reader from a []byte array, but when I do this, the length of the output stream is 0. However, using the traditional method of os.Open(), the length is 9589248. Both methods can play the audio, but I can't use .Seek() when using the byte array. I'm not sure what's wrong, any help would be appreciated.

package main

import (
	"bytes"
	"fmt"
	"io/ioutil"
	"os"

	"github.com/faiface/beep/mp3"
)

func main() {
	file, err := ioutil.ReadFile("audio.mp3")
	if err != nil {
		panic(err)
	}

	file2, err := os.Open("audio.mp3")
	if err != nil {
		panic(err)
	}

	fileString, _, _ := mp3.Decode(ioutil.NopCloser(bytes.NewReader(file))) // Outputs 0
	fileString2, _, _ := mp3.Decode(file2) // Outputs 9589248

	fmt.Println(fileString.Len())
	fmt.Println(fileString2.Len())
}

Original issue: faiface/beep#123

[MarkKremer]

Without testing this my hypothesis is that ioutil.NopCloser doesn't implement io.Seeker. According to the docs, go-mp3's Decoder.Length() method returns -1 when the total size isn't available. Beep tries to convert this to the number of samples by dividing this by the number of bytes in a frame and this value gets round down. -1 / 4 = 0.

This needs to be handled and documented better at Beep's side.

[lctech-daniel-hung]

help!!!

[duysqubix]

@lctech-daniel-hung, Could you elaborate a bit on what you need help with?

[lctech-daniel-hung]

@lctech-daniel-hung, Could you elaborate a bit on what you need help with?

I've also encountered this issue. I'm currently trying to parse the length of an MP3 to perform sampling, but the length is always zero. I'd like to ask if there have been any updates or fixes for this issue?"

[MarkKremer]

Do you pass an io.Seeker to the Decode function?

To be honest this issue got away from me. But from the information I have now a "fix" would be an explicit error when trying to seek or get the length using a non-io.Seeker as a reader.

Edit: if you need the duration of a reader which isn't seekable, it's possible to add it to a Buffer. The streamers returned by the Buffer do have a length and are seekable because everything is loaded into memory.

Some background info: it's not possible to get the length of an mp3 file without seeking or going through the whole file because the mp3 header doesn't encode the total duration of the file. Getting the duration is a bit of a manual process.

[MarkKremer]

Would it be more go-esque if instead of one Decode function there were 2?

func Decode(rc io.ReadCloser) (s beep.StreamCloser, format beep.Format, err error) {}
func DecodeWithSeekAndLen(rc io.ReadSeekCloser) (s beep.StreamSeekCloser, format beep.Format, err error) {}

---

I'm just thinking about solving the original issue but if your question is different in some way don't hesitate to reply @lctech-daniel-hung

[duysqubix]

@MarkKremer
What value add is there to having two functions? Do we expect there to be other variants as well?

[MarkKremer]

edit: I've rewritten this comment so it's less had-a-long-day-brained.

The benefit of having two functions is that we get compile-time checking. Currently, the Decode function returns a StreamSeekCloser, even though when no io.Seeker is passed in, the Seek part isn't actually available. Then we have to return errors at runtime or it is unclear to the user what is happening.

I expect the number of variants to be dependent on the number of input Reader variants. We have io.Reader, io.Seeker and io.Closer. io.Reader and io.Closer are always present (question mark?). So the only variants are if the input reader implements io.Seeker or not. So 2 variants.

The output type may differ depending on the Decoder. For example, mp3 needs an io.Seeker to be able to get the length of the audio, but some other types encode the length in the header so Len() would be available even for when the input reader doesn't implement io.Seeker.

The function names in my earlier comment can be improved. Maybe DecodeReader and DecodeReadSeeker?

[duysqubix]

That makes sense. Thanks for explaining. I'm alright with that approach it would be easier to manage as well.

[MarkKremer]

Started a proof of concept implementation in #132.

I noticed that the Len() and Position() functions are part of the StreamSeeker interface. So the beep.Streamer returned by DecodeStreamer doesn't allow you to get the position in the streamer even though seeking isn't required to do so. The same is true for some other decoders with the Len() method. I'm of the opinion that Len() and Position() shouldn't be part of the StreamSeeker interface and should probably be their own interfaces. Although that would spawn 2 additional interfaces and possibly a number of interface combinations.

I secretly hope that's just a bump in the road and it will figure itself out with some more refactoring...

[ivcz]

I've had this problem trying to stream mp3s, ended up just reloading the stream when it's finished buffering and starting it from the last position. Does go-mp3 already have a method that updates decoded buffer size?

[MarkKremer]

Could you explain what problem the larger buffer size would solve? A code snippet of what you're trying to do would be appreciated too.

Also see my previous comment. Do these solutions work for you?

Edit: It dawned on me that the original author may have just wanted to be able to decode a file without closing it. Although they also speak about reading from a byte slice so I'm a bit confused about this. Those are two problems that need different solutions in my opinion.

[ivcz]

I want to be able to play an mp3 before fully downloading it, and also be able to seek through it. The second part is where go-mp3s decode starts being a problem

[MarkKremer]

Thank you for your reply. That's indeed a limitation of go-mp3 but it has nothing to do with the buffer size. When a seeker is passed to it, it will enable the seek functionality, build a seek table and calculate the audio's length. In theory it should be possible to build the seek table on-demand to make your use-case possible, but it doesn't right now. The project has been archived also...

Your example does make clear that the Len() function/interface should be separate from Seek. That's good to know in future library design efforts!