untested, for discussion #11
Conversation
|
discussion continued in http://github.com/zikichombo/codec/issues/3 |
There was a problem hiding this comment.
@wsc1 Looks like a good start. I've added some comments for minor details.
Regarding the overall API, we can only determine if it is good once we have lets say two or three encoders and decoders for various formats (especially for formats which are not similar to each other, e.g. varying sample bit rate in the same sound file).
For this reason, I think that the API should be marked experimental, and only finalized once the above condition holds true. At which point we can evaluate if the API is a good match for the various different encoders/decoders.
Cheers,
/u
codec.go
Outdated
| "zikichombo.org/sound/sample" | ||
| ) | ||
|
|
||
| // ErrUnknownCodec is an error representing |
There was a problem hiding this comment.
- // ErrUnknownCodec is an error representing
+ // ErrUnknownCodec is an error representing an unknown codec.
codec.go
Outdated
| // DefaultSampleCodec gives a default or preferred sample codec for the codec. | ||
| // Some codecs, such as perception based compressed codecs, don't really have | ||
| // a defined sample.Codec and should use AnySampleCodec for this field. | ||
| DefaultSampleCodec sample.Codec // DefaultSampleCodec |
There was a problem hiding this comment.
What is the // DefaultSampleCodec comment used for? Can it be removed?
codec.go
Outdated
| // value should be AnySampleCodec if the error return value is nil. | ||
| Decoder func(io.ReadCloser) (sound.Source, sample.Codec, error) | ||
|
|
||
| // Encoder tries to turn a writeCloser into a sound.Sink. |
codec.go
Outdated
| return bestCodec, nil | ||
| } | ||
|
|
||
| // Decoder |
There was a problem hiding this comment.
Before final push, the Decoder, Encoder, Encode and EncodeWith functions should have documentation.
codec.go
Outdated
| Extensions []string // Filename extensions | ||
|
|
||
| // Magic represents the first few bytes, such as fLaC. | ||
| Magic string // Magic represents the first few bytes often |
There was a problem hiding this comment.
It may turn out that this is too limited to distinguish some sound file formats. If we find such a format, or perceive it likely that one will appear at some point, perhaps a Sniff function would make sense to use instead of Magic?
There was a problem hiding this comment.
hmm. thinking :) moved to central design issue.
codec.go
Outdated
| Priority int | ||
|
|
||
| // Extensions lists the filename extensions which this codec claims to support. | ||
| // Examples are .wav, .ogg, .caf. |
There was a problem hiding this comment.
Add note that "The extension string includes a dot prefix."?
codec.go
Outdated
| // DefaultSampleCodec gives a default or preferred sample codec for the codec. | ||
| // Some codecs, such as perception based compressed codecs, don't really have | ||
| // a defined sample.Codec and should use AnySampleCodec for this field. | ||
| DefaultSampleCodec sample.Codec // DefaultSampleCodec |
There was a problem hiding this comment.
Is DefaultSampleCodec only used for encoding? If so, perhaps this could be further clarified in the comment.
codec.go
Outdated
| // Decoder tries to turn an io.ReadCloser into a sound.Source. In the event | ||
| // the decoder does not use a defined sample.Codec, the second return | ||
| // value should be AnySampleCodec if the error return value is nil. | ||
| Decoder func(io.ReadCloser) (sound.Source, sample.Codec, error) |
There was a problem hiding this comment.
Could you clarify what you mean with "In the event/ the decoder does not use a defined sample.Codec"? Should a Decoder return a specific sample.Codec if the entire sound file is of only one sample.Codec (e.g. 32-bit little endian)? And it should return AnySampleCodec if the sound file contains more than one sample codec, e.g. each FLAC frame has a different sample bit rate?
codec.go
Outdated
| // value. In case of confict taking into account the priority, the first codec | ||
| // from a call to RegisterCodec is returned. As this might depend on package | ||
| // initialisation order, it is recommended to Codec implementations intended | ||
| // for library use use even valued priorities so a given application (with a |
There was a problem hiding this comment.
-for library use use
+for library use to use
codec.go
Outdated
|
|
||
| // Decoder | ||
| func Decoder(r io.ReadCloser) (sound.Source, sample.Codec, error) { | ||
| return nil, sample.SFloat32L, nil |
There was a problem hiding this comment.
Not yet implemented. Either return error, e.g. errors.New("codec.Decoder: not yet implemented") or implement ^^
|
Thanks, merge is mostly for gddo and redirecting to http://github.com/zikichombo/codec/issues/3 Sniff() turned out to be simpler! Still need to add at least the wav decoder that is there, or perhaps from github.com/go-audio if we advance on that |
This is a functionally a bit incomplete (with sniffing), and untested. It is for continuing the generic codec interface discussion, inviting community review.