Do I understand it correctly, that previous the assert() was checking, that in the current sink the number of frames still pending from previous copies wasn't "too large," specifically that it wasn't larger than the number of free frames over all sinks? Which is similar to saying, that no sink is more than half full (assuming they all have the same size buffers)? Now you replace that check with a different one, namely checking that the number of still pending frames isn't larger than what we want to copy now? Which might actually be even easier to hit - if your last transfer is small and you still have a few pending frames from the previous run.

@lyakh Correct. "frames_to_copy" already taken into account as "sinks_free_frames = MIN(sinks_free_frames, free_frames - pending_frames);" is done earlier in the function.

So old check assumes "start_frame" cannot ever be larger than sinks_free_frames. This is not correct and you either have undefined behaviour (this currently happens) or assert is hit (when we enable CONFIG_ASSERT=y).

My modified code takes account both how much we have leftovers in source, and how much we have space in output.

@kv2019i maybe I misunderstand... Can it not be that start_frame == 10, frames_to_copy == 5 just because we only want to copy 5 frames, because source_avail_frames == 5 and there's plenty of free space in the buffer, but here you decide that this is an error condition and set frames_to_copy to 10?

@lyakh Boy, this took a long time. But thanks, you are right, my fix was not correct. Looking at this again, it seems the problem is with the assert and it's actually not needed. If "start_frame == 10" and "frames_to_copy == 5", the "sinks_free_frames" can still be "3", "5" or "10", or are valid values. The "start_frame" is specific to a single mixout buffer, while the other variables are calculcated as a minimum across all mixout buffers. So it's perfectly possible that a single mixout buffer has more already mixed data, than what can be written in this cycle by mixin.

So long story short, the assert is not correct. And as the code can handle this case gracefully, no actions are needed in its place.

@kv2019i mmh, almost. I think sinks_free_frames == 3 would be invalid in your above example, but that's ok because it cannot be that. IIUC in mixin copying method we look over all mixouts, connected to this mixin, check free space in each of them for us and select the minimum over them all so that we copy the same number of frames to all to make sure that the mixin itself has a consistent state - it has pushed out frames_to_copy frames to all mixouts and its number of available for copying frames is reduced by that number. But otherwise yes, that assertion does seem wrong to me too. Maybe it got here by confusion with https://github.com/thesofproject/sof/blob/a88ba51bb4c5cd6b7a984179d39d1b9110e70d94/src/audio/mixin_mixout/mixin_mixout.c#L368 where a similarly named free_frames variable means something different - it should include pending frames and therefore cannot be smaller than those.

Right @lyakh , so what I mean that if "sinks_free_frames == 3", the existing code witll truncate frames_to_copy so it will be limited to 3 as well (even if "source_avail_frames == 5").

So we are good to go?

@kv2019i well, yeah, we can commit this, but there's a bigger problem with mixin-mixout that I dug out while working on #6864 : #6864 (comment) Any ideas?

Oh that #6864 is interesting. But let's continue over there. I'd rather get this merged soon so we can enable assert and gets the "gates up" in our CI.