search

ploopyco / headphones

A set of 3D-printed headphones, alongside a DAC/amp/EQ board powered by a Raspberry Pico.
833 stars 43 forks source link

Is fix16_mul correctly handling the overflow case? #6

Closed george-norton closed 1 year ago

george-norton commented 1 year ago

I have been browsing the code while I wait for my headphones to arrive, and I can't shake the feeling that something is wrong with the overflow handling here. If I understand right fix_t if a 32bit signed value where bit 31 is an overflow bit, bit 30 is the sign bit. This differs from the libfixmath library where there is no separate overflow bit, the sign bit is bit 31. It seems like the shift statement here should be adjusted to shift by 46 bits so the sign bit is included in the test, otherwise if we overflow into the sign bit the overflow will not be detected.

https://github.com/ploopyco/headphones/blob/1771ccab54b087d47403aaec6cd66c3d18addc57/firmware/code/fix16.c#L52

ploopyco commented 1 year ago

To be perfectly, completely frank, I have no clue. The libfixmath library from which I took the code is not easily decipherable to me. It's entirely possible that you're correct. My only "proof" that I'm right is that the audio sounds right.

If you find that I didn't implement the overflow correction correctly, I'll welcome a pull request.

george-norton commented 1 year ago

I will have a play when my kit arrives, I wondered if maybe it was the root cause of the audio distortion issues you have been seeing. You seem to have fixed that by reducing the input samples from 16bits to 14bits which could just be making the overflow cases very unlikely or impossible.

I also can't see how the overflow case is handled (although maybe in practice it doesn't happen). It seems like saturating the value would be a good strategy for PCM audio..

george-norton commented 1 year ago

@ploopyco I have been able to reproduce the audio distortion issue on my PC in a test harness. I have fixed it here: https://github.com/george-norton/headphones/commit/e6ece89049260f9be8d60c1ed7e397097e602183 But I am reluctant to raise a PR without any real hardware to run it on.

The issue occurs in bqf_transform where the value of y can over/underflow causing the sign to flip. With my change fix16_mul is allowed to temporarily overflow/underflow as we add and subtract multiple coefficients, the overflow is handled right at the end when we convert back to an int16_t.

If you get a chance to test this, and everything works, let me know and I will raise a PR.

george-norton commented 1 year ago

I also pushed my test application, along with some instructions here: https://github.com/george-norton/headphones/tree/master/firmware/test

I have attached a short PCM sample that I have been using to reproduce the distortion. trackerplatz_short.pcm.gz

george-norton commented 1 year ago

There is an extra *4 on the volume adjustment in _as_audio_packet that might also need to be removed since we are no longer dividing the input samples by 4.

ploopyco commented 1 year ago

I'll try this on my DAC on Monday. I'm also going to expedite your preorder so that it ships out sooner, so you can try some of this on your own. Really appreciate this sort of contribution; I'll let you know my results on Monday.

ploopyco commented 1 year ago

It works! Feel free to open a PR, and I'll merge it in.

I've had the privilege of working with some very gifted programmers over my career, but to diagnose and fix a problem at such a low level without actually testing it on the target hardware is one of most incredible feats of engineering I've seen. Well done.