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

[Michael Niedermayer]

On Tue, May 27, 2014 at 07:08:35AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> 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 ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140527/796543a7/attachment.asc>