dr_flac not checking for negative qlp shift

[ktmf01]

It seems dr_flac does not check for a negative qlp shift. Please see the flac-dev mailinglist on more information, specifically the "FLAC specification clarification" thread. http://lists.xiph.org/pipermail/flac-dev/2020-June/thread.html

[mackron]

Do you have a sample file I could use as a reference (you can email me privately if you prefer)? I would need to see what's required to support it before making a decision. If it results in a performance regression due to an extra per-sample conditional in the prediction routine I'm not going to be super enthusiastic about adding support because this feels like a very unusual scenario.

If there's no encoder in the real-world that outputs a negative shift then I'm not willing to support it because it's just not practical. However, if at some point in the future one does come up, I'm happy to consider it. I won't be doing anything without a sample file, though.

[ktmf01]

I could make a sample file, but that is not the point. libFLAC, the reference implementation, does not support this either. However, it does check (once per subframe, on reading the qlp shift) that it is non-negative, and throws an error when it is. dr_flac does not seem to check, and might produce wrong output instead of throwing an error.

I could still provide a sample file if you like, no problem.

[mackron]

Ah OK, I thought you meant adding explicit decoding support for it. Yeah I think checking for a negative shift once per sub-frame and throwing an error is a logical first step to get this cleaned up. Commit (dev branch): https://github.com/mackron/dr_libs/commit/28c97152a3daa5bc9d10e6670b8c33f641950eb8

If it's not a hassle, I actually wouldn't mind seeing a sample file. Since there's no other decoder to use as a benchmark I'll need to have an uncompressed version (preferably a WAV file) accompanying it for comparison. I'm thinking I can probably add this without a huge compromise - I have a commented out reference implementation of the LPC prediction routine so I'm thinking of uncommenting that and adding support there. That way it's out of the road of the optimized implementation for the positive-shift case, but still produces valid output for the negative case albeit without an optimized pipeline which I think is reasonable considering the rarity of a negative shift.

[ktmf01]

Well, actually, on that mailinglist thread I'm advocating for altering the specification to explicitly remove negative qlp shift. Based on how LPC quantization works in FLAC (and LPC in general) I'd say that if the decoder finds such a negative qlp shift, the chances that the reason is a corrupt file and not an encoder choice are 1000:1, even if this was ever implemented.

So, I think it is best if you consider this issue closed. However, as promised, here are links to a WAV file and a FLAC file made with a hacked FLAC encoder+decoder. The third link is an ana file including residual in text form, which I used to check whether the hack did what it was supposed to do. WAVE file FLAC file Analysis file

[mackron]

Thanks for that. When I get a chance I'll consider it and make a decision on how to proceed. I think you're right, though - a negative shift in this situation does look strange. It feels like an oversight in the spec, but then again if the spec explicitly defines it as signed then it feels weird not to support it (I still feel a bit troubled by not supporting 32-bit decoding).

[mackron]

I'm deciding not to explicitly support this since it just doesn't make sense. But as per my change earlier it's checked and returns an error if a negative shift is encountered. If the reference implementation is updated to support it I'll also make the update in dr_flac as well.