Repository navigation
[BUG] Few problems with audio_stream_avail_frames_aligned() behaviour #8340
Description
Describe the bug
(1)
audio_stream_avail_frames_aligned() uses shifts instead of divisions and so it returns wrong result for formats with frame size not equal to power of 2, e.g. for streams with 3, 5 or 7 channels.
(2)
Default alignment initialization in audio_stream_init() does not work as stream frame format is not yet initialized: it is always set later somewhen after that function call.
(3)
Probably, to overcome (2), every module have to call audio_stream_init_alignment_constants() to initialize alignment. Modules usually do this in .prepare() handler. However, we support binding to a module on a running pipeline without pausing it. In this case .prepare() is not called after new binding as pipeline was not paused or reset and that might left new attached buffer stream with uninitialized alignment.
Another problem is that it is easy to forget to call audio_stream_init_alignment_constants() (must be added to every module!). I see audio_stream_init_alignment_constants() is missing at least in copier, mixin and mixout. It would be better that in this case some default 1 byte alignment initialization take place.
(4)
if alignment initialization is missing -- because of (2) or (3) -- audio_stream_avail_frames_aligned() returns wrong (higher) number of frames. That frames value is used by module_adapter to invalidate buffer memory and is also passed as parameter to .process() func. Luckily, if all connected modules are on same core, buffer invalidate is skipped. For cross-core case, invalidate with wrong (higher) amount of frames leads to memory corruption and weird bugs.
Environment
- Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
- SOF: main branch.
audio_stream_avail_frames_aligned() source;
static inline uint32_t
audio_stream_avail_frames_aligned(const struct audio_stream *source,
const struct audio_stream *sink)
{
uint32_t src_frames = (audio_stream_get_avail_bytes(source) >>
source->runtime_stream_params.align_shift_idx) *
source->runtime_stream_params.align_frame_cnt;
uint32_t sink_frames = (audio_stream_get_free_bytes(sink) >>
sink->runtime_stream_params.align_shift_idx) *
sink->runtime_stream_params.align_frame_cnt;
return MIN(src_frames, sink_frames);
}
Activity
serhiy-katsyuba-intel commentedon Oct 17, 2023
Tagging @andrula-song , @lrgirdwo.
andrula-song commentedon Oct 18, 2023
for question(1), take 3 channels 16 bit format stream(frame size is 6 bytes) and there is no requirement of byte and frame align as example, then the align_frame_cnt should be 1 frame, and align_shift_idx should be 3(8bytes). Assume there is 46 frames(276 bytes) available, then we get 34 frames(46*6>>3) with audio_stream_avail_frames_aligned(), we will process the first 34 frames(204 bytes) this time and process the remaining data(72 bytes/ 12 frames) as well as the incoming data in the next run. Is this not acceptable?
And for the next 3, yes you are right, but I recall I used to call audio_stream_init after stream format has been set. Thanks for reminding, there will be a fix PR later.
lgirdwood commentedon Oct 18, 2023
@serhiy-katsyuba-intel are 3,5,7 channels really used, they sound like a corner case or added for completeness ?
serhiy-katsyuba-intel commentedon Oct 18, 2023
I'm unaware if 3,5,7 channels are really used, just spotted that these will not work correctly.
iganakov commentedon Oct 19, 2023
@lgirdwood at least 3 channels stream is used in Windows topology
andrula-song commentedon Oct 19, 2023
here is the fix #8351
andrula-song commentedon Oct 19, 2023
@iganakov @serhiy-katsyuba-intel , does my explanation for question 1 make sense?
serhiy-katsyuba-intel commentedon Oct 19, 2023
For your example of 3 ch 16 bit stream, i.e. 6 bytes frame, with shifts instead of division, bytes amount will be always divided by 8 instead of 6. And so always fewer frames amount will be returned then actual value is. So we will process data in pace slower then data arrives. This will lead to full buffers so host or dai will start sometimes skip feeding us data.
serhiy-katsyuba-intel commentedon Oct 19, 2023
The fix looks good: fixes issues 2, 3, 4. Does not fix issue 1, but that seems least important.
andrula-song commentedon Oct 20, 2023
I don't think our processing speed will always be slower than data arrivers, still take of 3 ch 16 bit stream as example, usually we allocate buffer can store 2 periods data, here is the processing table
From the 4th time, the processing speed is equal to data incoming, and all the left frames are smaller than a period time, so it won't lead to xrun.
andrula-song commentedon Oct 24, 2023
hi @serhiy-katsyuba-intel , does my explaination make sense?
serhiy-katsyuba-intel commentedon Oct 25, 2023
@wszypelt , do we have some tests which can be run with different channel configuration? So we could run same test with e.g., 2 channel stream and compare it with same test run with 3 channel stream. I suspect 3, 5 and 7 channel streams will not work (will have glitches), other stream configurations: 1, 2, 4 or 8 channels should work.