Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CTC topology #8840

Merged
merged 1 commit into from
Aug 19, 2024
Merged

CTC topology #8840

merged 1 commit into from
Aug 19, 2024

Conversation

eddy1021
Copy link

@eddy1021 eddy1021 commented Feb 6, 2024

Add topology for Google CTC component

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@eddy1021 I assume this PR will just squash into the main CTC PR ?

@lgirdwood
Copy link
Member

@eddy1021 any updates or ETA ? do you think this will land in v2.10 ?

@cujomalainey
Copy link
Member

@eddy1021 any updates or ETA ? do you think this will land in v2.10 ?

We are blocked on internal build problems, I wouldnt expect it to be resolved for at least another 2 weeks.

@eddy1021 eddy1021 changed the title [WIP] CTC topology CTC topology Aug 5, 2024
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

LGTM, but should get a review from @ranj063 and/or @singalsu

@eddy1021 eddy1021 force-pushed the ctc_topology branch 3 times, most recently from 468fc65 to 7a72198 Compare August 5, 2024 19:19
@eddy1021 eddy1021 marked this pull request as ready for review August 5, 2024 19:20
#
# Where N is the unique instance number for the parent object.

Class.Widget."ctc" {
Copy link
Member

Choose a reason for hiding this comment

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

@lgirdwood @ranj063 is there a way to set a min/max channel in widgets? I don't see anything in widget common. Might be useful for validation when pipelines define channels outside the range their components support.

Copy link
Member

Choose a reason for hiding this comment

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

Adding @singalsu too

Copy link
Collaborator

Choose a reason for hiding this comment

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

PCM has channels_min and channels_max but I don't know if it would here. In my experience when channels count can be other than default 2, it's better to list for the instances explicitly the in and out formats with possible channels counts (and all rates if non-48 kHz is possible). There's an arrays combiner than can help when there's many combinations.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense but is there a way to define that at the widget level.

E.g. dev A makes comp 1
Dev B makes pipeline with comp 1
Dev B fails to notice restrictions on formats

If format restrictions were part of the widget definition then it would be easy to identify if all widget formats were a superset of the pipelines set of supported formats. If they were a subset then the build would fail. Just a thought on finding bugs before runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cujomalainey audio formats are always part of individual widgets. A pipeline does not define a superset of audio formats at all. But its an interesting idea to identify discrepancies in audio formats across widgets within the same pipeline and fail during topology build. Let me give it a thought.

Add topology in development to test CTC.

Signed-off-by: Eddy Hsu <eddyhsu@google.com>
Copy link
Collaborator

@ranj063 ranj063 left a comment

Choose a reason for hiding this comment

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

Thanks for using the new feature @eddy1021. LGTM

@kv2019i
Copy link
Collaborator

kv2019i commented Aug 16, 2024

@wszypelt this now pends on the docker/topology-toolchain update (like #9354 ).

@cujomalainey
Copy link
Member

according to #9354 toolchain has been updated, merging this now

@cujomalainey cujomalainey merged commit 0e5ba1f into thesofproject:main Aug 19, 2024
45 of 47 checks passed
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.

6 participants