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

Missing Encoding support. #23

Closed
slimsag opened this issue Mar 6, 2016 · 16 comments
Closed

Missing Encoding support. #23

slimsag opened this issue Mar 6, 2016 · 16 comments
Assignees

Comments

@slimsag
Copy link
Member

slimsag commented Mar 6, 2016

From @slimsag on August 8, 2014 20:43

We should add the ability to encode a wav file.

Copied from original issue: azul3d-legacy/audio-wav#1

@slimsag slimsag self-assigned this Mar 6, 2016
@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

We should merge @mewmew's wav encoder (https://github.com/mewkiz/audio/wav) into the package.

See #1 for more information.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

I see that the NewEncoder function requires the use of an io.WriteSeeker:
func NewEncoder(w io.WriteSeeker, conf audio.Config) (audio.Encoder, error)
because we must know the size of the audio samples when writing the file header. What if we modified NewEncoder such that:

// NewEncoder creates a new WAV encoder, which stores the audio configuration in
// a WAV header and encodes any audio samples written to it. The contents of the
// WAV header and the encoded audio samples are written to w.
//
// Note: The Close method of the encoder must be called when finished using it.
//
// If the w parameter is only an io.Writer, then the entire audio stream must be
// buffered in memory before writing to a file completes (because the WAV header
// requires an explicit size). If the w parameter also implements an io.WriteCloser,
// then no buffering in this way will occur and the data is effectively streamed to
// the writer.

func NewEncoder(w io.Writer, conf audio.Config) (audio.Encoder, error)

And we could implement it doing something like: http://play.golang.org/p/QVy10b6QkD
Then the buffered encoder would literally just use a bytes.Buffer and wait for Close(), then write the entire wav file out.

What do you think @mewmew? Obviously an io.WriteSeeker is the best choice if we have one, but in some cases we might not (writing a wav file over a socket, for instance).

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

From @mewmew on August 11, 2014 22:38

What do you think @mewmew? Obviously an io.WriteSeeker is the best choice if we have one, but in some cases we might not (writing a wav file over a socket, for instance).

It is true that using an io.WriteSeeker does limit its use cases and we could implement a fallback. At first I thought it was a great idea to use an io.Writer fallback, but thinking about it still makes me wonder. Some WAV files can be huge and keeping those in memory may not always be feasible. One of the properties I like the most of the audio package is that it implements interfaces for working with streams of audio (of unknown length). While developing the flac library I've had my computer freeze on a number of occasions, simply because it was allocating huge amounts of memory while decoding. Before the API overhaul of flac (issue mewkiz/flac#4) memory was a big concern, and some FLAC files couldn't even be decoded practically. I think that the hopefully few use cases which require an io.Writer, such as network connections would consider creating a temporary file and encode the audio stream to it first. I'm not totally against the fallback, but think it's better to establish some governing principals rather than having the cleanest possible API. I don't want to come across as hash, but I do think that this requires some additional thought.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

I think you make a rather good point. If it accepts an io.Writer and buffers all the data in-memory, it's not totally obvious to the user that it does that (they could read the doc, but it's not obvious just from the API that it does that). Also I guess since io.Writer is kind of an implicit agreement that it is a stream, it would be kind of like breaking that implicit agreement. I guess in practice that it would not be hard to implement io.WriteSeeker on an in-memory buffer, too.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

From @mewmew on September 30, 2014 13:33

We should be able to release the encoding support in a minor release (v1.1) as it only adds new features and doesn't change any of the exported API. A major release (e.g. v2.0) would indicate that there has been backwards incompatible changes to the API.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

@mewmew thanks for the note! You are right and it was an oversight on my part. Looking at the current issues they can all be placed into a minor release.

We will need to bump to v2 when we move the package over to using azul3d.org/audio.v2 though, right?

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

From @mewmew on September 30, 2014 21:10

We will need to bump to v2 when we move the package over to using azul3d.org/audio.v2 though, right?

As far as I can tell, yes. I'm still quite new to Semantic Versioning :)

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

@mewmew okay, I need to read up a lot more on semantic versioning as well so no worries.

Lets see how all of the pending issues play out and maybe we will release just v2 instead of v1.1 then.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

From @mewmew on October 1, 2014 11:23

Lets see how all of the pending issues play out and maybe we will release just v2 instead of v1.1 then.

Sounds good.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

I've actually decided that we will let azul3d.org/audio.v2 brew for a bit longer. I want to review more of the code there and see what else I can find that we can improve upon.

I think the performance improvements here and the encoding feature are significant enough though that we should make a v1.1 release soon (in the next few days?).

I will start separating some issues between v1.1 and v2.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

From @mewmew on October 1, 2014 21:29

I've actually decided that we will let azul3d.org/audio.v2 brew for a bit longer. I want to review more of the code there and see what else I can find that we can improve upon.

I agree, bumping the major version should not be done too often. If we can do a thorough review of the exposed API just to make sure it feels right that would be great! I just need to find myself some time, uni is taking up most of it at the moment :)

I think the performance improvements here and the encoding feature are significant enough though that we should make a v1.1 release soon (in the next few days?).

Sounds good.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

bumping the major version should not be done too often. If we can do a thorough review of the exposed API just to make sure it feels right that would be great!

I completely agree.

I just need to find myself some time, uni is taking up most of it at the moment :)

No worries -- I've been in the same situation more than once. Contribute what you can/want to/have time for, no more is required =)

Sorry for not following through with the release of this as soon as I said I would. I will get around to finishing up v1.1 work in the next few days and I plan to release around now to the 15th.

I ended up spending the previous week learning / fixing our SemVer implementation (see this news article). We can have azul3d.org/audio/wav.v1 correctly point to a v1.1 Git tag/branch now, etc. =)

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

From @mewmew on October 8, 2014 12:46

I ended up spending the previous week learning / fixing our SemVer implementation (see this news article). We can have azul3d.org/audio/wav.v1 correctly point to a v1.1 Git tag/branch now, etc. =)

Thank you for working out the remaining issues related to semantic versioning! I love that Azul3D is now better integrated than ever with godoc.org.

There are still some remaining issues with godoc.org to iron out, for instance:
http://godoc.org/azul3d.org/audio.v1#pkg-files
http://godoc.org/github.com/azul3d/audio#pkg-files

Links to each individual source file only works for github.com links.

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

Links to each individual source file only works for github.com links.

I am aware of it. I've made azul3d-legacy/issues#32 for it now.

There are still some remaining issues with godoc.org to iron out, for instance:

If you find anything else please do let me know. =)

@slimsag
Copy link
Member Author

slimsag commented Mar 6, 2016

Lets move all talk of v1.1 to the PR: #10

@slimsag
Copy link
Member Author

slimsag commented Mar 7, 2016

audio/wav has encoding support already; closing

@slimsag slimsag closed this as completed Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant