| Message ID | 20220301132720.86396-1-onemda@gmail.com |
|---|---|
| State | Accepted |
| Commit | 0f5c964c5737170cd52c2a9501e81ea12946a66d |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/split: switch to activate() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Paul B Mahol (12022-03-01): > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > > Fix possible hangs if (a)split filter is used in graph and one of outputs ends > earlier than others. > Then filter may never receive EOF from input provided by (a)split filter. > > See ticket #9152 for commands how to reproduce issue. I am trying to reproduce the issue, and I notice it hangs with sine.mp3 but not with sine.wav (my build did not have a MP3 encoder), and I find it very strange, even with the current code. Please let me look into it closer; but if you have an idea about why MP3 input behaves different than WAV please let me know. Regards,
On 3/3/22, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12022-03-01): >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> >> Fix possible hangs if (a)split filter is used in graph and one of outputs >> ends >> earlier than others. >> Then filter may never receive EOF from input provided by (a)split filter. >> >> See ticket #9152 for commands how to reproduce issue. > > I am trying to reproduce the issue, and I notice it hangs with sine.mp3 > but not with sine.wav (my build did not have a MP3 encoder), and I find > it very strange, even with the current code. Please let me look into it > closer; but if you have an idea about why MP3 input behaves different > than WAV please let me know. It is caused by number of sample per frame. I tested actually with -f lavfi -i sine only. And patch resolves issue.
Paul B Mahol (12022-03-03): > It is caused by number of sample per frame. > > I tested actually with -f lavfi -i sine only. > > And patch resolves issue. I do not doubt it does. But even without activate, EOF should not depend on the number of samples per frame. There is something wrong going on there, and I want to understand what before this change makes it go away: otherwise, we might be missing other similar bugs. It has waited several months, a few days more will not hurt. Regards,
On 3/4/22, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12022-03-03): >> It is caused by number of sample per frame. >> >> I tested actually with -f lavfi -i sine only. >> >> And patch resolves issue. > > I do not doubt it does. But even without activate, EOF should not depend > on the number of samples per frame. There is something wrong going on > there, and I want to understand what before this change makes it go > away: otherwise, we might be missing other similar bugs. > > It has waited several months, a few days more will not hurt. > No, wait must stop. The issue is that EOF is never reported if there is >1 frame left in queue before EOF for filters that do not use .activate (and use >1 inputs). > Regards, > > -- > Nicolas George >
Paul B Mahol (12022-03-04): > No, wait must stop. > > The issue is that EOF is never reported if there is >1 frame left in > queue before EOF for filters that do not use .activate (and use >1 > inputs). Let me tell it one more time: split should not require switching to activate to work, including EOF. There is a bug somewhere, and your analysis is not enough to know exactly where. Until I understand what is going on exactly and if there is a framework bug that needs fixing, I demand you hold.
On 3/5/22, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (12022-03-04): >> No, wait must stop. >> >> The issue is that EOF is never reported if there is >1 frame left in >> queue before EOF for filters that do not use .activate (and use >1 >> inputs). > > Let me tell it one more time: > > split should not require switching to activate to work, including EOF. > There is a bug somewhere, and your analysis is not enough to know > exactly where. > > Until I understand what is going on exactly and if there is a framework > bug that needs fixing, I demand you hold. > Your patronizing and authoritarian tone is not helping you in any way. As you have no technical understanding in general and in this special case anyway, will gonna apply this working solution soon. Unless you provide clear and concise explanation why this should not be applied as is. My analysis was very clean, your ignorance and belittling tone is not helping at all. > -- > Nicolas George >
Paul B Mahol (12022-03-05):
> will gonna apply this working solution soon.
Unacceptable. And I say this for all the times you do this, once and for
all: if you push after I asked for more time to review, I will revert
and complain to the community committee.
Le me tell you, all you have won by being so annoying is that I cannot
consider looking into it right now because I am too annoyed and angry.
This is all I have to say to you.
Paul B Mahol (12022-03-01): > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > > Fix possible hangs if (a)split filter is used in graph and one of outputs ends > earlier than others. > Then filter may never receive EOF from input provided by (a)split filter. > > See ticket #9152 for commands how to reproduce issue. > > --- > libavfilter/split.c | 68 +++++++++++++++++++++++++++++++++------------ > 1 file changed, 51 insertions(+), 17 deletions(-) Ok, the problem is in forward_status_change() in avfilter.c: it makes a request on the first output, that triggers the status change on the input, and therefore it considers itself satisfied without making a request on the other outputs. It could be changed to make at least one round and requesting on each output, but it would be inefficient for filters that have several outputs connected to different inputs, and that is a more common case. Therefore, I agree, filters with several outputs connected to the same input like split need a real activate implementation. Patch ok, I did not look very carefully at the code itself. Can you disclose why it was so urgent to push a fix for a but seven months old that you would rather be rude than wait a few days?
diff --git a/libavfilter/split.c b/libavfilter/split.c index 6b9b656708..98b51f976e 100644 --- a/libavfilter/split.c +++ b/libavfilter/split.c @@ -63,28 +63,62 @@ static av_cold int split_init(AVFilterContext *ctx) return 0; } -static int filter_frame(AVFilterLink *inlink, AVFrame *frame) +static int activate(AVFilterContext *ctx) { - AVFilterContext *ctx = inlink->dst; - int i, ret = AVERROR_EOF; + AVFilterLink *inlink = ctx->inputs[0]; + AVFrame *in; + int status, ret; + int64_t pts; - for (i = 0; i < ctx->nb_outputs; i++) { - AVFrame *buf_out; + for (int i = 0; i < ctx->nb_outputs; i++) { + FF_FILTER_FORWARD_STATUS_BACK_ALL(ctx->outputs[i], ctx); + } - if (ff_outlink_get_status(ctx->outputs[i])) - continue; - buf_out = av_frame_clone(frame); - if (!buf_out) { - ret = AVERROR(ENOMEM); - break; + ret = ff_inlink_consume_frame(inlink, &in); + if (ret < 0) + return ret; + if (ret > 0) { + for (int i = 0; i < ctx->nb_outputs; i++) { + AVFrame *buf_out; + + if (ff_outlink_get_status(ctx->outputs[i])) + continue; + buf_out = av_frame_clone(in); + if (!buf_out) { + ret = AVERROR(ENOMEM); + break; + } + + ret = ff_filter_frame(ctx->outputs[i], buf_out); + if (ret < 0) + break; } - ret = ff_filter_frame(ctx->outputs[i], buf_out); + av_frame_free(&in); if (ret < 0) - break; + return ret; + } + + if (ff_inlink_acknowledge_status(inlink, &status, &pts)) { + for (int i = 0; i < ctx->nb_outputs; i++) { + if (ff_outlink_get_status(ctx->outputs[i])) + continue; + ff_outlink_set_status(ctx->outputs[i], status, pts); + } + return 0; } - av_frame_free(&frame); - return ret; + + for (int i = 0; i < ctx->nb_outputs; i++) { + if (ff_outlink_get_status(ctx->outputs[i])) + continue; + + if (ff_outlink_frame_wanted(ctx->outputs[i])) { + ff_inlink_request_frame(inlink); + return 0; + } + } + + return FFERROR_NOT_READY; } #define OFFSET(x) offsetof(SplitContext, x) @@ -100,7 +134,6 @@ static const AVFilterPad avfilter_vf_split_inputs[] = { { .name = "default", .type = AVMEDIA_TYPE_VIDEO, - .filter_frame = filter_frame, }, }; @@ -110,6 +143,7 @@ const AVFilter ff_vf_split = { .priv_size = sizeof(SplitContext), .priv_class = &split_class, .init = split_init, + .activate = activate, FILTER_INPUTS(avfilter_vf_split_inputs), .outputs = NULL, .flags = AVFILTER_FLAG_DYNAMIC_OUTPUTS | AVFILTER_FLAG_METADATA_ONLY, @@ -119,7 +153,6 @@ static const AVFilterPad avfilter_af_asplit_inputs[] = { { .name = "default", .type = AVMEDIA_TYPE_AUDIO, - .filter_frame = filter_frame, }, }; @@ -129,6 +162,7 @@ const AVFilter ff_af_asplit = { .priv_class = &split_class, .priv_size = sizeof(SplitContext), .init = split_init, + .activate = activate, FILTER_INPUTS(avfilter_af_asplit_inputs), .outputs = NULL, .flags = AVFILTER_FLAG_DYNAMIC_OUTPUTS | AVFILTER_FLAG_METADATA_ONLY,
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- Fix possible hangs if (a)split filter is used in graph and one of outputs ends earlier than others. Then filter may never receive EOF from input provided by (a)split filter. See ticket #9152 for commands how to reproduce issue. --- libavfilter/split.c | 68 +++++++++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 17 deletions(-)