[FFmpeg-devel] vf_fps: uses the last frame duration to duplicate it if needed

Submitted by Thierry Foucu on Sept. 6, 2017, 8:28 p.m.

Details

Message ID 20170906202807.17675-1-tfoucu@gmail.com
State New
Headers show

Commit Message

Thierry Foucu Sept. 6, 2017, 8:28 p.m.
---
 libavfilter/vf_fps.c        | 40 +++++++++++++++++++++++++++++++++++-----
 tests/ref/fate/filter-fps   |  6 ++++++
 tests/ref/fate/filter-fps-r |  4 ++++
 3 files changed, 45 insertions(+), 5 deletions(-)

Comments

Nicolas George Sept. 6, 2017, 9:07 p.m.
Le decadi 20 fructidor, an CCXXV, Thierry Foucu a écrit :
> ---
>  libavfilter/vf_fps.c        | 40 +++++++++++++++++++++++++++++++++++-----
>  tests/ref/fate/filter-fps   |  6 ++++++
>  tests/ref/fate/filter-fps-r |  4 ++++
>  3 files changed, 45 insertions(+), 5 deletions(-)

Already explained: no, pkt_duration is not part of the information taken
into account by libavfilter. The correct way of obtaining the end
timestamp is to use the link's timestamp after EOF. There is an old
patch series to ensure it is accurate, but it has been delayed by
bikeshedding.

Regards,
Paul B Mahol Sept. 6, 2017, 9:17 p.m.
On 9/6/17, Nicolas George <george@nsup.org> wrote:
> Le decadi 20 fructidor, an CCXXV, Thierry Foucu a ecrit :
>> ---
>>  libavfilter/vf_fps.c        | 40
>> +++++++++++++++++++++++++++++++++++-----
>>  tests/ref/fate/filter-fps   |  6 ++++++
>>  tests/ref/fate/filter-fps-r |  4 ++++
>>  3 files changed, 45 insertions(+), 5 deletions(-)
>
> Already explained: no, pkt_duration is not part of the information taken
> into account by libavfilter.

Why?

> The correct way of obtaining the end
> timestamp is to use the link's timestamp after EOF. There is an old
> patch series to ensure it is accurate, but it has been delayed by
> bikeshedding.

And why was it bikesheded?
Nicolas George Sept. 6, 2017, 9:25 p.m.
Le decadi 20 fructidor, an CCXXV, Paul B Mahol a écrit :
> Why?

Several reasons that I have explained in the past. To make it short, the
information there is not reliable, not filled by the applications, not
updated by many filters and likely to be inconsistent. Is that enough?

> And why was it bikesheded?

For completely unrelated reason within the code of ffmpeg.c.

Regards,
Thierry Foucu Sept. 7, 2017, 6:17 p.m.
Hi Nicolas,

On Wed, Sep 6, 2017 at 2:07 PM, Nicolas George <george@nsup.org> wrote:

> Le decadi 20 fructidor, an CCXXV, Thierry Foucu a écrit :
> > ---
> >  libavfilter/vf_fps.c        | 40 ++++++++++++++++++++++++++++++
> +++++-----
> >  tests/ref/fate/filter-fps   |  6 ++++++
> >  tests/ref/fate/filter-fps-r |  4 ++++
> >  3 files changed, 45 insertions(+), 5 deletions(-)
>
> Already explained: no, pkt_duration is not part of the information taken
> into account by libavfilter. The correct way of obtaining the end
> timestamp is to use the link's timestamp after EOF. There is an old
> patch series to ensure it is accurate, but it has been delayed by
> bikeshedding.
>
>
Sorry I did not know and I did not see any documentation in the header
explaining which field of the AVFrame can/cannot be used by filter.

If we cannot used the duration of the AVframe, how can we resolve the
problem I'm trying to resolve.
For example, if you take a video with a fps of 1 frame/second and you are
trying to convert it to 30fps, the duration of the output file will be
shorter by 1 second.
Knowing the last frame duration allows us to pad the last video frame with
the correct fps.

You are saying there is an old patch series to ensure it. How far are you
with it?
With the patch you are talking about, will we be able to do what i'm trying
to do with this patch?

Regards.


> Regards,
>
> --
>   Nicolas George
>
Nicolas George Sept. 7, 2017, 6:37 p.m.
[ Ccing you, but no need to Cc me. ]

Le primidi 21 fructidor, an CCXXV, Thierry Foucu a écrit :
> Sorry I did not know and I did not see any documentation in the header
> explaining which field of the AVFrame can/cannot be used by filter.

Yes, it lacks documentation.

> If we cannot used the duration of the AVframe, how can we resolve the
> problem I'm trying to resolve.
> For example, if you take a video with a fps of 1 frame/second and you are
> trying to convert it to 30fps, the duration of the output file will be
> shorter by 1 second.
> Knowing the last frame duration allows us to pad the last video frame with
> the correct fps.
> 
> You are saying there is an old patch series to ensure it. How far are you
> with it?
> With the patch you are talking about, will we be able to do what i'm trying
> to do with this patch?

Yes, it is meant exactly for that. The patch series I was referring is
this one:

https://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/215988.html
https://patchwork.ffmpeg.org/patch/5024/
(and next)

I thought I had cced you on one of the patches, but it may have been
ignored.

With this patch series, when a filter reaches EOF, the link->current_pts
contains the timestamp of the end, including the duration of the last
frame: from that, you can implement your patch in a very similar way.

(Although vf_fps would need to be rewritten to take advantage of the
link's FIFO, but that is more work, and not required immediately.)

Regards,
Thierry Foucu Sept. 7, 2017, 7:15 p.m.
On Thu, Sep 7, 2017 at 11:37 AM, Nicolas George <george@nsup.org> wrote:

> [ Ccing you, but no need to Cc me. ]
>
> Le primidi 21 fructidor, an CCXXV, Thierry Foucu a écrit :
> > Sorry I did not know and I did not see any documentation in the header
> > explaining which field of the AVFrame can/cannot be used by filter.
>
> Yes, it lacks documentation.
>
> > If we cannot used the duration of the AVframe, how can we resolve the
> > problem I'm trying to resolve.
> > For example, if you take a video with a fps of 1 frame/second and you are
> > trying to convert it to 30fps, the duration of the output file will be
> > shorter by 1 second.
> > Knowing the last frame duration allows us to pad the last video frame
> with
> > the correct fps.
> >
> > You are saying there is an old patch series to ensure it. How far are you
> > with it?
> > With the patch you are talking about, will we be able to do what i'm
> trying
> > to do with this patch?
>
> Yes, it is meant exactly for that. The patch series I was referring is
> this one:
>
> https://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/215988.html
> https://patchwork.ffmpeg.org/patch/5024/
> (and next)
>
> I thought I had cced you on one of the patches, but it may have been
> ignored.
>
>
Thanks Nicolas.


> With this patch series, when a filter reaches EOF, the link->current_pts
> contains the timestamp of the end, including the duration of the last
> frame: from that, you can implement your patch in a very similar way.
>

As soon as your patches landed, I will re-work on my patch and send it to
the list.

>
> (Although vf_fps would need to be rewritten to take advantage of the
> link's FIFO, but that is more work, and not required immediately.)
>

any pointer on this? maybe i can give it a shot as well.

>
> Regards,
>
> --
>   Nicolas George
>

Patch hide | download patch | download mbox

diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c
index 20ccd797d1..a231da415c 100644
--- a/libavfilter/vf_fps.c
+++ b/libavfilter/vf_fps.c
@@ -137,13 +137,43 @@  static int request_frame(AVFilterLink *outlink)
             AVFrame *buf;
 
             av_fifo_generic_read(s->fifo, &buf, sizeof(buf), NULL);
-            buf->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base,
-                                    outlink->time_base) + s->frames_out;
+            if (av_fifo_size(s->fifo)) {
+                buf->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base,
+                                        outlink->time_base) + s->frames_out;
 
-            if ((ret = ff_filter_frame(outlink, buf)) < 0)
-                return ret;
+                if ((ret = ff_filter_frame(outlink, buf)) < 0)
+                    return ret;
 
-            s->frames_out++;
+                s->frames_out++;
+            } else {
+                /* This is the last frame, we may have to duplicate it to match
+                 * the last frame duration */
+                int j;
+                int delta = av_rescale_q_rnd(buf->pts + buf->pkt_duration - s->first_pts,
+                                             ctx->inputs[0]->time_base,
+                                             outlink->time_base, s->rounding) - s->frames_out ;
+                if (delta > 0 ) {
+                    for (j = 0; j < delta; j++) {
+                        AVFrame *dup = av_frame_clone(buf);
+
+                        av_log(ctx, AV_LOG_DEBUG, "Duplicating frame.\n");
+                        dup->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base,
+                                                outlink->time_base) + s->frames_out;
+
+                        if ((ret = ff_filter_frame(outlink, dup)) < 0)
+                            return ret;
+
+                        s->frames_out++;
+                    }
+                } else {
+                    buf->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base,
+                                            outlink->time_base) + s->frames_out;
+
+                    if ((ret = ff_filter_frame(outlink, buf)) < 0)
+                        return ret;
+                    s->frames_out++;
+                }
+            }
         }
         return 0;
     }
diff --git a/tests/ref/fate/filter-fps b/tests/ref/fate/filter-fps
index 55712cfb1c..242fb04e85 100644
--- a/tests/ref/fate/filter-fps
+++ b/tests/ref/fate/filter-fps
@@ -85,3 +85,9 @@ 
 0,         79,         79,        1,    30576, 0xa2fcd06f
 0,         80,         80,        1,    30576, 0xa2fcd06f
 0,         81,         81,        1,    30576, 0xd4150aad
+0,         82,         82,        1,    30576, 0xd4150aad
+0,         83,         83,        1,    30576, 0xd4150aad
+0,         84,         84,        1,    30576, 0xd4150aad
+0,         85,         85,        1,    30576, 0xd4150aad
+0,         86,         86,        1,    30576, 0xd4150aad
+0,         87,         87,        1,    30576, 0xd4150aad
diff --git a/tests/ref/fate/filter-fps-r b/tests/ref/fate/filter-fps-r
index 826b1ed6c6..c1bc7d1547 100644
--- a/tests/ref/fate/filter-fps-r
+++ b/tests/ref/fate/filter-fps-r
@@ -72,3 +72,7 @@ 
 0,         79,         79,        1,    30576, 0xa2fcd06f
 0,         80,         80,        1,    30576, 0xa2fcd06f
 0,         82,         82,        1,    30576, 0xd4150aad
+0,         83,         83,        1,    30576, 0xd4150aad
+0,         84,         84,        1,    30576, 0xd4150aad
+0,         85,         85,        1,    30576, 0xd4150aad
+0,         86,         86,        1,    30576, 0xd4150aad