[MPlayer-dev-eng] [PATCH] af_scaletempo

[Uoti Urpala]

On Sat, 2007-10-06 at 20:05 -0400, Robert Juliano wrote:
> On Sat, Oct 06, 2007 at 10:26:01PM +0300, Uoti Urpala wrote:
> > I think it's better to fix some of the general filter issues before
> > adding the filter. It buffers more data internally than than the
> > existing resample filters.
> 
> Yeah, and it uses more memory than format and more CPU than
> channels?  I don't see the relevance of the comparison.

That behavior triggers problems elsewhere in MPlayer's filter system.
The existing filters are not as badly affected.

> > - enable the filter delay logic again and dynamically set the delay
> > value in af_scaletempo after each call
> 
> Unless someone shows me otherwise, delay = 0.  The way the
> filter works is by consuming input at a scaled rate and output
> at the nominal rate or it can be viewed as skipping forwards
> or backwards every few milliseconds.  I'm not sure delay even
> makes sense in this context.

In this context delay can be defined as
total_bytes_input/input_bitrate-total_bytes_output/output_bitrate
(that's assuming the average rates are exact; MPlayer could cope with a
filter with identical nominal input and output rate outputting 999
samples for each 1000 read, but it'd require a different definition
here). Most of the time it is not 0: if you feed input one sample at a
time there will be no output at first (increasing delay) then a bigger
output block at once (delay drops again).

>   And if it's disabled, what does
> it have to do with adding a filter today?

Ignoring delay doesn't matter that much with existing commonly used
filters which do not cause much delay. However if I read the code
correctly scaletempo has typical delay values varying by 60 ms, which is
quite a lot when video frame timing is based on audio position.

> > - when used for runtime playback speed adjustment, make sure
> > af_scaletempo never alters the audio when input and output rate are
> > identical, either by rebuilding the filter chain without it or by logic
> > inside the filter
> 
> Does what's in the latest patch (9/30) not do this?

Yes the added test should do that.

> > I haven't done those yet mainly because of the release plans.
> 
> Sounds to me like you have a number of backlogged tasks that
> would require changes to many filters and the audio chain,
> and from your follow-up email, it sound to me like
> af_scaletempo is fine but the filter framework has issues.

Kind of. However those are not just general "backlogged tasks" but ones
which are relevant now when considering the addition of af_scaletempo.
The issues don't matter that much when using the existing filters.

> So, I'm left with similar questions as Reimar.  What about
> af_scaletempo would prevent it from being committed today?
> What, if any, of the issues with the filter framework/audio
> chain should prevent it from being committed?  And, how much
> more difficult would it be to make the changes after it was
> committed, and is that sufficient to warrant delaying it?

I think the issues are not so severe as to make committing it completely
impossible, but I think they're still worth fixing before enabling the
filter. Fixing those parts of the audio framework should not take very
long; however I didn't want to make such changes just before a release
(I'm not sure about what the release status is supposed to be now -
whether it's meant to use already branched code or not), and having the
filter in the release with those issues and very little testing probably
isn't a good idea either.