[FFmpeg-devel] Reducing time spent in av_find_stream_info()

[Michael Niedermayer]

Hi

On Thu, Nov 15, 2007 at 09:39:41AM +0100, Diego Santa Cruz wrote:
> > > OK I have done both :)
> > 
> > [...]
> > 
> 
> Thanks for the remarks, see below...
> 
> > > Second patch attached (ffmpeg-info-fps.patch) modifies the table of
> > > standard framerates. The new set is much reduced (from 725 to 42). I
> > > replaced all framerates 1 to 60 in 1/12 steps by all regular
> framerates
> > > derived from 18, 24, 30 and 60 which are not below 1 fps and which
> are
> > > also a multiple of 1/12 (as was the case before). Note that this
> second
> > > patch is independent of the one above, although it may not apply
> cleanly
> > > without it due to nearby changes. I also added 17.98 = 18*1000/1001,
> > > since I guess this might appear in some cases...
> > 
> > i doubt these will be enough framerates also it looks a little more
> > complex than what i would have hoped ...
> > 
> 
> Do you have any hints on what else is common? Any ideas on how to
> simplify it without having a number as large as before?

well all the integers from 1...60 should at least be in there
then 1,1/2,1/3,1/4,1/6,2/3 of the standard framerates should be there
as well



> 
> > 
> > tabs are forbidden in svn
> 
> Sorry about that... I'll change the Emacs settings :)
> 
> > 
> > 
> > >                  for(i=1; i<MAX_STD_TIMEBASES; i++){
> > >                      int framerate= get_std_framerate(i);
> > > -                    int ticks= lrintf(dur*framerate/(1001*12));
> > > -                    double error= dur -
> ticks*1001*12/(double)framerate;
> > > -                    duration_error[index][i] += error*error;
> > > +		    int ticks_den = st->time_base.den * 1001 * 12;
> > 
> > this can overflow, also it can be moved out of this loop
> 
> I was wondering if there was a maximum value for st->time_base.den and
> what it might be... Or is the full range of int used?

full int, and ive seen (avi) files using such time bases


> 
> > 
> > 
> > > +		    int64_t ticks =
> > > +			(2 * duration * st->time_base.num *
> > > +			 framerate + ticks_den - 1) / (2*ticks_den);
> > 
> > 
> > 
> > > +		    /* 'error' is the difference between the duration
> > > +		     * as integer multiple of the frame rate and the
> > > +		     * actual duration in .16 fixed-point */
> > > +		    int64_t error=
> > > +			( (duration * st->time_base.num * framerate -
> > 
> > duration * st->time_base.num * framerate can be factorized out
> > 
> > 
> > > +			   ticks * ticks_den) << 16) /
> > > +			(st->time_base.den * framerate);
> > > +                    duration_error[index][i] += (error*error);
> > 
> > this is not accurate also the denominator is not needed all
> > values added have the same
> 
> Yep, there is a loss of precision due to the fixed point nature, but
> that is the problem with fixed-point. But I assumed time_base.den can
> change between different packets of the same stream, in which case I
> think it cannot be factored out, or is it constant once the codec has
> been opened? If constant it can be removed from the computation.

its constant


> 
> > 
> > 
> > >                  }
> > >                  duration_count[index]++;
> > >              }
> > 
> > > @@ -1939,11 +1950,11 @@
> > >                 && (st->codec->time_base.num*101LL <=
> st->codec->time_base.den || st->codec-
> > >codec_id == CODEC_ID_MPEG2VIDEO) /*&&
> > >                 //FIXME we should not special case mpeg2, but this
> needs testing with non mpeg2 ...
> > >
> st->time_base.num*duration_sum[i]/duration_count[i]*101LL >
> st->time_base.den*/){
> > > -                double best_error= 2*av_q2d(st->time_base);
> > > +                int64_t best_error=
> (st->time_base.num<<17)/st->time_base.den;
> > >                  best_error=
> best_error*best_error*duration_count[i]*1000*12*30;
> > >
> > >                  for(j=1; j<MAX_STD_TIMEBASES; j++){
> > > -                    double error= duration_error[i][j] *
> get_std_framerate(j);
> > > +                    int64_t error= duration_error[i][j] *
> get_std_framerate(j);
> > >  //                    if(st->codec->codec_type == CODEC_TYPE_VIDEO)
> > >  //                        av_log(NULL, AV_LOG_ERROR, "%f %f\n",
> get_std_framerate(j) / 12.0/1001,
> > error);
> > >                      if(error < best_error){
> > 
> > i dont think this matters speed wise, also both these changes can
> cause
> > overflows
> 
> You are right, this does not matter much speed-wise, but I did the
> change mostly for consistency with the above changes, although I
> overlooked the overflow...
> 
> BTW, why is the duration_error weighted by the framerate instead of just
> using the lowest error without weighting? Weighting favors large frame
> rates.

no weighting favors small framerates
and its there because its needed, IIRC to prevent for example a 30fps video
to be detected as 60fps (both would have approximately the same error)


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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071115/4c7f61d4/attachment.pgp>