[PATCH v2 3/3] resampler: Add support for resamplers that consume less data than asked.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux