[FFmpeg-devel] [PATCH] Rewrite COMMON_CORE resampling loop.

[Ronald S. Bultje]

Hi,

On Tue, May 27, 2014 at 7:55 AM, Michael Niedermayer <michaelni at gmx.at>wrote:

> On Tue, May 27, 2014 at 07:08:35AM -0400, Ronald S. Bultje wrote:
> > On Mon, May 26, 2014 at 11:24 PM, Michael Niedermayer <michaelni at gmx.at
> >wrote:
> >
> > > On Mon, May 26, 2014 at 06:09:56PM -0400, Ronald S. Bultje wrote:
> > > > This moves a condition in the loop outside. In one particular fate
> > > > test (fate-swr-resample-s32p-8000-2626), this makes the code about
> > > > 10% faster. It also simplifies the loop, allowing us to rewrite it
> > > > in yasm at some later point.
> > > > ---
> > > >  libswresample/resample_template.c | 30
> ++++++++++++++++--------------
> > > >  1 file changed, 16 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/libswresample/resample_template.c
> > > b/libswresample/resample_template.c
> > > > index becff12..44fb667 100644
> > > > --- a/libswresample/resample_template.c
> > > > +++ b/libswresample/resample_template.c
> > > > @@ -135,34 +135,36 @@ int RENAME(swri_resample)(ResampleContext *c,
> > > DELEM *dst, const DELEM *src, int
> > > >          *consumed= index;
> > > >          index = 0;
> > > >      }else if(compensation_distance == 0 && !c->linear && index >=
> 0){
> > > > +        int64_t end_index = (1 + src_size - c->filter_length) <<
> > > c->phase_shift;
> > >
> > > the shift is 32bit, probably would be safer as 64bit
> >
> >
> > The 64 bit was mainly to not have to cast in the next line:
> >
> >
> > > > +        int64_t delta_frac = (end_index - index) * c->src_incr -
> > > c->frac;
> > >
> > > cant this overflow ?
> >
> >
> > That's a good question, theoretically speaking I'm not sure. It obviously
> > doesn't in any of the tests, but I'm not sure what the limits o these
> > numbers are. I see phase_shift being 10 and then src_incr will be in that
> > area also (say 15 bit upper bound on massive resampling change), which is
> > only 25 bits so then it easily fits. But I don't know if that holds for
> all
> > input/configs.
>
> phase_shift is user settable with a current max of 24
> src_incr is set from the sampling rates with av_reduce with max of
> INT32_MAX/2, so if we would consider DSD512 like output rate with
> a random prime input then this should be in the 25bits
> and with the buffer size just limited by memory and int, this could
> overflow unless i miss something that would prevent this combination
> certainly not a common thing to happen by chance but then i guess
> a crafted input file could control the packet size, input and output
> sample rate which even with a 10bit default phase_shift could overflow
> id have to reread the code though to see what could be done by
> triggering this to overflow ...


Right, I certainly won't deny that this can happen with crafted input. For
such a case, I think the more sensible thing to do is to error out and tell
the user he should use more sensible settings.

Ronald