On 04/25/2012 05:35 PM, Tanu Kaskinen wrote: > libsamplerate_resample() assumed that src_process() would > always consume the whole input buffer. That was an invalid > assumption leading to crashes. > > This patch adds a leftover memchunk for storing any > non-consumed input. When pa_resampler_run() is called next > time, the leftover is prepended to the new input. Thanks for the patches. I've not gone into great detail about the below, but I trust your expertise. :-) Two nitpicks below. The other two patches look good. > > Changes in v2: > - If add_leftover() is called with zero-length input while > the leftover length is non-zero, we don't try to acquire > the input memblock. > - Instead of taking a reference to the original input in > libsamplerate_resample(), we copy the leftover data to a > new memblock. This is done, because otherwise, if the > input is one of the internal buffers, the data can get > overwritten before reading it in add_leftover(). > - Store add_leftover_buf size in bytes instead of samples > (more convenient, but less consistent with other code). > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=47156 > --- > src/pulsecore/resampler.c | 94 +++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 87 insertions(+), 7 deletions(-) > > diff --git a/src/pulsecore/resampler.c b/src/pulsecore/resampler.c > index 258d1fe..ef014f8 100644 > --- a/src/pulsecore/resampler.c > +++ b/src/pulsecore/resampler.c > @@ -58,12 +58,16 @@ struct pa_resampler { > > pa_memchunk to_work_format_buf; > pa_memchunk remap_buf; > + pa_memchunk add_leftover_buf; /* For concatenating leftover with input. */ > pa_memchunk resample_buf; > pa_memchunk from_work_format_buf; > + pa_memchunk leftover_buf; /* For storing the leftover data. */ > unsigned to_work_format_buf_samples; > unsigned remap_buf_samples; > + unsigned add_leftover_buf_bytes; > unsigned resample_buf_samples; > unsigned from_work_format_buf_samples; > + unsigned leftover_buf_bytes; > > pa_sample_format_t work_format; > > @@ -344,10 +348,14 @@ void pa_resampler_free(pa_resampler *r) { > pa_memblock_unref(r->to_work_format_buf.memblock); > if (r->remap_buf.memblock) > pa_memblock_unref(r->remap_buf.memblock); > + if (r->add_leftover_buf.memblock) > + pa_memblock_unref(r->add_leftover_buf.memblock); > if (r->resample_buf.memblock) > pa_memblock_unref(r->resample_buf.memblock); > if (r->from_work_format_buf.memblock) > pa_memblock_unref(r->from_work_format_buf.memblock); > + if (r->leftover_buf.memblock) > + pa_memblock_unref(r->leftover_buf.memblock); > > pa_xfree(r); > } > @@ -379,17 +387,24 @@ void pa_resampler_set_output_rate(pa_resampler *r, uint32_t rate) { > size_t pa_resampler_request(pa_resampler *r, size_t out_length) { > pa_assert(r); > > - /* Let's round up here */ > - > + /* Let's round up here to make it more likely that the caller will get at > + * least out_length amount of data from pa_resampler_run(). > + * > + * We don't take the leftover into account here. If we did, then it might > + * be in theory possible that this function would return 0 and > + * pa_resampler_run() would also return 0. That could lead to infinite > + * loops. When the leftover is ignored here, such loops would eventually > + * terminate, because the leftover would grow each round, finally > + * surpassing the minimum input threshold of the resampler. */ > return (((((out_length + r->o_fz-1) / r->o_fz) * r->i_ss.rate) + r->o_ss.rate-1) / r->o_ss.rate) * r->i_fz; > } > > size_t pa_resampler_result(pa_resampler *r, size_t in_length) { > pa_assert(r); > > - /* Let's round up here */ > - > - return (((((in_length + r->i_fz-1) / r->i_fz) * r->o_ss.rate) + r->i_ss.rate-1) / r->i_ss.rate) * r->o_fz; > + /* Let's round up here to ensure that the caller will always allocate big > + * enough output buffer. */ > + return (((((in_length + r->i_fz-1) / r->i_fz + r->leftover_buf.length / r->o_fz) * r->o_ss.rate) + r->i_ss.rate-1) / r->i_ss.rate) * r->o_fz; A temporary variable or two here would increase readability and ease debugging. > } > > size_t pa_resampler_max_block_size(pa_resampler *r) { > @@ -413,7 +428,7 @@ size_t pa_resampler_max_block_size(pa_resampler *r) { > > fs = pa_frame_size(&ss); > > - return (((block_size_max/fs - EXTRA_FRAMES)*r->i_ss.rate)/ss.rate)*r->i_fz; > + return (((block_size_max / fs - EXTRA_FRAMES - r->leftover_buf.length / r->o_fz) * r->i_ss.rate) / ss.rate) * r->i_fz; A temporary variable or two here would increase readability and ease debugging. > } > > void pa_resampler_reset(pa_resampler *r) { > @@ -421,6 +436,11 @@ void pa_resampler_reset(pa_resampler *r) { > > if (r->impl_reset) > r->impl_reset(r); > + > + if (r->leftover_buf.memblock) { > + pa_memblock_unref(r->leftover_buf.memblock); > + r->leftover_buf.length = 0; > + } > } > > pa_resample_method_t pa_resampler_get_method(pa_resampler *r) { > @@ -1143,6 +1163,48 @@ static pa_memchunk *remap_channels(pa_resampler *r, pa_memchunk *input) { > return&r->remap_buf; > } > > +static pa_memchunk *add_leftover(pa_resampler *r, pa_memchunk *input) { > + void *src; > + void *dst; > + > + pa_assert(r); > + pa_assert(input); > + > + /* Concatenate leftover and input and place the result in > + * add_leftover_buf. */ > + > + if (r->leftover_buf.length<= 0) > + return input; > + > + r->add_leftover_buf.length = r->leftover_buf.length + input->length; > + > + if (!r->add_leftover_buf.memblock || r->add_leftover_buf_bytes< r->add_leftover_buf.length) { > + if (r->add_leftover_buf.memblock) > + pa_memblock_unref(r->add_leftover_buf.memblock); > + > + r->add_leftover_buf_bytes = r->add_leftover_buf.length; > + r->add_leftover_buf.memblock = pa_memblock_new(r->mempool, r->add_leftover_buf.length); > + } > + > + src = (uint8_t *) pa_memblock_acquire(r->leftover_buf.memblock); > + dst = pa_memblock_acquire(r->add_leftover_buf.memblock); > + memcpy(dst, src, r->leftover_buf.length); > + pa_memblock_release(r->leftover_buf.memblock); > + > + if (input->length> 0) { > + src = (uint8_t *) pa_memblock_acquire(input->memblock) + input->index; > + dst = (uint8_t *) dst + r->leftover_buf.length; > + memcpy(dst, src, input->length); > + pa_memblock_release(input->memblock); > + } > + > + pa_memblock_release(r->add_leftover_buf.memblock); > + > + r->leftover_buf.length = 0; > + > + return&r->add_leftover_buf; > +} > + > static pa_memchunk *resample(pa_resampler *r, pa_memchunk *input) { > unsigned in_n_frames, in_n_samples; > unsigned out_n_frames, out_n_samples; > @@ -1229,6 +1291,7 @@ void pa_resampler_run(pa_resampler *r, const pa_memchunk *in, pa_memchunk *out) > buf = (pa_memchunk*) in; > buf = convert_to_work_format(r, buf); > buf = remap_channels(r, buf); > + buf = add_leftover(r, buf); > buf = resample(r, buf); > > if (buf->length) { > @@ -1266,7 +1329,24 @@ static void libsamplerate_resample(pa_resampler *r, const pa_memchunk *input, un > data.end_of_input = 0; > > pa_assert_se(src_process(r->src.state,&data) == 0); > - pa_assert((unsigned) data.input_frames_used == in_n_frames); > + > + if (data.input_frames_used< in_n_frames) { > + void *dst; > + > + r->leftover_buf.length = (in_n_frames - data.input_frames_used) * r->w_sz * r->o_ss.channels; > + > + if (!r->leftover_buf.memblock || r->leftover_buf_bytes< r->leftover_buf.length) { > + if (r->leftover_buf.memblock) > + pa_memblock_unref(r->leftover_buf.memblock); > + > + r->leftover_buf_bytes = r->leftover_buf.length; > + r->leftover_buf.memblock = pa_memblock_new(r->mempool, r->leftover_buf.length); > + } > + > + dst = pa_memblock_acquire(r->leftover_buf.memblock); > + memcpy(dst, data.data_in + data.input_frames_used * r->o_ss.channels, r->leftover_buf.length); > + pa_memblock_release(r->leftover_buf.memblock); > + } > > pa_memblock_release(input->memblock); > pa_memblock_release(output->memblock); -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
