Hi,
Great insight, thanks! Just tested it on an affected system and it
works! The patch has just been checked in to SVN trunk changeset 3085.
Honestly, this ticket was a headache. So thanks again!
BR,
nanang
On Sat, Jan 30, 2010 at 5:16 AM, Bram Kuijvenhoven
<mailinglists.bram at kuijvenhoven.net> wrote:
> Hi,
>
> Attached is a proposed patch for bug #766 (Bad audio quality (possibly in
> resampling) in Linux).
>
> The cause is indeed in the resampling code
> (third_party/resample/src/resamplesubs.c):
>
> ? Yend = Ystart + (unsigned)(nx * pFactor);
>
> where pFactor is a double holding the result of 1.0 * rate_out / rate_in,
> and nx is the size of a source frame (in samples).
>
> Since pFactor holds only a binary floating-point approximation to the sample
> rate conversion factor (e.g. 1/6) and since the C-style cast (unsigned)
> performs truncation instead of rounding, the result is that sometimes Yend -
> Ystart is one sample less than intended, causing the last sample of the
> output frame to contain garbage (often a zero). Always be careful with
> floating-point calculations, especially in combination with looping
> constructs!
>
> The attached patch fixes it by turning the above into proper rounding:
>
> ? Yend = Ystart + (unsigned)(nx * pFactor + 0.5);
>
> Though the bug does not appear with my compiler (GCC 4.3.2 on Linux) when
> doing
>
> ? Yend = Ystart + (unsigned)(nx * 1.0 * dest_rate / source_rate);
>
> this may still cause problems on other platforms since the correct outcome
> probably only results from the extra precision available in floating point
> registers (80-bit) compared to doubles (64-bit). Therefore, the patch also
> makes similar adjustments to code in conference.c and resample_resample.c.
>
> Regards,
>
> Bram
>
> Index: pjmedia/src/pjmedia/resample_resample.c
> ===================================================================
> --- pjmedia/src/pjmedia/resample_resample.c ? ? (revision 3083)
> +++ pjmedia/src/pjmedia/resample_resample.c ? ? (working copy)
> @@ -123,7 +123,7 @@
>
> ? ? ? ?/* Allocate temporary output buffer */
> ? ? ? ?size = (unsigned) (resample->frame_size * sizeof(pj_int16_t) *
> - ? ? ? ? ? ? ? ? ? ? ? ? ?resample->factor / channel_count);
> + ? ? ? ? ? ? ? ? ? ? ? ? ?resample->factor / channel_count + 0.5);
> ? ? ? ?resample->tmp_buffer = (pj_int16_t*) pj_pool_alloc(pool, size);
> ? ? ? ?PJ_ASSERT_RETURN(resample->tmp_buffer, PJ_ENOMEM);
> ? ? }
> @@ -245,7 +245,7 @@
> ? ? ? ? ? ?unsigned mono_frm_sz_out;
>
> ? ? ? ? ? ?mono_frm_sz_in ?= resample->frame_size / resample->channel_cnt;
> - ? ? ? ? ? mono_frm_sz_out = (unsigned)(mono_frm_sz_in * resample->factor);
> + ? ? ? ? ? mono_frm_sz_out = (unsigned)(mono_frm_sz_in * resample->factor +
> 0.5);
>
> ? ? ? ? ? ?/* Deinterleave input */
> ? ? ? ? ? ?dst_buf = resample->in_buffer[i] + resample->xoff*2;
> Index: pjmedia/src/pjmedia/conference.c
> ===================================================================
> --- pjmedia/src/pjmedia/conference.c ? ?(revision 3083)
> +++ pjmedia/src/pjmedia/conference.c ? ?(working copy)
> @@ -372,7 +372,7 @@
> ? ? ? ?//conf_port->rx_buf_cap = (unsigned)(conf_port->samples_per_frame +
> ? ? ? ?// ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? conf->samples_per_frame *
> ? ? ? ?// ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? conf_port->clock_rate * 1.0 /
> - ? ? ? // ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? conf->clock_rate);
> + ? ? ? // ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? conf->clock_rate + 0.5);
> ? ? ? ?conf_port->rx_buf_cap = conf_port->clock_rate * buff_ptime / 1000;
> ? ? ? ?if (conf_port->channel_count > conf->channel_count)
> ? ? ? ? ? ?conf_port->rx_buf_cap *= conf_port->channel_count;
> @@ -1437,7 +1437,7 @@
> ? ? ? ? */
>
> ? ? ? ?samples_req = (unsigned) (count * 1.0 *
> - ? ? ? ? ? ? ? ? ? ? cport->clock_rate / conf->clock_rate);
> + ? ? ? ? ? ? ? ? ? ? cport->clock_rate / conf->clock_rate + 0.5);
>
> ? ? ? ?while (cport->rx_buf_count < samples_req) {
>
> @@ -1509,7 +1509,7 @@
> ? ? ? ? ? ?pjmedia_resample_run( cport->rx_resample,cport->rx_buf, frame);
>
> ? ? ? ? ? ?src_count = (unsigned)(count * 1.0 * cport->clock_rate /
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?conf->clock_rate);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?conf->clock_rate + 0.5);
> ? ? ? ? ? ?cport->rx_buf_count -= src_count;
> ? ? ? ? ? ?if (cport->rx_buf_count) {
> ? ? ? ? ? ? ? ?pjmedia_move_samples(cport->rx_buf, cport->rx_buf+src_count,
> @@ -1693,7 +1693,7 @@
> ? ? ? ?pjmedia_resample_run( cport->tx_resample, buf,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?cport->tx_buf + cport->tx_buf_count );
> ? ? ? ?dst_count = (unsigned)(conf->samples_per_frame * 1.0 *
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?cport->clock_rate / conf->clock_rate);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?cport->clock_rate / conf->clock_rate + 0.5);
> ? ? } else {
> ? ? ? ?/* Same clock rate.
> ? ? ? ? * Just copy the samples to tx_buffer.
> Index: third_party/resample/src/resamplesubs.c
> ===================================================================
> --- third_party/resample/src/resamplesubs.c ? ? (revision 3083)
> +++ third_party/resample/src/resamplesubs.c ? ? (working copy)
> @@ -118,9 +118,13 @@
> ? ? dtb = dt*(1<<Np) + 0.5; ? ? /* Fixed-point representation */
>
> ? ? Ystart = Y;
> - ? ?Yend = Ystart + (unsigned)(nx * pFactor);
> + ? ?Yend = Ystart + (unsigned)(nx * pFactor + 0.5);
> ? ? endTime = time + (1<<Np)*(RES_WORD)nx;
> - ? ?while (time < endTime)
> +
> + ? ?// Integer round down in dtb calculation may cause (endTime % dtb > 0),
> + ? ?// so it may cause resample write pass the output buffer (Y >= Yend).
> + ? ?// while (time < endTime)
> + ? ?while (Y < Yend)
> ? ? {
> ? ? ? ?iconst = (time) & Pmask;
> ? ? ? ?xp = &X[(time)>>Np]; ? ? ?/* Ptr to current input sample */
> @@ -257,7 +261,7 @@
> ? ? dtb = dt*(1<<Np) + 0.5; ? ? /* Fixed-point representation */
>
> ? ? Ystart = Y;
> - ? ?Yend = Ystart + (unsigned)(nx * pFactor);
> + ? ?Yend = Ystart + (unsigned)(nx * pFactor + 0.5);
> ? ? endTime = time + (1<<Np)*(RES_WORD)nx;
>
> ? ? // Integer round down in dtb calculation may cause (endTime % dtb > 0),
> @@ -305,7 +309,7 @@
> ? ? dhb = dh*(1<<Na) + 0.5; ? ? /* Fixed-point representation */
>
> ? ? Ystart = Y;
> - ? ?Yend = Ystart + (unsigned)(nx * pFactor);
> + ? ?Yend = Ystart + (unsigned)(nx * pFactor + 0.5);
> ? ? endTime = time + (1<<Np)*(RES_WORD)nx;
>
> ? ? // Integer round down in dtb calculation may cause (endTime % dtb > 0),
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip at lists.pjsip.org
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
>
