| Message ID | 20240227094810.1182-2-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] swresample/resample: fix rounding errors with filter_size=1 and phase_shift=0 | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libswresample/resample.c | 29 +++++++---------------------- > libswresample/resample.h | 4 ++-- > libswresample/resample_template.c | 14 ++++++++++++-- > 3 files changed, 21 insertions(+), 26 deletions(-) what effect does this have on speed ? thx [...]
On Tue, 27 Feb 2024, Michael Niedermayer wrote: > On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libswresample/resample.c | 29 +++++++---------------------- >> libswresample/resample.h | 4 ++-- >> libswresample/resample_template.c | 14 ++++++++++++-- >> 3 files changed, 21 insertions(+), 26 deletions(-) > > what effect does this have on speed ? For the following command line time ./ffprobe -f lavfi \ "sine=440:r=8000:d=86400:samples_per_frame=2048,aresample=24000:filter_size=1:phase_shift=0" \ -show_packets >/dev/null Before the patch: real 0m3,916s user 0m3,812s sys 0m0,104s After the patch: real 0m3,597s user 0m3,457s sys 0m0,140s So it actually speed things up. Regards, Marton
On Tue, Feb 27, 2024 at 09:50:49PM +0100, Marton Balint wrote: > > > On Tue, 27 Feb 2024, Michael Niedermayer wrote: > > > On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > > --- > > > libswresample/resample.c | 29 +++++++---------------------- > > > libswresample/resample.h | 4 ++-- > > > libswresample/resample_template.c | 14 ++++++++++++-- > > > 3 files changed, 21 insertions(+), 26 deletions(-) > > > > what effect does this have on speed ? > > For the following command line > > time ./ffprobe -f lavfi \ > "sine=440:r=8000:d=86400:samples_per_frame=2048,aresample=24000:filter_size=1:phase_shift=0" \ > -show_packets >/dev/null > > Before the patch: > > real 0m3,916s > user 0m3,812s > sys 0m0,104s > > After the patch: > > real 0m3,597s > user 0m3,457s > sys 0m0,140s > > So it actually speed things up. is resample_one used in both cases ? thx [...]
On Wed, 28 Feb 2024, Michael Niedermayer wrote: > On Tue, Feb 27, 2024 at 09:50:49PM +0100, Marton Balint wrote: >> >> >> On Tue, 27 Feb 2024, Michael Niedermayer wrote: >> >>> On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: >>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>> --- >>>> libswresample/resample.c | 29 +++++++---------------------- >>>> libswresample/resample.h | 4 ++-- >>>> libswresample/resample_template.c | 14 ++++++++++++-- >>>> 3 files changed, 21 insertions(+), 26 deletions(-) >>> >>> what effect does this have on speed ? >> >> For the following command line >> >> time ./ffprobe -f lavfi \ >> "sine=440:r=8000:d=86400:samples_per_frame=2048,aresample=24000:filter_size=1:phase_shift=0" \ >> -show_packets >/dev/null >> >> Before the patch: >> >> real 0m3,916s >> user 0m3,812s >> sys 0m0,104s >> >> After the patch: >> >> real 0m3,597s >> user 0m3,457s >> sys 0m0,140s >> >> So it actually speed things up. > > is resample_one used in both cases ? Sure. The patch does not change the conditions when resample_one is used. Regards, Marton
On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libswresample/resample.c | 29 +++++++---------------------- > libswresample/resample.h | 4 ++-- > libswresample/resample_template.c | 14 ++++++++++++-- > 3 files changed, 21 insertions(+), 26 deletions(-) > > diff --git a/libswresample/resample.c b/libswresample/resample.c > index 17cebad01b..89859dec79 100644 > --- a/libswresample/resample.c > +++ b/libswresample/resample.c > @@ -356,26 +356,7 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A > > *consumed = 0; > > - if (c->filter_length == 1 && c->phase_count == 1) { > - int64_t index2= (1LL<<32)*c->frac/c->src_incr + (1LL<<32)*c->index + 1; > - int64_t incr= (1LL<<32) * c->dst_incr / c->src_incr + 1; > - int new_size = (src_size * (int64_t)c->src_incr - c->frac + c->dst_incr - 1) / c->dst_incr; > - > - dst_size = FFMAX(FFMIN(dst_size, new_size), 0); > - if (dst_size > 0) { > - for (i = 0; i < dst->ch_count; i++) { > - c->dsp.resample_one(dst->ch[i], src->ch[i], dst_size, index2, incr); > - if (i+1 == dst->ch_count) { > - c->index += dst_size * c->dst_incr_div; > - c->index += (c->frac + dst_size * (int64_t)c->dst_incr_mod) / c->src_incr; > - av_assert2(c->index >= 0); > - *consumed = c->index; > - c->frac = (c->frac + dst_size * (int64_t)c->dst_incr_mod) % c->src_incr; > - c->index = 0; > - } > - } > - } > - } else { > + { > int64_t end_index = (1LL + src_size - c->filter_length) * c->phase_count; > int64_t delta_frac = (end_index - c->index) * c->src_incr - c->frac; > int delta_n = (delta_frac + c->dst_incr - 1) / c->dst_incr; > @@ -386,8 +367,12 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A > if (dst_size > 0) { > /* resample_linear and resample_common should have same behavior > * when frac and dst_incr_mod are zero */ > - resample_func = (c->linear && (c->frac || c->dst_incr_mod)) ? > - c->dsp.resample_linear : c->dsp.resample_common; > + if (c->filter_length == 1 && c->phase_count == 1) > + resample_func = c->dsp.resample_one; > + else if (c->linear && (c->frac || c->dst_incr_mod)) > + resample_func = c->dsp.resample_linear; > + else > + resample_func = c->dsp.resample_common; > for (i = 0; i < dst->ch_count; i++) > *consumed = resample_func(c, dst->ch[i], src->ch[i], dst_size, i+1 == dst->ch_count); > } > diff --git a/libswresample/resample.h b/libswresample/resample.h > index 1731dad3cf..8cc29effe8 100644 > --- a/libswresample/resample.h > +++ b/libswresample/resample.h > @@ -51,8 +51,8 @@ typedef struct ResampleContext { > int phase_count_compensation; /* desired phase_count when compensation is enabled */ > > struct { > - void (*resample_one)(void *dst, const void *src, > - int n, int64_t index, int64_t incr); > + int (*resample_one)(struct ResampleContext *c, void *dst, > + const void *src, int n, int update_ctx); > int (*resample_common)(struct ResampleContext *c, void *dst, > const void *src, int n, int update_ctx); > int (*resample_linear)(struct ResampleContext *c, void *dst, > diff --git a/libswresample/resample_template.c b/libswresample/resample_template.c > index 4c227b9940..a8114ea918 100644 > --- a/libswresample/resample_template.c > +++ b/libswresample/resample_template.c > @@ -72,17 +72,27 @@ > > #endif > > -static void RENAME(resample_one)(void *dest, const void *source, > - int dst_size, int64_t index2, int64_t incr) > +static int RENAME(resample_one)(ResampleContext *c, > + void *dest, const void *source, > + int dst_size, int update_ctx) > { > DELEM *dst = dest; > const DELEM *src = source; > int dst_index; > + int64_t index2 = (1LL << 32) * c->frac / c->src_incr + 1 + (1LL << 32) * c->index; > + int64_t incr = (1LL << 32) * c->dst_incr / c->src_incr + 1; This computation is done repeatedly for each channel, thats not needed its enough if its done once thx [...]
On Thu, 29 Feb 2024, Michael Niedermayer wrote: > On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libswresample/resample.c | 29 +++++++---------------------- >> libswresample/resample.h | 4 ++-- >> libswresample/resample_template.c | 14 ++++++++++++-- >> 3 files changed, 21 insertions(+), 26 deletions(-) >> >> diff --git a/libswresample/resample.c b/libswresample/resample.c >> index 17cebad01b..89859dec79 100644 >> --- a/libswresample/resample.c >> +++ b/libswresample/resample.c >> @@ -356,26 +356,7 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A >> >> *consumed = 0; >> >> - if (c->filter_length == 1 && c->phase_count == 1) { >> - int64_t index2= (1LL<<32)*c->frac/c->src_incr + (1LL<<32)*c->index + 1; >> - int64_t incr= (1LL<<32) * c->dst_incr / c->src_incr + 1; >> - int new_size = (src_size * (int64_t)c->src_incr - c->frac + c->dst_incr - 1) / c->dst_incr; >> - >> - dst_size = FFMAX(FFMIN(dst_size, new_size), 0); >> - if (dst_size > 0) { >> - for (i = 0; i < dst->ch_count; i++) { >> - c->dsp.resample_one(dst->ch[i], src->ch[i], dst_size, index2, incr); >> - if (i+1 == dst->ch_count) { >> - c->index += dst_size * c->dst_incr_div; >> - c->index += (c->frac + dst_size * (int64_t)c->dst_incr_mod) / c->src_incr; >> - av_assert2(c->index >= 0); >> - *consumed = c->index; >> - c->frac = (c->frac + dst_size * (int64_t)c->dst_incr_mod) % c->src_incr; >> - c->index = 0; >> - } >> - } >> - } >> - } else { >> + { >> int64_t end_index = (1LL + src_size - c->filter_length) * c->phase_count; >> int64_t delta_frac = (end_index - c->index) * c->src_incr - c->frac; >> int delta_n = (delta_frac + c->dst_incr - 1) / c->dst_incr; >> @@ -386,8 +367,12 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A >> if (dst_size > 0) { >> /* resample_linear and resample_common should have same behavior >> * when frac and dst_incr_mod are zero */ >> - resample_func = (c->linear && (c->frac || c->dst_incr_mod)) ? >> - c->dsp.resample_linear : c->dsp.resample_common; >> + if (c->filter_length == 1 && c->phase_count == 1) >> + resample_func = c->dsp.resample_one; >> + else if (c->linear && (c->frac || c->dst_incr_mod)) >> + resample_func = c->dsp.resample_linear; >> + else >> + resample_func = c->dsp.resample_common; >> for (i = 0; i < dst->ch_count; i++) >> *consumed = resample_func(c, dst->ch[i], src->ch[i], dst_size, i+1 == dst->ch_count); >> } >> diff --git a/libswresample/resample.h b/libswresample/resample.h >> index 1731dad3cf..8cc29effe8 100644 >> --- a/libswresample/resample.h >> +++ b/libswresample/resample.h >> @@ -51,8 +51,8 @@ typedef struct ResampleContext { >> int phase_count_compensation; /* desired phase_count when compensation is enabled */ >> >> struct { >> - void (*resample_one)(void *dst, const void *src, >> - int n, int64_t index, int64_t incr); >> + int (*resample_one)(struct ResampleContext *c, void *dst, >> + const void *src, int n, int update_ctx); >> int (*resample_common)(struct ResampleContext *c, void *dst, >> const void *src, int n, int update_ctx); >> int (*resample_linear)(struct ResampleContext *c, void *dst, >> diff --git a/libswresample/resample_template.c b/libswresample/resample_template.c >> index 4c227b9940..a8114ea918 100644 >> --- a/libswresample/resample_template.c >> +++ b/libswresample/resample_template.c >> @@ -72,17 +72,27 @@ >> >> #endif >> >> -static void RENAME(resample_one)(void *dest, const void *source, >> - int dst_size, int64_t index2, int64_t incr) >> +static int RENAME(resample_one)(ResampleContext *c, >> + void *dest, const void *source, >> + int dst_size, int update_ctx) >> { >> DELEM *dst = dest; >> const DELEM *src = source; >> int dst_index; > >> + int64_t index2 = (1LL << 32) * c->frac / c->src_incr + 1 + (1LL << 32) * c->index; >> + int64_t incr = (1LL << 32) * c->dst_incr / c->src_incr + 1; > > This computation is done repeatedly for each channel, thats not needed > its enough if its done once I consider that negligable for real cases, and it makes the code cleaner doing the computations here. If you insist on this, then it is better to rework all the resample funcs to work on all channels in a separate patch. Regards, Marton
On Thu, Feb 29, 2024 at 06:55:01PM +0100, Marton Balint wrote: > > > On Thu, 29 Feb 2024, Michael Niedermayer wrote: > > > On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > > --- > > > libswresample/resample.c | 29 +++++++---------------------- > > > libswresample/resample.h | 4 ++-- > > > libswresample/resample_template.c | 14 ++++++++++++-- > > > 3 files changed, 21 insertions(+), 26 deletions(-) > > > > > > diff --git a/libswresample/resample.c b/libswresample/resample.c > > > index 17cebad01b..89859dec79 100644 > > > --- a/libswresample/resample.c > > > +++ b/libswresample/resample.c > > > @@ -356,26 +356,7 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A > > > > > > *consumed = 0; > > > > > > - if (c->filter_length == 1 && c->phase_count == 1) { > > > - int64_t index2= (1LL<<32)*c->frac/c->src_incr + (1LL<<32)*c->index + 1; > > > - int64_t incr= (1LL<<32) * c->dst_incr / c->src_incr + 1; > > > - int new_size = (src_size * (int64_t)c->src_incr - c->frac + c->dst_incr - 1) / c->dst_incr; > > > - > > > - dst_size = FFMAX(FFMIN(dst_size, new_size), 0); > > > - if (dst_size > 0) { > > > - for (i = 0; i < dst->ch_count; i++) { > > > - c->dsp.resample_one(dst->ch[i], src->ch[i], dst_size, index2, incr); > > > - if (i+1 == dst->ch_count) { > > > - c->index += dst_size * c->dst_incr_div; > > > - c->index += (c->frac + dst_size * (int64_t)c->dst_incr_mod) / c->src_incr; > > > - av_assert2(c->index >= 0); > > > - *consumed = c->index; > > > - c->frac = (c->frac + dst_size * (int64_t)c->dst_incr_mod) % c->src_incr; > > > - c->index = 0; > > > - } > > > - } > > > - } > > > - } else { > > > + { > > > int64_t end_index = (1LL + src_size - c->filter_length) * c->phase_count; > > > int64_t delta_frac = (end_index - c->index) * c->src_incr - c->frac; > > > int delta_n = (delta_frac + c->dst_incr - 1) / c->dst_incr; > > > @@ -386,8 +367,12 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A > > > if (dst_size > 0) { > > > /* resample_linear and resample_common should have same behavior > > > * when frac and dst_incr_mod are zero */ > > > - resample_func = (c->linear && (c->frac || c->dst_incr_mod)) ? > > > - c->dsp.resample_linear : c->dsp.resample_common; > > > + if (c->filter_length == 1 && c->phase_count == 1) > > > + resample_func = c->dsp.resample_one; > > > + else if (c->linear && (c->frac || c->dst_incr_mod)) > > > + resample_func = c->dsp.resample_linear; > > > + else > > > + resample_func = c->dsp.resample_common; > > > for (i = 0; i < dst->ch_count; i++) > > > *consumed = resample_func(c, dst->ch[i], src->ch[i], dst_size, i+1 == dst->ch_count); > > > } > > > diff --git a/libswresample/resample.h b/libswresample/resample.h > > > index 1731dad3cf..8cc29effe8 100644 > > > --- a/libswresample/resample.h > > > +++ b/libswresample/resample.h > > > @@ -51,8 +51,8 @@ typedef struct ResampleContext { > > > int phase_count_compensation; /* desired phase_count when compensation is enabled */ > > > > > > struct { > > > - void (*resample_one)(void *dst, const void *src, > > > - int n, int64_t index, int64_t incr); > > > + int (*resample_one)(struct ResampleContext *c, void *dst, > > > + const void *src, int n, int update_ctx); > > > int (*resample_common)(struct ResampleContext *c, void *dst, > > > const void *src, int n, int update_ctx); > > > int (*resample_linear)(struct ResampleContext *c, void *dst, > > > diff --git a/libswresample/resample_template.c b/libswresample/resample_template.c > > > index 4c227b9940..a8114ea918 100644 > > > --- a/libswresample/resample_template.c > > > +++ b/libswresample/resample_template.c > > > @@ -72,17 +72,27 @@ > > > > > > #endif > > > > > > -static void RENAME(resample_one)(void *dest, const void *source, > > > - int dst_size, int64_t index2, int64_t incr) > > > +static int RENAME(resample_one)(ResampleContext *c, > > > + void *dest, const void *source, > > > + int dst_size, int update_ctx) > > > { > > > DELEM *dst = dest; > > > const DELEM *src = source; > > > int dst_index; > > > > > + int64_t index2 = (1LL << 32) * c->frac / c->src_incr + 1 + (1LL << 32) * c->index; > > > + int64_t incr = (1LL << 32) * c->dst_incr / c->src_incr + 1; > > > > This computation is done repeatedly for each channel, thats not needed > > its enough if its done once > > I consider that negligable for real cases, and it makes the code cleaner > doing the computations here. It would make asm optimized functions more difficult to implement too while being less efficient. > > If you insist on this, then it is better to rework all the resample funcs to > work on all channels in a separate patch. That would be work as IIRC there are asm optimiezd versions of these. Also iam not sure about the complexity overall decreasing with such change either way iam not "insisting" on anything, i just want the code to be simple, clean and fast. I think the original code was close to achieving this, it just was buggy. Really iam happy about any ovarall improvment you want to do. But this started out as a bugfix, and in that context seeing a non bugfix change looks wrong to me If you want to simplify or otherwise improve swr, iam not in your way but please make sure the code really does improve and doesnt just look better to you. For example when its obvious a change would affect cases with small blocks and many channels the worst, the speed of such case should be tested. Not just a mono case thx [...]
On Fri, 1 Mar 2024, Michael Niedermayer wrote: > On Thu, Feb 29, 2024 at 06:55:01PM +0100, Marton Balint wrote: >> >> >> On Thu, 29 Feb 2024, Michael Niedermayer wrote: >> >>> On Tue, Feb 27, 2024 at 10:48:10AM +0100, Marton Balint wrote: >>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>> --- >>>> libswresample/resample.c | 29 +++++++---------------------- >>>> libswresample/resample.h | 4 ++-- >>>> libswresample/resample_template.c | 14 ++++++++++++-- >>>> 3 files changed, 21 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/libswresample/resample.c b/libswresample/resample.c >>>> index 17cebad01b..89859dec79 100644 >>>> --- a/libswresample/resample.c >>>> +++ b/libswresample/resample.c >>>> @@ -356,26 +356,7 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A >>>> >>>> *consumed = 0; >>>> >>>> - if (c->filter_length == 1 && c->phase_count == 1) { >>>> - int64_t index2= (1LL<<32)*c->frac/c->src_incr + (1LL<<32)*c->index + 1; >>>> - int64_t incr= (1LL<<32) * c->dst_incr / c->src_incr + 1; >>>> - int new_size = (src_size * (int64_t)c->src_incr - c->frac + c->dst_incr - 1) / c->dst_incr; >>>> - >>>> - dst_size = FFMAX(FFMIN(dst_size, new_size), 0); >>>> - if (dst_size > 0) { >>>> - for (i = 0; i < dst->ch_count; i++) { >>>> - c->dsp.resample_one(dst->ch[i], src->ch[i], dst_size, index2, incr); >>>> - if (i+1 == dst->ch_count) { >>>> - c->index += dst_size * c->dst_incr_div; >>>> - c->index += (c->frac + dst_size * (int64_t)c->dst_incr_mod) / c->src_incr; >>>> - av_assert2(c->index >= 0); >>>> - *consumed = c->index; >>>> - c->frac = (c->frac + dst_size * (int64_t)c->dst_incr_mod) % c->src_incr; >>>> - c->index = 0; >>>> - } >>>> - } >>>> - } >>>> - } else { >>>> + { >>>> int64_t end_index = (1LL + src_size - c->filter_length) * c->phase_count; >>>> int64_t delta_frac = (end_index - c->index) * c->src_incr - c->frac; >>>> int delta_n = (delta_frac + c->dst_incr - 1) / c->dst_incr; >>>> @@ -386,8 +367,12 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A >>>> if (dst_size > 0) { >>>> /* resample_linear and resample_common should have same behavior >>>> * when frac and dst_incr_mod are zero */ >>>> - resample_func = (c->linear && (c->frac || c->dst_incr_mod)) ? >>>> - c->dsp.resample_linear : c->dsp.resample_common; >>>> + if (c->filter_length == 1 && c->phase_count == 1) >>>> + resample_func = c->dsp.resample_one; >>>> + else if (c->linear && (c->frac || c->dst_incr_mod)) >>>> + resample_func = c->dsp.resample_linear; >>>> + else >>>> + resample_func = c->dsp.resample_common; >>>> for (i = 0; i < dst->ch_count; i++) >>>> *consumed = resample_func(c, dst->ch[i], src->ch[i], dst_size, i+1 == dst->ch_count); >>>> } >>>> diff --git a/libswresample/resample.h b/libswresample/resample.h >>>> index 1731dad3cf..8cc29effe8 100644 >>>> --- a/libswresample/resample.h >>>> +++ b/libswresample/resample.h >>>> @@ -51,8 +51,8 @@ typedef struct ResampleContext { >>>> int phase_count_compensation; /* desired phase_count when compensation is enabled */ >>>> >>>> struct { >>>> - void (*resample_one)(void *dst, const void *src, >>>> - int n, int64_t index, int64_t incr); >>>> + int (*resample_one)(struct ResampleContext *c, void *dst, >>>> + const void *src, int n, int update_ctx); >>>> int (*resample_common)(struct ResampleContext *c, void *dst, >>>> const void *src, int n, int update_ctx); >>>> int (*resample_linear)(struct ResampleContext *c, void *dst, >>>> diff --git a/libswresample/resample_template.c b/libswresample/resample_template.c >>>> index 4c227b9940..a8114ea918 100644 >>>> --- a/libswresample/resample_template.c >>>> +++ b/libswresample/resample_template.c >>>> @@ -72,17 +72,27 @@ >>>> >>>> #endif >>>> >>>> -static void RENAME(resample_one)(void *dest, const void *source, >>>> - int dst_size, int64_t index2, int64_t incr) >>>> +static int RENAME(resample_one)(ResampleContext *c, >>>> + void *dest, const void *source, >>>> + int dst_size, int update_ctx) >>>> { >>>> DELEM *dst = dest; >>>> const DELEM *src = source; >>>> int dst_index; >>> >>>> + int64_t index2 = (1LL << 32) * c->frac / c->src_incr + 1 + (1LL << 32) * c->index; >>>> + int64_t incr = (1LL << 32) * c->dst_incr / c->src_incr + 1; >>> >>> This computation is done repeatedly for each channel, thats not needed >>> its enough if its done once >> >> I consider that negligable for real cases, and it makes the code cleaner >> doing the computations here. > > It would make asm optimized functions more difficult to implement too while > being less efficient. > > >> >> If you insist on this, then it is better to rework all the resample funcs to >> work on all channels in a separate patch. > > That would be work as IIRC there are asm optimiezd versions > of these. Also iam not sure about the complexity overall decreasing with > such change > > either way iam not "insisting" on anything, i just want the code to be > simple, clean and fast. I think the original code was close to achieving > this, it just was buggy. > > Really iam happy about any ovarall improvment you want to do. > But this started out as a bugfix, and in that context seeing a non > bugfix change looks wrong to me > > If you want to simplify or otherwise improve swr, iam not in your way > but please make sure the code really does improve and doesnt just > look better to you. > For example when its obvious a change would affect cases with small > blocks and many channels the worst, the speed of such case should be > tested. Not just a mono case I just feel we draw the line between code simplicity and performance of rare or artificially constructed cases in different places. I will drop this patch for now, I don't want to delay the set this was initially part of. (which by the way is an attempt to fix the ffmpeg.c audio frame performance issue caused by its threading changes) Regards, Marton
diff --git a/libswresample/resample.c b/libswresample/resample.c index 17cebad01b..89859dec79 100644 --- a/libswresample/resample.c +++ b/libswresample/resample.c @@ -356,26 +356,7 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A *consumed = 0; - if (c->filter_length == 1 && c->phase_count == 1) { - int64_t index2= (1LL<<32)*c->frac/c->src_incr + (1LL<<32)*c->index + 1; - int64_t incr= (1LL<<32) * c->dst_incr / c->src_incr + 1; - int new_size = (src_size * (int64_t)c->src_incr - c->frac + c->dst_incr - 1) / c->dst_incr; - - dst_size = FFMAX(FFMIN(dst_size, new_size), 0); - if (dst_size > 0) { - for (i = 0; i < dst->ch_count; i++) { - c->dsp.resample_one(dst->ch[i], src->ch[i], dst_size, index2, incr); - if (i+1 == dst->ch_count) { - c->index += dst_size * c->dst_incr_div; - c->index += (c->frac + dst_size * (int64_t)c->dst_incr_mod) / c->src_incr; - av_assert2(c->index >= 0); - *consumed = c->index; - c->frac = (c->frac + dst_size * (int64_t)c->dst_incr_mod) % c->src_incr; - c->index = 0; - } - } - } - } else { + { int64_t end_index = (1LL + src_size - c->filter_length) * c->phase_count; int64_t delta_frac = (end_index - c->index) * c->src_incr - c->frac; int delta_n = (delta_frac + c->dst_incr - 1) / c->dst_incr; @@ -386,8 +367,12 @@ static int multiple_resample(ResampleContext *c, AudioData *dst, int dst_size, A if (dst_size > 0) { /* resample_linear and resample_common should have same behavior * when frac and dst_incr_mod are zero */ - resample_func = (c->linear && (c->frac || c->dst_incr_mod)) ? - c->dsp.resample_linear : c->dsp.resample_common; + if (c->filter_length == 1 && c->phase_count == 1) + resample_func = c->dsp.resample_one; + else if (c->linear && (c->frac || c->dst_incr_mod)) + resample_func = c->dsp.resample_linear; + else + resample_func = c->dsp.resample_common; for (i = 0; i < dst->ch_count; i++) *consumed = resample_func(c, dst->ch[i], src->ch[i], dst_size, i+1 == dst->ch_count); } diff --git a/libswresample/resample.h b/libswresample/resample.h index 1731dad3cf..8cc29effe8 100644 --- a/libswresample/resample.h +++ b/libswresample/resample.h @@ -51,8 +51,8 @@ typedef struct ResampleContext { int phase_count_compensation; /* desired phase_count when compensation is enabled */ struct { - void (*resample_one)(void *dst, const void *src, - int n, int64_t index, int64_t incr); + int (*resample_one)(struct ResampleContext *c, void *dst, + const void *src, int n, int update_ctx); int (*resample_common)(struct ResampleContext *c, void *dst, const void *src, int n, int update_ctx); int (*resample_linear)(struct ResampleContext *c, void *dst, diff --git a/libswresample/resample_template.c b/libswresample/resample_template.c index 4c227b9940..a8114ea918 100644 --- a/libswresample/resample_template.c +++ b/libswresample/resample_template.c @@ -72,17 +72,27 @@ #endif -static void RENAME(resample_one)(void *dest, const void *source, - int dst_size, int64_t index2, int64_t incr) +static int RENAME(resample_one)(ResampleContext *c, + void *dest, const void *source, + int dst_size, int update_ctx) { DELEM *dst = dest; const DELEM *src = source; int dst_index; + int64_t index2 = (1LL << 32) * c->frac / c->src_incr + 1 + (1LL << 32) * c->index; + int64_t incr = (1LL << 32) * c->dst_incr / c->src_incr + 1; for (dst_index = 0; dst_index < dst_size; dst_index++) { dst[dst_index] = src[index2 >> 32]; index2 += incr; } + + if (update_ctx) { + c->frac = (c->frac + dst_size * (int64_t)c->dst_incr_mod) % c->src_incr; + c->index = 0; + } + + return index2 >> 32; } static int RENAME(resample_common)(ResampleContext *c,
Signed-off-by: Marton Balint <cus@passwd.hu> --- libswresample/resample.c | 29 +++++++---------------------- libswresample/resample.h | 4 ++-- libswresample/resample_template.c | 14 ++++++++++++-- 3 files changed, 21 insertions(+), 26 deletions(-)