[FFmpeg-devel] [PATCH] Add a time_base field to AVFilterPad.

[Stefano Sabatini]

On date Wednesday 2010-10-06 22:48:16 +0200, Michael Niedermayer encoded:
> On Wed, Oct 06, 2010 at 10:07:54PM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2010-10-06 21:43:46 +0200, Michael Niedermayer encoded:
> > > On Wed, Oct 06, 2010 at 08:20:18PM +0200, Stefano Sabatini wrote:
> > [...]
> > > >  avfilter.c |   21 +++++++++++++++++++++
> > > >  avfilter.h |   15 ++++++++++++++-
> > > >  defaults.c |    1 +
> > > >  3 files changed, 36 insertions(+), 1 deletion(-)
> > > > dffa3d20b8e8d74cbbf2d67ab170242471e1dbfa  0001-Add-a-time_base-field-to-AVFilterPad.patch
> > > > From 01f1ac855d628bfb1ddea394dea3bad5fd74350b Mon Sep 17 00:00:00 2001
> > > > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > > > Date: Mon, 27 Sep 2010 20:24:44 +0200
> > > > Subject: [PATCH 1/9] Add a time_base field to AVFilterPad.
> > > > 
> > > > This is required for allowing a filter to use a time base different
> > > > from AV_TIME_BASE_Q, as it was previously assumed.
> > > > ---
> > > >  libavfilter/avfilter.c |   21 +++++++++++++++++++++
> > > >  libavfilter/avfilter.h |   15 ++++++++++++++-
> > > >  libavfilter/defaults.c |    1 +
> > > >  3 files changed, 36 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > > index ea6f9fe..bbe8d78 100644
> > > > --- a/libavfilter/avfilter.c
> > > > +++ b/libavfilter/avfilter.c
> > > > @@ -23,6 +23,7 @@
> > > >  
> > > >  #include "libavcodec/audioconvert.c"
> > > >  #include "libavutil/pixdesc.h"
> > > > +#include "libavutil/rational.h"
> > > >  #include "libavcore/imgutils.h"
> > > >  #include "avfilter.h"
> > > >  #include "internal.h"
> > > > @@ -179,10 +180,20 @@ int avfilter_config_links(AVFilterContext *filter)
> > > >              if (config_link(link))
> > > >                  return -1;
> > > >  
> > > > +            if (link->srcpad->time_base.num == 0 && link->srcpad->time_base.den == 0) {
> > > > +                if (link->src->input_count)
> > > > +                    link->srcpad->time_base = link->src->input_pads[0].time_base;
> > > > +                else
> > > 
> > > {}
> > > 
> > > 
> > > [...]
> > > > @@ -336,6 +348,15 @@ void avfilter_start_frame(AVFilterLink *link, AVFilterBufferRef *picref)
> > > >      else
> > > >          link->cur_buf = picref;
> > > >  
> > > > +    if (av_cmp_q(dst->time_base, src->time_base)) {
> > > > +        int64_t av_unused pts1 = picref->pts;
> > > > +        link->cur_buf->pts = av_rescale_q(picref->pts, src->time_base, dst->time_base);
> > > 
> > > missing AV_NOPTS check
> > > 
> > > 
> > > [...]
> > > > @@ -424,6 +431,12 @@ struct AVFilterPad {
> > > >       * and another value on error.
> > > >       */
> > > >      int (*config_props)(AVFilterLink *link);
> > > > +
> > > > +    /**
> > > > +     * Define the time base used by the PTS of the frames/samples
> > > > +     * which will pass through this link.
> > > > +     */
> > > > +    AVRational time_base;
> > > >  };
> > > 
> > > Belongs to AVFilterLink not Pad IMHO
> > 
> > Would be acceptable to create a src_time_base and dst_time_base in the
> > link?
> > 
> > BTW we need to set the timebase in the two ends of a link (either in
> > the link itself or in the input and output pads) if we want to keep
> > the timestamp conversion code in the framework rather than in the
> > filters code.
> 
> when does this design simplify filters?
> do you have an example?
> it seems simpler to have just one tb per link

A filter in general may to set the output timebase, and another filter
the input timebase.

Think for example of a source, which generates video frames with given
framerate, or a movie sink which needs to use the same timebase as set
in the muxer, so I believe this in general is a useful feature that we
want to keep.

If we want to keep the PTS conversion code out of the filters, we need
to set it in the link or in the link ends.

Let's consider the idea to put just one timebase in the link, and
consider the simple filterchain composed by one source and one sink.

source <- TB1 or TB2? -> sink

The source wants to set the output TB, which is usually done in
configure_props for the output pad, which will set the TB in the
output link.

Now the sink wants to set the input TB, it checks the timebase of the
input link and see that it is already set. Since it wants to set
another timebase it needs some way for converting the PTS. I see two
possible mechanisms:
1) auto-inserting a converttb filter during this stage
2) store the input and output timebases in the link, and auto-insert
   them during the query-formats stage, or just after configuration.

At the end of the configuration we'll end up with:

source <- TB1 -> converttb <- TB2 -> sink

1) doesn't look very nice since involves the auto-insertion of the
converttb filter done by the filters code (brittle, more complexity
exposed to the filter authors)
2) requires anyway to store the input and output values in the link
before the timebase configuration process.

It looks to me that the current approach (using two timebases per
link, and letting avfilter_start_frame() do the timebase conversion)
is simpler than this.

I can try to explore the other variants if you think that it's worth
it, and/or that it may result in a simpler/cleaner design.

Regards.
-- 
FFmpeg = Fostering Frenzy Magnificient Purposeless Elastic Geisha