Missing Encoding support.
slimsag opened this issue · 16 comments
We should add the ability to encode a wav file.
We should merge @mewmew's wav encoder (https://github.com/mewkiz/audio/wav) into the package.
See #1 for more information.
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).
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.
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.
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.
@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?
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 :)
@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.
Lets see how all of the pending issues play out and maybe we will release just v2 instead of v1.1 then.
Sounds good.
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.
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.
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. =)
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.
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. =)
This issue was moved to azul3d/engine#23