-
Notifications
You must be signed in to change notification settings - Fork 314
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
buffer: Create audio stream buffer #2283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can we not reuse the existing buffer logic ? We should be able to instantiate it directly without IPC, although we may need some new API calls to help with this. I just don't want to see duplication here.
@lgirdwood I see that in currently we have tendency to duplications, see hb struct in sof/audio/kpb.h and as I far as I know there are similar issue currently under development. So I see need to split comp_buffer to part responsible directly for holding audio samples in circular buffer, base argument for data processing functions and universal brick for nesting when it is such a need. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, it looks like duplication because patch 2 duplicates existing APIs and patch 3 removes the old API. You need to merge the parts of patch 2 into 3 to show that there is no duplication, this makes it easier to follow.
@ktrzcinx one more question, will there be further updates to kpb to deduplicate code and fully use this update ? |
@lgirdwood do You want to just fully merge patch #2 i #3 ? |
Const keyword is added to explicitly indicate that nothing in given source buffer can be changed in processing functions - read pointers also. Signed-off-by: Karol Trzcinski <[email protected]>
To keep code clean and consistent create processing function interface outside comp_data function definition to omit duplicated definitions. Signed-off-by: Karol Trzcinski <[email protected]>
Const keyword is added to explicitly indicate that nothing in given input devices buffer can be changed in processing functions - - read pointers also. Signed-off-by: Karol Trzcinski <[email protected]>
Functions like src_get_copy_limits should have const marking to avoid error related with unintentional fields editing. Signed-off-by: Karol Trzcinski <[email protected]>
Const keyword is added to explicitly indicate that nothing in given input devices buffer can be changed in processing functions - - read pointers also. Signed-off-by: Karol Trzcinski <[email protected]>
To keep code clean and consistent create processing function interface outside comp_data function definition to omit duplicated definitions. Signed-off-by: Karol Trzcinski <[email protected]>
Const keyword is added to explicitly indicate that nothing in given input devices buffer can be changed in processing functions - - read pointers also. Signed-off-by: Karol Trzcinski <[email protected]>
Rebase required because of conflicts |
To keep code clean and consistent create processing function interface outside comp_data function definition to omit duplicated definitions. Signed-off-by: Karol Trzcinski <[email protected]>
Const keyword is added to explicitly indicate that nothing in given source buffer can be changed in processing functions - read pointers also. Signed-off-by: Karol Trzcinski <[email protected]>
This variable is unused in whole project. Signed-off-by: Karol Trzcinski <[email protected]>
Changed line was longer than 80 chars and refer to nested variable twice - after changes both problems disappears. Signed-off-by: Karol Trzcinski <[email protected]>
@zrombel could you comment on the scan. Thanks. |
Xtensa licese server issue. Rerunning build and scan. I've mode some changes to suppress those kind of issues. |
When new component will need to process audio data in two steps, then temporary circular buffer will be needed. Also extra information about data stream may be needed to choose right algorithm or assert data compatibility. Currently only 'comp_buffer' has such a possibility but it is component defined in topology - with much wider capabilities - and shouldn't be created inside component just to hold temporary data stream. Use introduced structure in processing functions, move api responsible for data processing from buffer to audio_stream module. Signed-off-by: Karol Trzcinski <[email protected]>
In source code there are places where are implemented pointer wrapping by hand, what looks quite complicated for a first eye catch and is messy because of pointer type casting. Using as_wrap is very well descriptive method and consist with DRY principle. as_get_frag has been changed to preprocessor function as it is just moving given pointer and wrapping result. Signed-off-by: Karol Trzcinski <[email protected]>
@xiulipan checkpatch is fine, but showing an error ? |
@lgirdwood If you scroll down, you will find some warning:
|
@xiulipan @lgirdwood checkpack results is located on github inside iframe with size restrictions: It's quite misleading for me too, maybe it's time to use full available size instead? |
@xiulipan that was not visible yesterday when I checked (and I did double check as I found it odd), it was all good ? Could there be a sync issue ? |
@ktrzcinx can you send a followup PR with the memcpy fixes as per check patch. |
@lgirdwood I didn't introduce memcpy functions in this place, it was before, but yes, I can fix it |
Memcpy functions seem to all be in unit tests? And the actual issue was old but only now discovered due to touching said code? |
@paulstelian97 Right |
@kakulesza @lrgirdwo @plbossart I think it would be some html issue. I wil try to and some fix to make the iframe scroll-able if the checkpatch result is very long. |
When new component will need to process audio data in two steps,
then temporary circular buffer will be needed. Also extra information
about data stream may be needed to choose right algorithm or assert data
compatibility.
Currently only 'comp_buffer' has such a possibility but it is component
defined in topology - with much wider capabilities - and shouldn't be
created inside component just to hold temporary data stream.
Signed-off-by: Karol Trzcinski [email protected]