HVScrollView

HVScrollView copied to clipboard

Added support for constructing AudioFile instances using integer sample types.

Thank you @mrpossoms for your pull request. It is exactly what I need ! Would it be possible to merge the pull request ?

Glad to contribute @JonathanGirardeau. I'm just waiting on a review from @adamstark. But if you need it desperately asap, you could always reference my fork!

Thank you for adding this and sorry it has taken a little while to look at it! It looks really good, I've just a few questions...

• The resample() function is really cool, very nicely done. Could it have a more descriptive name? Is it specifically about integer samples? In which case maybe resampleIntegerSample() or similar?

• It would be good to add some unit tests for this code so it is easy to check future changes still support this.

Let me know your thoughts! :)

@adamstark Thanks! I'm happy to help!

• The resample() function is really cool, very nicely done. Could it have a more descriptive name? Is it specifically about integer samples? In which case maybe resampleIntegerSample() or similar?

Ah yes, good call! I'll change that to be more self documenting.

• It would be good to add some unit tests for this code so it is easy to check future changes still support this.

Totally agree, I'll add some as well!

Hey @adamstark, sorry it took me so long to finish this out. I added some tests that I think should cover most of the surface area effected by the integer sample change. Let me know what you think!

@adamstark just bumping on this

This all looks great, thank you and I'm happy to merge - so sorry it took me such a long time to come back to you on it.

One question - why the change from uint8_t to int8_t for sampleToSingleByte()? I always assumed the unsigned uint8_t would give a more predictable representation (or at least easier to think about).

This all looks great, thank you and I'm happy to merge - so sorry it took me such a long time to come back to you on it.

One question - why the change from uint8_t to int8_t for sampleToSingleByte()? I always assumed the unsigned uint8_t would give a more predictable representation (or at least easier to think about).

Hard to recall exactly, but I don't think there was a good reason for it. Switched back to uint8_t where possible. Tests are still happy 😄

Thanks for this - merging it in now :)

I'm actually getting some problems with this - I'm seeing that the files written by the library are often not valid or produce silence - so I'm going to revert this for now while I investigate.

To reproduce this, add...

#define PEFORM_TEST_WRITE_TO_ALL_FORMATS 1

to FileWritingTests.cpp and then on the audio files it produces have a listen (without headphones!) from mono_22050_8bit.aif onwards alphabetically.... that one seems fine but after that there are some errors and several file types have silence. I think the changes to the conversion functions have caused some issues.

Ok - it won't let me revert, but I'll investigate and try to resolve it

A bit of good news - I have tried writing a file at a time via the Examples script and most of them seem ok. The issue I think is to do with the file writing test which I don't think is quite right and produces incorrect output for several file / sample rate / bit depth configurations.

After digging a bit I can confirm...

• All floating point uses of the library are ok (although I did find a pre-existing bug that I just fixed)

• Some integer formats are not writing correctly (e.g. see below for sine wave written at 48kHz 8-bit:

or mono 44.1kHz 24-bit (there is a sine wave here it is just incredibly quiet):

• Additionally, MacOS seems to not like the integer files. Some of them sound terrible, but when loaded in Audacity they appear normal.

So from all this it'd be good to identify which integer formats are writing out files correctly and which aren't.

Final update - sorry for the spam! I have now added some tests on develop that read-in the audio files that are written and confirm we are getting back what we think we are writing. I do this for 16-bit and 24-bit and mono/stereo files (as I think there are some differences for other files that I need to iron out).

But yes - those tests fail for integer samples. Perhaps you can have a go at making them work?

@adamstark interesting, sorry about the trouble and thanks for investigating! I think the sine wave you took a screenshot of is quite enlightening. I'll put some more cycles into fixing this with audacity in the loop.

Ok thank you :)

I found this article BTW, which implies that 8-bit data should be unsigned:

https://gist.github.com/endolith/e8597a58bcd11a6462f33fa8eb75c43d

(it also indicates I'm calculating other things very slightly wrongly).

Take a look also at the way I was writing unit tests in the FileWritingTests.cpp file - in particular just underneath this comment...

    //-----------------------------------------------------------------
    // for some key bit depths and mono/stereo files, read in the audio file
    // we just wrote and do a sample-by-sample comparison to confirm we are
    // writing good files

I think this could be a good way to validate that your code is working :)

Ok - I had a bit of a breakthrough tonight and ended up re-writing the functions that convert samples to single bytes. There are now 4 functions...

uint8_t sampleToUnsignedByte (T sample);
int8_t sampleToSignedByte (T sample);
T unsignedByteToSample (uint8_t sample);
T signedByteToSample (int8_t sample);

so this creates neat separation between two different needs. This is particularly important as AIFF files require signed 8-bit samples, while WAV files seem to want unsigned 8-bit samples. This was the reason that some of the files were sounding bad.

So hopefully building on those 4 functions should help you to get what you need working and confirm the integer representations are working. I haven't looked at the integer conversions in those functions.

Thank you for all this - it has helped unpick quite a few issues in the library :)

Another update here - I've now moved the audio sample conversion functions to a dedicated AudioSampleConverter struct and added explicit unit tests for each conversion function. I have validated the float and double conversions, but the integer ones are not working for me.

E.g. the resampleIntegerSample() function, when converting a uint8_t with [0, 255] range, to a uint16_t with a [0, 65535] range doesn't quite seem to give the right value. And it also has to account for the signed int16_t range too [-32768, 32767] (as well as others). So it is a difficult problem.

I think getting this function to cover all these situations (plus the other bit depths, e.g. int32_t) might involve splitting it out into two (or more) functions to handled signed input vs unsigned output and vice versa. Plus the situations where both are signed and both unsigned.

Anyway, let me know if you plan on re-visiting this. No pressure at all - I'm happy to have a go myself if you don't think you've got time :)

Hey, a final bit of news on this - I went on a bit of a deep-dive and ended up working out a new solution for writing to and reading from integer sample types. Basically, you can now work with any integer type, signed or unsigned - as long as the integer type can handle the bit depth of the samples (you can't read a 16-bit sample into an 8-bit int). I've added a table in the README of the project to explain how each integer type is interpreted in the library.

I've also added lots of tests where files are written out and then read back in to confirm they are the same samples we wrote in the first place. So I think this is in a nice place now - thank you for kicking it off, it's definitely great to have this functionality in there :)