Issue #35 - implemented missing Int32_to_Int24_Dither function

[whyismynamerudy]

Title. Implemented the missing function "Int32_To_Int24_Dither" in "pa_converters.c" using the specified coding guidelines.

I had a few questions:

1. Is doing conversions differently for Little Endian vs Big Endian machines required for all conversion functions? I noticed that some functions did make the distinction while some did not, and I cannot figure out why that is the case.
2. What is the purpose of the sourceStride and the destinationStride variables in the conversion functions? Based off of how the other functions were implemented I gather that they're used for navigating across the bits of the binary numbers (I think?) but I am not sure. If that is the case, then why is it so that some strides are directly added onto src while others (such as the sourceStride in the function Int24_To_Float32) are multiplied by a certain number before adding them onto src?

[RossBencina]

• review dither scaling
• assign to the three bytes of dest: dest[0], dest[1], dest[2]
• will need separate big/little endian cases

what I would probably do is have a temporary 32 bit int to which I assign the scaled dithered result, and then have three statements that fan the scaled dithered result out to dest.

[RossBencina]

dest stride times 3

[RossBencina]

Regarding dither scaling, click the link and check the full doc comment for PaUtil_Generate16BitTriangularDither here:

portaudio/src/common/pa_dither.h

Line 73 in 68e963a

@brief Calculate 2 LSB dither signal with a triangular distribution.

Note that when it says: "Ranged for adding to a 1 bit right-shifted 32 bit integer" that is for the case where the output is 16-bits. But you're working on 24-bit output so I think you need to scale the dither by 8 bits, that is

((((*src) >> 1) + (dither >> 8)) >> 7);

Let us know if that is consistent with your reading of the code and documentation in pa_dither.h/pa_dither.c and the other converter functions.

Alternatively (and probably better) we could add new function PaUtil_Generate24BitTriangularDither which is correctly scaled for dithering to 24 bit.

[RossBencina]

@whyismynamerudy I'll keep helping out here, but I have marked this PR as a draft so that other people don't feel the need to review it.

For the future, here are instructions for marking preliminary work in PRs as draft:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request

[dechamps]

@whyismynamerudy I would strongly recommend you use the program in test/patest_converters to test your code. If I run that test on your code, it spits out 5.5 as measured dynamic range for i32→i24, which is clearly wrong and indicates your converter is broken, most likely for the reasons @RossBencina mentioned.

Is doing conversions differently for Little Endian vs Big Endian machines required for all conversion functions? I noticed that some functions did make the distinction while some did not, and I cannot figure out why that is the case.

If you look closely, you will notice that the only converters that care about endianness are the converters that deal with 24-bit data. Other converters don't need to care about endianness because they can use the standard C types to represent the samples: Int16 (short), Float (float). (And of course the question of endianness doesn't apply to (U)Int8 at all.) When manipulating numbers using the standard C types, C takes care of the endianness for you by always reading and writing the data using the endianness of the host. But, sadly, there is no standard C type for a 24-bit integer. Code that wants to read or write 24-bit integers has no choice but to read or write the 3 individual bytes manually. Such code has to pay attention to endianness in order to deal with the bytes in the correct order.

What is the purpose of the sourceStride and the destinationStride variables in the conversion functions?

The term "stride" in this context means step size: how many elements to skip on every iteration. This matters for interleaved audio (i.e. audio where the samples from all channels are interleaved, as in [left1 right 1 left2 right2 etc] as opposed to [left1 left2 etc] [right1 right2 etc]), which is the most common case. Depending on how the dithering is done, it might make sense to dither one channel at a time, so the converter is called on each channel separately. On each converter call, the stride parameter is used to ensure the converter "skips over" the other channels.

In this particular piece of code, the stride is expressed in samples. For non-interleaved audio, the stride is always 1. For interleaved mono audio, the converter will be called with a stride of 1; for interleaved stereo (2 channels), the stride is 2; for interleaved 5.1 (6 channels), the stride is 6; etc.

why is it so that some strides are directly added onto src while others (such as the sourceStride in the function Int24_To_Float32) are multiplied by a certain number before adding them onto src

This again has to do with C number types and how C does pointer arithmetic, and again you will notice that only 24-bit converters do this kind of multiplication.

In that particular code, the stride is expressed in samples. Converters that can get away with using standard C types (e.g. char, short, float) to represent the samples can simply add the sourceStride or destinationStride to a T* pointer, and in C that means advancing the pointer by sourceStride * sizeof(T) bytes, so there is no need to multiply - C already does the multiplication for you based on the pointer type. But 24-bit converters cannot use standard C types and have to deal with the individual bytes directly (see above), and therefore have to multiply the stride by the sample size in bytes (in this case 3) themselves when doing pointer arithmetic.

[whyismynamerudy]

@RossBencina @dechamps Thank you both for you feedback on my code and answering my questions. I apologize for not being as active for the past 2 months - I had exams, but now that they are over I look forward to contributing to this project again.

Currently, I'm looking through the dither generation functions to see how I would be able to implement PaUtil_Generate24BitTriangularDither - it looks like an interesting challenge. Initially I struggled to understand why certain things were being done in the other dither generation functions, but as I spend more time trying to understand it, it begins to make more sense. I'll aim to include the function in my next pull request.

As for the conversion functions, I am working on implementing the feedback I've gotten. For now (until the PaUtil_Generate24BitTriangularDither function I hope to implement gets approved), I will manually try to scale the dither and use the temporary 32 bit int that Ross suggested earlier to spread it out among the 3 bytes of dest depending on the endianness.

Again, thanks to both of you for the feedback - it's helping me learn quite a bit about coding and tackling issues I've never encountered before.

[whyismynamerudy]

Pushed the changes I discussed in my previous comment.

If the PaUtil_Generate24BitTriangularDither function gets approved, I may need help coming up with the function documentation comment - I'm unsure as to what to include in it.

[dmitrykos]

@whyismynamerudy check how dither is applied in this example:

portaudio/src/common/pa_converters.c

Line 1280 in f1733cc

*dest = (signed char) (((temp >> 1) + dither) >> 23);

You do not need to shift dither in your implementation.

[whyismynamerudy]

@whyismynamerudy check how dither is applied in this example:

portaudio/src/common/pa_converters.c

Line 1280 in f1733cc

*dest = (signed char) (((temp >> 1) + dither) >> 23);

You do not need to shift dither in your implementation.

I looked through this and went back to my implementation and I see now - I agree that right shifting dither is unnecessary, I'll make the change and commit ASAP

[dmitrykos]

I think you need to remove PaUtil_Generate24BitTriangularDither from this PR because it is currently unused (dangling code) and due to that PR will not be accepted. Just keep it for the future, if it ever makes sense to use it.

[philburk]

Thanks for tackling this. Data type conversions, particularly ones involving packed 24-bit data, are very tricky.

[philburk]

Thanks for tackling this. Data type conversions, particularly ones involving packed 24-bit data, are very tricky.

[whyismynamerudy]

Thanks for the feedback! I've implemented the changes suggested by @philburk and removed PaUtil_Generate24BitTriangularDither for now - I still have the code, and I can put it back if needed.

If Int32_To_Int24_Dither looks good now, I'll begin working on some of the other unimplemented conversion functions.

[RossBencina]

I've made some suggestions.

Have you run this on the test program?

[RossBencina]

The most obvious red flag here is that you're casting a non-aligned ptr to a 32-bit aligned pointer. But also, why is the scaled dither result being stored in the destination buffer? In any case there's no reason to be storing a temporary result in the destination buffer.

Suggested change

PaInt32 *scaledDitherResult = (PaInt32*)destinationBuffer;

[RossBencina]

Suggested change

	dither = PaUtil_Generate16BitTriangularDither(ditherGenerator);
	PaInt32 dither = PaUtil_Generate16BitTriangularDither(ditherGenerator);

[RossBencina]

Suggested change

PaInt32 dither;

[RossBencina]

Suggested change

	*scaledDitherResult = (PaInt32) ((((*src) >> 1) + dither) >> 7);
	PaInt32 scaledDitherResult = (PaInt32) ((((*src) >> 1) + dither) >> 7);

[RossBencina]

remove the * now.

[RossBencina]

declare variable here as in previous function.

[philburk]

Good progress.

[philburk]

dither could be declared here.

[philburk]

UInt8 audio is centered around 128 and ranges from 0 to 255.
So silence would be all 128s.

[philburk]

To clarify, you need to add an offset to convert Int32 to Uint8 format.

[philburk]

@whyismynamerudy - Good progress so far. Do you want to finish this up?

[whyismynamerudy]

@whyismynamerudy - Good progress so far. Do you want to finish this up?

Hi @philburk , sorry I was inactive for the past few months. I got busy with some other things over summer. I'm getting back to this and I hope to have provided an update over the next two weeks.