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

Message ID 1445977054-15625-2-git-send-email-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Oct. 27, 2015, 8:17 p.m.
From: Nicolas George <nicolas.george@normalesup.org>

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

Comments

Timothy Gu Oct. 27, 2015, 8:23 p.m. | #1
Just so that nobody misses this:

On Tue, Oct 27, 2015 at 1:18 PM John Stebbins <stebbins@jetheaddev.com>
wrote:

> +     * It is similar to the r_frae_rate field in AVStream.
>

r_frame_rate

[...]

Timothy
John Stebbins Oct. 27, 2015, 8:39 p.m. | #2

Vittorio Giovara Oct. 28, 2015, 1:51 a.m. | #3
On Tue, Oct 27, 2015 at 9:17 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
> From: Nicolas George <nicolas.george@normalesup.org>
>
> (cherry picked from commit 7b42036b3b23c85f473bf9369e37fa8da22eaf93)

(this comment is valid for the next patches too) there is no such hash
in the libav tree, you should either mention its a ffmpeg hash or just
remove it since the commit name is unchanged.

> ---
>  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..0b670e0 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;

would 0/1 be safer? also where is it initialized?

> +     * 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 r_frae_rate field in AVStream.

that field has been absent for a long time, I suppose it refers to
avg_frame_rate now

> +     */
> +    AVRational frame_rate;
>  };

question, would it be simpler or is it possible at all to avoid adding
a new field and use time_base to calculate framerate when needed
instead?

thanks
John Stebbins Oct. 28, 2015, 4:24 a.m. | #4
On 10/27/2015 07:51 PM, Vittorio Giovara wrote:
> On Tue, Oct 27, 2015 at 9:17 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>> From: Nicolas George <nicolas.george@normalesup.org>
>>
>> (cherry picked from commit 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> (this comment is valid for the next patches too) there is no such hash
> in the libav tree, you should either mention its a ffmpeg hash or just
> remove it since the commit name is unchanged.

I can amend the comments, but most people know where these are being 
cherry picked from

>> ---
>>   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..0b670e0 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;
> would 0/1 be safer? also where is it initialized?

This comment seems to be misleading. Any filter that
doesn't explicitly override frame_rate passes input frame_rate along
to the next stage in the filter chain. If it's not set in buffersrc, it's
value is 0/0 and that just gets passed through the chain (until some
filter sets the frame_rate or it reaches buffersink).

IMO the whole initial bit about "unknown" value should just
be omitted.

>> +     * 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 r_frae_rate field in AVStream.
> that field has been absent for a long time, I suppose it refers to
> avg_frame_rate now

It's an old comment.  This code was introduced in 2012.

>> +     */
>> +    AVRational frame_rate;
>>   };
> question, would it be simpler or is it possible at all to avoid adding
> a new field and use time_base to calculate framerate when needed
> instead?
>
> thanks

No.  time_base != 1 / frame_rate.  For example, the time_base in 
HandBrake is 1/90000.
This is what we initialize the time_base to in buffersrc.  You need a 
time_base such as
this to represent variable frame rate timestamps for frames.  The filter 
chain can modify
the time_base to make it == 1 / frame_rate in special cases (e.g. 
vf_fps), but in general
they are completely different things.
Luca Barbato Oct. 28, 2015, 10:31 a.m. | #5
On 28/10/15 05:24, John Stebbins wrote:
> On 10/27/2015 07:51 PM, Vittorio Giovara wrote:
>> On Tue, Oct 27, 2015 at 9:17 PM, John Stebbins
>> <stebbins@jetheaddev.com> wrote:
>>> From: Nicolas George <nicolas.george@normalesup.org>
>>>
>>> (cherry picked from commit 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
>> (this comment is valid for the next patches too) there is no such hash
>> in the libav tree, you should either mention its a ffmpeg hash or just
>> remove it since the commit name is unchanged.
> 
> I can amend the comments, but most people know where these are being
> cherry picked from
> 
>>> ---
>>>   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..0b670e0 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;
>> would 0/1 be safer? also where is it initialized?
> 
> This comment seems to be misleading. Any filter that
> doesn't explicitly override frame_rate passes input frame_rate along
> to the next stage in the filter chain. If it's not set in buffersrc, it's
> value is 0/0 and that just gets passed through the chain (until some
> filter sets the frame_rate or it reaches buffersink).
> 
> IMO the whole initial bit about "unknown" value should just
> be omitted.

Since the set got scattered I couldn't follow and I assumed that would
be implemented as documented.

If is not please get an updated set and then lets think again what that
should be.

Ideally makes sense to build a filter chain that preserves the frame
rate and forwards it if it is known (since then some rate controls could
use it), but also would be beneficial forward the information that the
data doesn't have a frame rate, even a nominal one.

lu
Vittorio Giovara Oct. 28, 2015, 11:05 a.m. | #6
On Wed, Oct 28, 2015 at 5:24 AM, John Stebbins <stebbins@jetheaddev.com> wrote:
> On 10/27/2015 07:51 PM, Vittorio Giovara wrote:
>>
>> On Tue, Oct 27, 2015 at 9:17 PM, John Stebbins <stebbins@jetheaddev.com>
>> wrote:
>>>
>>> From: Nicolas George <nicolas.george@normalesup.org>
>>>
>>> (cherry picked from commit 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
>>
>> (this comment is valid for the next patches too) there is no such hash
>> in the libav tree, you should either mention its a ffmpeg hash or just
>> remove it since the commit name is unchanged.
>
>
> I can amend the comments, but most people know where these are being cherry
> picked from

sure but we're going to edit this patch, so the hash will be even more
meanlingless

>
>>> ---
>>>   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..0b670e0 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;
>>
>> would 0/1 be safer? also where is it initialized?
>
>
> This comment seems to be misleading. Any filter that
> doesn't explicitly override frame_rate passes input frame_rate along
> to the next stage in the filter chain. If it's not set in buffersrc, it's
> value is 0/0 and that just gets passed through the chain (until some
> filter sets the frame_rate or it reaches buffersink).
>
> IMO the whole initial bit about "unknown" value should just
> be omitted.

sure

>>> +     * 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 r_frae_rate field in AVStream.
>>
>> that field has been absent for a long time, I suppose it refers to
>> avg_frame_rate now
>
>
> It's an old comment.  This code was introduced in 2012.

yes, it doesn't make sense to commit code and then apply patch on top
it for typos and whatnot -- let's modify the patch directly and avoid
commit history noise

>>> +     */
>>> +    AVRational frame_rate;
>>>   };
>>
>> question, would it be simpler or is it possible at all to avoid adding
>> a new field and use time_base to calculate framerate when needed
>> instead?
>>
>> thanks
>
>
> No.  time_base != 1 / frame_rate.  For example, the time_base in HandBrake
> is 1/90000.
> This is what we initialize the time_base to in buffersrc.  You need a
> time_base such as
> this to represent variable frame rate timestamps for frames.  The filter
> chain can modify
> the time_base to make it == 1 / frame_rate in special cases (e.g. vf_fps),
> but in general
> they are completely different things.

yes i keep forgetting about vfr (self prevervaiton?)
thanks for clearing that out
Nicolas George Oct. 28, 2015, 11:31 a.m. | #7
Le septidi 7 brumaire, an CCXXIV, Vittorio Giovara a écrit :
> yes, it doesn't make sense to commit code and then apply patch on top
> it for typos and whatnot -- let's modify the patch directly and avoid
> commit history noise

I hope you realize that by doing so, you are making it harder to for the
authors of the patches (and for anyone interested in this feature) to find
the enhancements that you made and consider importing them. For code that is
being written directly for your project, it is certainly fine, but for code
that has been public for a long time in a separate project, the practice has
a number of drawbacks.

Regards,
John Stebbins Oct. 28, 2015, 3:14 p.m. | #8
On Wed, 2015-10-28 at 11:31 +0100, Luca Barbato wrote:
> On 28/10/15 05:24, John Stebbins wrote:
> > On 10/27/2015 07:51 PM, Vittorio Giovara wrote:
> > > On Tue, Oct 27, 2015 at 9:17 PM, John Stebbins
> > > <stebbins@jetheaddev.com> wrote:
> > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > 
> > > > (cherry picked from commit
> > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > (this comment is valid for the next patches too) there is no such
> > > hash
> > > in the libav tree, you should either mention its a ffmpeg hash or
> > > just
> > > remove it since the commit name is unchanged.
> > 
> > I can amend the comments, but most people know where these are being
> > cherry picked from
> > 
> > > > ---
> > > >   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..0b670e0 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;
> > > would 0/1 be safer? also where is it initialized?
> > 
> > This comment seems to be misleading. Any filter that
> > doesn't explicitly override frame_rate passes input frame_rate along
> > to the next stage in the filter chain. If it's not set in buffersrc,
> > it's
> > value is 0/0 and that just gets passed through the chain (until some
> > filter sets the frame_rate or it reaches buffersink).
> > 
> > IMO the whole initial bit about "unknown" value should just
> > be omitted.
> 
> Since the set got scattered I couldn't follow and I assumed that would
> be implemented as documented.
> 
> If is not please get an updated set and then lets think again what
> that
> should be.
> 
> Ideally makes sense to build a filter chain that preserves the frame
> rate and forwards it if it is known (since then some rate controls
> could
> use it), but also would be beneficial forward the information that the
> data doesn't have a frame rate, even a nominal one.
> 
> 

I will address current comments and post the patches again.
But I'm not quite sure what you are asking for.  The code as it stands
forwards whatever value frame_rate has through the filter chain.
frame_rate is initialized to 0/0 when the AVFilterLink is av_mallocz'd
and it can be initialized by buffersrc which sets a default value of
0/0.  

So if you do not explicitly set frame_rate in buffersrc and no filter
converts to a specific frame_rate (e.g. vf_fps), you will get 0/0 at the
end of the filter chain.  There are filters (interlace, framepack, and
yadif) that multiply input frame_rate.num or frame_rate.den by 2, but if
input frame_rate is 0/0, it will still be 0/0 at the end of the filter
chain.

Maybe you are just asking to keep the comment, but correct it to say "or
0/0 if unknown"?  That would make sense.
Luca Barbato Oct. 28, 2015, 3:19 p.m. | #9
On 28/10/15 16:14, John Stebbins wrote:
> The code as it stands forwards whatever value frame_rate has through the
> filter chain.
> frame_rate is initialized to 0/0 when the AVFilterLink is av_mallocz'd
> and it can be initialized by buffersrc which sets a default value of
> 0/0.
> 
> So if you do not explicitly set frame_rate in buffersrc and no filter
> converts to a specific frame_rate (e.g. vf_fps), you will get 0/0 at the
> end of the filter chain.  There are filters (interlace, framepack, and
> yadif) that multiply input frame_rate.num or frame_rate.den by 2, but if
> input frame_rate is 0/0, it will still be 0/0 at the end of the filter
> chain.
> 
> Maybe you are just asking to keep the comment, but correct it to say "or
> 0/0 if unknown"?  That would make sense.

Pretty much. I'd make sure that comment and code match and do not do
something completely unexpected.

lu
John Stebbins Oct. 28, 2015, 3:50 p.m. | #10
On Wed, 2015-10-28 at 08:14 -0700, John Stebbins wrote:
> On Wed, 2015-10-28 at 11:31 +0100, Luca Barbato wrote:
> > On 28/10/15 05:24, John Stebbins wrote:
> > > On 10/27/2015 07:51 PM, Vittorio Giovara wrote:
> > > > On Tue, Oct 27, 2015 at 9:17 PM, John Stebbins
> > > > <stebbins@jetheaddev.com> wrote:
> > > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > > 
> > > > > (cherry picked from commit
> > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93)
> > > > (this comment is valid for the next patches too) there is no
> > > > such
> > > > hash
> > > > in the libav tree, you should either mention its a ffmpeg hash
> > > > or
> > > > just
> > > > remove it since the commit name is unchanged.
> > > 
> > > I can amend the comments, but most people know where these are
> > > being
> > > cherry picked from
> > > 
> > > > > ---
> > > > >   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..0b670e0 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;
> > > > would 0/1 be safer? also where is it initialized?
> > > 
> > > This comment seems to be misleading. Any filter that
> > > doesn't explicitly override frame_rate passes input frame_rate
> > > along
> > > to the next stage in the filter chain. If it's not set in
> > > buffersrc,
> > > it's
> > > value is 0/0 and that just gets passed through the chain (until
> > > some
> > > filter sets the frame_rate or it reaches buffersink).
> > > 
> > > IMO the whole initial bit about "unknown" value should just
> > > be omitted.
> > 
> > Since the set got scattered I couldn't follow and I assumed that
> > would
> > be implemented as documented.
> > 
> > If is not please get an updated set and then lets think again what
> > that
> > should be.
> > 
> > Ideally makes sense to build a filter chain that preserves the frame
> > rate and forwards it if it is known (since then some rate controls
> > could
> > use it), but also would be beneficial forward the information that
> > the
> > data doesn't have a frame rate, even a nominal one.
> > 
> > 
> 
> I will address current comments and post the patches again.
> But I'm not quite sure what you are asking for.  The code as it stands
> forwards whatever value frame_rate has through the filter chain.
> frame_rate is initialized to 0/0 when the AVFilterLink is av_mallocz'd
> and it can be initialized by buffersrc which sets a default value of
> 0/0.  
> 
> So if you do not explicitly set frame_rate in buffersrc and no filter
> converts to a specific frame_rate (e.g. vf_fps), you will get 0/0 at
> the
> end of the filter chain.  There are filters (interlace, framepack, and
> yadif) that multiply input frame_rate.num or frame_rate.den by 2, but
> if
> input frame_rate is 0/0, it will still be 0/0 at the end of the filter
> chain.
> 
> Maybe you are just asking to keep the comment, but correct it to say
> "or
> 0/0 if unknown"?  That would make sense.
> 

Sorry, I'm being dense.  I see what you mean.  The comment currently
implies that if you explicitly set frame_rate to 1/0, nobody in the
filter chain will modify it.  The code obviously does not do this and I
see no attempt in the ffmpeg code to try to preserve 1/0 in any way. In
addition, there are few filters or sources where you can explicitly set
the frame_rate, and all of these with the exception of buffersrc require
a valid framerate (i.e. they will not accept 1/0).

It seems easier to me to define 0/0 as the "unknown" value because this
*is* preserved by the code as it currently stands.  There is no filter
aside from filters that convert to a known constant frame_rate that will
modify a value of 0/0.
Nicolas George Oct. 28, 2015, 4:03 p.m. | #11
Le septidi 7 brumaire, an CCXXIV, John Stebbins a écrit :
> Sorry, I'm being dense.  I see what you mean.  The comment currently
> implies that if you explicitly set frame_rate to 1/0, nobody in the
> filter chain will modify it.  The code obviously does not do this and I
> see no attempt in the ffmpeg code to try to preserve 1/0 in any way. In
> addition, there are few filters or sources where you can explicitly set
> the frame_rate, and all of these with the exception of buffersrc require
> a valid framerate (i.e. they will not accept 1/0).
> 
> It seems easier to me to define 0/0 as the "unknown" value because this
> *is* preserved by the code as it currently stands.  There is no filter
> aside from filters that convert to a known constant frame_rate that will
> modify a value of 0/0.

Your proposal does not work because it does not distinguish "the frame rate
is not known, maybe there is not even one" from "this filter did not bother
setting the frame rate".

A lot of filters were written before the frame_rate field was added. Most of
them are simple one-in-one-out filters where the output frame rate is the
same as the input. Your proposal would require going all these adding the
copy explicitly. This would be annoying, and you rightfully did not do it.

The cleared value 0/0 means that the filter did not bother setting it. If
you assign it a different semantic, you create ambiguity and make the API
more fragile.

The convention chosen in FFmpeg is: if 0/0, that means the filter did not
bother to set it, then assume the simplest case and let the framework set
the value as the same as the input.

Regards,
John Stebbins Oct. 28, 2015, 4:30 p.m. | #12
On Wed, 2015-10-28 at 17:03 +0100, Nicolas George wrote:
> Le septidi 7 brumaire, an CCXXIV, John Stebbins a écrit :
> > Sorry, I'm being dense.  I see what you mean.  The comment currently
> > implies that if you explicitly set frame_rate to 1/0, nobody in the
> > filter chain will modify it.  The code obviously does not do this
> > and I
> > see no attempt in the ffmpeg code to try to preserve 1/0 in any way.
> > In
> > addition, there are few filters or sources where you can explicitly
> > set
> > the frame_rate, and all of these with the exception of buffersrc
> > require
> > a valid framerate (i.e. they will not accept 1/0).
> > 
> > It seems easier to me to define 0/0 as the "unknown" value because
> > this
> > *is* preserved by the code as it currently stands.  There is no
> > filter
> > aside from filters that convert to a known constant frame_rate that
> > will
> > modify a value of 0/0.
> 
> Your proposal does not work because it does not distinguish "the frame
> rate
> is not known, maybe there is not even one" from "this filter did not
> bother
> setting the frame rate".
> 
> A lot of filters were written before the frame_rate field was added.
> Most of
> them are simple one-in-one-out filters where the output frame rate is
> the
> same as the input. Your proposal would require going all these adding
> the
> copy explicitly. This would be annoying, and you rightfully did not do
> it.
> 
> The cleared value 0/0 means that the filter did not bother setting it.
> If
> you assign it a different semantic, you create ambiguity and make the
> API
> more fragile.
> 
> The convention chosen in FFmpeg is: if 0/0, that means the filter did
> not
> bother to set it, then assume the simplest case and let the framework
> set
> the value as the same as the input.
> 
> Regards,
> 
> 

Ah, ok.  I misread some code.  Now it makes sense.  The code does
exactly what the comment says it does.  Sorry for all the spam.

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..0b670e0 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 r_frae_rate field in AVStream.
+     */
+    AVRational frame_rate;
 };
 
 /**