Skip to content

Conversation

@RanderWang
Copy link

@RanderWang RanderWang commented Mar 13, 2023

When walking the list of connected widgets, the pipeline_params for each
widget is modified by the source widget in the path in prepare case. The
first source widget is a AIF type for playback and DAI type for capture
case. In AEC case a feedback buffer is shared between two streams, one
prepared DAI widget in the stream for producing feedback buffer is included
in widget list for AEC stream and used for source widget. This will result
to incorrect pipeline_params.

This patch skip prepared widget in anther stream and use the widget in
the current stream for pipeline_param prepare.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang I'm not following the issue here. Even if you didn't have this check here, if the widget were already prepared and so were all it's sink widgets, the function prepare _widgets_in_path() would skip this anyway, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 yes, it is a little tricky. Let me explain it. For DMIC stream only support S32_LE and aec module use S16_LE format, we will use copier module to convert S32_LE to S16_LE. Please check the picture. If the stream is prepared from copier.SSP.10.1, this widget is prepared so it will be skipped and start to prepare google-rtc-aec.18.1. At this time pipeline params use S32_LE but google-rtc-aec.18.1 only support S16_LE, so we will get error "sof_ipc4_init_audio_fmt: Unsupported audio format: 48000Hz, 32bit, 2 channels". But if the stream is prepared from copier.DMIC.14.1, the copier.module.19.1 will change pipeline params to use S16_LE since we define the conversion in copier config, so the next prepared google-rtc-aec.18.1 will match this format. The problem is that we can't start from another pipeline since we may change pipeline params in current stream.

example

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang I don't understand your explanations. what does the copier.SSP.10.1 provide to the google-rtc-aec.18.1 module? The echo reference? If yes, why can't we demote the output of the copier.SSP.10.1 to match what the AEC needs?

I also don't understand the notion of 'prepared' from different sources that may or may not all be available. what happens if we start/stop the playback, there's no way this sort of topology can adjust.

I also don't think this integration is quite right, as I mentioned in another PR you want the output of the AEC to go to the host AND to the noise suppression.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang yes, it looks like you've got the wrong audio formats set for output pin 1 for copier.SSP.10.1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang I don't understand your explanations. what does the copier.SSP.10.1 provide to the google-rtc-aec.18.1 module? The echo reference? If yes, why can't we demote the output of the copier.SSP.10.1 to match what the AEC needs?

SSP.10.1 is used for reference. The Copier.SSP.10.1 is prepared in another stream so it will be skipped by driver, so we use fe_params which uses S32_LE

I also don't understand the notion of 'prepared' from different sources that may or may not all be available. what happens if we start/stop the playback, there's no way this sort of topology can adjust.

Currently we build module params from source (DAI or HOST) and change the params in some modules like SRC or Copier and propagate params to sink module. We can't use incorrect source for prepare. Playback also get such issue if the host copier linked to another stream directly.

I also don't think this integration is quite right, as I mentioned in another PR you want the output of the AEC to go to the host AND to the noise suppression.

please check thesofproject/sof#7237 (comment). And It is also defined in chrome document that AEC is linked to noise suppression. We discussed in my WSR001 2023

PCM99C <----------RTNR ----------< GOOGLE_AEC <---------------DMIC01

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I am not going to move until we agree on the topology. You added 2 instances of AEC in thesofproject/sof#7237 and I don't see how this can fly.

@RanderWang RanderWang closed this Mar 13, 2023
@RanderWang RanderWang reopened this Mar 14, 2023
When walking the list of connected widgets, the pipeline_params for each
widget is modified by the source widget in the path in prepare case. The
first source widget is a AIF type for playback and DAI type for capture
case. In AEC case a feedback buffer is shared between two streams, one
prepared DAI widget in the stream for producing feedback buffer is included
in widget list for AEC stream and used for source widget. This will result
to incorrect pipeline_params.

This patch skip prepared widget in anther stream and use the widget in
the current stream for pipeline_param prepare.

Signed-off-by: Rander Wang <[email protected]>
@RanderWang RanderWang force-pushed the ipc4_prepare_refine branch from 6061b96 to ce148c8 Compare March 14, 2023 08:46
@aiChaoSONG
Copy link

SOFCI TEST

@thesofproject thesofproject deleted a comment from keqiaozhang Mar 14, 2023
@RanderWang RanderWang closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants