[FFmpeg-devel] [PATCH] libavfilter: add atempo filter (revised patch v3)

[Stefano Sabatini]

On date Sunday 2012-06-10 16:50:52 -0600, Pavel Koshevoy encoded:
> On 06/10/2012 01:17 PM, Stefano Sabatini wrote:
> >On date Friday 2012-06-08 09:16:36 -0600, Pavel Koshevoy encoded:
[...]
> >>+/**
> >>+ * Filter state machine
> >>+ */
> >>+typedef struct {
> >>+    // ring-buffer of input samples, necessary because some times
> >>+    // input fragment position may be adjusted backwards:
> >>+    uint8_t *buffer;
> >>+
> >>+    // ring-buffer maximum capacity,
> >>+    // expressed as number of multi-channel sample units;
> >>+    //
> >>+    // for example, given stereo data 1 multi-channel sample unit
> >>+    // refers to 2 samples for left/right channels:
> >>+    int ring;
> >>+
> >>+    // ring-buffer house keeping:
> >>+    int size;
> >>+    int head;
> >>+    int tail;
> >Also note that we have several buffer implementations, in particular
> >libavutil/fifo.h and audio_fifo.h, don't know if they could be used in
> >this particular case.
> 
> I probably could have used the fifo.h because it provides
> av_fifo_peek2(..), although I would still need to keep some of the
> house keeping variables for input/output sample positions.  I don't
> want to switch now because the code is already written and tested.

This can be done later if you or someone else feels like that, also
audio_fifo may be extended in order to support a peek method.

[...]
> Done, although I disagree.  A list of variables with one variable
> per line can be easily scanned vertically and variables of different
> types can be grouped together.  When variables are grouped on one
> line based on their type readability suffers -- the list has to be
> scanned vertically and horizontally, and logical grouping of
> variables of different types may be disrupted.

Well this is your code so feel free to switch back, nit issues should
be treated as such so the final word belongs to the contributor.

[...]
> >>+            // adjust the PTS:
> >>+            atempo->dst_buffer->pts =
> >>+                av_rescale(outlink->time_base.den,
> >>+                           atempo->nsamples_out,
> >>+                           outlink->time_base.num * outlink->sample_rate);
> >not sure but maybe av_rescale_q(atempo->nsamples_out, (AVRational){1, outlink->sample_rate}, outlink->time_base)
> >
> >may be safer, as it avoids a possible int*int ->  int64_t overflow:
> >outlink->time_base.num * outlink->sample_rate
> 
> Done, although I line-wrapped it to be under 80-chars per line (helps readability).
> 
> I am more concerned about the fact that the PTS is computed from the
> number of samples output.  If this filter is used interactively with
> seeking and allowing user to change tempo continuously then this PTS
> calculation will not be enough.

I see, we have more filters which suffer the same problem. So yes this
is a serious issue which may affect more filters once seek is
implemented (I'm currently going to add that to the movie sources).

As a workaround we can add an asetpts filter (still missing) to
normalize the input timestamps before audio filters which are affected
by timestamps.
-- 
FFmpeg = Fancy and Frightening Magical Pitiless Enlightened Gadget