[01/11] lavfi: add a frame_rate field to AVFilterLink.

Message ID 1446050900-7329-1-git-send-email-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Oct. 28, 2015, 4:48 p.m.
From: Nicolas George <nicolas.george@normalesup.org>

(cherry picked from ffmpeg commit 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
---
 libavfilter/avfilter.c |  2 ++
 libavfilter/avfilter.h | 12 ++++++++++++
 2 files changed, 14 insertions(+)

Comments

Anton Khirnov Oct. 29, 2015, 6:47 p.m. | #1
Quoting John Stebbins (2015-10-28 17:48:17)
> From: Nicolas George <nicolas.george@normalesup.org>
> 
> (cherry picked from ffmpeg commit 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> ---
>  libavfilter/avfilter.c |  2 ++
>  libavfilter/avfilter.h | 12 ++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index 64b2645..cd98d16 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -195,6 +195,8 @@ int avfilter_config_links(AVFilterContext *filter)
>                          link->src->inputs[0]->sample_aspect_ratio : (AVRational){1,1};
>  
>                  if (link->src->nb_inputs) {
> +                    if (!link->frame_rate.num && !link->frame_rate.den)
> +                        link->frame_rate = link->src->inputs[0]->frame_rate;
>                      if (!link->w)
>                          link->w = link->src->inputs[0]->w;
>                      if (!link->h)
> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index 9dbfeea..b7b97ce 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -375,6 +375,18 @@ struct AVFilterLink {
>          AVLINK_STARTINIT,       ///< started, but incomplete
>          AVLINK_INIT             ///< complete
>      } init_state;
> +
> +    /**
> +     * Frame rate of the stream on the link, or 1/0 if unknown;
> +     * if left to 0/0, will be automatically be copied from the first input
> +     * of the source filter if it exists.
> +     *
> +     * Sources should set it to the best estimation of the real frame rate.
> +     * Filters should update it if necessary depending on their function.
> +     * Sinks can use it to set a default output frame rate.
> +     * It is similar to the avg_frame_rate field in AVStream.
> +     */

Please no! No guesses or estimates, we already have plenty of this crap
in lavf and we want to get rid of it, not add more. VFR streams with no
such thing as "real frame rate" exist.

So IMO this should be:
- if AND ONLY if the stream is known to be of some specific constant
  framerate, this field should be set to that number
- in ALL OTHER CASES, this field should be set to 0/0 (or whatever other
  invalid value) and treated as VFR.

Also, this needs a bump+APIchanges (disregard this comment if this
happens in some later patch I didn't see yet).
John Stebbins Oct. 29, 2015, 6:59 p.m. | #2
On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-28 17:48:17)
> > From: Nicolas George <nicolas.george@normalesup.org>
> > 
> > (cherry picked from ffmpeg commit
> > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > ---
> >  libavfilter/avfilter.c |  2 ++
> >  libavfilter/avfilter.h | 12 ++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > index 64b2645..cd98d16 100644
> > --- a/libavfilter/avfilter.c
> > +++ b/libavfilter/avfilter.c
> > @@ -195,6 +195,8 @@ int avfilter_config_links(AVFilterContext
> > *filter)
> >                          link->src->inputs[0]->sample_aspect_ratio :
> > (AVRational){1,1};
> >  
> >                  if (link->src->nb_inputs) {
> > +                    if (!link->frame_rate.num && !link
> > ->frame_rate.den)
> > +                        link->frame_rate = link->src->inputs[0]
> > ->frame_rate;
> >                      if (!link->w)
> >                          link->w = link->src->inputs[0]->w;
> >                      if (!link->h)
> > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > index 9dbfeea..b7b97ce 100644
> > --- a/libavfilter/avfilter.h
> > +++ b/libavfilter/avfilter.h
> > @@ -375,6 +375,18 @@ struct AVFilterLink {
> >          AVLINK_STARTINIT,       ///< started, but incomplete
> >          AVLINK_INIT             ///< complete
> >      } init_state;
> > +
> > +    /**
> > +     * Frame rate of the stream on the link, or 1/0 if unknown;
> > +     * if left to 0/0, will be automatically be copied from the
> > first input
> > +     * of the source filter if it exists.
> > +     *
> > +     * Sources should set it to the best estimation of the real
> > frame rate.
> > +     * Filters should update it if necessary depending on their
> > function.
> > +     * Sinks can use it to set a default output frame rate.
> > +     * It is similar to the avg_frame_rate field in AVStream.
> > +     */
> 
> Please no! No guesses or estimates, we already have plenty of this
> crap
> in lavf and we want to get rid of it, not add more. VFR streams with
> no
> such thing as "real frame rate" exist.
> 
> So IMO this should be:
> - if AND ONLY if the stream is known to be of some specific constant
>   framerate, this field should be set to that number
> - in ALL OTHER CASES, this field should be set to 0/0 (or whatever
> other
>   invalid value) and treated as VFR.

An actual framerate has to be written at the muxing stage by a
transcoder such as HandBrake.  We can't know how the filter chain will
affect our nominal framerate without passing the nominal value to the
input of the filter chain and seeing what comes out the other side. So
although nominal framerates are a PITA, they are a requirement we can't
ignore.

Although HandBrake generates output that is technically variable
framerate by default (i.e. vfr flags are set in h.264 headers), it is
often the case that the measured framerate is constant throughout the
stream.  There is just no way to know this in advance with certain
source types (e.g. DVD).

We could document this as you prefer as a means of discouraging this
kind of usage.  But this is a real world case that is going to happen.

> 
> Also, this needs a bump+APIchanges (disregard this comment if this
> happens in some later patch I didn't see yet).
> 

Yes, I was going to do this and then forgot.  Thanks for the reminder.
Anton Khirnov Oct. 29, 2015, 7:12 p.m. | #3
Quoting John Stebbins (2015-10-29 19:59:44)
> On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > Quoting John Stebbins (2015-10-28 17:48:17)
> > > From: Nicolas George <nicolas.george@normalesup.org>
> > > 
> > > (cherry picked from ffmpeg commit
> > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > ---
> > >  libavfilter/avfilter.c |  2 ++
> > >  libavfilter/avfilter.h | 12 ++++++++++++
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > index 64b2645..cd98d16 100644
> > > --- a/libavfilter/avfilter.c
> > > +++ b/libavfilter/avfilter.c
> > > @@ -195,6 +195,8 @@ int avfilter_config_links(AVFilterContext
> > > *filter)
> > >                          link->src->inputs[0]->sample_aspect_ratio :
> > > (AVRational){1,1};
> > >  
> > >                  if (link->src->nb_inputs) {
> > > +                    if (!link->frame_rate.num && !link
> > > ->frame_rate.den)
> > > +                        link->frame_rate = link->src->inputs[0]
> > > ->frame_rate;
> > >                      if (!link->w)
> > >                          link->w = link->src->inputs[0]->w;
> > >                      if (!link->h)
> > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > index 9dbfeea..b7b97ce 100644
> > > --- a/libavfilter/avfilter.h
> > > +++ b/libavfilter/avfilter.h
> > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > >          AVLINK_STARTINIT,       ///< started, but incomplete
> > >          AVLINK_INIT             ///< complete
> > >      } init_state;
> > > +
> > > +    /**
> > > +     * Frame rate of the stream on the link, or 1/0 if unknown;
> > > +     * if left to 0/0, will be automatically be copied from the
> > > first input
> > > +     * of the source filter if it exists.
> > > +     *
> > > +     * Sources should set it to the best estimation of the real
> > > frame rate.
> > > +     * Filters should update it if necessary depending on their
> > > function.
> > > +     * Sinks can use it to set a default output frame rate.
> > > +     * It is similar to the avg_frame_rate field in AVStream.
> > > +     */
> > 
> > Please no! No guesses or estimates, we already have plenty of this
> > crap
> > in lavf and we want to get rid of it, not add more. VFR streams with
> > no
> > such thing as "real frame rate" exist.
> > 
> > So IMO this should be:
> > - if AND ONLY if the stream is known to be of some specific constant
> >   framerate, this field should be set to that number
> > - in ALL OTHER CASES, this field should be set to 0/0 (or whatever
> > other
> >   invalid value) and treated as VFR.
> 
> An actual framerate has to be written at the muxing stage by a
> transcoder such as HandBrake.  We can't know how the filter chain will
> affect our nominal framerate without passing the nominal value to the
> input of the filter chain and seeing what comes out the other side. So
> although nominal framerates are a PITA, they are a requirement we can't
> ignore.
> 
> Although HandBrake generates output that is technically variable
> framerate by default (i.e. vfr flags are set in h.264 headers), it is
> often the case that the measured framerate is constant throughout the
> stream.  There is just no way to know this in advance with certain
> source types (e.g. DVD).
> 
> We could document this as you prefer as a means of discouraging this
> kind of usage.  But this is a real world case that is going to happen.

If you really need some kind of a framerate, you could e.g. pass two
dummy frames through the filtergraph and check the pts difference on the
output. Would that be a lot more work for you?

I recall you complaining about the lavf timestamps mess yourself. And
one of the main reasons it is so horrible is precisely that it somehow
makes up some guesses or estimates and then yet more guesses on top of
that until we have no clue what are real data and what is made up. I
would really like to avoid that in any new code, if reasonably possible.
John Stebbins Oct. 29, 2015, 8:17 p.m. | #4
On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-29 19:59:44)
> > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > > Quoting John Stebbins (2015-10-28 17:48:17)
> > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > 
> > > > (cherry picked from ffmpeg commit
> > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > ---
> > > >  libavfilter/avfilter.c |  2 ++
> > > >  libavfilter/avfilter.h | 12 ++++++++++++
> > > >  2 files changed, 14 insertions(+)
> > > > 
> > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > > index 64b2645..cd98d16 100644
> > > > --- a/libavfilter/avfilter.c
> > > > +++ b/libavfilter/avfilter.c
> > > > @@ -195,6 +195,8 @@ int avfilter_config_links(AVFilterContext
> > > > *filter)
> > > >                          link->src->inputs[0]
> > > > ->sample_aspect_ratio :
> > > > (AVRational){1,1};
> > > >  
> > > >                  if (link->src->nb_inputs) {
> > > > +                    if (!link->frame_rate.num && !link
> > > > ->frame_rate.den)
> > > > +                        link->frame_rate = link->src->inputs[0]
> > > > ->frame_rate;
> > > >                      if (!link->w)
> > > >                          link->w = link->src->inputs[0]->w;
> > > >                      if (!link->h)
> > > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > > index 9dbfeea..b7b97ce 100644
> > > > --- a/libavfilter/avfilter.h
> > > > +++ b/libavfilter/avfilter.h
> > > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > > >          AVLINK_STARTINIT,       ///< started, but incomplete
> > > >          AVLINK_INIT             ///< complete
> > > >      } init_state;
> > > > +
> > > > +    /**
> > > > +     * Frame rate of the stream on the link, or 1/0 if unknown;
> > > > +     * if left to 0/0, will be automatically be copied from the
> > > > first input
> > > > +     * of the source filter if it exists.
> > > > +     *
> > > > +     * Sources should set it to the best estimation of the real
> > > > frame rate.
> > > > +     * Filters should update it if necessary depending on their
> > > > function.
> > > > +     * Sinks can use it to set a default output frame rate.
> > > > +     * It is similar to the avg_frame_rate field in AVStream.
> > > > +     */
> > > 
> > > Please no! No guesses or estimates, we already have plenty of this
> > > crap
> > > in lavf and we want to get rid of it, not add more. VFR streams
> > > with
> > > no
> > > such thing as "real frame rate" exist.
> > > 
> > > So IMO this should be:
> > > - if AND ONLY if the stream is known to be of some specific
> > > constant
> > >   framerate, this field should be set to that number
> > > - in ALL OTHER CASES, this field should be set to 0/0 (or whatever
> > > other
> > >   invalid value) and treated as VFR.
> > 
> > An actual framerate has to be written at the muxing stage by a
> > transcoder such as HandBrake.  We can't know how the filter chain
> > will
> > affect our nominal framerate without passing the nominal value to
> > the
> > input of the filter chain and seeing what comes out the other side.
> > So
> > although nominal framerates are a PITA, they are a requirement we
> > can't
> > ignore.
> > 
> > Although HandBrake generates output that is technically variable
> > framerate by default (i.e. vfr flags are set in h.264 headers), it
> > is
> > often the case that the measured framerate is constant throughout
> > the
> > stream.  There is just no way to know this in advance with certain
> > source types (e.g. DVD).
> > 
> > We could document this as you prefer as a means of discouraging this
> > kind of usage.  But this is a real world case that is going to
> > happen.
> 
> If you really need some kind of a framerate, you could e.g. pass two
> dummy frames through the filtergraph and check the pts difference on
> the
> output. Would that be a lot more work for you?
> 
> I recall you complaining about the lavf timestamps mess yourself. And
> one of the main reasons it is so horrible is precisely that it somehow
> makes up some guesses or estimates and then yet more guesses on top of
> that until we have no clue what are real data and what is made up. I
> would really like to avoid that in any new code, if reasonably
> possible.
> 

In theory, HandBrake could calculate the average framerate over the
entire video (or keep track of the framerate of the majority of the
video) and update muxer header information before finalizing the file.
Headers for many formats are not actually written till muxing is
complete anyway. But this would rarely result in a different value than
what we estimate up front.  We initially sample the source at various
locations to collect statistics about several properties of the source
prior to transcoding.  So our estimates are pretty accurate. Probably
more accurate that running the first few frames through the filter
chain.

I see your point about the timestamp mess.  But I think this case is (or
should be) a little different. There are no filters that directly use
this frame_rate information yet. It is informational and not used to
calculate timestamps. The only case I can think of where a filter might
want to use the input link's frame_rate is if the filter needs to insert
extra frames or duplicate frames *and* the filter is not required to
generate constant framerate output.  In which case it's going to need to
know *something* about the nominal input frame_rate in order to do it's
function. Such a filter would have to fail initialization if given an
unknown frame_rate.
Anton Khirnov Oct. 29, 2015, 8:31 p.m. | #5
Quoting John Stebbins (2015-10-29 21:17:35)
> On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
> > Quoting John Stebbins (2015-10-29 19:59:44)
> > > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > > > Quoting John Stebbins (2015-10-28 17:48:17)
> > > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > > 
> > > > > (cherry picked from ffmpeg commit
> > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > > ---
> > > > >  libavfilter/avfilter.c |  2 ++
> > > > >  libavfilter/avfilter.h | 12 ++++++++++++
> > > > >  2 files changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > > > index 64b2645..cd98d16 100644
> > > > > --- a/libavfilter/avfilter.c
> > > > > +++ b/libavfilter/avfilter.c
> > > > > @@ -195,6 +195,8 @@ int avfilter_config_links(AVFilterContext
> > > > > *filter)
> > > > >                          link->src->inputs[0]
> > > > > ->sample_aspect_ratio :
> > > > > (AVRational){1,1};
> > > > >  
> > > > >                  if (link->src->nb_inputs) {
> > > > > +                    if (!link->frame_rate.num && !link
> > > > > ->frame_rate.den)
> > > > > +                        link->frame_rate = link->src->inputs[0]
> > > > > ->frame_rate;
> > > > >                      if (!link->w)
> > > > >                          link->w = link->src->inputs[0]->w;
> > > > >                      if (!link->h)
> > > > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > > > index 9dbfeea..b7b97ce 100644
> > > > > --- a/libavfilter/avfilter.h
> > > > > +++ b/libavfilter/avfilter.h
> > > > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > > > >          AVLINK_STARTINIT,       ///< started, but incomplete
> > > > >          AVLINK_INIT             ///< complete
> > > > >      } init_state;
> > > > > +
> > > > > +    /**
> > > > > +     * Frame rate of the stream on the link, or 1/0 if unknown;
> > > > > +     * if left to 0/0, will be automatically be copied from the
> > > > > first input
> > > > > +     * of the source filter if it exists.
> > > > > +     *
> > > > > +     * Sources should set it to the best estimation of the real
> > > > > frame rate.
> > > > > +     * Filters should update it if necessary depending on their
> > > > > function.
> > > > > +     * Sinks can use it to set a default output frame rate.
> > > > > +     * It is similar to the avg_frame_rate field in AVStream.
> > > > > +     */
> > > > 
> > > > Please no! No guesses or estimates, we already have plenty of this
> > > > crap
> > > > in lavf and we want to get rid of it, not add more. VFR streams
> > > > with
> > > > no
> > > > such thing as "real frame rate" exist.
> > > > 
> > > > So IMO this should be:
> > > > - if AND ONLY if the stream is known to be of some specific
> > > > constant
> > > >   framerate, this field should be set to that number
> > > > - in ALL OTHER CASES, this field should be set to 0/0 (or whatever
> > > > other
> > > >   invalid value) and treated as VFR.
> > > 
> > > An actual framerate has to be written at the muxing stage by a
> > > transcoder such as HandBrake.  We can't know how the filter chain
> > > will
> > > affect our nominal framerate without passing the nominal value to
> > > the
> > > input of the filter chain and seeing what comes out the other side.
> > > So
> > > although nominal framerates are a PITA, they are a requirement we
> > > can't
> > > ignore.
> > > 
> > > Although HandBrake generates output that is technically variable
> > > framerate by default (i.e. vfr flags are set in h.264 headers), it
> > > is
> > > often the case that the measured framerate is constant throughout
> > > the
> > > stream.  There is just no way to know this in advance with certain
> > > source types (e.g. DVD).
> > > 
> > > We could document this as you prefer as a means of discouraging this
> > > kind of usage.  But this is a real world case that is going to
> > > happen.
> > 
> > If you really need some kind of a framerate, you could e.g. pass two
> > dummy frames through the filtergraph and check the pts difference on
> > the
> > output. Would that be a lot more work for you?
> > 
> > I recall you complaining about the lavf timestamps mess yourself. And
> > one of the main reasons it is so horrible is precisely that it somehow
> > makes up some guesses or estimates and then yet more guesses on top of
> > that until we have no clue what are real data and what is made up. I
> > would really like to avoid that in any new code, if reasonably
> > possible.
> > 
> 
> In theory, HandBrake could calculate the average framerate over the
> entire video (or keep track of the framerate of the majority of the
> video) and update muxer header information before finalizing the file.
> Headers for many formats are not actually written till muxing is
> complete anyway. But this would rarely result in a different value than
> what we estimate up front.  We initially sample the source at various
> locations to collect statistics about several properties of the source
> prior to transcoding.  So our estimates are pretty accurate. Probably
> more accurate that running the first few frames through the filter
> chain.

Well, that's an estimate of the _input_ framerate, while the point here
is getting the _output_ one, no?

> 
> I see your point about the timestamp mess.  But I think this case is (or
> should be) a little different. There are no filters that directly use
> this frame_rate information yet. It is informational and not used to
> calculate timestamps.

Of course there are no such filters, when we're just adding it. But by
making it unreliable from the ouset, we are preventing the existing of
any such filters in the future (or dooming them to be randomly
unreliable as well).

> The only case I can think of where a filter might
> want to use the input link's frame_rate is if the filter needs to insert
> extra frames or duplicate frames *and* the filter is not required to
> generate constant framerate output.  In which case it's going to need to
> know *something* about the nominal input frame_rate in order to do it's
> function. Such a filter would have to fail initialization if given an
> unknown frame_rate. 

IMO a clean failure (with a clearly stated error message) is much better
than random guessage resulting in unpredictable output.

If you are still convinced that it's useful to have this kind of a
maybe-framerate-but-not-always field, then I would very much prefer if
it's not called 'framerate', but something different that makes it clear
to the callers and the filter developers what it actually is. Perhaps
something like a 'timestamp multiplier'?
John Stebbins Oct. 29, 2015, 9:35 p.m. | #6
On Thu, 2015-10-29 at 21:31 +0100, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-29 21:17:35)
> > On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
> > > Quoting John Stebbins (2015-10-29 19:59:44)
> > > > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > > > > Quoting John Stebbins (2015-10-28 17:48:17)
> > > > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > > > 
> > > > > > (cherry picked from ffmpeg commit
> > > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > > > ---
> > > > > >  libavfilter/avfilter.c |  2 ++
> > > > > >  libavfilter/avfilter.h | 12 ++++++++++++
> > > > > >  2 files changed, 14 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > > > > index 64b2645..cd98d16 100644
> > > > > > --- a/libavfilter/avfilter.c
> > > > > > +++ b/libavfilter/avfilter.c
> > > > > > @@ -195,6 +195,8 @@ int
> > > > > > avfilter_config_links(AVFilterContext
> > > > > > *filter)
> > > > > >                          link->src->inputs[0]
> > > > > > ->sample_aspect_ratio :
> > > > > > (AVRational){1,1};
> > > > > >  
> > > > > >                  if (link->src->nb_inputs) {
> > > > > > +                    if (!link->frame_rate.num && !link
> > > > > > ->frame_rate.den)
> > > > > > +                        link->frame_rate = link->src
> > > > > > ->inputs[0]
> > > > > > ->frame_rate;
> > > > > >                      if (!link->w)
> > > > > >                          link->w = link->src->inputs[0]->w;
> > > > > >                      if (!link->h)
> > > > > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > > > > index 9dbfeea..b7b97ce 100644
> > > > > > --- a/libavfilter/avfilter.h
> > > > > > +++ b/libavfilter/avfilter.h
> > > > > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > > > > >          AVLINK_STARTINIT,       ///< started, but
> > > > > > incomplete
> > > > > >          AVLINK_INIT             ///< complete
> > > > > >      } init_state;
> > > > > > +
> > > > > > +    /**
> > > > > > +     * Frame rate of the stream on the link, or 1/0 if
> > > > > > unknown;
> > > > > > +     * if left to 0/0, will be automatically be copied from
> > > > > > the
> > > > > > first input
> > > > > > +     * of the source filter if it exists.
> > > > > > +     *
> > > > > > +     * Sources should set it to the best estimation of the
> > > > > > real
> > > > > > frame rate.
> > > > > > +     * Filters should update it if necessary depending on
> > > > > > their
> > > > > > function.
> > > > > > +     * Sinks can use it to set a default output frame rate.
> > > > > > +     * It is similar to the avg_frame_rate field in
> > > > > > AVStream.
> > > > > > +     */
> > > > > 
> > > > > Please no! No guesses or estimates, we already have plenty of
> > > > > this
> > > > > crap
> > > > > in lavf and we want to get rid of it, not add more. VFR
> > > > > streams
> > > > > with
> > > > > no
> > > > > such thing as "real frame rate" exist.
> > > > > 
> > > > > So IMO this should be:
> > > > > - if AND ONLY if the stream is known to be of some specific
> > > > > constant
> > > > >   framerate, this field should be set to that number
> > > > > - in ALL OTHER CASES, this field should be set to 0/0 (or
> > > > > whatever
> > > > > other
> > > > >   invalid value) and treated as VFR.
> > > > 
> > > > An actual framerate has to be written at the muxing stage by a
> > > > transcoder such as HandBrake.  We can't know how the filter
> > > > chain
> > > > will
> > > > affect our nominal framerate without passing the nominal value
> > > > to
> > > > the
> > > > input of the filter chain and seeing what comes out the other
> > > > side.
> > > > So
> > > > although nominal framerates are a PITA, they are a requirement
> > > > we
> > > > can't
> > > > ignore.
> > > > 
> > > > Although HandBrake generates output that is technically variable
> > > > framerate by default (i.e. vfr flags are set in h.264 headers),
> > > > it
> > > > is
> > > > often the case that the measured framerate is constant
> > > > throughout
> > > > the
> > > > stream.  There is just no way to know this in advance with
> > > > certain
> > > > source types (e.g. DVD).
> > > > 
> > > > We could document this as you prefer as a means of discouraging
> > > > this
> > > > kind of usage.  But this is a real world case that is going to
> > > > happen.
> > > 
> > > If you really need some kind of a framerate, you could e.g. pass
> > > two
> > > dummy frames through the filtergraph and check the pts difference
> > > on
> > > the
> > > output. Would that be a lot more work for you?
> > > 
> > > I recall you complaining about the lavf timestamps mess yourself.
> > > And
> > > one of the main reasons it is so horrible is precisely that it
> > > somehow
> > > makes up some guesses or estimates and then yet more guesses on
> > > top of
> > > that until we have no clue what are real data and what is made up.
> > > I
> > > would really like to avoid that in any new code, if reasonably
> > > possible.
> > > 
> > 
> > In theory, HandBrake could calculate the average framerate over the
> > entire video (or keep track of the framerate of the majority of the
> > video) and update muxer header information before finalizing the
> > file.
> > Headers for many formats are not actually written till muxing is
> > complete anyway. But this would rarely result in a different value
> > than
> > what we estimate up front.  We initially sample the source at
> > various
> > locations to collect statistics about several properties of the
> > source
> > prior to transcoding.  So our estimates are pretty accurate.
> > Probably
> > more accurate that running the first few frames through the filter
> > chain.
> 
> Well, that's an estimate of the _input_ framerate, while the point
> here
> is getting the _output_ one, no?
> 

Given what filters like setpts can do, I'm going to have to concede that
input framerate can be meaningless in the general case.  I hadn't really
looked closely at what this filter does.  I hadn't considered that some
filter in the middle of the filter chain could generate a completely
unknown framerate. HandBrake doesn't make use of such filters, but I
should probably be planning for the day that it does. The only way to
handle the output of such a filter is going to be to measure the
framerate at the output and delay writing the muxer headers till
sufficient frames have been processed to get a reliable framerate.  This
is probably not a bad idea.  Doing so however completely eliminates my
original need for this data at the output of the filter chain.  If I
can't reliably retrieve some nominal framerate value from the output of
the filter chain, it might as will not be there and this patch series is
pointless for me.  You could argue that it could be useful in a use case
where it is known that all video will be constant framerate. But such a
use case would never be able to use filters such as setpts, even when
setpts may be configured to generate constant framerate output.  So even
for this use case, this new field is of limited value.

So now I am wondering if this patch series should just be trashed all
together.  Because providing the frame_rate in this way is going to
provide a crutch for people to take shortcuts that only work for a
subset of use cases.  And they may not realize under which circumstances
it will fail to provide what they need.
Anton Khirnov Oct. 30, 2015, 7:59 a.m. | #7
Quoting John Stebbins (2015-10-29 22:35:24)
> On Thu, 2015-10-29 at 21:31 +0100, Anton Khirnov wrote:
> > Quoting John Stebbins (2015-10-29 21:17:35)
> > > On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
> > > > Quoting John Stebbins (2015-10-29 19:59:44)
> > > > > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > > > > > Quoting John Stebbins (2015-10-28 17:48:17)
> > > > > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > > > > 
> > > > > > > (cherry picked from ffmpeg commit
> > > > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > > > > ---
> > > > > > >  libavfilter/avfilter.c |  2 ++
> > > > > > >  libavfilter/avfilter.h | 12 ++++++++++++
> > > > > > >  2 files changed, 14 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > > > > > > index 64b2645..cd98d16 100644
> > > > > > > --- a/libavfilter/avfilter.c
> > > > > > > +++ b/libavfilter/avfilter.c
> > > > > > > @@ -195,6 +195,8 @@ int
> > > > > > > avfilter_config_links(AVFilterContext
> > > > > > > *filter)
> > > > > > >                          link->src->inputs[0]
> > > > > > > ->sample_aspect_ratio :
> > > > > > > (AVRational){1,1};
> > > > > > >  
> > > > > > >                  if (link->src->nb_inputs) {
> > > > > > > +                    if (!link->frame_rate.num && !link
> > > > > > > ->frame_rate.den)
> > > > > > > +                        link->frame_rate = link->src
> > > > > > > ->inputs[0]
> > > > > > > ->frame_rate;
> > > > > > >                      if (!link->w)
> > > > > > >                          link->w = link->src->inputs[0]->w;
> > > > > > >                      if (!link->h)
> > > > > > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > > > > > > index 9dbfeea..b7b97ce 100644
> > > > > > > --- a/libavfilter/avfilter.h
> > > > > > > +++ b/libavfilter/avfilter.h
> > > > > > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > > > > > >          AVLINK_STARTINIT,       ///< started, but
> > > > > > > incomplete
> > > > > > >          AVLINK_INIT             ///< complete
> > > > > > >      } init_state;
> > > > > > > +
> > > > > > > +    /**
> > > > > > > +     * Frame rate of the stream on the link, or 1/0 if
> > > > > > > unknown;
> > > > > > > +     * if left to 0/0, will be automatically be copied from
> > > > > > > the
> > > > > > > first input
> > > > > > > +     * of the source filter if it exists.
> > > > > > > +     *
> > > > > > > +     * Sources should set it to the best estimation of the
> > > > > > > real
> > > > > > > frame rate.
> > > > > > > +     * Filters should update it if necessary depending on
> > > > > > > their
> > > > > > > function.
> > > > > > > +     * Sinks can use it to set a default output frame rate.
> > > > > > > +     * It is similar to the avg_frame_rate field in
> > > > > > > AVStream.
> > > > > > > +     */
> > > > > > 
> > > > > > Please no! No guesses or estimates, we already have plenty of
> > > > > > this
> > > > > > crap
> > > > > > in lavf and we want to get rid of it, not add more. VFR
> > > > > > streams
> > > > > > with
> > > > > > no
> > > > > > such thing as "real frame rate" exist.
> > > > > > 
> > > > > > So IMO this should be:
> > > > > > - if AND ONLY if the stream is known to be of some specific
> > > > > > constant
> > > > > >   framerate, this field should be set to that number
> > > > > > - in ALL OTHER CASES, this field should be set to 0/0 (or
> > > > > > whatever
> > > > > > other
> > > > > >   invalid value) and treated as VFR.
> > > > > 
> > > > > An actual framerate has to be written at the muxing stage by a
> > > > > transcoder such as HandBrake.  We can't know how the filter
> > > > > chain
> > > > > will
> > > > > affect our nominal framerate without passing the nominal value
> > > > > to
> > > > > the
> > > > > input of the filter chain and seeing what comes out the other
> > > > > side.
> > > > > So
> > > > > although nominal framerates are a PITA, they are a requirement
> > > > > we
> > > > > can't
> > > > > ignore.
> > > > > 
> > > > > Although HandBrake generates output that is technically variable
> > > > > framerate by default (i.e. vfr flags are set in h.264 headers),
> > > > > it
> > > > > is
> > > > > often the case that the measured framerate is constant
> > > > > throughout
> > > > > the
> > > > > stream.  There is just no way to know this in advance with
> > > > > certain
> > > > > source types (e.g. DVD).
> > > > > 
> > > > > We could document this as you prefer as a means of discouraging
> > > > > this
> > > > > kind of usage.  But this is a real world case that is going to
> > > > > happen.
> > > > 
> > > > If you really need some kind of a framerate, you could e.g. pass
> > > > two
> > > > dummy frames through the filtergraph and check the pts difference
> > > > on
> > > > the
> > > > output. Would that be a lot more work for you?
> > > > 
> > > > I recall you complaining about the lavf timestamps mess yourself.
> > > > And
> > > > one of the main reasons it is so horrible is precisely that it
> > > > somehow
> > > > makes up some guesses or estimates and then yet more guesses on
> > > > top of
> > > > that until we have no clue what are real data and what is made up.
> > > > I
> > > > would really like to avoid that in any new code, if reasonably
> > > > possible.
> > > > 
> > > 
> > > In theory, HandBrake could calculate the average framerate over the
> > > entire video (or keep track of the framerate of the majority of the
> > > video) and update muxer header information before finalizing the
> > > file.
> > > Headers for many formats are not actually written till muxing is
> > > complete anyway. But this would rarely result in a different value
> > > than
> > > what we estimate up front.  We initially sample the source at
> > > various
> > > locations to collect statistics about several properties of the
> > > source
> > > prior to transcoding.  So our estimates are pretty accurate.
> > > Probably
> > > more accurate that running the first few frames through the filter
> > > chain.
> > 
> > Well, that's an estimate of the _input_ framerate, while the point
> > here
> > is getting the _output_ one, no?
> > 
> 
> Given what filters like setpts can do, I'm going to have to concede that
> input framerate can be meaningless in the general case.  I hadn't really
> looked closely at what this filter does.  I hadn't considered that some
> filter in the middle of the filter chain could generate a completely
> unknown framerate. HandBrake doesn't make use of such filters, but I
> should probably be planning for the day that it does.

It's not really limited to the setpts filter. Some webcams or streaming
sources will also give you truly VFR video.

> The only way to
> handle the output of such a filter is going to be to measure the
> framerate at the output and delay writing the muxer headers till
> sufficient frames have been processed to get a reliable framerate.  This
> is probably not a bad idea.  Doing so however completely eliminates my
> original need for this data at the output of the filter chain.  If I
> can't reliably retrieve some nominal framerate value from the output of
> the filter chain, it might as will not be there and this patch series is
> pointless for me.  You could argue that it could be useful in a use case
> where it is known that all video will be constant framerate. But such a
> use case would never be able to use filters such as setpts, even when
> setpts may be configured to generate constant framerate output.  So even
> for this use case, this new field is of limited value.
> 
> So now I am wondering if this patch series should just be trashed all
> together.  Because providing the frame_rate in this way is going to
> provide a crutch for people to take shortcuts that only work for a
> subset of use cases.  And they may not realize under which circumstances
> it will fail to provide what they need.

I still believe it is useful in certain cases. Yes, it won't give you a
number in all possible situations. But it will tell you when you can be
absolutely sure that your stream is of a given constant framerate.
And I think having a reliable number only sometimes is still more useful
than always having something that you can't rely on.
John Stebbins Oct. 30, 2015, 3:08 p.m. | #8
On Fri, 2015-10-30 at 08:59 +0100, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-29 22:35:24)
> > On Thu, 2015-10-29 at 21:31 +0100, Anton Khirnov wrote:
> > > Quoting John Stebbins (2015-10-29 21:17:35)
> > > > On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
> > > > > Quoting John Stebbins (2015-10-29 19:59:44)
> > > > > > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > > > > > > Quoting John Stebbins (2015-10-28 17:48:17)
> > > > > > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > > > > > 
> > > > > > > > (cherry picked from ffmpeg commit
> > > > > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > > > > > ---
> > > > > > > >  libavfilter/avfilter.c |  2 ++
> > > > > > > >  libavfilter/avfilter.h | 12 ++++++++++++
> > > > > > > >  2 files changed, 14 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/libavfilter/avfilter.c
> > > > > > > > b/libavfilter/avfilter.c
> > > > > > > > index 64b2645..cd98d16 100644
> > > > > > > > --- a/libavfilter/avfilter.c
> > > > > > > > +++ b/libavfilter/avfilter.c
> > > > > > > > @@ -195,6 +195,8 @@ int
> > > > > > > > avfilter_config_links(AVFilterContext
> > > > > > > > *filter)
> > > > > > > >                          link->src->inputs[0]
> > > > > > > > ->sample_aspect_ratio :
> > > > > > > > (AVRational){1,1};
> > > > > > > >  
> > > > > > > >                  if (link->src->nb_inputs) {
> > > > > > > > +                    if (!link->frame_rate.num && !link
> > > > > > > > ->frame_rate.den)
> > > > > > > > +                        link->frame_rate = link->src
> > > > > > > > ->inputs[0]
> > > > > > > > ->frame_rate;
> > > > > > > >                      if (!link->w)
> > > > > > > >                          link->w = link->src->inputs[0]
> > > > > > > > ->w;
> > > > > > > >                      if (!link->h)
> > > > > > > > diff --git a/libavfilter/avfilter.h
> > > > > > > > b/libavfilter/avfilter.h
> > > > > > > > index 9dbfeea..b7b97ce 100644
> > > > > > > > --- a/libavfilter/avfilter.h
> > > > > > > > +++ b/libavfilter/avfilter.h
> > > > > > > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > > > > > > >          AVLINK_STARTINIT,       ///< started, but
> > > > > > > > incomplete
> > > > > > > >          AVLINK_INIT             ///< complete
> > > > > > > >      } init_state;
> > > > > > > > +
> > > > > > > > +    /**
> > > > > > > > +     * Frame rate of the stream on the link, or 1/0 if
> > > > > > > > unknown;
> > > > > > > > +     * if left to 0/0, will be automatically be copied
> > > > > > > > from
> > > > > > > > the
> > > > > > > > first input
> > > > > > > > +     * of the source filter if it exists.
> > > > > > > > +     *
> > > > > > > > +     * Sources should set it to the best estimation of
> > > > > > > > the
> > > > > > > > real
> > > > > > > > frame rate.
> > > > > > > > +     * Filters should update it if necessary depending
> > > > > > > > on
> > > > > > > > their
> > > > > > > > function.
> > > > > > > > +     * Sinks can use it to set a default output frame
> > > > > > > > rate.
> > > > > > > > +     * It is similar to the avg_frame_rate field in
> > > > > > > > AVStream.
> > > > > > > > +     */
> > > > > > > 
> > > > > > > Please no! No guesses or estimates, we already have plenty
> > > > > > > of
> > > > > > > this
> > > > > > > crap
> > > > > > > in lavf and we want to get rid of it, not add more. VFR
> > > > > > > streams
> > > > > > > with
> > > > > > > no
> > > > > > > such thing as "real frame rate" exist.
> > > > > > > 
> > > > > > > So IMO this should be:
> > > > > > > - if AND ONLY if the stream is known to be of some
> > > > > > > specific
> > > > > > > constant
> > > > > > >   framerate, this field should be set to that number
> > > > > > > - in ALL OTHER CASES, this field should be set to 0/0 (or
> > > > > > > whatever
> > > > > > > other
> > > > > > >   invalid value) and treated as VFR.
> > > > > > 
> > > > > > An actual framerate has to be written at the muxing stage by
> > > > > > a
> > > > > > transcoder such as HandBrake.  We can't know how the filter
> > > > > > chain
> > > > > > will
> > > > > > affect our nominal framerate without passing the nominal
> > > > > > value
> > > > > > to
> > > > > > the
> > > > > > input of the filter chain and seeing what comes out the
> > > > > > other
> > > > > > side.
> > > > > > So
> > > > > > although nominal framerates are a PITA, they are a
> > > > > > requirement
> > > > > > we
> > > > > > can't
> > > > > > ignore.
> > > > > > 
> > > > > > Although HandBrake generates output that is technically
> > > > > > variable
> > > > > > framerate by default (i.e. vfr flags are set in h.264
> > > > > > headers),
> > > > > > it
> > > > > > is
> > > > > > often the case that the measured framerate is constant
> > > > > > throughout
> > > > > > the
> > > > > > stream.  There is just no way to know this in advance with
> > > > > > certain
> > > > > > source types (e.g. DVD).
> > > > > > 
> > > > > > We could document this as you prefer as a means of
> > > > > > discouraging
> > > > > > this
> > > > > > kind of usage.  But this is a real world case that is going
> > > > > > to
> > > > > > happen.
> > > > > 
> > > > > If you really need some kind of a framerate, you could e.g.
> > > > > pass
> > > > > two
> > > > > dummy frames through the filtergraph and check the pts
> > > > > difference
> > > > > on
> > > > > the
> > > > > output. Would that be a lot more work for you?
> > > > > 
> > > > > I recall you complaining about the lavf timestamps mess
> > > > > yourself.
> > > > > And
> > > > > one of the main reasons it is so horrible is precisely that it
> > > > > somehow
> > > > > makes up some guesses or estimates and then yet more guesses
> > > > > on
> > > > > top of
> > > > > that until we have no clue what are real data and what is made
> > > > > up.
> > > > > I
> > > > > would really like to avoid that in any new code, if reasonably
> > > > > possible.
> > > > > 
> > > > 
> > > > In theory, HandBrake could calculate the average framerate over
> > > > the
> > > > entire video (or keep track of the framerate of the majority of
> > > > the
> > > > video) and update muxer header information before finalizing the
> > > > file.
> > > > Headers for many formats are not actually written till muxing is
> > > > complete anyway. But this would rarely result in a different
> > > > value
> > > > than
> > > > what we estimate up front.  We initially sample the source at
> > > > various
> > > > locations to collect statistics about several properties of the
> > > > source
> > > > prior to transcoding.  So our estimates are pretty accurate.
> > > > Probably
> > > > more accurate that running the first few frames through the
> > > > filter
> > > > chain.
> > > 
> > > Well, that's an estimate of the _input_ framerate, while the point
> > > here
> > > is getting the _output_ one, no?
> > > 
> > 
> > Given what filters like setpts can do, I'm going to have to concede
> > that
> > input framerate can be meaningless in the general case.  I hadn't
> > really
> > looked closely at what this filter does.  I hadn't considered that
> > some
> > filter in the middle of the filter chain could generate a completely
> > unknown framerate. HandBrake doesn't make use of such filters, but I
> > should probably be planning for the day that it does.
> 
> It's not really limited to the setpts filter. Some webcams or
> streaming
> sources will also give you truly VFR video.
> 
> > The only way to
> > handle the output of such a filter is going to be to measure the
> > framerate at the output and delay writing the muxer headers till
> > sufficient frames have been processed to get a reliable framerate. 
> >  This
> > is probably not a bad idea.  Doing so however completely eliminates
> > my
> > original need for this data at the output of the filter chain.  If I
> > can't reliably retrieve some nominal framerate value from the output
> > of
> > the filter chain, it might as will not be there and this patch
> > series is
> > pointless for me.  You could argue that it could be useful in a use
> > case
> > where it is known that all video will be constant framerate. But
> > such a
> > use case would never be able to use filters such as setpts, even
> > when
> > setpts may be configured to generate constant framerate output.  So
> > even
> > for this use case, this new field is of limited value.
> > 
> > So now I am wondering if this patch series should just be trashed
> > all
> > together.  Because providing the frame_rate in this way is going to
> > provide a crutch for people to take shortcuts that only work for a
> > subset of use cases.  And they may not realize under which
> > circumstances
> > it will fail to provide what they need.
> 
> I still believe it is useful in certain cases. Yes, it won't give you
> a
> number in all possible situations. But it will tell you when you can
> be
> absolutely sure that your stream is of a given constant framerate.
> And I think having a reliable number only sometimes is still more
> useful
> than always having something that you can't rely on.
> 

What would you do for sources that are *potentially* VFR (i.e. you don't
know without checking every frame)?  Every codec that has repeat field
flags is potentially VFR, even when the codec flags all say CFR.  When
video uses repeat field flags, it often has sequences where it also
*does not* use repeat field flags.  So you end up with a mix of, for
example, 29.97fps and 23.976fps.  This is very common with TV transfered
to DVD where the original source material can be a mix of film, video,
or CGI. There is no way to know in advance whether this is going to
happen.  So if you want to only allow 100% accurate frame_rate, you
would have to set it to unknown for any source codec that supports
repeat field flags (mpeg12, h.264, vc1, etc).

I ask this, because this is really what I am talking about when I say
HandBrake generates VFR output by default.
Anton Khirnov Nov. 1, 2015, 7:29 a.m. | #9
Quoting John Stebbins (2015-10-30 16:08:42)
> On Fri, 2015-10-30 at 08:59 +0100, Anton Khirnov wrote:
> > Quoting John Stebbins (2015-10-29 22:35:24)
> > > On Thu, 2015-10-29 at 21:31 +0100, Anton Khirnov wrote:
> > > > Quoting John Stebbins (2015-10-29 21:17:35)
> > > > > On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
> > > > > > Quoting John Stebbins (2015-10-29 19:59:44)
> > > > > > > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
> > > > > > > > Quoting John Stebbins (2015-10-28 17:48:17)
> > > > > > > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > > > > > > 
> > > > > > > > > (cherry picked from ffmpeg commit
> > > > > > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > > > > > > ---
> > > > > > > > >  libavfilter/avfilter.c |  2 ++
> > > > > > > > >  libavfilter/avfilter.h | 12 ++++++++++++
> > > > > > > > >  2 files changed, 14 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/libavfilter/avfilter.c
> > > > > > > > > b/libavfilter/avfilter.c
> > > > > > > > > index 64b2645..cd98d16 100644
> > > > > > > > > --- a/libavfilter/avfilter.c
> > > > > > > > > +++ b/libavfilter/avfilter.c
> > > > > > > > > @@ -195,6 +195,8 @@ int
> > > > > > > > > avfilter_config_links(AVFilterContext
> > > > > > > > > *filter)
> > > > > > > > >                          link->src->inputs[0]
> > > > > > > > > ->sample_aspect_ratio :
> > > > > > > > > (AVRational){1,1};
> > > > > > > > >  
> > > > > > > > >                  if (link->src->nb_inputs) {
> > > > > > > > > +                    if (!link->frame_rate.num && !link
> > > > > > > > > ->frame_rate.den)
> > > > > > > > > +                        link->frame_rate = link->src
> > > > > > > > > ->inputs[0]
> > > > > > > > > ->frame_rate;
> > > > > > > > >                      if (!link->w)
> > > > > > > > >                          link->w = link->src->inputs[0]
> > > > > > > > > ->w;
> > > > > > > > >                      if (!link->h)
> > > > > > > > > diff --git a/libavfilter/avfilter.h
> > > > > > > > > b/libavfilter/avfilter.h
> > > > > > > > > index 9dbfeea..b7b97ce 100644
> > > > > > > > > --- a/libavfilter/avfilter.h
> > > > > > > > > +++ b/libavfilter/avfilter.h
> > > > > > > > > @@ -375,6 +375,18 @@ struct AVFilterLink {
> > > > > > > > >          AVLINK_STARTINIT,       ///< started, but
> > > > > > > > > incomplete
> > > > > > > > >          AVLINK_INIT             ///< complete
> > > > > > > > >      } init_state;
> > > > > > > > > +
> > > > > > > > > +    /**
> > > > > > > > > +     * Frame rate of the stream on the link, or 1/0 if
> > > > > > > > > unknown;
> > > > > > > > > +     * if left to 0/0, will be automatically be copied
> > > > > > > > > from
> > > > > > > > > the
> > > > > > > > > first input
> > > > > > > > > +     * of the source filter if it exists.
> > > > > > > > > +     *
> > > > > > > > > +     * Sources should set it to the best estimation of
> > > > > > > > > the
> > > > > > > > > real
> > > > > > > > > frame rate.
> > > > > > > > > +     * Filters should update it if necessary depending
> > > > > > > > > on
> > > > > > > > > their
> > > > > > > > > function.
> > > > > > > > > +     * Sinks can use it to set a default output frame
> > > > > > > > > rate.
> > > > > > > > > +     * It is similar to the avg_frame_rate field in
> > > > > > > > > AVStream.
> > > > > > > > > +     */
> > > > > > > > 
> > > > > > > > Please no! No guesses or estimates, we already have plenty
> > > > > > > > of
> > > > > > > > this
> > > > > > > > crap
> > > > > > > > in lavf and we want to get rid of it, not add more. VFR
> > > > > > > > streams
> > > > > > > > with
> > > > > > > > no
> > > > > > > > such thing as "real frame rate" exist.
> > > > > > > > 
> > > > > > > > So IMO this should be:
> > > > > > > > - if AND ONLY if the stream is known to be of some
> > > > > > > > specific
> > > > > > > > constant
> > > > > > > >   framerate, this field should be set to that number
> > > > > > > > - in ALL OTHER CASES, this field should be set to 0/0 (or
> > > > > > > > whatever
> > > > > > > > other
> > > > > > > >   invalid value) and treated as VFR.
> > > > > > > 
> > > > > > > An actual framerate has to be written at the muxing stage by
> > > > > > > a
> > > > > > > transcoder such as HandBrake.  We can't know how the filter
> > > > > > > chain
> > > > > > > will
> > > > > > > affect our nominal framerate without passing the nominal
> > > > > > > value
> > > > > > > to
> > > > > > > the
> > > > > > > input of the filter chain and seeing what comes out the
> > > > > > > other
> > > > > > > side.
> > > > > > > So
> > > > > > > although nominal framerates are a PITA, they are a
> > > > > > > requirement
> > > > > > > we
> > > > > > > can't
> > > > > > > ignore.
> > > > > > > 
> > > > > > > Although HandBrake generates output that is technically
> > > > > > > variable
> > > > > > > framerate by default (i.e. vfr flags are set in h.264
> > > > > > > headers),
> > > > > > > it
> > > > > > > is
> > > > > > > often the case that the measured framerate is constant
> > > > > > > throughout
> > > > > > > the
> > > > > > > stream.  There is just no way to know this in advance with
> > > > > > > certain
> > > > > > > source types (e.g. DVD).
> > > > > > > 
> > > > > > > We could document this as you prefer as a means of
> > > > > > > discouraging
> > > > > > > this
> > > > > > > kind of usage.  But this is a real world case that is going
> > > > > > > to
> > > > > > > happen.
> > > > > > 
> > > > > > If you really need some kind of a framerate, you could e.g.
> > > > > > pass
> > > > > > two
> > > > > > dummy frames through the filtergraph and check the pts
> > > > > > difference
> > > > > > on
> > > > > > the
> > > > > > output. Would that be a lot more work for you?
> > > > > > 
> > > > > > I recall you complaining about the lavf timestamps mess
> > > > > > yourself.
> > > > > > And
> > > > > > one of the main reasons it is so horrible is precisely that it
> > > > > > somehow
> > > > > > makes up some guesses or estimates and then yet more guesses
> > > > > > on
> > > > > > top of
> > > > > > that until we have no clue what are real data and what is made
> > > > > > up.
> > > > > > I
> > > > > > would really like to avoid that in any new code, if reasonably
> > > > > > possible.
> > > > > > 
> > > > > 
> > > > > In theory, HandBrake could calculate the average framerate over
> > > > > the
> > > > > entire video (or keep track of the framerate of the majority of
> > > > > the
> > > > > video) and update muxer header information before finalizing the
> > > > > file.
> > > > > Headers for many formats are not actually written till muxing is
> > > > > complete anyway. But this would rarely result in a different
> > > > > value
> > > > > than
> > > > > what we estimate up front.  We initially sample the source at
> > > > > various
> > > > > locations to collect statistics about several properties of the
> > > > > source
> > > > > prior to transcoding.  So our estimates are pretty accurate.
> > > > > Probably
> > > > > more accurate that running the first few frames through the
> > > > > filter
> > > > > chain.
> > > > 
> > > > Well, that's an estimate of the _input_ framerate, while the point
> > > > here
> > > > is getting the _output_ one, no?
> > > > 
> > > 
> > > Given what filters like setpts can do, I'm going to have to concede
> > > that
> > > input framerate can be meaningless in the general case.  I hadn't
> > > really
> > > looked closely at what this filter does.  I hadn't considered that
> > > some
> > > filter in the middle of the filter chain could generate a completely
> > > unknown framerate. HandBrake doesn't make use of such filters, but I
> > > should probably be planning for the day that it does.
> > 
> > It's not really limited to the setpts filter. Some webcams or
> > streaming
> > sources will also give you truly VFR video.
> > 
> > > The only way to
> > > handle the output of such a filter is going to be to measure the
> > > framerate at the output and delay writing the muxer headers till
> > > sufficient frames have been processed to get a reliable framerate. 
> > >  This
> > > is probably not a bad idea.  Doing so however completely eliminates
> > > my
> > > original need for this data at the output of the filter chain.  If I
> > > can't reliably retrieve some nominal framerate value from the output
> > > of
> > > the filter chain, it might as will not be there and this patch
> > > series is
> > > pointless for me.  You could argue that it could be useful in a use
> > > case
> > > where it is known that all video will be constant framerate. But
> > > such a
> > > use case would never be able to use filters such as setpts, even
> > > when
> > > setpts may be configured to generate constant framerate output.  So
> > > even
> > > for this use case, this new field is of limited value.
> > > 
> > > So now I am wondering if this patch series should just be trashed
> > > all
> > > together.  Because providing the frame_rate in this way is going to
> > > provide a crutch for people to take shortcuts that only work for a
> > > subset of use cases.  And they may not realize under which
> > > circumstances
> > > it will fail to provide what they need.
> > 
> > I still believe it is useful in certain cases. Yes, it won't give you
> > a
> > number in all possible situations. But it will tell you when you can
> > be
> > absolutely sure that your stream is of a given constant framerate.
> > And I think having a reliable number only sometimes is still more
> > useful
> > than always having something that you can't rely on.
> > 
> 
> What would you do for sources that are *potentially* VFR (i.e. you don't
> know without checking every frame)?  Every codec that has repeat field
> flags is potentially VFR, even when the codec flags all say CFR.  When
> video uses repeat field flags, it often has sequences where it also
> *does not* use repeat field flags.  So you end up with a mix of, for
> example, 29.97fps and 23.976fps.  This is very common with TV transfered
> to DVD where the original source material can be a mix of film, video,
> or CGI. There is no way to know in advance whether this is going to
> happen.  So if you want to only allow 100% accurate frame_rate, you
> would have to set it to unknown for any source codec that supports
> repeat field flags (mpeg12, h.264, vc1, etc).
> 
> I ask this, because this is really what I am talking about when I say
> HandBrake generates VFR output by default.

If you don't know for sure, you don't set it. If you want to be sure,
you have to do two-pass decoding.
John Stebbins Nov. 1, 2015, 3:07 p.m. | #10
On 11/01/2015 12:29 AM, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-30 16:08:42)
>> On Fri, 2015-10-30 at 08:59 +0100, Anton Khirnov wrote:
>>> Quoting John Stebbins (2015-10-29 22:35:24)
>>>> On Thu, 2015-10-29 at 21:31 +0100, Anton Khirnov wrote:
>>>>> Quoting John Stebbins (2015-10-29 21:17:35)
>>>>>> On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote:
>>>>>>> Quoting John Stebbins (2015-10-29 19:59:44)
>>>>>>>> On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote:
>>>>>>>>> Quoting John Stebbins (2015-10-28 17:48:17)
>>>>>>>>>> From: Nicolas George <nicolas.george@normalesup.org>
>>>>>>>>>>
>>>>>>>>>> (cherry picked from ffmpeg commit
>>>>>>>>>> 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
>>>>>>>>>> ---
>>>>>>>>>>  libavfilter/avfilter.c |  2 ++
>>>>>>>>>>  libavfilter/avfilter.h | 12 ++++++++++++
>>>>>>>>>>  2 files changed, 14 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/libavfilter/avfilter.c
>>>>>>>>>> b/libavfilter/avfilter.c
>>>>>>>>>> index 64b2645..cd98d16 100644
>>>>>>>>>> --- a/libavfilter/avfilter.c
>>>>>>>>>> +++ b/libavfilter/avfilter.c
>>>>>>>>>> @@ -195,6 +195,8 @@ int
>>>>>>>>>> avfilter_config_links(AVFilterContext
>>>>>>>>>> *filter)
>>>>>>>>>>                          link->src->inputs[0]
>>>>>>>>>> ->sample_aspect_ratio :
>>>>>>>>>> (AVRational){1,1};
>>>>>>>>>>  
>>>>>>>>>>                  if (link->src->nb_inputs) {
>>>>>>>>>> +                    if (!link->frame_rate.num && !link
>>>>>>>>>> ->frame_rate.den)
>>>>>>>>>> +                        link->frame_rate = link->src
>>>>>>>>>> ->inputs[0]
>>>>>>>>>> ->frame_rate;
>>>>>>>>>>                      if (!link->w)
>>>>>>>>>>                          link->w = link->src->inputs[0]
>>>>>>>>>> ->w;
>>>>>>>>>>                      if (!link->h)
>>>>>>>>>> diff --git a/libavfilter/avfilter.h
>>>>>>>>>> b/libavfilter/avfilter.h
>>>>>>>>>> index 9dbfeea..b7b97ce 100644
>>>>>>>>>> --- a/libavfilter/avfilter.h
>>>>>>>>>> +++ b/libavfilter/avfilter.h
>>>>>>>>>> @@ -375,6 +375,18 @@ struct AVFilterLink {
>>>>>>>>>>          AVLINK_STARTINIT,       ///< started, but
>>>>>>>>>> incomplete
>>>>>>>>>>          AVLINK_INIT             ///< complete
>>>>>>>>>>      } init_state;
>>>>>>>>>> +
>>>>>>>>>> +    /**
>>>>>>>>>> +     * Frame rate of the stream on the link, or 1/0 if
>>>>>>>>>> unknown;
>>>>>>>>>> +     * if left to 0/0, will be automatically be copied
>>>>>>>>>> from
>>>>>>>>>> the
>>>>>>>>>> first input
>>>>>>>>>> +     * of the source filter if it exists.
>>>>>>>>>> +     *
>>>>>>>>>> +     * Sources should set it to the best estimation of
>>>>>>>>>> the
>>>>>>>>>> real
>>>>>>>>>> frame rate.
>>>>>>>>>> +     * Filters should update it if necessary depending
>>>>>>>>>> on
>>>>>>>>>> their
>>>>>>>>>> function.
>>>>>>>>>> +     * Sinks can use it to set a default output frame
>>>>>>>>>> rate.
>>>>>>>>>> +     * It is similar to the avg_frame_rate field in
>>>>>>>>>> AVStream.
>>>>>>>>>> +     */
>>>>>>>>> Please no! No guesses or estimates, we already have plenty
>>>>>>>>> of
>>>>>>>>> this
>>>>>>>>> crap
>>>>>>>>> in lavf and we want to get rid of it, not add more. VFR
>>>>>>>>> streams
>>>>>>>>> with
>>>>>>>>> no
>>>>>>>>> such thing as "real frame rate" exist.
>>>>>>>>>
>>>>>>>>> So IMO this should be:
>>>>>>>>> - if AND ONLY if the stream is known to be of some
>>>>>>>>> specific
>>>>>>>>> constant
>>>>>>>>>   framerate, this field should be set to that number
>>>>>>>>> - in ALL OTHER CASES, this field should be set to 0/0 (or
>>>>>>>>> whatever
>>>>>>>>> other
>>>>>>>>>   invalid value) and treated as VFR.
>>>>>>>> An actual framerate has to be written at the muxing stage by
>>>>>>>> a
>>>>>>>> transcoder such as HandBrake.  We can't know how the filter
>>>>>>>> chain
>>>>>>>> will
>>>>>>>> affect our nominal framerate without passing the nominal
>>>>>>>> value
>>>>>>>> to
>>>>>>>> the
>>>>>>>> input of the filter chain and seeing what comes out the
>>>>>>>> other
>>>>>>>> side.
>>>>>>>> So
>>>>>>>> although nominal framerates are a PITA, they are a
>>>>>>>> requirement
>>>>>>>> we
>>>>>>>> can't
>>>>>>>> ignore.
>>>>>>>>
>>>>>>>> Although HandBrake generates output that is technically
>>>>>>>> variable
>>>>>>>> framerate by default (i.e. vfr flags are set in h.264
>>>>>>>> headers),
>>>>>>>> it
>>>>>>>> is
>>>>>>>> often the case that the measured framerate is constant
>>>>>>>> throughout
>>>>>>>> the
>>>>>>>> stream.  There is just no way to know this in advance with
>>>>>>>> certain
>>>>>>>> source types (e.g. DVD).
>>>>>>>>
>>>>>>>> We could document this as you prefer as a means of
>>>>>>>> discouraging
>>>>>>>> this
>>>>>>>> kind of usage.  But this is a real world case that is going
>>>>>>>> to
>>>>>>>> happen.
>>>>>>> If you really need some kind of a framerate, you could e.g.
>>>>>>> pass
>>>>>>> two
>>>>>>> dummy frames through the filtergraph and check the pts
>>>>>>> difference
>>>>>>> on
>>>>>>> the
>>>>>>> output. Would that be a lot more work for you?
>>>>>>>
>>>>>>> I recall you complaining about the lavf timestamps mess
>>>>>>> yourself.
>>>>>>> And
>>>>>>> one of the main reasons it is so horrible is precisely that it
>>>>>>> somehow
>>>>>>> makes up some guesses or estimates and then yet more guesses
>>>>>>> on
>>>>>>> top of
>>>>>>> that until we have no clue what are real data and what is made
>>>>>>> up.
>>>>>>> I
>>>>>>> would really like to avoid that in any new code, if reasonably
>>>>>>> possible.
>>>>>>>
>>>>>> In theory, HandBrake could calculate the average framerate over
>>>>>> the
>>>>>> entire video (or keep track of the framerate of the majority of
>>>>>> the
>>>>>> video) and update muxer header information before finalizing the
>>>>>> file.
>>>>>> Headers for many formats are not actually written till muxing is
>>>>>> complete anyway. But this would rarely result in a different
>>>>>> value
>>>>>> than
>>>>>> what we estimate up front.  We initially sample the source at
>>>>>> various
>>>>>> locations to collect statistics about several properties of the
>>>>>> source
>>>>>> prior to transcoding.  So our estimates are pretty accurate.
>>>>>> Probably
>>>>>> more accurate that running the first few frames through the
>>>>>> filter
>>>>>> chain.
>>>>> Well, that's an estimate of the _input_ framerate, while the point
>>>>> here
>>>>> is getting the _output_ one, no?
>>>>>
>>>> Given what filters like setpts can do, I'm going to have to concede
>>>> that
>>>> input framerate can be meaningless in the general case.  I hadn't
>>>> really
>>>> looked closely at what this filter does.  I hadn't considered that
>>>> some
>>>> filter in the middle of the filter chain could generate a completely
>>>> unknown framerate. HandBrake doesn't make use of such filters, but I
>>>> should probably be planning for the day that it does.
>>> It's not really limited to the setpts filter. Some webcams or
>>> streaming
>>> sources will also give you truly VFR video.
>>>
>>>> The only way to
>>>> handle the output of such a filter is going to be to measure the
>>>> framerate at the output and delay writing the muxer headers till
>>>> sufficient frames have been processed to get a reliable framerate. 
>>>>  This
>>>> is probably not a bad idea.  Doing so however completely eliminates
>>>> my
>>>> original need for this data at the output of the filter chain.  If I
>>>> can't reliably retrieve some nominal framerate value from the output
>>>> of
>>>> the filter chain, it might as will not be there and this patch
>>>> series is
>>>> pointless for me.  You could argue that it could be useful in a use
>>>> case
>>>> where it is known that all video will be constant framerate. But
>>>> such a
>>>> use case would never be able to use filters such as setpts, even
>>>> when
>>>> setpts may be configured to generate constant framerate output.  So
>>>> even
>>>> for this use case, this new field is of limited value.
>>>>
>>>> So now I am wondering if this patch series should just be trashed
>>>> all
>>>> together.  Because providing the frame_rate in this way is going to
>>>> provide a crutch for people to take shortcuts that only work for a
>>>> subset of use cases.  And they may not realize under which
>>>> circumstances
>>>> it will fail to provide what they need.
>>> I still believe it is useful in certain cases. Yes, it won't give you
>>> a
>>> number in all possible situations. But it will tell you when you can
>>> be
>>> absolutely sure that your stream is of a given constant framerate.
>>> And I think having a reliable number only sometimes is still more
>>> useful
>>> than always having something that you can't rely on.
>>>
>> What would you do for sources that are *potentially* VFR (i.e. you don't
>> know without checking every frame)?  Every codec that has repeat field
>> flags is potentially VFR, even when the codec flags all say CFR.  When
>> video uses repeat field flags, it often has sequences where it also
>> *does not* use repeat field flags.  So you end up with a mix of, for
>> example, 29.97fps and 23.976fps.  This is very common with TV transfered
>> to DVD where the original source material can be a mix of film, video,
>> or CGI. There is no way to know in advance whether this is going to
>> happen.  So if you want to only allow 100% accurate frame_rate, you
>> would have to set it to unknown for any source codec that supports
>> repeat field flags (mpeg12, h.264, vc1, etc).
>>
>> I ask this, because this is really what I am talking about when I say
>> HandBrake generates VFR output by default.
> If you don't know for sure, you don't set it. If you want to be sure,
> you have to do two-pass decoding.
>

That's what I thought you might say.  Which is why I coded up an experimental extra pass to do just this in HandBrake
yesterday ;) Since the avfilter chain isn't guaranteed to generate a valid framerate, even in the case I feed it a valid
one in buffersrc, my extra pass decodes and filters, with a special filter at the end of the chain that does framerate
analysis. I use the framerate analysis to set my final framerate that I must give to encoders and muxer (these must be
given some nominal value, even when framerate is variable).

I've got some updates to the patches to address your comments.  I'll post those to the mailing list tomorrow.

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index 64b2645..cd98d16 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -195,6 +195,8 @@  int avfilter_config_links(AVFilterContext *filter)
                         link->src->inputs[0]->sample_aspect_ratio : (AVRational){1,1};
 
                 if (link->src->nb_inputs) {
+                    if (!link->frame_rate.num && !link->frame_rate.den)
+                        link->frame_rate = link->src->inputs[0]->frame_rate;
                     if (!link->w)
                         link->w = link->src->inputs[0]->w;
                     if (!link->h)
diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
index 9dbfeea..b7b97ce 100644
--- a/libavfilter/avfilter.h
+++ b/libavfilter/avfilter.h
@@ -375,6 +375,18 @@  struct AVFilterLink {
         AVLINK_STARTINIT,       ///< started, but incomplete
         AVLINK_INIT             ///< complete
     } init_state;
+
+    /**
+     * Frame rate of the stream on the link, or 1/0 if unknown;
+     * if left to 0/0, will be automatically be copied from the first input
+     * of the source filter if it exists.
+     *
+     * Sources should set it to the best estimation of the real frame rate.
+     * Filters should update it if necessary depending on their function.
+     * Sinks can use it to set a default output frame rate.
+     * It is similar to the avg_frame_rate field in AVStream.
+     */
+    AVRational frame_rate;
 };
 
 /**