mux: add separated .params() for demux to fix noise issue #2653
Conversation
keyonjie
requested review from
bardliao,
bkokoszx,
juimonen,
lgirdwood,
slawblauciak and
xiulipan
|
This is verified to fix the noise happened in DSM+Echo Ref scenario. |
src/audio/mux/mux.c
Outdated
There was a problem hiding this comment.
@keyonjie the change looks correct but to avoid duplication of code, can we re-use the same mux_params and check for dev->drv->type and set the buffer to sourcebuffer or sink_buffer accordingly?
There was a problem hiding this comment.
@ranj063 we can for sure making change like that, but that is actually another kind of waste, we already know it is mux or demux, why we need to check it again?
There was a problem hiding this comment.
Can we have an inline comment here describing what we are doing with channels and frame format here.
There was a problem hiding this comment.
@keyonjie what I was suggesting to avoid unnecessary code duplication. It is true that we already know it's a mux or a demux, then we can also argue to separate these into 2 files, no? This change really is about whether we're getting the params from the source or the sink buffer. So adding an extra line to add the check is a lot less wasteful that duplicating the entire params function
There was a problem hiding this comment.
@keyonjie what I was suggesting to avoid unnecessary code duplication. It is true that we already know it's a mux or a demux, then we can also argue to separate these into 2 files, no? This change really is about whether we're getting the params from the source or the sink buffer. So adding an extra line to add the check is a lot less wasteful that duplicating the entire params function
Fair enough, let me change it, thanks.
There was a problem hiding this comment.
Can we have an inline comment here describing what we are doing with channels and frame format here.
I had difficulty to understand it at first glance also, let me add some comment here.
src/audio/mux/mux.c
Outdated
There was a problem hiding this comment.
Can we have an inline comment here describing what we are doing with channels and frame format here.
@akloniex I think so if the mux/demux doesn't care the num_channels anymore, but as your PR is doing refinement and ABI bump required, so it's better to fix issues exposed inside the old ABI first, that will help on releases delivered with the old version mux/demux component, agree? |
The sof_mux_config.num_channels is designed to denote the channel number of the "1" branch of "1->N" or "N->1", that is the source of demux (1->N) and the sink of mux (N->1), add handle of the demux type to fix the noise issue root caused to be the wrong num_channels in the demux scenarios that input_channels != output_channels. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
@keyonjie ABI bump is in the last commit in my PR, and it really only is there, because I wanted to leave the interface clean. In fact, ABI, binary interface, is completely unaffected. |
|
@akloniex the review process with ABI bumping related PR might take quite long time and the issue keep staying there and blocking releases, if you can send separated PR to fix it and get it merged soon, it will be really appreciated. By the way, during the smart amplifier implementation, we found there are issues that the frame_fmt among buffers connected to the mux/demux are not aligned sometimes, please pay attention and figure out solution for it also. |
|
This will be handled by #2788, closing it. |
4 participants
The sof_mux_config.num_channels is designed to denote the channel number
of the "1" branch of "1->N" or "N->1", that is the source of demux
(1->N) and the sink of mux (N->1), add an separated .params() for demux
to fix the noise issue root caused to be the wrong num_channels in the
demux scenarios that input_channels != output_channels.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com