-
Notifications
You must be signed in to change notification settings - Fork 349
[PATCH RFC] audio_stream: Introduce new API to copy data to/from raw buffer #3436
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.
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
Conversation
Introduce two new API functions, that will copy audio streams data to/from raw buffers. This is useful for the new codec adapter where we need to interface data copying between SOF modules and library buffers. For example, in this new usecase data sent from an audio component to a decoder component is compressed so we cannot interpret it with builtin types (s16/s24/s32) but we interpret it as 'raw'. Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the function also check that there is data (for the from_ variant) or room (for the to_ variant) for the data, and limit the amount of data copied? Or do we assume the caller knows what they're doing and we can for example copy and wrap around multiple times?
|
@paulstelian97 audio_stream functions internally deal with wrap around, so I assumed that it is ok for now. |
| static inline void audio_stream_copy_to_buf(const struct audio_stream *source, | ||
| uint32_t ioffset_bytes, | ||
| void *snk, uint32_t bytes_snk, | ||
| uint32_t bytes) | ||
| { | ||
| void *src = audio_stream_wrap(source, | ||
| (char *)source->r_ptr + ioffset_bytes); | ||
| uint32_t bytes_src; | ||
| uint32_t bytes_copied; | ||
| int ret; | ||
|
|
||
| /* sink buf is linear, don't go over its limit */ | ||
| if (bytes > bytes_snk) | ||
| bytes = bytes_snk; | ||
|
|
||
| while (bytes) { | ||
| bytes_src = audio_stream_bytes_without_wrap(source, src); | ||
| bytes_copied = MIN(bytes, MIN(bytes_src, bytes_snk)); | ||
|
|
||
| ret = memcpy_s(snk, bytes_snk, src, bytes_copied); | ||
| assert(!ret); | ||
|
|
||
| bytes -= bytes_copied; | ||
| src = (char *)src + bytes_copied; | ||
| snk = (char *)snk + bytes_copied; | ||
|
|
||
| src = audio_stream_wrap(source, src); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for while loop - only source is circular buffer, let's look for my solution:
static void audio_stream_copy_to_buf(const struct audio_stream *source, void *lib_buff, size_t bytes)
{
uint32_t head_size = MIN(bytes, audio_stream_bytes_without_wrap(source, source->r_ptr));
uint32_t tail_size = bytes - head_size;
memcpy(lib_buff, source->r_ptr, head_size);
if (tail_size)
memcpy((uint8_t *)lib_buff + head_size, source->addr, tail_size);
}
With the wraparound they do (you won't overrun the buffer itself). But none of the functions you have used check the avail/free bytes. So you can read from an empty buffer/write in a full one. You won't get anything that Valgrind would complain about in the unit tests, that's true. But you can extract 1536 bytes from a 768 byte buffer (and you'd see the contents of that 768 byte buffer twice in the extracted data), for example. |
|
@mrajwa I'm bringing up this PR in order to discuss how we should copy the data from SOF component to library buffer. If my understanding is correct, this code assumes: sof/src/audio/codec_adapter/codec_adapter.c Line 294 in 8862fb1
that data copied is not compressed, while this is not true for most of the decoders. Other issue, is that after processing is done library output buffer size might not be equal with input buffer size. |
|
@dbaluta any update here ? |
|
@lgirdwood this patch together with suggestions was picked up by @mrajwa in 97a9482 @mrajwa, as I suggested in your commit lets move these copy functions in audio_stream.h and use them in your PR. I can do that, you only need to use it in your PR. |
|
@dbaluta ok, so does this mean we can close or are there other parts of this PR to merge ? |
|
@lgirdwood we can close this after @mrajwa takes care of my comments for updating 97a9482 Later edit: I just closed this and keep an eye on @mrajwa 's PR |
Introduce two new API functions, that will copy audio streams data
to/from raw buffers.
This is useful for the new codec adapter where we need to interface
data copying between SOF modules and library buffers.
For example, in this new usecase data sent from an audio component to
a decoder component is compressed so we cannot interpret it with builtin
types (s16/s24/s32) but we interpret it as 'raw'.
Signed-off-by: Daniel Baluta daniel.baluta@nxp.com