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

[Michael Niedermayer]

On Tue, May 27, 2014 at 09:45:29AM -0400, Ronald S. Bultje wrote:
> 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.

hmm, yes, aggree thats probably the simplest/sanest thing to do


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/a501a98d/attachment.asc>