Skip to content

Conversation

@RanderWang
Copy link
Collaborator

It will use google rtc audio processing mock config

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.

@marcinszkudlinski pls review for IPC4

@RanderWang RanderWang requested a review from a team as a code owner January 12, 2023 01:11
@RanderWang
Copy link
Collaborator Author

RanderWang commented Jan 12, 2023

A simple data flow is like this, but this is not the final solution. SSP sends capture data to AEC as reference

Untitled

@lgirdwood
Copy link
Member

@perahgren @johnylin76 ping for review.

@lgirdwood
Copy link
Member

@cujomalainey any comments ?

@cujomalainey
Copy link
Contributor

Apologies, that Cavs memory bug has consumed my time this week, will resume reviews next week

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Some comments inline.

@RanderWang
Copy link
Collaborator Author

updated to fix buffer cache release issue in copier. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like generic info no? Is there not a generic struct to use here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a generic info for all AEC modules for SOF IPC4 version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then it kind of begs the question whether this should be in some more generic header...? Could be that we don't really have a good place for this sort of shared headers that are used by a subset of modules, but worth a check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cujomalainey sure, I get it. I will work on it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done! please check it. Thanks!

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @RanderWang !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then it kind of begs the question whether this should be in some more generic header...? Could be that we don't really have a good place for this sort of shared headers that are used by a subset of modules, but worth a check.

@ranj063
Copy link
Collaborator

ranj063 commented Feb 16, 2023

@RanderWang i dont know why you want to add more work for yourself. Please use the module adapter directly when adapting to ipc4

@RanderWang
Copy link
Collaborator Author

@RanderWang i dont know why you want to add more work for yourself. Please use the module adapter directly when adapting to ipc4

I get your idea, but I plan to do it step by step. I want to make it merged with simple change first, then add more change.

@gkbldcig
Copy link
Collaborator

Can one of the admins verify this patch?

@lgirdwood
Copy link
Member

SOFCI TEST

@cujomalainey
Copy link
Contributor

Can one of the admins verify this patch?

Any chance we can get this marked as a bot?

@lgirdwood
Copy link
Member

Can one of the admins verify this patch?

Any chance we can get this marked as a bot?

I think this may happen when CI loses context due to a reboot @wszypelt @greg-intel @keqiaozhang pls correct me if I'm wrong or provide some context why we see this. Thanks.

@lgirdwood
Copy link
Member

@RanderWang can you also check the CI result here. Thanks

@RanderWang
Copy link
Collaborator Author

RanderWang commented Feb 22, 2023

@RanderWang can you also check the CI result here. Thanks

@lgirdwood google RTC AEC is not enabled in default build so CI failure is not caused by this PR. Chrome team will has their overlay for chrome like ipc3 and enabled AEC in their overlay file

@cujomalainey
Copy link
Contributor

cujomalainey commented Feb 22, 2023

Can one of the admins verify this patch?

Any chance we can get this marked as a bot?

I think this may happen when CI loses context due to a reboot @wszypelt @greg-intel @keqiaozhang pls correct me if I'm wrong or provide some context why we see this. Thanks.

Yes but it should be registered as a bot so it can appear as one
image

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 22, 2023

https://github.com/gkbldcig does not really look "alive"

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 22, 2023

google RTC AEC is not enabled in default build so CI failure is not caused by this PR.

Always include the URL because it's lost after a force-push: https://sof-ci.01.org/sofpr/PR6927/build4076/devicetest/index.html

@lgirdwood
Copy link
Member

@RanderWang can you check again, somethings not right is the code is not used ?

@RanderWang
Copy link
Collaborator Author

RanderWang commented Feb 24, 2023

@RanderWang can you check again, somethings not right is the code is not used ?

@lgirdwood I am %100 confident that this patch can't result to CI failure since (1) we don't add AEC support in topology, so we can't test it (2) we don't enabled GOOGLE_RTC_AEC in config. According to Kai this config should be enabled by chrome overlay config. I also check the building log, no google files are compiled. And my patch only change google file and cmake file

@cujomalainey
Copy link
Contributor

https://github.com/gkbldcig does not really look "alive"

neither do some of the accounts that I get follows from e.g. https://github.com/Woodster-cloud

I still think things should be labeled what they are

Add ipc4 config support and hw_params setting

Signed-off-by: Rander Wang <rander.wang@intel.com>
Enable dummy AEC building support but still need to
enable google AEC in overlay file for chrome.

Signed-off-by: Rander Wang <rander.wang@intel.com>
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.

@cujomalainey good for you ?

@cujomalainey cujomalainey merged commit 8d5e3a8 into thesofproject:main Mar 2, 2023
@RanderWang RanderWang deleted the ipc4-google-aec branch May 8, 2023 07:11
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.

8 participants