[02/11] buffersrc: accept the frame rate as argument.

Message ID 1446050900-7329-2-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 9ca440679dc535b31edd569393d8d3dda59db90e)
---
 libavfilter/buffersrc.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Anton Khirnov Oct. 29, 2015, 6:49 p.m. | #1
Quoting John Stebbins (2015-10-28 17:48:18)
> From: Nicolas George <nicolas.george@normalesup.org>
> 
> (cherry picked from ffmpeg commit 9ca440679dc535b31edd569393d8d3dda59db90e)
> ---
>  libavfilter/buffersrc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> index a9b893c..f5b852f 100644
> --- a/libavfilter/buffersrc.c
> +++ b/libavfilter/buffersrc.c
> @@ -44,6 +44,7 @@ typedef struct BufferSourceContext {
>      const AVClass    *class;
>      AVFifoBuffer     *fifo;
>      AVRational        time_base;     ///< time_base to set in the output link
> +    AVRational        frame_rate;    ///< frame_rate to set in the output link
>  
>      /* video only */
>      int               h, w;
> @@ -191,6 +192,7 @@ static const AVOption video_options[] = {
>  #endif
>      { "sar",           "sample aspect ratio",    OFFSET(pixel_aspect),     AV_OPT_TYPE_RATIONAL, { .dbl = 1 }, 0, DBL_MAX, V },
>      { "time_base",     NULL,                     OFFSET(time_base),        AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },
> +    { "frame_rate",    NULL,                     OFFSET(frame_rate),       AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },

I'm not quite sure the default value this will evalate to is the same as
the "unknown" value mandated by the AVBufferLink doxy. Please make sure
the two are the same.
John Stebbins Oct. 29, 2015, 8:26 p.m. | #2
On Thu, 2015-10-29 at 19:49 +0100, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-28 17:48:18)
> > From: Nicolas George <nicolas.george@normalesup.org>
> > 
> > (cherry picked from ffmpeg commit
> > 9ca440679dc535b31edd569393d8d3dda59db90e)
> > ---
> >  libavfilter/buffersrc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> > index a9b893c..f5b852f 100644
> > --- a/libavfilter/buffersrc.c
> > +++ b/libavfilter/buffersrc.c
> > @@ -44,6 +44,7 @@ typedef struct BufferSourceContext {
> >      const AVClass    *class;
> >      AVFifoBuffer     *fifo;
> >      AVRational        time_base;     ///< time_base to set in the
> > output link
> > +    AVRational        frame_rate;    ///< frame_rate to set in the
> > output link
> >  
> >      /* video only */
> >      int               h, w;
> > @@ -191,6 +192,7 @@ static const AVOption video_options[] = {
> >  #endif
> >      { "sar",           "sample aspect ratio",   
> >  OFFSET(pixel_aspect),     AV_OPT_TYPE_RATIONAL, { .dbl = 1 }, 0,
> > DBL_MAX, V },
> >      { "time_base",     NULL,                     OFFSET(time_base),
> >         AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },
> > +    { "frame_rate",    NULL,                    
> >  OFFSET(frame_rate),       AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0,
> > DBL_MAX, V },
> 
> I'm not quite sure the default value this will evalate to is the same
> as
> the "unknown" value mandated by the AVBufferLink doxy. Please make
> sure
> the two are the same.
> 

I'm not sure this was meant to be "unknown" at this point.  I think the
intention may have been for it to be "unset".  But I really don't know
what was in the mind of the author here.  It does seem like "unknown"
would be appropriate for buffersrc.  Also, should AV_OPT_TYPE_RATIONAL
be initialized like { .q = { .num = 0, .den = 0 }?
Anton Khirnov Oct. 29, 2015, 8:35 p.m. | #3
Quoting John Stebbins (2015-10-29 21:26:11)
> On Thu, 2015-10-29 at 19:49 +0100, Anton Khirnov wrote:
> > Quoting John Stebbins (2015-10-28 17:48:18)
> > > From: Nicolas George <nicolas.george@normalesup.org>
> > > 
> > > (cherry picked from ffmpeg commit
> > > 9ca440679dc535b31edd569393d8d3dda59db90e)
> > > ---
> > >  libavfilter/buffersrc.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> > > index a9b893c..f5b852f 100644
> > > --- a/libavfilter/buffersrc.c
> > > +++ b/libavfilter/buffersrc.c
> > > @@ -44,6 +44,7 @@ typedef struct BufferSourceContext {
> > >      const AVClass    *class;
> > >      AVFifoBuffer     *fifo;
> > >      AVRational        time_base;     ///< time_base to set in the
> > > output link
> > > +    AVRational        frame_rate;    ///< frame_rate to set in the
> > > output link
> > >  
> > >      /* video only */
> > >      int               h, w;
> > > @@ -191,6 +192,7 @@ static const AVOption video_options[] = {
> > >  #endif
> > >      { "sar",           "sample aspect ratio",   
> > >  OFFSET(pixel_aspect),     AV_OPT_TYPE_RATIONAL, { .dbl = 1 }, 0,
> > > DBL_MAX, V },
> > >      { "time_base",     NULL,                     OFFSET(time_base),
> > >         AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },
> > > +    { "frame_rate",    NULL,                    
> > >  OFFSET(frame_rate),       AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0,
> > > DBL_MAX, V },
> > 
> > I'm not quite sure the default value this will evalate to is the same
> > as
> > the "unknown" value mandated by the AVBufferLink doxy. Please make
> > sure
> > the two are the same.
> > 
> 
> I'm not sure this was meant to be "unknown" at this point.  I think the
> intention may have been for it to be "unset".  But I really don't know
> what was in the mind of the author here.  It does seem like "unknown"
> would be appropriate for buffersrc. 

Are those supposed to be semantically different?

> Also, should AV_OPT_TYPE_RATIONAL
> be initialized like { .q = { .num = 0, .den = 0 }?

No, defaults for TYPE_RATIONAL are .dbl (for histerical reasons).


> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
John Stebbins Oct. 29, 2015, 9:45 p.m. | #4
On Thu, 2015-10-29 at 21:35 +0100, Anton Khirnov wrote:
> Quoting John Stebbins (2015-10-29 21:26:11)
> > On Thu, 2015-10-29 at 19:49 +0100, Anton Khirnov wrote:
> > > Quoting John Stebbins (2015-10-28 17:48:18)
> > > > From: Nicolas George <nicolas.george@normalesup.org>
> > > > 
> > > > (cherry picked from ffmpeg commit
> > > > 9ca440679dc535b31edd569393d8d3dda59db90e)
> > > > ---
> > > >  libavfilter/buffersrc.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
> > > > index a9b893c..f5b852f 100644
> > > > --- a/libavfilter/buffersrc.c
> > > > +++ b/libavfilter/buffersrc.c
> > > > @@ -44,6 +44,7 @@ typedef struct BufferSourceContext {
> > > >      const AVClass    *class;
> > > >      AVFifoBuffer     *fifo;
> > > >      AVRational        time_base;     ///< time_base to set in
> > > > the
> > > > output link
> > > > +    AVRational        frame_rate;    ///< frame_rate to set in
> > > > the
> > > > output link
> > > >  
> > > >      /* video only */
> > > >      int               h, w;
> > > > @@ -191,6 +192,7 @@ static const AVOption video_options[] = {
> > > >  #endif
> > > >      { "sar",           "sample aspect ratio",   
> > > >  OFFSET(pixel_aspect),     AV_OPT_TYPE_RATIONAL, { .dbl = 1 },
> > > > 0,
> > > > DBL_MAX, V },
> > > >      { "time_base",     NULL,                    
> > > >  OFFSET(time_base),
> > > >         AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },
> > > > +    { "frame_rate",    NULL,                    
> > > >  OFFSET(frame_rate),       AV_OPT_TYPE_RATIONAL, { .dbl = 0 },
> > > > 0,
> > > > DBL_MAX, V },
> > > 
> > > I'm not quite sure the default value this will evalate to is the
> > > same
> > > as
> > > the "unknown" value mandated by the AVBufferLink doxy. Please make
> > > sure
> > > the two are the same.
> > > 
> > 
> > I'm not sure this was meant to be "unknown" at this point.  I think
> > the
> > intention may have been for it to be "unset".  But I really don't
> > know
> > what was in the mind of the author here.  It does seem like
> > "unknown"
> > would be appropriate for buffersrc. 
> 
> Are those supposed to be semantically different?

If a link input framerate is unset (0/0), it is populated with the link
output framerate of the previous filter during avfilter_config_links. If
it is unknown (1/0) it is not overwritten.  So a filter implementation
needs to do nothing in order for the framerate to propagate through it.
But filters such as setpts can halt the propagation of the upstream
framerate simply by setting framerate to the unknown value.

For a video source that has no previous filter, it may be a distinction
without a difference.  Whether you pass 0/0 down to unset subsequent
filters or 1/0, makes no real difference.

Patch

diff --git a/libavfilter/buffersrc.c b/libavfilter/buffersrc.c
index a9b893c..f5b852f 100644
--- a/libavfilter/buffersrc.c
+++ b/libavfilter/buffersrc.c
@@ -44,6 +44,7 @@  typedef struct BufferSourceContext {
     const AVClass    *class;
     AVFifoBuffer     *fifo;
     AVRational        time_base;     ///< time_base to set in the output link
+    AVRational        frame_rate;    ///< frame_rate to set in the output link
 
     /* video only */
     int               h, w;
@@ -191,6 +192,7 @@  static const AVOption video_options[] = {
 #endif
     { "sar",           "sample aspect ratio",    OFFSET(pixel_aspect),     AV_OPT_TYPE_RATIONAL, { .dbl = 1 }, 0, DBL_MAX, V },
     { "time_base",     NULL,                     OFFSET(time_base),        AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },
+    { "frame_rate",    NULL,                     OFFSET(frame_rate),       AV_OPT_TYPE_RATIONAL, { .dbl = 0 }, 0, DBL_MAX, V },
     { NULL },
 };
 
@@ -308,6 +310,7 @@  static int config_props(AVFilterLink *link)
     }
 
     link->time_base = c->time_base;
+    link->frame_rate = c->frame_rate;
     return 0;
 }