Skip to content

Commit

Permalink
audio_stream: Persist requirements and lazy-recalculate alignment
Browse files Browse the repository at this point in the history
As specified, this function was a bit of a booby trap: it has to be
called exactly once, after all other setup that modifies frame size
has been done.  If it is called in the wrong order, or not at all, any
consumers of the frame alignment API on this stream will see garbage
data in the computed fields.  That's way too much complexity for the
component code that needs to use this tool, especially as the impact
is not in the component itself (e.g. it's a downstream copier widget
with SIMD code that fails).

Instead, preserve the two requirements set by this function in the
audio_stream struct and recompute them any time any of the upstream
values change.  That allows this to be safely used from any context.

There's a mild complexity with audio_sink/source layer, which is
written to directly modify the format (it keeps a pointer into
audio_stream of its own) without going through the audio_stream API
itself.  There, the framework gives us "on_audio_format_set()"
callbacks on source and sink, so implement it there.

This is a partial fix for Issue #8639

Signed-off-by: Andy Ross <andyross@google.com>
  • Loading branch information
andyross committed Jan 8, 2024
1 parent 3c26552 commit 99e3c64
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 8 deletions.
35 changes: 32 additions & 3 deletions src/audio/audio_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ static uint32_t audio_stream_frame_align_get(const uint32_t byte_align,
}


void audio_stream_init_alignment_constants(const uint32_t byte_align,
const uint32_t frame_align_req,
struct audio_stream *stream)
void audio_stream_recalc_align(struct audio_stream *stream)
{
const uint32_t byte_align = stream->byte_align_req;
const uint32_t frame_align_req = stream->frame_align_req;
uint32_t process_size;
uint32_t frame_size = audio_stream_frame_bytes(stream);

Expand All @@ -93,6 +93,16 @@ void audio_stream_init_alignment_constants(const uint32_t byte_align,
(is_power_of_2(process_size) ? 31 : 32) - clz(process_size);
}

void audio_stream_init_alignment_constants(const uint32_t byte_align,
const uint32_t frame_align_req,
struct audio_stream *stream)
{
stream->byte_align_req = byte_align;
stream->frame_align_req = frame_align_req;
audio_stream_recalc_align(stream);
}


static int audio_stream_release_data(struct sof_source *source, size_t free_size)
{
struct audio_stream *audio_stream = container_of(source, struct audio_stream, source_api);
Expand Down Expand Up @@ -145,11 +155,28 @@ static int audio_stream_sink_set_alignment_constants(struct sof_sink *sink,
return 0;
}

static int source_format_set(struct sof_source *source)
{
struct audio_stream *s = container_of(source, struct audio_stream, source_api);

audio_stream_recalc_align(s);
return 0;
}

static int sink_format_set(struct sof_sink *sink)
{
struct audio_stream *s = container_of(sink, struct audio_stream, sink_api);

audio_stream_recalc_align(s);
return 0;
}

static const struct source_ops audio_stream_source_ops = {
.get_data_available = audio_stream_get_data_available,
.get_data = audio_stream_get_data,
.release_data = audio_stream_release_data,
.audio_set_ipc_params = audio_stream_set_ipc_params_source,
.on_audio_format_set = source_format_set,
.set_alignment_constants = audio_stream_source_set_alignment_constants
};

Expand All @@ -158,6 +185,7 @@ static const struct sink_ops audio_stream_sink_ops = {
.get_buffer = audio_stream_get_buffer,
.commit_buffer = audio_stream_commit_buffer,
.audio_set_ipc_params = audio_stream_set_ipc_params_sink,
.on_audio_format_set = sink_format_set,
.set_alignment_constants = audio_stream_sink_set_alignment_constants
};

Expand All @@ -167,6 +195,7 @@ void audio_stream_init(struct audio_stream *audio_stream, void *buff_addr, uint3
audio_stream->addr = buff_addr;
audio_stream->end_addr = (char *)audio_stream->addr + size;

audio_stream_init_alignment_constants(1, 1, audio_stream);
source_init(audio_stream_get_source(audio_stream), &audio_stream_source_ops,
&audio_stream->runtime_stream_params);
sink_init(audio_stream_get_sink(audio_stream), &audio_stream_sink_ops,
Expand Down
12 changes: 7 additions & 5 deletions src/include/sof/audio/audio_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,15 @@ struct audio_stream {
void *r_ptr; /**< Buffer read position */
void *addr; /**< Buffer base address */
void *end_addr; /**< Buffer end address */
uint8_t byte_align_req;
uint8_t frame_align_req;

/* runtime stream params */
struct sof_audio_stream_params runtime_stream_params;
};

void audio_stream_recalc_align(struct audio_stream *stream);

static inline void *audio_stream_get_rptr(const struct audio_stream *buf)
{
return buf->r_ptr;
Expand Down Expand Up @@ -175,6 +179,7 @@ static inline void audio_stream_set_frm_fmt(struct audio_stream *buf,
enum sof_ipc_frame val)
{
buf->runtime_stream_params.frame_fmt = val;
audio_stream_recalc_align(buf);
}

static inline void audio_stream_set_valid_fmt(struct audio_stream *buf,
Expand All @@ -191,6 +196,7 @@ static inline void audio_stream_set_rate(struct audio_stream *buf, uint32_t val)
static inline void audio_stream_set_channels(struct audio_stream *buf, uint16_t val)
{
buf->runtime_stream_params.channels = val;
audio_stream_recalc_align(buf);
}

static inline void audio_stream_set_underrun(struct audio_stream *buf,
Expand Down Expand Up @@ -363,11 +369,7 @@ static inline int audio_stream_set_params(struct audio_stream *buffer,
buffer->runtime_stream_params.rate = params->rate;
buffer->runtime_stream_params.channels = params->channels;

/* set the default alignment info.
* set byte_align as 1 means no alignment limit on byte.
* set frame_align as 1 means no alignment limit on frame.
*/
audio_stream_init_alignment_constants(1, 1, buffer);
audio_stream_recalc_align(buffer);

return 0;
}
Expand Down

0 comments on commit 99e3c64

Please sign in to comment.