[Sound-open-firmware] [PATCH 1/2] SRC: Bug fix for handling a deleted conversion
This patch fixes a regression that caused SRC to try to initialize for a mode that has been disabled from in/out rates matrix. The feature exist to save tables RAM with modes those are not required. The bug caused a divide by zero to happen in src_buffer_lengths() function. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src_core.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/audio/src_core.c b/src/audio/src_core.c index dc22772..d8b9a3d 100644 --- a/src/audio/src_core.c +++ b/src/audio/src_core.c @@ -133,7 +133,6 @@ int src_buffer_lengths(struct src_param *a, int fs_in, int fs_out, int nch, { struct src_stage *stage1; struct src_stage *stage2; - int k; int q; int den; int num; @@ -149,18 +148,25 @@ int src_buffer_lengths(struct src_param *a, int fs_in, int fs_out, int nch, a->idx_in = src_find_fs(src_in_fs, NUM_IN_FS, fs_in); a->idx_out = src_find_fs(src_out_fs, NUM_OUT_FS, fs_out); - /* Set blk_in, blk_out so that the muted fallback SRC keeps - * just source & sink in sync in pipeline without drift. - */ + /* Check that both in and out rates are supported */ if ((a->idx_in < 0) || (a->idx_out < 0)) { - k = gcd(fs_in, fs_out); - a->blk_in = fs_in / k; - a->blk_out = fs_out / k; + trace_src_error("us1"); + tracev_value(fs_in); + tracev_value(fs_out); return -EINVAL; } stage1 = src_table1[a->idx_out][a->idx_in]; stage2 = src_table2[a->idx_out][a->idx_in]; + + /* Check from stage1 parameter for a deleted in/out rate combination.*/ + if (stage1->filter_length < 1) { + trace_src_error("us2"); + tracev_value(fs_in); + tracev_value(fs_out); + return -EINVAL; + } + a->fir_s1 = nch * src_fir_delay_length(stage1); a->out_s1 = nch * src_out_delay_length(stage1); -- 2.11.0
This patch prevents SRC to stall due to too demanding criteria of function comp_buffer_can_copy_bytes. Since the input and output rate and period length of SRC are different for source and sink both buffers do not need to meet both criteria. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/audio/src.c b/src/audio/src.c index 3378065..0103e2e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -496,7 +496,6 @@ static int src_copy(struct comp_dev *dev) int need_sink; size_t consumed = 0; size_t produced = 0; - int res; trace_src("SRC"); @@ -519,30 +518,26 @@ static int src_copy(struct comp_dev *dev) /* make sure source component buffer has enough data available and that * the sink component buffer has enough free bytes for copy. Also * check for XRUNs */ - res = comp_buffer_can_copy_bytes(source, sink, need_source); - if (res) { + if (source->avail < need_source) { trace_src_error("xru"); return -EIO; /* xrun */ } - res = comp_buffer_can_copy_bytes(source, sink, need_sink); - if (res) { + if (sink->free < need_sink) { trace_src_error("xro"); return -EIO; /* xrun */ } - /* Run SRC function if buffers avail and free allow */ - if (((int) source->avail >= need_source) && ((int) sink->free >= need_sink)) { - cd->src_func(dev, source, sink, &consumed, &produced); + cd->src_func(dev, source, sink, &consumed, &produced); - /* Calc new free and available if data was processed. These - * functions must not be called with 0 consumed/produced. - */ - if (consumed > 0) - comp_update_buffer_consume(source, consumed); + /* Calc new free and available if data was processed. These + * functions must not be called with 0 consumed/produced. + */ + if (consumed > 0) + comp_update_buffer_consume(source, consumed); + + if (produced > 0) + comp_update_buffer_produce(sink, produced); - if (produced > 0) - comp_update_buffer_produce(sink, produced); - } return 0; } -- 2.11.0
On Mon, 2017-11-20 at 19:56 +0200, Seppo Ingalsuo wrote:
This patch prevents SRC to stall due to too demanding criteria of function comp_buffer_can_copy_bytes. Since the input and output rate and period length of SRC are different for source and sink both buffers do not need to meet both criteria.
These functions only check that there is enough avail bytes in source and free bytes in sink before we do any copy, what errors did you see ? Liam
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3378065..0103e2e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -496,7 +496,6 @@ static int src_copy(struct comp_dev *dev) int need_sink; size_t consumed = 0; size_t produced = 0; - int res;
trace_src("SRC");
@@ -519,30 +518,26 @@ static int src_copy(struct comp_dev *dev) /* make sure source component buffer has enough data available and that * the sink component buffer has enough free bytes for copy. Also * check for XRUNs */ - res = comp_buffer_can_copy_bytes(source, sink, need_source); - if (res) { + if (source->avail < need_source) { trace_src_error("xru"); return -EIO; /* xrun */ } - res = comp_buffer_can_copy_bytes(source, sink, need_sink); - if (res) { + if (sink->free < need_sink) { trace_src_error("xro"); return -EIO; /* xrun */ }
- /* Run SRC function if buffers avail and free allow */ - if (((int) source->avail >= need_source) && ((int) sink->free >= need_sink)) { - cd->src_func(dev, source, sink, &consumed, &produced); + cd->src_func(dev, source, sink, &consumed, &produced);
- /* Calc new free and available if data was processed. These - * functions must not be called with 0 consumed/produced. - */ - if (consumed > 0) - comp_update_buffer_consume(source, consumed); + /* Calc new free and available if data was processed. These + * functions must not be called with 0 consumed/produced. + */ + if (consumed > 0) + comp_update_buffer_consume(source, consumed); + + if (produced > 0) + comp_update_buffer_produce(sink, produced);
- if (produced > 0) - comp_update_buffer_produce(sink, produced); - } return 0; }
On 21.11.2017 02:59, Liam Girdwood wrote:
On Mon, 2017-11-20 at 19:56 +0200, Seppo Ingalsuo wrote:
This patch prevents SRC to stall due to too demanding criteria of function comp_buffer_can_copy_bytes. Since the input and output rate and period length of SRC are different for source and sink both buffers do not need to meet both criteria. These functions only check that there is enough avail bytes in source and free bytes in sink before we do any copy, what errors did you see ?
The stall happens in SRC test bench. The pipeline cannot be processed because SRC can't fill e.g. the sink and therefore cannot drain the source. The trace is flooded by xru or xro and nothing is processed. As example the number of free/avail bytes in source 2x 1ms buffer @ 48 kHz and sink 2x 1ms buffer @ 8 kHz should not be compared with same number of bytes. SRC resizes in params() the sink buffer and it becomes quite small with low sample rates. The risk of this to trigger is the higher the larger the ratio of rates is. Another way to fix this would to add to the function a parameter and have different thresholds for source and sink.
Liam
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com> --- src/audio/src.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/audio/src.c b/src/audio/src.c index 3378065..0103e2e 100644 --- a/src/audio/src.c +++ b/src/audio/src.c @@ -496,7 +496,6 @@ static int src_copy(struct comp_dev *dev) int need_sink; size_t consumed = 0; size_t produced = 0; - int res;
trace_src("SRC");
@@ -519,30 +518,26 @@ static int src_copy(struct comp_dev *dev) /* make sure source component buffer has enough data available and that * the sink component buffer has enough free bytes for copy. Also * check for XRUNs */ - res = comp_buffer_can_copy_bytes(source, sink, need_source); - if (res) { + if (source->avail < need_source) { trace_src_error("xru"); return -EIO; /* xrun */ } - res = comp_buffer_can_copy_bytes(source, sink, need_sink); - if (res) { + if (sink->free < need_sink) { trace_src_error("xro"); return -EIO; /* xrun */ }
- /* Run SRC function if buffers avail and free allow */ - if (((int) source->avail >= need_source) && ((int) sink->free >= need_sink)) { - cd->src_func(dev, source, sink, &consumed, &produced); + cd->src_func(dev, source, sink, &consumed, &produced);
- /* Calc new free and available if data was processed. These - * functions must not be called with 0 consumed/produced. - */ - if (consumed > 0) - comp_update_buffer_consume(source, consumed); + /* Calc new free and available if data was processed. These + * functions must not be called with 0 consumed/produced. + */ + if (consumed > 0) + comp_update_buffer_consume(source, consumed); + + if (produced > 0) + comp_update_buffer_produce(sink, produced);
- if (produced > 0) - comp_update_buffer_produce(sink, produced); - } return 0; }
_______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (2)
-
Liam Girdwood -
Seppo Ingalsuo