-
Notifications
You must be signed in to change notification settings - Fork 43
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
Coping with damaged flac files #31
Comments
Turning CRC mismatches into warnings, rather than fatal errors seems reasonable. We'll push this change momentarily.
As for CRC, sure. However, for the later two cases, it seems better to try to get a working FLAC file when it is broken is incorrectly encoded. Otherwise we may end up with something similar to quirks mode in HTML, which is a mess. Better to stay true to the FLAC standard. As far as I'm aware, it should never be the case that a valid FLAC file has different number of samples in the side and left channels (i.e. in For the LPC case, it seems your file does not have I.e. add the following to if subframe.NSamples != len(subframe.Samples) {
log.Fatalf("subframe sample count mismatch; expected %d, got %d", subframe.NSamples, len(subframe.Samples))
} |
Done as of rev 8c55685. |
Let us not call it quirks mode. Quirks mode is about dealing with human ignorance. Here we have flipping bits. Why not call it error correction? Or maybe better "error resilience". Apparently mp3 decoders never panic on such issues. They attempt to be robust about damages and try to (sometimes audibly) skip over. I find this preferable.
Thank you. However, this change alone may not be sufficient. What this does is that...
may now return incomplete number of subframes or samples. You either get "len(frame.Subframes) < channels" or "len(frame.Subframes[n].Samples) < int(frame.BlockSize). Without any precaution this will likely crash the consumer of the API. Some guidance is expedient. Btw, you can use this to find damaged flac files (I just learned):
|
* flac: encode frame header * flac: calculate CRC-8 when encoding frame headers * flac: fix encoding of frame header * flac: add preliminary subframe encoder * flac: fix UTF-8 encoding of frame number * frame: add sanity check for sample count in decodeLPC Updates #31. * flac: update flac encoding API, depricate flac.Encode Encode has been removed in favour of using NewEncoder. The Encode function was temporarily added to support re-encoding FLAC streams to update the metadata, but it had no support for encoding audio samples. The added flac.Encoder has support for encoding both metadata and audio samples. It also does not require that you first decode a FLAC file to later re-encode it by calling Encode (as was the previous behaviour). * flac: add MD5 running hash of unencoded audio samples to StreamInfo * flac: remove unused encodePadding Reported by golangci * flac: fix golangci lint issues frame/utf8.go:57:6: `decodeUTF8Int` is unused (deadcode) func decodeUTF8Int(r io.Reader) (n uint64, err error) { ^ internal/utf8/encode.go:32:16: unnecessary conversion (unconvert) bits = uint64(t2 | (x>>6)&mask2) ^ internal/utf8/encode.go:37:16: unnecessary conversion (unconvert) bits = uint64(t3 | (x>>(6*2))&mask3) ^ internal/utf8/encode.go:42:16: unnecessary conversion (unconvert) bits = uint64(t4 | (x>>(6*3))&mask4) ^ * flac: fix golangci lint issues encode_frame.go:89:1: cyclomatic complexity 52 of func `(*Encoder).encodeFrameHeader` is high (> 30) (gocyclo) func (enc *Encoder) encodeFrameHeader(w io.Writer, hdr frame.Header) error { ^ internal/utf8/encode.go:66:17: unnecessary conversion (unconvert) bits := uint64(tx | (x>>uint(6*i))&maskx) ^ encode_subframe.go:105:46: unnecessary conversion (unconvert) if err := bw.WriteBits(uint64(sample), byte(hdr.BitsPerSample)); err != nil { ^ * flac: clarify that frame.Header.Num is calculated by the encoder * flac: minor re-phrasing
Hi. With some flac files (rather rare, but still) I get "crc mismatch" messages, either on startup or at some later point during playback. In some cases I see an additional message such as this: "frame.Subframe.decodeRicePart: The flac library test cases do not yet include any audio files with Rice parameter escape codes. If possible please consider contributing this audio sample to improve the reliability of the test cases." Is it fair to assume that an internally damaged flac file could cause such a message to show up? All crc mismatch messages lead to a program abort, which is not ideal.
In some cases I didn't get a crc mismatch message, but a direct array overrun panic. I added three "if" statements to make the code skip over these situations. I have not seen a single array overrun since. See my code changes ("tmtmtm") below. Do you agree that making the decoder more robust against damaged flac files is a reasonable goal?
frame.go:
subframe.go:
The text was updated successfully, but these errors were encountered: