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

audio: codec_adapter: add Waves api #3977

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

stolx
Copy link
Contributor

@stolx stolx commented Mar 29, 2021

add Waves MaxxEffect API definition.
API is required to build SOF with Waves codec.

Signed-off-by: Oleksandr Strelchenko oleksandr.strelchenko@waves.com

@stolx
Copy link
Contributor Author

stolx commented Mar 29, 2021

compared to #3938 removed trail whitespace checkpatch errors

@cujomalainey cujomalainey self-requested a review March 29, 2021 22:39
@lgirdwood
Copy link
Member

SOFCI TEST

@@ -0,0 +1,133 @@
/* SPDX-License-Identifier: BSD-3-Clause
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be under sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect.

Copy link
Contributor

@cujomalainey cujomalainey Apr 2, 2021

Choose a reason for hiding this comment

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

I wonder if maybe we should do a codec_adapter/codecs/ and in the kconfig just include a kconfig in the respective dir so all include info can be pushed down to individual kconfigs/cmakelists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cujomalainey
I think this is a good idea but I prefer it to be handled in a separate PR.

Another thing I thought about is to upstream pass-through code for Waves codec.
It will come in handy to add SOF with Waves codec to CI checks.
Where is a place to discuss if this is needed?

@dbaluta
Copy link
Collaborator

dbaluta commented Mar 31, 2021

If the headers as provided "as it is" we shouldn't fix checkpatch errors. Just let them be. Otherwise, updating the headers to a new version will be painful.

@lgirdwood
Copy link
Member

If the headers as provided "as it is" we shouldn't fix checkpatch errors. Just let them be. Otherwise, updating the headers to a new version will be painful.

Ack, this was my opinion too - we dont want to change 3rd party headers as it will have unnecessary impact on 3rd party non upstream code - we upstream them "as-is" but with permissive license.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 31, 2021

Github Actions hang fixed by #3992, please git commit --amend and force push to re-trigger.

@lgirdwood
Copy link
Member

@stolx same rebase with main branch latest is needed here too.

@stolx stolx requested a review from mrajwa as a code owner April 5, 2021 16:04
@@ -13,4 +13,5 @@ endif()
if(CONFIG_WAVES_CODEC)
add_local_sources(sof codec/waves.c)
sof_add_static_library(MaxxChrome ${CMAKE_CURRENT_LIST_DIR}/lib/release/libMaxxChrome.a)
target_include_directories(sof PUBLIC ${CMAKE_CURRENT_LIST_DIR}/../../include/sof/audio/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@stolx thanks for moving the header file location. It looks good now.

I have one question though. Do you really need to add the line above? I would have expected to just work. If it is really needed that fine by me. The change looks good and useful!

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, if your stuff is in include you shouldnt need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed include dir to be ${CMAKE_CURRENT_LIST_DIR}/../../include/sof/audio/codec_adapter/codec/
we need it to keep MaxxEffect folder on includes path

Copy link
Collaborator

@dbaluta dbaluta Apr 6, 2021

Choose a reason for hiding this comment

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

@stolx OK, I got your point. It works for me. Another approach would have been to use

#include "codec_adapter/codec/MaxxEffect/MaxxEffect.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately, it will not work due to how MaxxEffect API is designed.
For example, take a look at MaxxEffect_Revision.h, it contains the following lines
#include "MaxxEffect/MaxxEffect.h" #include "MaxxEffect/MaxxStatus.h"
So API headers themself expect MaxxEffect folder to be on the include path.
And since API headers are provided "as-is", the only way is to add the MaxxEffect folder to includes

Copy link
Collaborator

Choose a reason for hiding this comment

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

@stolx thanks for your explanation! It makes sense.

add Waves MaxxEffect API definition.
API is required to build SOF with Waves codec.

Signed-off-by: Oleksandr Strelchenko <oleksandr.strelchenko@waves.com>
@dbaluta
Copy link
Collaborator

dbaluta commented Apr 6, 2021

@lgirdwood this is read to merge from my point of view.

@lgirdwood
Copy link
Member

@zrombel good to merge , looks like CI pending ?
@stolx can you try and build docs ./scripts/gen-doc.sh as it looks like doxygen is failing to build one of your files.

@dbaluta
Copy link
Collaborator

dbaluta commented Apr 6, 2021

@lgirdwood I don't think we should fix the header to pass the doxygen build just because these are 3rd party headers and we need to have them in the tree without any modification.

I think the best way is to instruct doxygen to skip these files.

@lgirdwood
Copy link
Member

@lgirdwood I don't think we should fix the header to pass the doxygen build just because these are 3rd party headers and we need to have them in the tree without any modification.

I think the best way is to instruct doxygen to skip these files.

This could be a valid doxygen issue that also impacts the private repo. i.e. a typo
I dont seen any harm here since this should be all standard doxygen, but I'd like to see the errors being reported first.

@dbaluta
Copy link
Collaborator

dbaluta commented Apr 6, 2021

@lgirdwood you are right. we should at least figure out the cause of the errors.

@cujomalainey
Copy link
Contributor

@lgirdwood you are right. we should at least figure out the cause of the errors.

I think we should just add an exclusion case https://stackoverflow.com/questions/1354056/ignoring-files-in-project-folder-for-doxygen

@lgirdwood
Copy link
Member

CI failure is unrelated kernel suspend and resume

@lgirdwood lgirdwood merged commit d9a7dfd into thesofproject:main Apr 7, 2021
@lgirdwood
Copy link
Member

/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxEffect.h:47: warning: unable to resolve reference to 'CoreConcepts_MaxxEffect' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:60: warning: unable to resolve reference to 'CoreConcepts_DataPath_Buffer' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:83: warning: unable to resolve reference to 'CoreConcepts_DataPath_MultichannelStreams' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:109: warning: unable to resolve reference to 'CoreConcepts_DataPath_Stream' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:95: warning: unable to resolve reference to 'CoreConcepts_DataPath_StreamFormat' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxEffect.h:47: warning: unable to resolve reference to 'CoreConcepts_MaxxEffect' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:83: warning: unable to resolve reference to 'CoreConcepts_DataPath_MultichannelStreams' for \ref command
/home/lrg/work/sof/sof/src/include/sof/audio/codec_adapter/codec/MaxxEffect/MaxxStream.h:60: warning: unable to resolve reference to 'CoreConcepts_DataPath_Buffer' for \ref command

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 9, 2021

The doxygen build is now broken for every PR.

@dbaluta
Copy link
Collaborator

dbaluta commented Apr 9, 2021

@marc-hb should be fixed with updated version of 55cf12b

@stolx stolx deleted the add-waves-api branch April 26, 2021 14:50
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.

5 participants