Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #35 - implemented missing Int32_to_Int24_Dither function #798

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Issue #35 - implemented missing Int32_to_Int24_Dither function #798

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b122784
Implemented function "Int32_To_Int24_Dither".
whyismynamerudy Mar 19, 2023
806d4b9
added review comment
whyismynamerudy Mar 20, 2023
f1733cc
amended Int32_To_Int24_Dither following feedback; first attempt at im…
whyismynamerudy May 11, 2023
dea6d98
removed dither scaling
whyismynamerudy May 11, 2023
8dc7928
removed PaUtil_Generate24BitTriangularDither; implemented Phil's feed…
whyismynamerudy May 13, 2023
76c10e3
removed unnecessary "unused parameter" from Int32_To_Int24_Dither; in…
whyismynamerudy May 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 33 additions & 13 deletions src/common/pa_converters.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,13 +935,30 @@ static void Int32_To_Int24_Dither(
void *sourceBuffer, signed int sourceStride,
unsigned int count, struct PaUtilTriangularDitherGenerator *ditherGenerator )
{
(void) destinationBuffer; /* unused parameters */
(void) destinationStride; /* unused parameters */
(void) sourceBuffer; /* unused parameters */
(void) sourceStride; /* unused parameters */
(void) count; /* unused parameters */
(void) ditherGenerator; /* unused parameters */
/* IMPLEMENT ME */

PaInt32 *src = (PaInt32*)sourceBuffer;
unsigned char *dest = (unsigned char*)destinationBuffer;
PaInt32 *scaledDitherResult = (PaInt32*)destinationBuffer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

PaInt32 dither;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PaInt32 dither;


while ( count-- )
{
/* REVIEW */
dither = PaUtil_Generate16BitTriangularDither(ditherGenerator);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#if defined(PA_LITTLE_ENDIAN)
dest[0] = (unsigned char)(*scaledDitherResult);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the * now.

dest[1] = (unsigned char)(*scaledDitherResult >> 8);
dest[2] = (unsigned char)(*scaledDitherResult >> 16);
#elif defined(PA_BIG_ENDIAN)
dest[0] = (unsigned char)(*scaledDitherResult >> 16);
dest[1] = (unsigned char)(*scaledDitherResult >> 8);
dest[2] = (unsigned char)(*scaledDitherResult);
#endif
src += sourceStride;
dest += destinationStride * 3;
}
}

/* -------------------------------------------------------------------------- */
Expand Down Expand Up @@ -1055,16 +1072,19 @@ static void Int32_To_UInt8_Dither(
void *sourceBuffer, signed int sourceStride,
unsigned int count, struct PaUtilTriangularDitherGenerator *ditherGenerator )
{
/* PaInt32 *src = (PaInt32*)sourceBuffer;
unsigned char *dest = (unsigned char*)destinationBuffer; */
(void)ditherGenerator; /* unused parameter */
PaInt32 *src = (PaInt32*)sourceBuffer;
unsigned char *dest = (unsigned char*)destinationBuffer;
PaInt32 dither;

while( count-- )
{
/* IMPLEMENT ME */
/* REVIEW */
dither = PaUtil_Generate16BitTriangularDither( ditherGenerator );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare variable here as in previous function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dither could be declared here.


/* src += sourceStride;
dest += destinationStride; */
*dest = (unsigned char)((((*src) >> 1) + dither) >> 23);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src += sourceStride;
dest += destinationStride;
}
}

Expand Down