| Message ID | 20170615180031.GA1157777@phare.normalesup.org |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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!
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