audio: codec_adapter: add Waves MaxxEffect API#3938
audio: codec_adapter: add Waves MaxxEffect API#3938stolx wants to merge 1 commit intothesofproject:masterfrom
Conversation
|
Hi, @cujomalainey @lgirdwood |
lgirdwood
left a comment
There was a problem hiding this comment.
Looks all fine, just minor stuff really.
There was a problem hiding this comment.
All these headers should be under src/include/sof/audio/
There was a problem hiding this comment.
We als need a SPDX licence at the top of each file too.
There was a problem hiding this comment.
@lgirdwood regarding files placement
our API uses relative include paths like #include "MaxxEffect/MaxxEffect.h"
this requires that a folder with API is on includes path
by default src/include/sof/audio/ is not on path
will it be ok if I add it on path? something like this:
--- a/src/audio/codec_adapter/CMakeLists.txt
+++ b/src/audio/codec_adapter/CMakeLists.txt
@@ -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/)
endif()
There was a problem hiding this comment.
@lgirdwood moved files and added SPDX licence identifier
There was a problem hiding this comment.
Yep, adding this to Makefile is good.
There was a problem hiding this comment.
Will need to include <stdint.h> for every file that uses the standard int types.
There was a problem hiding this comment.
What if <stdint.h> is included in a header that is included in another file?
Even then?
There was a problem hiding this comment.
@stolx Yes, you should include it even there. Do not ever rely on indirect inclusions, because they may change and cause headaches for future work.
There was a problem hiding this comment.
okay, got it, thanks.
There was a problem hiding this comment.
SOF uses doxygen for code documentation, this looks like one of the doxygen supported formats, can you confirm ?
There was a problem hiding this comment.
yes, this is a doxygen annotation
add Waves MaxxEffect API definition. API is required to build SOF with Waves codec. Signed-off-by: Oleksandr Strelchenko <oleksandr.strelchenko@waves.com>
|
this looks good to me. This is much appreciated. One request, please associate your @waves.com email with your github account so we can ping people easily if bugs arise and we are looking up authors. |
|
sure, done |
|
https://github.com/thesofproject/sof/pull/3938/checks?check_run_id=2159191749 has @fredoh9 https://github.com/thesofproject/sof/pull/3938/checks?check_run_id=2159191993 has more QEMU or Docker errors and cancellations like in #3941 |
|
SOFCI TEST |
|
Retry CI, seems to be some unrelated issues being reported too. |
|
@stolx can you fix the doxygen CI issues, the others look unrelated and could be a Docker quota limit on GH actions or Travis. |
|
GH actions seem to be working again. Lets rerun CI. |
|
SOFCI TEST |
| 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/) |
There was a problem hiding this comment.
this is implied by default isnt it?
|
@stolx You may need to resubmit this pull request, due to change of branch name from "master" to "main". |
add Waves MaxxEffect API definition.
API is required to build SOF with Waves codec.
Signed-off-by: Oleksandr Strelchenko oleksandr.strelchenko@waves.com