[MPlayer-dev-eng] [PATCH] af_scale* command-line only

[Robert Juliano]

Reimar Doeffinger wrote:
> Hello,
> On Wed, Jun 13, 2007 at 12:50:47AM -0400, Robert Juliano wrote:
> [...]
> > + * scale rate (both time and tempo) (i.e. chipmunk effect)
> > + *
> > + * basic technique:
> > + *   pretend input rate is scaled rate and resample to input rate
> [...]
> > +#ifdef USE_LIBAVCODEC
> > +  #define RESAMPLER "lavcresample"
> > +#else
> > +  #define RESAMPLER "resample"
> > +#endif
> 
> Hm... is there any specific reason to do it in a separate filter?
> Seems to me like adding it to resample would be a three-line change (
> adding a speedup double to af_resample_t and using it in two places in
> REINIT) + adding the two controls.
> Advantages of that approach: An already loaded resample filter can be
> reused so this feature does not use extra CPU in this case, the
> selection of fast vs. high-quality resampling via -af-adv works.

I'll give that a shot.

> > +int fill_window(struct af_instance_s* af, af_data_t* c, int ic) {
> 
> Why do you "always" name the af_data_t pointers "c"? That name is really
> unhelpful and even "d" would be a saner (but still horrible) choice.
> 
> > +  af_scaletempo_t* s = af->setup;
> > +  int ns_in = c->len/c->bps - ic;
> 
> "ns" is not a descriptive name either.
> ...
> > +  int ns_in    = c->len/c->bps; // Length of input in samples
> 
> How about not commenting it but instead giving it a better name like
> samplecnt? There sure are even better names for it to be found...

The naming convention I've used is "nb" means number of bytes,
"ns" number of samples, and "np" number of packed samples
(i.e. ns / num_channels). 

As for the comment and "c", I based my code on the other
filters and I've gotten a lot of criticism because of it.
So I've learned: the other filters are not necessarily up
to spec.

[Re: af_scaletempo.c:fill_window]
> Hmm... the code looks to me like you're doing something fifo-like?
> Maybe you can use libavutil/fifo.h to simplify things?

It's not exactly a FIFO because I need to rewind and skip
forwards.  It's a sliding window.  A FIFO reads x samples,
and consumes x samples.  I need to read x, consume y.

I would feel better using a library, so if you know of any
utils that do this I would be happy to use it instead.  If
not and there's other code that uses sliding windows, I would
be willing to write it.

As it stands, I'm okay with the way it is, though I would
feel better thrashing it with a test harness.

> > +    // output overlap
> > +    for (i=0, io=0, iw=ns_off; i<s->np_overlap; i++) {
> > +      float t = i / (float)s->np_overlap;
> 
> Doing a floating point division for every sample is bound to be really
> slow.
> While avoiding floating point completely would be the nicest solution
> for many reasons, pre-calculating 1/(float)s->np_overlap is still an
> improvement (though I somewhat have the feeling that operating on 16 bit
> samples instead of float would only barely make the code more
> complicated).

Yeah, that doesn't need calculated each time.  What a waste!

The goal is to support both formats.  The 16-bit one is a
little tricky because it very easily overflows.  I did the
easier one first.  I'll get working on the second.

ciao,
robert