Skip ID3v2 data prepended to flac files on parsing #21
Conversation
In order to disambiguate the introduction of the ID3v2 signature.
Note: a new import for decoding synchronized integers from ID3v2 headers was introduced.
|
Should we add a new test case? Edit: Looks like the travis conf is broken. I'll leave that one to you :) |
flac.go
Outdated
| @@ -80,7 +82,10 @@ func New(r io.Reader) (stream *Stream, err error) { | |||
| } | |||
|
|
|||
| // signature marks the beginning of a FLAC stream. | |||
| var signature = []byte("fLaC") | |||
| var flacSignature = []byte("fLaC") | |||
There was a problem hiding this comment.
-// signature marks the beginning of a FLAC stream.
+// flacSignature marks the beginning of a FLAC stream.
flac.go
Outdated
| var flacSignature = []byte("fLaC") | ||
|
|
||
| // signature marks the beginning of a FLAC stream. | ||
| var id3Signature = []byte("ID3") |
There was a problem hiding this comment.
-// signature marks the beginning of a FLAC stream.
+// id3Signature marks the beginning of a FLAC stream.| @@ -30,6 +30,8 @@ import ( | |||
|
|
|||
| "github.com/mewkiz/flac/frame" | |||
| "github.com/mewkiz/flac/meta" | |||
|
|
|||
| "github.com/mikkyang/id3-go/encodedbytes" | |||
There was a problem hiding this comment.
Should we start using vgo to track versions of external dependencies? We can do this in a follow-up PR.
There was a problem hiding this comment.
Sounds good, I'll create the issue.
flac.go
Outdated
| @@ -111,6 +130,33 @@ func (stream *Stream) parseStreamInfo() (isLast bool, err error) { | |||
| return block.IsLast, nil | |||
| } | |||
|
|
|||
| // skipId3v2 skips ID3v2 data prepended to flac files. | |||
| func (stream *Stream) skipId3v2() (err error) { | |||
There was a problem hiding this comment.
Capitalize ID, I think golint may be upset otherwise: https://github.com/golang/lint/blob/a5f4a247366d8fc436941822e14fc3eac7727015/lint_test.go#L224
Mostly nit picks, but use named return values only when they add to documentation: https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters
flac.go
Outdated
|
|
||
| // Discard unnecessary data from the ID3v2 header. | ||
| _, err = r.Discard(2) | ||
| if err != nil { |
There was a problem hiding this comment.
Personal preference, keep scope of variable to a minimum if possible. This makes it clear we don't use the value later.
if _, err := r.Discard(2) {
return err
}
flac.go
Outdated
|
|
||
| // Read the size from the ID3v2 header. | ||
| var sizeBuf [4]byte | ||
| _, err = r.Read(sizeBuf[:]) |
| @@ -24,7 +24,7 @@ func Encode(w io.Writer, stream *Stream) error { | |||
| enc := &encoder{bw: bitio.NewWriter(buf)} | |||
|
|
|||
| // Store FLAC signature. | |||
| if _, err := enc.bw.Write(signature); err != nil { | |||
| if _, err := enc.bw.Write(flacSignature); err != nil { | |||
There was a problem hiding this comment.
I was reading enc and thought, hmm. Shouldn't it be dec? Haha, I forgot for a moment we have FLAC encoding.
Well, we don't re-encode the samples yes, that is perhaps part of project metalstorm :)
There was a problem hiding this comment.
Haha, yeah I got the same reaction when I got the error. Like, "Wuah..? Ah yeah, now I remember."
However, the code we've written is very nice. I don't have that embarrased feeling I usually have when reading old code! 🐫
There was a problem hiding this comment.
However, the code we've written is very nice. I don't have that embarrased feeling I usually have when reading old code! 🐫
That's great to hear! I feel the same. Small things we can clean up here and there, e.g. #22, but mostly it is straightforward and consistent code.
flac.go
Outdated
| } | ||
|
|
||
| // Second attempt at verifying signature. | ||
| _, err = io.ReadFull(r, buf[:]) |
There was a problem hiding this comment.
Limit scope of err, see comment below for similar example.
Fix comments, thank you! 🌵
|
hallon bror! |
This shoud close #20.
Note: A new import was introduced. [0]
[0]: https://github.com/mikkyang/id3-go/encodedbytes