[FFmpeg-devel,v3] avfilter/vf_fps: fix duration

Submitted by Nicolas George on June 15, 2017, 6 p.m.

Details

Message ID 20170615180031.GA1157777@phare.normalesup.org
State New
Headers show

Commit Message

Nicolas George June 15, 2017, 6 p.m.
Le quintidi 25 prairial, an CCXXV, Thomas Mundt a écrit :
> Patch attached. This fixes ticket #2674. I inserted a FIXME message as a
> reminder.
> Please comment.

I am sorry to say I do not like it. The timestamp computation code in
vf_fps is already quite complex, and this patch is making it more
complex, introducing frames_in_proc which should not be needed just to
fix the last timestamp, and obviously the EOF handling duplicates the
filter_frame() logic.

Rule of thumb: if you are about to copy-paste a non-trivial block of
code, stop and refactor.

I think the way forward for this filter is to rewrite the core logic
using the activate() design. It should be much simpler since the
framework already handles a FIFO. If you are interested in it, you
probably will need to rebase push this series:

https://ffmpeg.org/pipermail/ffmpeg-devel/2017-April/209678.html

and use the attached patch.

Regards,

Comments

Thomas Mundt June 15, 2017, 7:11 p.m.
2017-06-15 20:00 GMT+02:00 Nicolas George <george@nsup.org>:

> Le quintidi 25 prairial, an CCXXV, Thomas Mundt a écrit :
> > Patch attached. This fixes ticket #2674. I inserted a FIXME message as a
> > reminder.
> > Please comment.
>
> I am sorry to say I do not like it. The timestamp computation code in
> vf_fps is already quite complex, and this patch is making it more
> complex, introducing frames_in_proc which should not be needed just to
> fix the last timestamp, and obviously the EOF handling duplicates the
> filter_frame() logic.
>
> Hmm, before rewriting and sending this patch I asked if it would have a
chance to be pushed just to fix the ticket which is open for a very long
time. Your answer gave me the assumtion that you´re okay with it.
I thought your only concern was the not allowed use of pkt_duration.


> I think the way forward for this filter is to rewrite the core logic
> using the activate() design. It should be much simpler since the
> framework already handles a FIFO. If you are interested in it, you
> probably will need to rebase push this series:
>
> https://ffmpeg.org/pipermail/ffmpeg-devel/2017-April/209678.html
>
> and use the attached patch.
>
> Well, my problem is that I´m not deep enough in ffmpeg programming to
understand the interrelations of timestamp handling and unfortunately I
don´t have the time to figure it out.
Working with patches that I don´t fully understand doesn´t make any sense.
My intention was a fix within the filter to close the ticket. Just to
bridge the time until a proper fix is made.

Regards,
Thomas
Nicolas George June 15, 2017, 7:23 p.m.
Le septidi 27 prairial, an CCXXV, Thomas Mundt a écrit :
> Hmm, before rewriting and sending this patch I asked if it would have a
> chance to be pushed just to fix the ticket which is open for a very long
> time. Your answer gave me the assumtion that you´re okay with it.
> I thought your only concern was the not allowed use of pkt_duration.

I am ok in principle with a patch that uses the frame rate instead of
the correct final timestamp. But not any such patch.

I am not ok with a patch that makes the code more complex. I will at
some point start again working on this, and any extra code complexity
will make my life harder.

In particular, I am never ok with a patch that copy-pastes and
duplicates a non-trivial block of code.

In fact, even if your patch was perfect in its logic, i.e. used the
correct final timestamp, I would reject it based on the code complexity
and duplication.

As I said : the moment you used the "copy" feature of your editor on a
non-trivial block of code, you should have stopped and taken a step back
to look at the big picture.

Regards,
Thomas Mundt June 15, 2017, 7:58 p.m.
2017-06-15 21:23 GMT+02:00 Nicolas George <george@nsup.org>:

> Le septidi 27 prairial, an CCXXV, Thomas Mundt a écrit :
> > Hmm, before rewriting and sending this patch I asked if it would have a
> > chance to be pushed just to fix the ticket which is open for a very long
> > time. Your answer gave me the assumtion that you´re okay with it.
> > I thought your only concern was the not allowed use of pkt_duration.
>
> I am ok in principle with a patch that uses the frame rate instead of
> the correct final timestamp. But not any such patch.
>
> I am not ok with a patch that makes the code more complex. I will at
> some point start again working on this, and any extra code complexity
> will make my life harder.
>
> In particular, I am never ok with a patch that copy-pastes and
> duplicates a non-trivial block of code.
>
> In fact, even if your patch was perfect in its logic, i.e. used the
> correct final timestamp, I would reject it based on the code complexity
> and duplication.
>
> As I said : the moment you used the "copy" feature of your editor on a
> non-trivial block of code, you should have stopped and taken a step back
> to look at the big picture.
>

I´m afraid the big picture might be to big for me.
The only thing I see that could be factored out is the delta for loop. And
even this would need much more investigation and testing form my side.
Unfortunately my time is too limited atm.
I think I understand your position, so I hope one day you will find the
time for the right fix.

Regards,
Thomas
Paul B Mahol June 15, 2017, 8:31 p.m.
On 6/15/17, Thomas Mundt <tmundt75@gmail.com> wrote:
> 2017-06-15 21:23 GMT+02:00 Nicolas George <george@nsup.org>:
>
>> Le septidi 27 prairial, an CCXXV, Thomas Mundt a ecrit :
>> > Hmm, before rewriting and sending this patch I asked if it would have a
>> > chance to be pushed just to fix the ticket which is open for a very long
>> > time. Your answer gave me the assumtion that you're okay with it.
>> > I thought your only concern was the not allowed use of pkt_duration.
>>
>> I am ok in principle with a patch that uses the frame rate instead of
>> the correct final timestamp. But not any such patch.
>>
>> I am not ok with a patch that makes the code more complex. I will at
>> some point start again working on this, and any extra code complexity
>> will make my life harder.
>>
>> In particular, I am never ok with a patch that copy-pastes and
>> duplicates a non-trivial block of code.
>>
>> In fact, even if your patch was perfect in its logic, i.e. used the
>> correct final timestamp, I would reject it based on the code complexity
>> and duplication.
>>
>> As I said : the moment you used the "copy" feature of your editor on a
>> non-trivial block of code, you should have stopped and taken a step back
>> to look at the big picture.
>>
>
> I'm afraid the big picture might be to big for me.
> The only thing I see that could be factored out is the delta for loop. And
> even this would need much more investigation and testing form my side.
> Unfortunately my time is too limited atm.
> I think I understand your position, so I hope one day you will find the
> time for the right fix.

Yeah, right!

Patch hide | download patch | download mbox

From b7a20596861a8baeb746dbe8922c874ab48e26f5 Mon Sep 17 00:00:00 2001
From: Nicolas George <george@nsup.org>
Date: Sun, 23 Apr 2017 14:05:01 +0200
Subject: [PATCH 5/6] lavfi: add ff_inlink_query_fifo().

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavfilter/avfilter.c | 14 ++++++++++++++
 libavfilter/filters.h  |  7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index ecfb872ed8..ee79743928 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1638,6 +1638,20 @@  void ff_inlink_request_frame(AVFilterLink *link)
     ff_filter_set_ready(link->src, 100);
 }
 
+void ff_inlink_query_fifo(AVFilterLink *link,
+                          size_t *frames, uint64_t *samples,
+                          int *status, int64_t *status_pts)
+{
+    if (*frames)
+        *frames = ff_framequeue_queued_frames(&link->fifo);
+    if (*samples)
+        *samples = ff_framequeue_queued_samples(&link->fifo);
+    if (*status)
+        *status = link->status_in;
+    if (*status_pts)
+        *status_pts = link->status_in_pts;
+}
+
 const AVClass *avfilter_get_class(void)
 {
     return &avfilter_class;
diff --git a/libavfilter/filters.h b/libavfilter/filters.h
index 2c78d60e62..870fbc4708 100644
--- a/libavfilter/filters.h
+++ b/libavfilter/filters.h
@@ -134,4 +134,11 @@  int ff_inlink_acknowledge_status(AVFilterLink *link, int *rstatus, int64_t *rpts
  */
 void ff_inlink_request_frame(AVFilterLink *link);
 
+/**
+ * Query the properties of the link FIFO and surrounding properties.
+ */
+void ff_inlink_query_fifo(AVFilterLink *link,
+                          size_t *frames, uint64_t *samples,
+                          int *status, int64_t *status_pts);
+
 #endif /* AVFILTER_FILTERS_H */
-- 
2.11.0