| Message ID | 20161222111828.GA767956@phare.normalesup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 22 Dec 2016, Nicolas George wrote: > Le primidi 1er nivôse, an CCXXV, Marton Balint a écrit : >> It seems after this patch I got an infinite loop if I try to convert the >> attached file using this command line: >> >> ./ffmpeg -i amerge-test.mov -filter_complex "[0:a:0][0:a:1]amerge=2[aout]" >> -map "[aout]" out.wav > > Can you confirm the attached patch fixes the issue? Yes, thanks. > > If so, please do not hesitate to push it without waiting for me if (and > only if) that is convenient for you. Ok, pushed. > >> Note that in my build FF_BUFQUEUE_SIZE in libavfilter/bufferqueue.h is set >> to 256, because otherwise it errors out with ENOMEM, but that also happens >> with the old filtering code. On the other hand, the old filtering code and >> FF_BUFQUEUE_SIZE set to 256 does allow the file to properly convert. > > I wish I remembered that precision before spending time tracking the > ENOMEM :( > > Fortunately, one of the perks of these changes is they are a step closer > to getting rid of that particular issue once and for all. I suspect > rudimentary memory limitation will be needed first, but that can be > managed. I guess we could buffer the undecoded packets instead of the decoded frames if there is a higher-than-usual delay between the streams. Is this also your plan? Thanks, Marton
Le tridi 3 nivôse, an CCXXV, Marton Balint a écrit : > I guess we could buffer the undecoded packets instead of the decoded frames > if there is a higher-than-usual delay between the streams. Is this also your > plan? Well, at some point in the far future I would have libavfilter capable of handling AVMEDIA_TYPE_DATA frames and codecs as filters. If that happens, then tweaking the scheduling to prioritize consuming large frames first would be useful. But that is not the issue at all. The problem with bufferqueue is this: For normal cases, cases that are supposed to work, the need of bufferqueue is bounded. But with some codecs (those with huge frames that get split, for example) or formats (depending on the A-V muxing delay), it can sometimes be quite large. You observed that 64 is not enough for your uses but 256 is. Probably, someone somewhere needs 1024 or more. Increasing the size of the queue wastes a little memory. Not much, but still not satisfactory. The obvious solution is to make it dynamic. Inserting the fifo filter as input achieves it, because fifo has an unlimited buffer, not implemented using bufferqueue. Bug fifo breaks the scheduling because the buffer is hidden. The new data structure I added to AVFilterLink a few days ago is exactly what we need, it gives us a dynamic FIFO that works fine with the scheduling. It still requires some code to permit filters to use it directly (done, in my work tree), and then adapt amerge and the other filters to do it (not yet done). But there is a catch: sometimes people do something crazy, often inadvertently, and that causes unlimited amounts of frames to accumulate in the filter's input before any processing can be done. Currently, when that happens, they see "buffer queue overflow" and the encoding stops, and they come and ask for help on the mailing-list. If the queue is not bounded, it will grow, eat all memory, then start filling the swap, until the OOM-killer intervenes. Not good, really not good. My solution: make the queue unbounded in principle, but keep stats on the use and add a configurable limit. Quite obvious, actually. I have two bonus in my plans. First, make the stats global to the filter graph. Instead of limiting to (numbers completely arbitrary) 1000 frames on each of the 2 inputs of each of the 4 instances of amerge (which would never be used, since when all inputs on amerge have frames, amerge can process), limit to 8000 frames on the whole filter graph. Second, keep stats on the approximate memory use of the frames, and set a limit on that. That way, we can allow thousandths of 16-samples frames, but bail after only a dozen 4k video frames. Regards,
From 7006d17b617279f1fc0a35650869e552ca5eefcb Mon Sep 17 00:00:00 2001 From: Nicolas George <george@nsup.org> Date: Thu, 22 Dec 2016 12:04:12 +0100 Subject: [PATCH] af_amerge: detect EOF immediately. Fix an infinite loop in forward_status_change(). Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/af_amerge.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libavfilter/af_amerge.c b/libavfilter/af_amerge.c index 4a8c6d5bd0..40bf7ab209 100644 --- a/libavfilter/af_amerge.c +++ b/libavfilter/af_amerge.c @@ -23,6 +23,9 @@ * Audio merging filter */ +#define FF_INTERNAL_FIELDS 1 +#include "framequeue.h" + #include "libavutil/avstring.h" #include "libavutil/bprint.h" #include "libavutil/channel_layout.h" @@ -182,7 +185,9 @@ static int request_frame(AVFilterLink *outlink) int i, ret; for (i = 0; i < s->nb_inputs; i++) - if (!s->in[i].nb_samples) + if (!s->in[i].nb_samples || + /* detect EOF immediately */ + (ctx->inputs[i]->status_in && !ctx->inputs[i]->status_out)) if ((ret = ff_request_frame(ctx->inputs[i])) < 0) return ret; return 0; -- 2.11.0