| Message ID | 20161121010937.1307-1-cus@passwd.hu |
|---|---|
| State | Rejected |
| Headers | show |
2016-11-21 2:09 GMT+01:00 Marton Balint <cus@passwd.hu>: > This reverts d300f5f6f570659e4b58567b35c9e8600c9f2956. > > Further reference: > https://ffmpeg.org/pipermail/ffmpeg-devel/2013-October/149935.html > > I can't reproduce ticket #2899 I just tested the following with FFmpeg 2.1: $ ffmpeg -i test.wav -af "pan=0x4|c0=c4" out.wav Output is silent ("empty"). Works fine with current FFmpeg and also with your patch applied. Carl Eugen
Le primidi 1er frimaire, an CCXXV, Marton Balint a écrit : > This reverts d300f5f6f570659e4b58567b35c9e8600c9f2956. > > Further reference: > https://ffmpeg.org/pipermail/ffmpeg-devel/2013-October/149935.html > > I can't reproduce ticket #2899 so I am not sure the original patch is still > needed. Reverting it fixes unknown channel layout support for trivial > resamples, such as: > > ffmpeg -f lavfi -i "aevalsrc=0|0|0|0|0|0|0|0|0:d=1,asetnsamples" -f null none > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavfilter/avfiltergraph.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c > index 3af698d..379bc1a 100644 > --- a/libavfilter/avfiltergraph.c > +++ b/libavfilter/avfiltergraph.c > @@ -802,8 +802,7 @@ static int reduce_formats_on_filter(AVFilterContext *filter) > if (inlink->type != outlink->type || fmts->nb_channel_layouts == 1) > continue; > > - if (fmts->all_layouts && > - (!FF_LAYOUT2COUNT(fmt) || fmts->all_counts)) { > + if (fmts->all_layouts) { > /* Turn the infinite list into a singleton */ > fmts->all_layouts = fmts->all_counts = 0; > if (ff_add_channel_layout(&outlink->in_channel_layouts, fmt) < 0) This is not correct. It makes a difference if the condition you remove is false, i.e. if !fmts->all_counts && FF_LAYOUT2COUNT(fmt): the filter does not support unknown layouts, and you propose to give it one nonetheless. Regards,
On Mon, 21 Nov 2016, Nicolas George wrote: > Le primidi 1er frimaire, an CCXXV, Marton Balint a écrit : >> This reverts d300f5f6f570659e4b58567b35c9e8600c9f2956. >> >> Further reference: >> https://ffmpeg.org/pipermail/ffmpeg-devel/2013-October/149935.html >> >> I can't reproduce ticket #2899 so I am not sure the original patch is still >> needed. Reverting it fixes unknown channel layout support for trivial >> resamples, such as: >> >> ffmpeg -f lavfi -i "aevalsrc=0|0|0|0|0|0|0|0|0:d=1,asetnsamples" -f null none >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavfilter/avfiltergraph.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c >> index 3af698d..379bc1a 100644 >> --- a/libavfilter/avfiltergraph.c >> +++ b/libavfilter/avfiltergraph.c >> @@ -802,8 +802,7 @@ static int reduce_formats_on_filter(AVFilterContext *filter) >> if (inlink->type != outlink->type || fmts->nb_channel_layouts == 1) >> continue; >> >> - if (fmts->all_layouts && >> - (!FF_LAYOUT2COUNT(fmt) || fmts->all_counts)) { >> + if (fmts->all_layouts) { >> /* Turn the infinite list into a singleton */ >> fmts->all_layouts = fmts->all_counts = 0; >> if (ff_add_channel_layout(&outlink->in_channel_layouts, fmt) < 0) > > This is not correct. It makes a difference if the condition you remove > is false, i.e. if !fmts->all_counts && FF_LAYOUT2COUNT(fmt): the filter > does not support unknown layouts, and you propose to give it one > nonetheless. Okay, so the proper fix would be to make asetnsamples support unknown channel layouts? I thought it does, but as far as I see, the default, if no query_formats callback is specified is to only allow known layouts. Wouldn't it make sense to change the default to allow unknown channel layouts as well, if no query_formats is specified? Thanks, Marton
Le duodi 2 frimaire, an CCXXV, Marton Balint a écrit : > Wouldn't it make sense to change the default to allow unknown channel > layouts as well, if no query_formats is specified? Only if you volunteer to monitor all merges from the fork to check that they work with unknown layouts. Because that is the issue; filters from the fork that need the number of channels get it from the channel layout, and if a filter uses the invalid number of channels it may cause crashes, possibly exploitable. Regards,
On Wed, 23 Nov 2016, Nicolas George wrote: > Le duodi 2 frimaire, an CCXXV, Marton Balint a écrit : >> Wouldn't it make sense to change the default to allow unknown channel >> layouts as well, if no query_formats is specified? > > Only if you volunteer to monitor all merges from the fork to check that > they work with unknown layouts. > > Because that is the issue; filters from the fork that need the number of > channels get it from the channel layout, and if a filter uses the > invalid number of channels it may cause crashes, possibly exploitable. I thought we are trying to move away from workarounds introduced only to be able to be compatible with libav API. So this is clearly libav dirving ffmpeg development, which is not fortunate IMHO. I also think that the chance of an exploitable filter because of this is small. An audio filter with no query_formats is quite rare in itself. Even if such a filter got merged, making it work with unknown channel layouts is a feature which we would want anyway, becase ffmpeg does support unknown channel layouts. Yes, it is not me who does the merges, but IMHO this does not add too much burden for the people who does it. Hendrik, Clement, what do you think? And even if such an issue got in the codebase, isn't this something that coverity should be able to easily detect most of the times? Thanks, Marton
Le tridi 3 frimaire, an CCXXV, Marton Balint a écrit : > I thought we are trying to move away from workarounds introduced only to be > able to be compatible with libav API. So this is clearly libav dirving > ffmpeg development, which is not fortunate IMHO. > > I also think that the chance of an exploitable filter because of this is > small. An audio filter with no query_formats is quite rare in itself. Even > if such a filter got merged, making it work with unknown channel layouts is > a feature which we would want anyway, becase ffmpeg does support unknown > channel layouts. > > Yes, it is not me who does the merges, but IMHO this does not add too much > burden for the people who does it. Hendrik, Clement, what do you think? > > And even if such an issue got in the codebase, isn't this something that > coverity should be able to easily detect most of the times? In principle, I think we would want to get rid of the work-ardounds and have all filters support unknown layouts. But we also want our users to have a working and reliable tool. Meeting both objectives requires auditing and testing. If we make unknown layouts the default and a filter that does not work with them sneaks through, it will sometimes use 0 as the number of channels. Depending on how much it does so, it can have several unfortunate consequences, most probably either a generic error message (probably "invalid argument" or "out of memory"), a corrupted output or a crash (and unless proven otherwise, a crash must be assumed to be exploitable). Automated testing can not help us catching them. Neither FATE nor coverity, the way they currently work. On the other hand, detecting filters that do not work correctly is not very hard in itself. The use of av_get_channel_layout_nb_channels() is a very reliable condition. But it could be hidden in a call to an external function. Making them work is usually rather easy too: replace av_get_channel_layout_nb_channels() with the corresponding "channels" field. Here is, to the best of my knowledge, the current state of affairs. With that information, we can decide to make unknown layouts the default. But that is not my decision alone. If it happens, I will try to help paying attention to new filters, but I can not promise I will succeed catching them all. Also, if we decide to proceed, I think we should not just make all_counts the default: the all_counts / all_layouts logic is rather complex, and with all_counts the default it becomes essentially dead code. But the decision comes first. Regards,
On Sun, 27 Nov 2016, Nicolas George wrote: > Le tridi 3 frimaire, an CCXXV, Marton Balint a écrit : >> I thought we are trying to move away from workarounds introduced only to be >> able to be compatible with libav API. So this is clearly libav dirving >> ffmpeg development, which is not fortunate IMHO. >> >> I also think that the chance of an exploitable filter because of this is >> small. An audio filter with no query_formats is quite rare in itself. Even >> if such a filter got merged, making it work with unknown channel layouts is >> a feature which we would want anyway, becase ffmpeg does support unknown >> channel layouts. >> >> Yes, it is not me who does the merges, but IMHO this does not add too much >> burden for the people who does it. Hendrik, Clement, what do you think? >> >> And even if such an issue got in the codebase, isn't this something that >> coverity should be able to easily detect most of the times? > > In principle, I think we would want to get rid of the work-ardounds and > have all filters support unknown layouts. But we also want our users to > have a working and reliable tool. Meeting both objectives requires > auditing and testing. > > If we make unknown layouts the default and a filter that does not work > with them sneaks through, it will sometimes use 0 as the number of > channels. Depending on how much it does so, it can have several > unfortunate consequences, most probably either a generic error message > (probably "invalid argument" or "out of memory"), a corrupted output or > a crash (and unless proven otherwise, a crash must be assumed to be > exploitable). > > Automated testing can not help us catching them. Neither FATE nor > coverity, the way they currently work. > > On the other hand, detecting filters that do not work correctly is not > very hard in itself. The use of av_get_channel_layout_nb_channels() is a > very reliable condition. But it could be hidden in a call to an external > function. Making them work is usually rather easy too: replace > av_get_channel_layout_nb_channels() with the corresponding "channels" > field. > > Here is, to the best of my knowledge, the current state of affairs. With > that information, we can decide to make unknown layouts the default. But > that is not my decision alone. > > If it happens, I will try to help paying attention to new filters, but I > can not promise I will succeed catching them all. > > Also, if we decide to proceed, I think we should not just make > all_counts the default: the all_counts / all_layouts logic is rather > complex, and with all_counts the default it becomes essentially dead > code. But the decision comes first. > There can be filters where .query_format is defined and they still can refer to all_formats and not all_counts, so i am not sure we can remove the all_formats/all_counts logic so easily. Anyway, how can we move forward here? Apparently there is not too much interest in the topic... Since this can be easily changed later, I can also rework the patch to add the .query_formats callback to all filters currently not supporting unkown layouts until this is decided. What do you think? Obviously I prefer my original approach, but since it is not too much work, I can change the patch as well. Thanks, Marton
Le nonidi 9 frimaire, an CCXXV, Marton Balint a écrit : > There can be filters where .query_format is defined and they still can refer > to all_formats and not all_counts, so i am not sure we can remove the > all_formats/all_counts logic so easily. There is no difference between these filters and those who just do not have the query_format method: they need small adjustments. Or do you think there are filters that can not be adapted to work with unknown layouts? > Anyway, how can we move forward here? Apparently there is not too much > interest in the topic... Since this can be easily changed later, I can also > rework the patch to add the .query_formats callback to all filters currently > not supporting unkown layouts until this is decided. What do you think? > Obviously I prefer my original approach, but since it is not too much work, > I can change the patch as well. At this point, the issue is no longer technical. I have summarized the pros and cons of both sides, and my role as "the guy who knows that part of the code best" ends here. Now people must decide which side is preferred, and I have no particular authority to do that. Once the direction is decided, we can discuss the implementation details. Regards,
On Wed, 30 Nov 2016, Nicolas George wrote: > Le nonidi 9 frimaire, an CCXXV, Marton Balint a écrit : >> There can be filters where .query_format is defined and they still can refer >> to all_formats and not all_counts, so i am not sure we can remove the >> all_formats/all_counts logic so easily. > > There is no difference between these filters and those who just do not > have the query_format method: they need small adjustments. > > Or do you think there are filters that can not be adapted to work with > unknown layouts? E.g. loudnorm or ebur128 cannot simply work with unkown layouts, since coefficients are channel dependant. > >> Anyway, how can we move forward here? Apparently there is not too much >> interest in the topic... Since this can be easily changed later, I can also >> rework the patch to add the .query_formats callback to all filters currently >> not supporting unkown layouts until this is decided. What do you think? >> Obviously I prefer my original approach, but since it is not too much work, >> I can change the patch as well. > > At this point, the issue is no longer technical. I have summarized the > pros and cons of both sides, and my role as "the guy who knows that part > of the code best" ends here. Now people must decide which side is > preferred, and I have no particular authority to do that. > > Once the direction is decided, we can discuss the implementation > details. Since nobody else commented, I assume they don't have strong opinions, therefore I suggest we decide. I prefer the all-accepting default, but if you disagree, I accept that as well. What do you prefer? If you don't have a preference, I will follow my own approach and apply the patch which changed the default. Thanks, Marton
On Wed, Nov 30, 2016 at 08:29:52PM +0100, Marton Balint wrote: > > On Wed, 30 Nov 2016, Nicolas George wrote: > > >Le nonidi 9 frimaire, an CCXXV, Marton Balint a écrit : > >>There can be filters where .query_format is defined and they still can refer > >>to all_formats and not all_counts, so i am not sure we can remove the > >>all_formats/all_counts logic so easily. > > > >There is no difference between these filters and those who just do not > >have the query_format method: they need small adjustments. > > > >Or do you think there are filters that can not be adapted to work with > >unknown layouts? > > E.g. loudnorm or ebur128 cannot simply work with unkown layouts, > since coefficients are channel dependant. > > > > >>Anyway, how can we move forward here? Apparently there is not too much > >>interest in the topic... Since this can be easily changed later, I can also > >>rework the patch to add the .query_formats callback to all filters currently > >>not supporting unkown layouts until this is decided. What do you think? > >>Obviously I prefer my original approach, but since it is not too much work, > >>I can change the patch as well. > > > >At this point, the issue is no longer technical. I have summarized the > >pros and cons of both sides, and my role as "the guy who knows that part > >of the code best" ends here. Now people must decide which side is > >preferred, and I have no particular authority to do that. > > > >Once the direction is decided, we can discuss the implementation > >details. > > Since nobody else commented, I assume they don't have strong > opinions, therefore I suggest we decide. I prefer the all-accepting > default, but if you disagree, I accept that as well. What do you > prefer? If you don't have a preference, I will follow my own > approach and apply the patch which changed the default. I would pick the choice that the developers prefer, whatever that is. libavfilter is primarly developed in FFmpeg. [...]
diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c index 3af698d..379bc1a 100644 --- a/libavfilter/avfiltergraph.c +++ b/libavfilter/avfiltergraph.c @@ -802,8 +802,7 @@ static int reduce_formats_on_filter(AVFilterContext *filter) if (inlink->type != outlink->type || fmts->nb_channel_layouts == 1) continue; - if (fmts->all_layouts && - (!FF_LAYOUT2COUNT(fmt) || fmts->all_counts)) { + if (fmts->all_layouts) { /* Turn the infinite list into a singleton */ fmts->all_layouts = fmts->all_counts = 0; if (ff_add_channel_layout(&outlink->in_channel_layouts, fmt) < 0)
This reverts d300f5f6f570659e4b58567b35c9e8600c9f2956. Further reference: https://ffmpeg.org/pipermail/ffmpeg-devel/2013-October/149935.html I can't reproduce ticket #2899 so I am not sure the original patch is still needed. Reverting it fixes unknown channel layout support for trivial resamples, such as: ffmpeg -f lavfi -i "aevalsrc=0|0|0|0|0|0|0|0|0:d=1,asetnsamples" -f null none Signed-off-by: Marton Balint <cus@passwd.hu> --- libavfilter/avfiltergraph.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)