-
Notifications
You must be signed in to change notification settings - Fork 318
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
sink / source API #7622
sink / source API #7622
Conversation
cb65cf2
to
5dfb460
Compare
e3995ba
to
1f25e82
Compare
Still fighting with some compilations checks. Anyway - ready for review |
It means this PR contains significant changes. Have you considered spreading the commits in this PR across multiple PRs?
Less "big-bang" and more "continuous" integration. Not just for compilation checks but for daily tests too. |
Believe me, it IS split. More changes coming soon. |
1f25e82
to
b847986
Compare
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.
I think the API can and should be improved
b847986
to
37163f9
Compare
37163f9
to
60c615f
Compare
All stream parameters are now available as a single structure Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
As no internals of audio_stream nor comp_buffer should be accessible directly, move all calls to buffer_fmt to API In addition, to let sink/source interface working, move the parameter to audio_stream structure Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
50268a3
to
fb16c16
Compare
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.
LGTM for google files
only 2k bytes adding, looks great. |
#include <sof/audio/audio_stream.h> | ||
|
||
void source_init(struct source __sparse_cache *source, const struct source_ops *ops, | ||
struct sof_audio_stream_params *audio_stream_params) |
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.
maybe annotate audio_stream_params
with __sparse_cache
too instead of casting it out? Same for sink_init()
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.
no, it cannot be sparse. It is up to the implementation how to ensure coherency - This pointer may be uncached - if the queue is shared between cores like dp_queue
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.
The intent of this PR sounds good.
I am still going to request changes for
a) naming. using just 'struct sink' is madness, you need to prefix this API to avoid conflicts
b) locking is absolutely unclear and undocumented.
|
||
/** alignment limit of stream copy, this value indicates how many | ||
* integer frames which can meet both byte align and frame align | ||
* requirement. it should be set in component prepare or param functions |
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.
this comment is very very unclear. What does 'byte align' mean? What does 'frame align' mean?
I know the text is the same as before but I still can't figure out what the intent of this flag is.
Consider rewording or clarifying this field.
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.
This is NOT my comment, I just moved frame_align from buffer.h to audio_stream.h
I don't want to "fix" audio_stream in this PR, even if in comments
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.
@andrula-song can you please take the AR to fix those comments?
they are confusing and make little sense.
Adding @singalsu as reviewer :-)
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.
here is the link #7833
@@ -238,7 +237,8 @@ int create_endpoint_buffer(struct comp_dev *parent_dev, | |||
audio_stream_set_rate(&buffer_c->stream, copier_cfg->base.audio_fmt.sampling_frequency); | |||
audio_stream_set_frm_fmt(&buffer_c->stream, config->frame_fmt); | |||
audio_stream_set_valid_fmt(&buffer_c->stream, valid_fmt); | |||
buffer_c->buffer_fmt = copier_cfg->base.audio_fmt.interleaving_style; | |||
audio_stream_set_buffer_fmt(&buffer_c->stream, | |||
copier_cfg->base.audio_fmt.interleaving_style); |
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.
we're clearly confusing format and interleaving style....
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.
again - not my code, just API change
This PR is not intended to fix copier
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.
So, I beg to disagree. YOU are adding a new set_buffer_fmt L153 which only sets the interleaving style.
Naming matters and we'd miss an opportunity to clean-up the APIs and later their implementation.
this is a definition of API to source of audio data THE SOURCE is any component in the system that have data stored somehow and can give the data outside at request. The source API does not define who and how has produced the data The user - a module - sees this as a producer that PROVIDES data for processing The IMPLEMENTATION - audio_stream, DP Queue - sees this API as a destination it must send data to Examples of components that should expose source API: - DMIC. Data are coming from the outside world, stores in tmp buffer and can be presented to the rest of the system using source_api - a memory ring buffer Data are coming from other module (usually using sink_api, but it does not matter in fact) Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
this is a definition of API to sink of audio data THE SINK is any component that can store data somehow and provide a buffer to be filled with data at request. The sink API does not define how the data will be processed/used The user - a module - sees this API as a destination it must send data to The IMPLEMENTATION - audio_stream, DP Queue - sees this as a producer that PROVIDES data for processing Examples of components that should expose SINK api - /dev/null all the data stored in sink buffer are just simply discarded - I2S sender Data stored in sink buffer will be sent to the external world - a memory ring buffer data stored in the buffer will be sent to another module (usually using source API, but it does not matter in fact). Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
There are many operations on sink/source that may be put into a common library. This is the place for it. Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
Make audio_stream capable of using pipeline2.0 sink and source API This change makes integration of sink/src api possible in incremental way Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
Target is to make all modules use sink/source API as data source and destination. However, current implementation of module adapter allows 2 other completely different interfaces to be used: "simple_copy" modules receive output_stream_buffer and input_stream_buffer table. void * data pointers from both structures point to audio_stream other fields in the structures are in fact not needed but are used "! simple_copy" modules receive output_stream_buffer and input_stream_buffer table. void * data pointers from both structures point to raw linear data to make transition smooth and easy, both legacy ways have been kept, just to make the code more clear - put at separate module API calls, Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
Module prepare is an operation that needs to set up sink and source according to needs. Therefore it must have access to sink/source handlers This commit adds handlers to API. In case the module uses legacy audio stream sink/source pointers will be NULLs and number of sinks/sources will be zero Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
This is a porting of SRC module to use sink/src API To have src fully ported there's a need to get 100% independent of audio_stream and buffer.c This step is necessary because other implementations of sink/src, like DP Queue, may not be based on buffer implementation Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
fb16c16
to
b900d5e
Compare
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.
Some improvements since the last version, more suggested below.
I don't quite get the direction of travel with all these process_ functions and zero sinks/sources, the code ends-up quite confusing to the reader.
|
||
/** alignment limit of stream copy, this value indicates how many | ||
* integer frames which can meet both byte align and frame align | ||
* requirement. it should be set in component prepare or param functions |
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.
@andrula-song can you please take the AR to fix those comments?
they are confusing and make little sense.
Adding @singalsu as reviewer :-)
@@ -238,7 +237,8 @@ int create_endpoint_buffer(struct comp_dev *parent_dev, | |||
audio_stream_set_rate(&buffer_c->stream, copier_cfg->base.audio_fmt.sampling_frequency); | |||
audio_stream_set_frm_fmt(&buffer_c->stream, config->frame_fmt); | |||
audio_stream_set_valid_fmt(&buffer_c->stream, valid_fmt); | |||
buffer_c->buffer_fmt = copier_cfg->base.audio_fmt.interleaving_style; | |||
audio_stream_set_buffer_fmt(&buffer_c->stream, | |||
copier_cfg->base.audio_fmt.interleaving_style); |
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.
So, I beg to disagree. YOU are adding a new set_buffer_fmt L153 which only sets the interleaving style.
Naming matters and we'd miss an opportunity to clean-up the APIs and later their implementation.
|
||
/* set the default alignment info. | ||
* set byte_align as 1 means no alignment limit on byte. | ||
* set frame_align as 1 means no alignment limit on frame. |
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.
no idea what this means either.
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.
there is a limited calculation frame number function audio_stream_avail_frames_aligned, it use shift to replace division, the byte_align and frame_align is for shift index calculation.
struct input_stream_buffer *input_buffers, | ||
int num_input_buffers, | ||
struct output_stream_buffer *output_buffers, | ||
int num_output_buffers); |
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.
not fully understanding the concept of this PR, there was one process_ and now there are two process_, both deprecated. What's the direction then?
@@ -171,7 +171,7 @@ static int setup(void **state) | |||
mod->stream_params->channels = params->channels; | |||
mod->period_bytes = get_frame_bytes(params->source_format, params->channels) * 48000 / 1000; | |||
|
|||
ret = module_prepare(mod); | |||
ret = module_prepare(mod, NULL, 0, NULL, 0); |
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.
so an EQ has no sources and no sinks?
That's difficult to follow.... What is this supposed to represent?
Since I don't see any major issues left in the comments I am going to proceed with merge. @plbossart the PR is hanging till May so I would like to kindly ask you next time to raise questions about implementation earlier and not after the PR gets approved and is ready to move on. We have already very bad statistics on how long SOF PRs are waiting in review and last minute comments with questions are not helping to improve the situation. |
It doesn't make my comments go away. There were a couple of bad misses, such as not having a prefix for library functions which is borderline really, and blaming the reviewer is extremely bad form in open source. Edit: This sort of comments is also just wrong, I was one of the first ones to review this PR. Don't tell me I didn't help. |
@marcinszkudlinski could you try to address some of the comments in new, follow-up PRs? Maybe focusing on any backwards-incompatible changes before these new APIs get widely used? My 2 cents. |
{ | ||
source->ops = ops; | ||
source->requested_read_frag_size = 0; | ||
source->audio_stream_params = audio_stream_params; |
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.
@marcinszkudlinski in fact this is a bug. You call this function with an acquired object and you store a pointer to that acquired object permanently and then you use it outside of the object ownership scope. This will cause failures. Please fix in a follow up.
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.
In case of audio_stream handlers to sink/sources are __sparse
static inline struct sof_sink __sparse_cache *
audio_stream_get_sink(struct audio_stream __sparse_cache *audio_stream)
{
return &audio_stream->sink_api;
}
That implies that the handlers may be used when buffer is aquired, and only there
This is enforced by module_adapter and there's no danger here
The only concern is academic - we keep something that should not be used without lock.
At the moment we may think about shadowing audio_stream_params in sink/source structure as a temporary sollution (till comp_buffer/audio stream is still in use), this will cost some RAM.
Next implementation (dp_queue) will keep audio_stream_params in shared, never cached memory - no shadowing needed. Other upcoming implementations - like shared LL buffer - will be used exclusively by single core (so may be safely always cached - no shadowing needed).
My intention is not to ignore your comments, they can be addressed in a follow up PRs. The problem is |
BTW the Zephyr project has a lot of experience with "non-technical" issues in code reviews and more recently they formalized the review process a bit and wrote it down:
Don't get me wrong: I'm not saying all this process applies to SOF too. SOF is of course a different project with a different structure. However this is at the very least a very good example and most SOF developers rely on Zephyr so they must be familiar with Zephyr processes anyway. |
Due to a complex sequence of events described in fix #7846 (please review), this PR surfaced a latent @marcinszkudlinski which toolchain do you typically use and how come it did not report this? https://github.com/thesofproject/sof/actions/runs/5311578359/jobs/9614975368
|
This introduces sink and source API
in summary: audio_stream is bind to data buffer - it contains write/read pointers etc. Has no encapsulation, so anybody can access buffer directly
Sink/source provide an API to get to the data. It does not assume the data are in any buffer etc. Usage of API is enforced. API may do anything with the data - move in memory for linearization, take care of coherency, etc.
THE POINT IS: any module that use the API may get data from many sources and send to many sources, no matter if it is a circular buffer (audio_stream), or a coherent DP queue, shared LL buffer, DAI, data linearization layer - the transition from one source/sink to another is transparent and may be done in runtime
To make transition smooth, an implementation of sink/source has been added to audio_stream.
There's also a POC - src module, with some TODOs
Sink/source API is a part of pipeline 2.0 will be required for