search

melchor629 / node-flac-bindings

Nodejs bindings to libFLAC
ISC License
17 stars 1 forks source link

24bit encoding #5

Closed dmooney65 closed 6 years ago

dmooney65 commented 6 years ago

Hi,

I've been playing around with 24bit encoding and it looks like there's a problem. Here's a gist that illustrates the problem.

The output.raw file is playable with aplay -f S24_LE -r 48000 -c 2 output.raw but the out.flac file is corrupt.

I played around with arecord and found that using arecord -f S24_3LE -r 48000 -c 2 > out.raw and then encoding that with FileEncoder produced a semi-playable file, but with periodic interference. Looks like something may be wrong with the 32bit to 24bit conversion?

Thanks

melchor629 commented 6 years ago

Thanks for reporting the bug. Yep, that error is currently happening. It's kind weird the output flac :) I think the problem is not related on how the conversion is done, but on how extra bytes are handled. But I will investigate right now to fix the issue.

melchor629 commented 6 years ago

After doing some tests I figured out that the problem was exactly what I though (see 7d6fc19). I make some tests with 24bit raw audio and the output was correct.

I hope this fixes the problem. Try it. I created the version 1.2.3 so updating the library version should be fine to test.

dmooney65 commented 6 years ago

OK so that's a partial fix. The original test with S24_LE still fails but it encodes fine if I use S24_3LE, which I suppose is "proper" 24bit, i.e. 3x8bit. S24_LE as I understand it is actually 32bit but with the first byte zeroed.

melchor629 commented 6 years ago

Well, as I see if S24_LE is 24 bit audio in 32 bit integers, that's something you must take care of, because the encoder expects 24 bit audio input if it is said to take as input 24 bit audio.

I could implement a version to take care of this situation (S24_LE). But in my opinion, the issue is fixed because 24 bit audio is encoded correctly.

melchor629 commented 6 years ago

In 016a49f I implemented the version when you have 24 bit input stored in 32 bit integers, but must be explicitly set to the encoder. Use the option inputAs32 set to true to tell that the input is 24 bit audio but stored in 32 bit ints (no conversion done internally with this option because flac expects all inputs to be 32 bit integers).

dmooney65 commented 6 years ago

OK that works, but there's a typo on line 34 of encoder.js. I changed self._inputAs32 = Boolean(option.inputAs32) || false; to read options.inputAs32 to make it work.

IMO that option should probably be default for 24bit as S24_LE is the norm and S24_3LE the exception. I've tried running ffprobe on my 24/96 and 24/192 FLAC files and they all report something like this: Duration: 00:03:39.88, start: 0.000000, bitrate: 3340 kb/s Stream #0:0: Audio: flac, 96000 Hz, stereo, s32 (24 bit)

Either way I'm happy. Thanks a lot for your quick response.

melchor629 commented 6 years ago

Yep, that's a typo, I couldn't test anything, I'm kind busy right now :). Fixing that.

Also I like you're idea of making this option as true by default when the input is 24 bit, so I will make the change. I didn't play a lot with 24 bit audio, and I see you know more about that in this topic. I will add this thing right now, because is quickly to change.