Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flac.Decode handling of io.EOF #127

Merged
merged 9 commits into from
Nov 2, 2023
Merged

Fix flac.Decode handling of io.EOF #127

merged 9 commits into from
Nov 2, 2023

Conversation

MarkKremer
Copy link
Contributor

@MarkKremer MarkKremer commented Oct 30, 2023

Resolves #126

Testscript I used to check the return codes are conform the Streamer interface below. I've not included the test in the codebase because I don't want to add another audio file to the repo.

package flac

import (
	"github.com/gopxl/beep"
	"github.com/stretchr/testify/assert"
	"os"
	"testing"
)

func TestDecode(t *testing.T) {
	f, err := os.Open("path/to/file.flac")
	if err != nil {
		panic(err)
	}

	streamer, _, err := Decode(f)
	if err != nil {
		panic(err)
	}
	defer streamer.Close()

	collect(beep.Take(streamer.Len()-5, streamer))

	buf := make([][2]float64, 512)

	// Case 2: return ok with 0 < n < 512 samples
	n, ok := streamer.Stream(buf[:])
	assert.True(t, ok)
	assert.Equal(t, 5, n)

	// Case 3: return !ok with n == 0
	n, ok = streamer.Stream(buf[:])
	assert.False(t, ok)
	assert.Equal(t, 0, n)
}

// collect drains Streamer s and returns all of the samples it streamed.
func collect(s beep.Streamer) [][2]float64 {
	var (
		result [][2]float64
		buf    [479][2]float64
	)
	for {
		n, ok := s.Stream(buf[:])
		if !ok {
			return result
		}
		result = append(result, buf[:n]...)
	}
}

@MarkKremer MarkKremer marked this pull request as ready for review October 31, 2023 13:11
@duysqubix
Copy link
Contributor

@MarkKremer

Is there a way we could mock even a simple and super small audio file so that we can inject it into the test itself? I think it's important to have these tests as part of the automated builds and tests

Copy link
Contributor

@duysqubix duysqubix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@MarkKremer
Copy link
Contributor Author

I've been thinking about mocking the flac library itself to really test fail scenario's etc. Ofc with mocking this would make some assumptions on how the flac package works, but I think that's the best option.

@duysqubix
Copy link
Contributor

I'm just thinking without having to copy over the entire library itself, the flac packaging protocol itself, we implement that ourselves and synthetically create a valid flac format string of binary that can be saved in a buffer that is passed to the 'file open'. It doesn't even have to be valid audio or an actual song or noise.. just something that is valid flac to test that the decode/encode works properly.

I'm not very familiar with implementing audio formats, but this is a good start?

@MarkKremer
Copy link
Contributor Author

My original thought was mocking flac.Stream and it's ParseNext() method which returns flac.Frame which can easily be filled with whatever data we want.

We've been working on flac encoding so it's also possible to just encode some data. However, that is not possible for some of the other formats Beep supports. I think it's easier to create audio test files manually than programmatically. Audio formats can be pretty complex.

@duysqubix
Copy link
Contributor

okay fair enough. I don't mind adding audio files to the repo for testing, let's try to keep them very small in size and reuse them for all tests... one for flac, one for mp3, etc...

@MarkKremer MarkKremer marked this pull request as draft November 1, 2023 13:35
@MarkKremer MarkKremer marked this pull request as ready for review November 1, 2023 14:21
@MarkKremer
Copy link
Contributor Author

Thanks for keeping me sharp!

I've added a testfile and test. I've moved existing test functions into the internal directory so they can be used in the subpackages.

@duysqubix duysqubix merged commit f20a09c into main Nov 2, 2023
1 check passed
@duysqubix duysqubix deleted the fix-flac-decode-eof branch November 2, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Beep.Loop() not looping
2 participants