Skip to content

Audio: Fix pointer arithmetic mistake in ASRC initialize #3254

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

Merged
lgirdwood merged 2 commits into from
Aug 10, 2020
Merged

Audio: Fix pointer arithmetic mistake in ASRC initialize #3254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f93f519
Audio: Fix pointer arithmetic mistake in ASRC initialize
singalsu Jul 29, 2020
c256109
Audio: ASRC: Reformat header file comments with longer lines
singalsu Aug 7, 2020
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
150 changes: 72 additions & 78 deletions src/audio/asrc/asrc_farrow.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ static const struct asrc_filter_params c_filter_params[CR_NUM] = {
/*
* Initialises the pointers to the buffers and zeroes their content
*/
static enum asrc_error_code initialise_buffer(struct asrc_farrow *src_obj);
static enum asrc_error_code initialise_buffer(struct comp_dev *dev,
struct asrc_farrow *src_obj);

/*
* Initialise the pointers to the filters, set the number of filters
Expand All @@ -212,15 +213,14 @@ static enum asrc_error_code initialise_filter(struct comp_dev *dev,
* ------------------------------|-------------------------------------|
* 0x0000 |asrc_farrow src_obj |
* ------------------------------|-------------------------------------|
* &src_obj + sizeof(src_obj) |int_x *buffer_pointer[num_channels] |
* | |
* &src_obj + 1 |int32 impulse_response[filter_length]|
* ------------------------------|-------------------------------------|
* &impulse_response[0] + |int_x *buffer_pointer[num_channels] |
* filter_length | |
* ------------------------------|-------------------------------------|
* &buffer_pointer[0] |int_x ring_buffer[num_channels |
* &buffer_pointer[0] + | int_x ring_buffer[num_channels * |
* + num_channels*sizeof(int_x *)| *buffer_size] |
* ------------------------------|---------------------------------_---|
* &ring_buffer[0] |int32 impulse_response[filter_length]|
* + num_channels*buffer_size | |
* *sizeof(int_x) | |
* ------------------------------|-------------------------------------|
*
* Info:
*
Expand All @@ -237,8 +237,6 @@ enum asrc_error_code asrc_get_required_size(struct comp_dev *dev,
int num_channels,
int bit_depth)
{
int filter_length = 128;
int buffer_length = 256;
int size;

/* check for parameter errors */
Expand Down Expand Up @@ -273,14 +271,17 @@ enum asrc_error_code asrc_get_required_size(struct comp_dev *dev,

/* accumulate the size */
size = sizeof(struct asrc_farrow);
size += sizeof(int32_t *) * num_channels; /* pointers the the buffers */
/* size of the ring buffers */
size += buffer_length * num_channels * (bit_depth / 8);

/* size of the impulse response */
size += filter_length * sizeof(int32_t);
size += ASRC_MAX_FILTER_LENGTH * sizeof(int32_t);

*required_size = size;
/* size of pointers to the buffers */
size += sizeof(int32_t *) * num_channels;

/* size of the ring buffers */
size += ASRC_MAX_BUFFER_LENGTH * num_channels * (bit_depth / 8);

*required_size = size;
return ASRC_EC_OK;
}

Expand Down Expand Up @@ -366,18 +367,10 @@ enum asrc_error_code asrc_initialise(struct comp_dev *dev,
}

/*
* The pointer to the internal ring buffer pointers is
* pointing to the memory subsequently to the memory where the
* src_obj lies. Only one of the buffers is initialised,
* depending on the specified bit depth.
* Set the pointer for the impulse response. It is just after
* src_obj in memory.
*/
if (src_obj->bit_depth == 32) {
src_obj->ring_buffers32 = (int32_t **)(src_obj + 1);
src_obj->ring_buffers16 = NULL;
} else if (src_obj->bit_depth == 16) {
src_obj->ring_buffers16 = (int16_t **)(src_obj + 1);
src_obj->ring_buffers32 = NULL;
}
src_obj->impulse_response = (int32_t *)(src_obj + 1);

/*
* Load the filter coefficients and parameters. This function
Expand All @@ -392,35 +385,30 @@ enum asrc_error_code asrc_initialise(struct comp_dev *dev,
return error_code;
}

/*
* The pointer to the internal ring buffer pointers is
* after impulse_response. Only one of the buffers is initialised,
* depending on the specified bit depth.
*/
if (src_obj->bit_depth == 32) {
src_obj->ring_buffers16 = NULL;
src_obj->ring_buffers32 = (int32_t **)(src_obj->impulse_response +
src_obj->filter_length);
Copy link
Collaborator

@paulstelian97 paulstelian97 Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Wait, how does this work? There is a bit of a reserved region of size filter_length before the first of the buffers? Also, 2D array is incompatible with (int32_t **)

Copy link
Collaborator Author

@singalsu singalsu Aug 4, 2020

Choose a reason for hiding this comment

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

I moved the filter coefficients array to be before buffers32/16. This way I don't need to take into account the variable word length of buffers when computing next array location. The buffers32/16 is the last part of the allocated blob.

Copy link
Collaborator Author

@singalsu singalsu Aug 4, 2020

Choose a reason for hiding this comment

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

As far as I understand a dynamic size 2D array needs to use (int32_t **) pointer.

Copy link
Collaborator

@paulstelian97 paulstelian97 Aug 4, 2020

Choose a reason for hiding this comment

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

Dynamic sized 2D array isn't a continuous memory region unlike statically allocated 2D arrays. Just to be aware of that again.

Of course, when the lower size varies you sure can only go the array-of-pointers with each pointer being an individual array way, and if you want allocate the individual arrays continuously in a big chunk -- but you still need a separate memory region to hold the pointers.

Copy link
Collaborator Author

@singalsu singalsu Aug 4, 2020

Choose a reason for hiding this comment

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

Yes I know. The array of pointers to each row data allows that. However in this case the memory is allocated continuously for simplicity. I'm not aware of ways to do this in C in much cleaner way. We discussed the possibility to fix array first dimension to SOF max channels for simpler initialization but it would consume more RAM.

Now the channel # form the array rows and the channel samples are the columns for "src_obj->ring_buffers32[ch][frame]". The memory region to hold the pointers to column index zeros is in the beginning of the memory area for this "dynamic 2D array". This can be seen in function initialise_buffer().

Actually now I think there's a mistake in asrc_get_required_size(). When computing "size += ASRC_MAX_BUFFER_LENGTH * num_channels * (bit_depth / 8);" the array of pointers to colums data is missing. I think if should be num_channels * sizeof(int32 **). The bug has not triggered because the used filters are significantly shorter than MAX. Do you agree?

Copy link
Collaborator Author

@singalsu singalsu Aug 4, 2020

Choose a reason for hiding this comment

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

Sorry, no mistake, there's the "size += sizeof(int32_t *) * num_channels;". It holds the pointers in beginning of ring_buffers32/16 the pointers to columns data. So this proposal should be OK.

Copy link
Collaborator

@paulstelian97 paulstelian97 Aug 4, 2020

Choose a reason for hiding this comment

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

I'd say the array of pointers should have MAX, but null out any channels between the actual available ones and the max. So for stereo, ring_buffers32[0] and ring_buffers32[1] are valid, while ring_buffers32[2] through ring_buffers32[7] are reliably null.

Copy link
Collaborator Author

@singalsu singalsu Aug 6, 2020

Choose a reason for hiding this comment

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

It would change the initialization and consume more allocated RAM. I don't think the stream channels count could change between prepare() and copy(). So such channels could increase should not happen. Also the channel count for ASRC is initialized only in prepare(), it's not monitored/updated in copy(). Access to more than initialized would require cd->asrc_obj corruption.

Copy link
Collaborator

@paulstelian97 paulstelian97 Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Changing an allocation from 8 to 32 bytes? FWIW I don't think there are blocks in the heap smaller than 32 bytes in most memory configurations anyway. So for the array of pointers you can go ahead an allocate with [MAX] anyway, to protect against mistakes that escape review. Not gonna block on this though.

Copy link
Collaborator Author

@singalsu singalsu Aug 7, 2020

Choose a reason for hiding this comment

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

The ASRC object allocation goes to 2 kB or 4 kB pool usually. This is relatively small in that amount yes. I'll do the next version with channels max fixed length for pointers if keep the channel specific buffers. If channel interleaved (as SRC), no need for such.

} else if (src_obj->bit_depth == 16) {
src_obj->ring_buffers32 = NULL;
src_obj->ring_buffers16 = (int16_t **)(src_obj->impulse_response +
src_obj->filter_length);
}

/* set the channel pointers and fill buffers with zeros */
error_code = initialise_buffer(src_obj);
error_code = initialise_buffer(dev, src_obj);

/* check for errors */
if (error_code != ASRC_EC_OK) {
comp_err(dev, "asrc_initialise(), failed buffer initialise");
return error_code;
}

/*
* Set the pointer for the impulse response.
* &m_impulse_response[0] = ring_buffers_x
* + num_channels * (1 + buffer_length);
* Here ring_buffer_x already points to the memory just behind
* the src_obj. The offset results from the number of pointers
* pointing to each channel and each channels buffer data.
*/
if (src_obj->bit_depth == 32) {
src_obj->impulse_response =
(int32_t *)(src_obj->ring_buffers32 +
src_obj->num_channels *
(1 + src_obj->buffer_length));
} else if (src_obj->bit_depth == 16) {
src_obj->impulse_response =
(int32_t *)(src_obj->ring_buffers16 +
src_obj->num_channels *
(1 + src_obj->buffer_length / 2));
}

/* return ok, if everything worked out */
src_obj->is_initialised = true;
return ASRC_EC_OK;
Expand Down Expand Up @@ -486,7 +474,7 @@ enum asrc_error_code asrc_set_fs_ratio(struct comp_dev *dev,
}

/* Set the channel pointers and zero the buffers */
error_code = initialise_buffer(src_obj);
error_code = initialise_buffer(dev, src_obj);
/* check for errors */
if (error_code != ASRC_EC_OK) {
comp_err(dev, "asrc_set_fs_ratio(), failed buffer initialise");
Expand Down Expand Up @@ -551,52 +539,51 @@ enum asrc_error_code asrc_set_output_format(struct comp_dev *dev,
/*
* BUFFER FUNCTIONS
*/
static enum asrc_error_code initialise_buffer(struct asrc_farrow *src_obj)
static enum asrc_error_code initialise_buffer(struct comp_dev *dev,
struct asrc_farrow *src_obj)
{
uint8_t *buffer;
int32_t *start_32;
int16_t *start_16;
int ch;
int n;

/*
* base_address points to the first address subsequently to the
* memory where the pointers to each ring buffer are stored.
*/
if (src_obj->bit_depth == 32)
buffer = (uint8_t *)(src_obj->ring_buffers32 +
src_obj->num_channels);
else if (src_obj->bit_depth == 16)
buffer = (uint8_t *)(src_obj->ring_buffers16 +
src_obj->num_channels);

/*
* set buffer_length to filter_length * 2 to compensate for
* Set buffer_length to filter_length * 2 to compensate for
* missing element wise wrap around while loading but allowing
* aligned loads.
*/
src_obj->buffer_length = src_obj->filter_length * 2;
if (src_obj->buffer_length > ASRC_MAX_BUFFER_LENGTH) {
comp_err(dev, "initialise_buffer(), buffer_length %d exceeds max.",
src_obj->buffer_length);
return ASRC_EC_INVALID_BUFFER_LENGTH;
}

src_obj->buffer_write_position = src_obj->filter_length;

/* set the base addresses for every channel and initialise the
* buffers to zero
/*
* Initialize the dynamically allocated 2D array and clear the
* buffers to zero.
*/
if (src_obj->bit_depth == 32) {
for (ch = 0; ch < src_obj->num_channels; ch++) {
src_obj->ring_buffers32[ch] = ((int32_t *)buffer) +
start_32 = (int32_t *)(src_obj->ring_buffers32 +
src_obj->num_channels);
for (ch = 0; ch < src_obj->num_channels; ch++)
src_obj->ring_buffers32[ch] = start_32 +
ch * src_obj->buffer_length;

/* initialise to zero */
for (n = 0; n < src_obj->buffer_length; n++)
src_obj->ring_buffers32[ch][n] = 0;
}
} else if (src_obj->bit_depth == 16) {
for (ch = 0; ch < src_obj->num_channels; ch++) {
src_obj->ring_buffers16[ch] = ((int16_t *)buffer) +
/* initialise to zero */
memset(start_32, 0, src_obj->num_channels *
src_obj->buffer_length * sizeof(int32_t));
} else {
start_16 = (int16_t *)(src_obj->ring_buffers16 +
src_obj->num_channels);
for (ch = 0; ch < src_obj->num_channels; ch++)
src_obj->ring_buffers16[ch] = start_16 +
ch * src_obj->buffer_length;

/* initialise to zero */
for (n = 0; n < src_obj->buffer_length; n++)
src_obj->ring_buffers16[ch][n] = 0;
}
/* initialise to zero */
memset(start_16, 0, src_obj->num_channels *
src_obj->buffer_length * sizeof(int16_t));
}
Copy link
Collaborator

@lyakh lyakh Aug 4, 2020

Choose a reason for hiding this comment

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

Today I learned something new about C... And it's even in K&R and I wasn't aware of it. Here's the thing: &(a[1]) - &(a[0]) == 1 for both int a[2]; and int *a; and in both cases it's known at compile-time. However &(a[0][1]) - &(a[0][0]) is also valid for both int a[2][3]; and e.g. int *a[2]; but - for the former it's also known at compile time and is always 1 whereas for the latter it depends on values of a[0] and a[1] and consequently only can be calculated at run-time... Maybe it's obvious to everyone here except me, then we can certainly use this code, but if I'm not the only one who finds it... confusing - to put it mildly - maybe we want to make the code a bit clearer about what exactly we mean :-)

Copy link
Collaborator

@paulstelian97 paulstelian97 Aug 4, 2020

Choose a reason for hiding this comment

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

@lyakh How does it depend on a[1] when both values are read from a[0]?

Copy link
Collaborator

@lyakh lyakh Aug 4, 2020

Choose a reason for hiding this comment

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

@paulstelian97 yes, sorry, see - it's that confusing (for me) :-) Make that &(a[1][0]) - &(a[0][0]);

Copy link
Collaborator

@paulstelian97 paulstelian97 Aug 4, 2020

Choose a reason for hiding this comment

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

For that one yeah, it is statically known for 2D arrays but determined as an actual runtime computation for jagged arrays (double pointer).

Copy link
Collaborator Author

@singalsu singalsu Aug 4, 2020

Choose a reason for hiding this comment

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

Please share your proposal how to make the code a bit clearer.

(I'd like to do a rewrite the data I/O part as this wastes some RAM but I'll do after I've managed to fix the stress test glitches issue. I would remove the "2D" array and use normal channel interleaved circular buffers for FIR as I use in other components like EQ and SRC).

Copy link
Collaborator

@lyakh lyakh Aug 5, 2020

Choose a reason for hiding this comment

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

@singalsu as long as you use a dynamically allocated 2D array like int **a; or int *a[]; and you refer to individual elements using a[i][j] there doesn't seem to be much choice - C wants to have an array of pointers exactly like it's done now. If you can do that as an interleaved array and it would be easier to work with - that could be good, but if you're also fine with the current implementation - it's correct too, basically it's just me who wasn't aware of this C... peculiarity...

Copy link
Collaborator Author

@singalsu singalsu Aug 6, 2020

Choose a reason for hiding this comment

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

I'd like get this simple fix merged unless there are still violations against good practices. I don't promise when but I will do the rewrite.

If it would be just for MCPS improve it would be low priority (current speed should be quite acceptable) but the RAM usage is much more than needed. It's not a large task after there's a stable version that passes the testing. I have now fix PR for 2 of 3 different type know bug. The third is under work. If that fixes remaining issues the rewrite can be done.


return ASRC_EC_OK;
Expand Down Expand Up @@ -764,6 +751,13 @@ static enum asrc_error_code initialise_filter(struct comp_dev *dev,
return ASRC_EC_INVALID_SAMPLE_RATE;
}

/* Check that filter length does not exceed allocated */
if (src_obj->filter_length > ASRC_MAX_FILTER_LENGTH) {
comp_err(dev, "initialise_filter(), filter_length %d exceeds max",
src_obj->filter_length);
return ASRC_EC_INVALID_FILTER_LENGTH;
}

/* The function pointer is set according to the number of polyphase
* filters
*/
Expand Down
60 changes: 26 additions & 34 deletions src/include/sof/audio/asrc/asrc_farrow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@
#include <stddef.h>
#include <stdint.h>

/*
* @brief FIR filter is max 128 taps and delay lines per channel are
* max 256 samples long.
*/
#define ASRC_MAX_FILTER_LENGTH 128
#define ASRC_MAX_BUFFER_LENGTH 256

/*
* @brief Define whether the input and output buffers shall be
* interleaved or not.
Expand Down Expand Up @@ -103,41 +110,26 @@ enum asrc_operation_mode {
*/
enum asrc_error_code {
ASRC_EC_OK = 0, /*!< Operation successful. */
ASRC_EC_INIT_FAILED = -1, /*!< Initialization of the */
/*!< component failed. */
ASRC_EC_UPDATE_FS_FAILED = -2, /*!< Control mode is set to */
/*!< CM_FIXED and update */
/*!< drift is not called in */
/*!< time. */
ASRC_EC_INVALID_POINTER = -3, /*!< Couldn't allocate */
/*!< memory. Bad pointer. */
ASRC_EC_INVALID_BUFFER_POINTER = -4, /*!< Internal buffer pointers */
/*!< are invalid. */
ASRC_EC_INVALID_SAMPLE_RATE = -5, /*!< Sample rate is not */
/*!< supported. */
ASRC_EC_INVALID_CONVERSION_RATIO = -6, /*!< Conversion ratio is not */
/*!< supported. */
ASRC_EC_INVALID_BIT_DEPTH = -7, /*!< Bit depth is not */
/*!< supported. Choose either */
ASRC_EC_INIT_FAILED = -1, /*!< Initialization of the component failed. */
ASRC_EC_UPDATE_FS_FAILED = -2, /*!< Control mode is set to CM_FIXED and update */
/*!< drift is not called in time. */
ASRC_EC_INVALID_POINTER = -3, /*!< Couldn't allocate memory. Bad pointer. */
ASRC_EC_INVALID_BUFFER_POINTER = -4, /*!< Internal buffer pointers are invalid. */
ASRC_EC_INVALID_SAMPLE_RATE = -5, /*!< Sample rate is not supported. */
ASRC_EC_INVALID_CONVERSION_RATIO = -6, /*!< Conversion ratio is not supported. */
ASRC_EC_INVALID_BIT_DEPTH = -7, /*!< Bit depth is not supported. Choose either */
/*!< 16 or 32 bit. */
ASRC_EC_INVALID_NUM_CHANNELS = -8, /*!< Nummber of channels must */
/*!< be larger than zero. */
ASRC_EC_INVALID_BUFFER_LENGTH = -9, /*!< Buffer length must be */
/*!< larger than one. */
ASRC_EC_INVALID_FRAME_SIZE = -10, /*!< Invalid frame size: must */
/*!< be greater than 0 for */
/*!< primary side and */
/*!< secondary side. */
ASRC_EC_INVALID_CLOCK_SKEW = -11, /*!< The clock drift is out */
/*!< of bounds. */
ASRC_EC_INVALID_CONTROL_MODE = -12, /*!< Call update_fs_ratio() */
/*!< for feedback, and */
/*!< update_drift() for fixed */
/*!< control mode. */
ASRC_EC_FAILED_PUSH_MODE = -13, /*!< Push mode operation */
/*!< failed. */
ASRC_EC_FAILED_PULL_MODE = -14 /*!< Pull mode operation */
/*!< failed. */
ASRC_EC_INVALID_NUM_CHANNELS = -8, /*!< Nummber of channels must be larger */
/*!< than zero. */
ASRC_EC_INVALID_BUFFER_LENGTH = -9, /*!< Buffer length must be larger than one. */
ASRC_EC_INVALID_FRAME_SIZE = -10, /*!< Invalid frame size: must be greater than 0 */
/*!< for primary side and secondary side. */
ASRC_EC_INVALID_CLOCK_SKEW = -11, /*!< The clock drift is out of bounds. */
ASRC_EC_INVALID_CONTROL_MODE = -12, /*!< Call update_fs_ratio() for feedback, and */
/*!< update_drift() for fixed control mode. */
ASRC_EC_FAILED_PUSH_MODE = -13, /*!< Push mode operation failed. */
ASRC_EC_FAILED_PULL_MODE = -14, /*!< Pull mode operation failed. */
ASRC_EC_INVALID_FILTER_LENGTH = -15, /*!< Length exceeds max. */
};

/*
Expand Down