2012/5/2 Tanu Kaskinen <tanuk at iki.fi>: > On Wed, 2012-05-02 at 06:08 +0300, Tanu Kaskinen wrote: >> On Wed, 2012-05-02 at 10:44 +0800, Wang Xingchao wrote: >> > Hi Tanu, >> > >> > 2012/4/25 Tanu Kaskinen <tanuk at iki.fi>: >> > > 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. >> > > >> > > 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 >> > [snip] >> > >> > > +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); >> > > + ? ?} >> > > + >> > >> > it's a bit different with original >> > patch(https://bugs.freedesktop.org/attachment.cgi?id=60461). :) ?i >> > guess most of time the leftover buffer length should be little, and >> > the main input buffer is bigger, so there're two times waste copy ?and >> > why not reduce that to one time copy? >> >> I don't understand what you mean. How would you achieve only one copy? >> >> If you mean that the leftover buffer should be copied to the input >> buffer, thus avoiding copying the input buffer, that doesn't work. The >> input buffer doesn't have any spare room for the leftover data. > > I now realized that copying the leftover data twice (once when storing > it and once in add_leftover()) could be reduced to only one copy by > using the same buffer in those two phases. The leftover size tends to be > small, so the amount of work that is saved is also small, but I think it > wouldn't make the code significantly more complex, so maybe I should > implement that. Sorry for confuse first. Not exactly. If there's left_over bytes, we could put them at the start of r->buf2, and change the "dst" of do_remap for next memchunk. Is that okay for you? --xingchao > > -- > Tanu >
