| Message ID | 1470588939-19655-1-git-send-email-pburt0@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Le primidi 21 thermidor, an CCXXIV, Burt P a écrit : > Signed-off-by: Burt P <pburt0@gmail.com> > --- > libavfilter/af_hdcd.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavfilter/af_hdcd.c b/libavfilter/af_hdcd.c > index e4e37e2..ebfe0f1 100644 > --- a/libavfilter/af_hdcd.c > +++ b/libavfilter/af_hdcd.c > @@ -1714,7 +1714,9 @@ static int config_input(AVFilterLink *inlink) { > AVFilterLink *lk = inlink; > while(lk != NULL) { > AVFilterContext *nextf = lk->src; > - if (lk->format != AV_SAMPLE_FMT_S16 || lk->sample_rate != 44100) { > + int sfok = (lk->format == AV_SAMPLE_FMT_S16 || > + lk->format == AV_SAMPLE_FMT_S16P); > + if ( !sfok || lk->sample_rate != 44100) { > av_log(ctx, AV_LOG_WARNING, "An input format is %s@%dHz at %s. It will truncated/resampled to s16@44100Hz.\n", > av_get_sample_fmt_name(lk->format), lk->sample_rate, > (nextf->name) ? nextf->name : "<unknown>" Sorry if it has been addressed before, but what are these tests? Why is this filter invading other filters' privacy? Regards,
On Sun, Aug 7, 2016 at 12:20 PM, Nicolas George <george@nsup.org> wrote: > Sorry if it has been addressed before, but what are these tests? Why is this > filter invading other filters' privacy? The HDCD codes are stored in the LSB of consecutive samples, and anything that would change a sample could cause problems. So it looks through the AVFilterLink chain and warns if any resampling or truncation is happening that might destroy the HDCD code before or undo the filter's work after.
Le primidi 21 thermidor, an CCXXIV, Burt P. a écrit : > The HDCD codes are stored in the LSB of consecutive samples, and > anything that would change a sample could cause problems. So it looks > through the AVFilterLink chain and warns if any resampling or > truncation is happening that might destroy the HDCD code before or > undo the filter's work after. I understand the concern, but intruding in other filters is really outside from lavfi's design. Just to illustrate this point, the tests as they are could call av_get_sample_fmt_name() on something that is not a sample format. You can advise users to disable automatic conversions when using this filter (there is an API for that, but I think there is no user interface yet), but beyond that the policy should be to always leave enough rope to shoot oneself in the foot. Regards,
2016-08-07 19:20 GMT+02:00 Nicolas George <george@nsup.org>: > Sorry if it has been addressed before, but what are these tests? > Why is this filter invading other filters' privacy? i wanted to suggest since some time to remove sample_fmts_in[] (that is responsible for a possibly auto-inserted resampler) and instead error out if the input signal is not S16 (or S16P) 44100. Carl Eugen
Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : > i wanted to suggest since some time to remove sample_fmts_in[] > (that is responsible for a possibly auto-inserted resampler) and > instead error out if the input signal is not S16 (or S16P) 44100. That would not work: the pixel format can be constrained by any filter in the chain. Regards,
2016-08-07 23:07 GMT+02:00 Nicolas George <george@nsup.org>: > Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : >> i wanted to suggest since some time to remove sample_fmts_in[] >> (that is responsible for a possibly auto-inserted resampler) and >> instead error out if the input signal is not S16 (or S16P) 44100. > > That would not work: the pixel format can be constrained by any filter in > the chain. (Not sure if I understand correctly.) It would require the user to precisely declare what he wants which is a good idea in this case because the filter can only work for stereo s16 44100. Carl Eugen
Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : > It would require the user to precisely declare what he wants which > is a good idea in this case because the filter can only work for > stereo s16 44100. No, it would not do that: there may be another filter in the chain that forces a conversion to S16. To avoid automatic conversions, the correct thing to do is to disable automatic conversions. Regards,
2016-08-07 23:30 GMT+02:00 Nicolas George <george@nsup.org>: > Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : >> It would require the user to precisely declare what he wants which >> is a good idea in this case because the filter can only work for >> stereo s16 44100. > > No, it would not do that: there may be another filter in the chain that > forces a conversion to S16. I considered that acceptable but if better solutions exist (that are also acceptable), I am happy. Carl Eugen
Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : > I considered that acceptable but if better solutions exist (that are > also acceptable), I am happy. As I already pointed twice: /** * Enable or disable automatic format conversion inside the graph. * * Note that format conversion can still happen inside explicitly inserted * scale and aresample filters. * * @param flags any of the AVFILTER_AUTO_CONVERT_* constants */ void avfilter_graph_set_auto_convert(AVFilterGraph *graph, unsigned flags); enum { AVFILTER_AUTO_CONVERT_ALL = 0, /**< all automatic conversions enabled */ AVFILTER_AUTO_CONVERT_NONE = -1, /**< all automatic conversions disabled */ }; Right now, with the command-line ffmpeg tool, it is only accessible through this: -pix_fmt[:stream_specifier] format (input/output,per-stream) the encoder. If pix_fmt is prefixed by a "+", ffmpeg will exit with an error if the requested pixel format can not be selected, and automatic conversions inside filtergraphs are disabled. If pix_fmt is a single "+", ffmpeg selects the same pixel format as the input (or graph output) and automatic conversions are disabled. Implementing it the same way for audio should be rather straightforward. Also, I notice that the code for taking this flag into account have been lost some time ago during merges from the fork. Restoring it would be easy, though. I added it to my TODO, but anybody can do it earlier. Regards,
Hi! 2016-08-07 23:50 GMT+02:00 Nicolas George <george@nsup.org>: > Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : >> I considered that acceptable but if better solutions exist (that are >> also acceptable), I am happy. > > As I already pointed twice: > > /** > * Enable or disable automatic format conversion inside the graph. Wouldn't that disable any automatic conversion behind (after) the hdcd filter? If that is correct, I would not consider it a better solution. Carl Eugen
Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : > Wouldn't that disable any automatic conversion behind (after) the > hdcd filter? If that is correct, I would not consider it a better solution. The filter already checks for conversions after itself too. Conversion can still happen, but only at explicit points. Regards,
Are you now talking about plans of the future or this specific case? As it is, automatic conversion is very helpful, for example from WavePack, which uses s16p. This filter is only looking at the AVFilterLinks between filters, not at the filters themselves. This scan and warn behavior was added to address a real issue. Consider these example cases: ffmpeg -i hdcd16.wv -af hdcd hdcd24.flac * .wv uses s16p, auto-converted to s16, nice! * hdcd is decoded without problem * 24-bit flac is output, ok ffmpeg -i hdcd16.wv -af hdcd hdcd24-expected.wav * .wv uses s16p, auto-converted to s16, nice! * hdcd is decoded without problem * s32 is converted to s16 for wav by default, but user expects it to be more than 16-bit! --- a warning is issued about the truncation ffmpeg -i hdcd16.flac -af hdcd hdcdout.m4a * .flac has s16 samples, passed without problem * hdcd is decoded without problem * native aac encodes the full range, ok! ffmpeg -i hdcd16.flac -af hdcd -c:a libfdk_aac hdcd-dec.m4a * .flac has s16 samples, passed without problem * hdcd is decoded without problem * autoconverted to s16 for libfdk-aac, but the user doesn't know that the "best available aac encoder" only accepts s16 input! --- a warning is issued about the truncation ffmpeg -i anything_not_s16 -af hdcd hdcd24.flac * input is anything that is not likely to have hdcd * it is converted to s16 for the filter --- a warning is issued about the format conversion I think the automatic conversion from s16p, and into fltp when needed is very good and I'd like to keep it, but I would also like to warn when there might be a problem. As I understand it though, you think auto-inserted filters should be off, and the user will have to manually add conversions for WavePack/s16 and the like before, and conversion to fltp, s24, etc. after. On Sun, Aug 7, 2016 at 5:23 PM, Nicolas George <george@nsup.org> wrote: > Le primidi 21 thermidor, an CCXXIV, Carl Eugen Hoyos a écrit : >> Wouldn't that disable any automatic conversion behind (after) the >> hdcd filter? If that is correct, I would not consider it a better solution. > > The filter already checks for conversions after itself too. > > Conversion can still happen, but only at explicit points. > > Regards, > > -- > Nicolas George > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Le primidi 21 thermidor, an CCXXIV, Burt P. a écrit : > Are you now talking about plans of the future or this specific case? Both. > As it is, automatic conversion is very helpful, for example from > WavePack, which uses s16p. Apparently, not all of them are to your liking. FFmpeg can not guess which ones. You could try to teach it, but it may prove tricky. The best solution is to let the user choose, and that is exactly what disabling the automatic conversion does. Note that it does not disable conversions, only automatic ones. It also does not disable format negotiation. The only thing that changes is that the user has to choose where the conversion happens. > This filter is only looking at the AVFilterLinks between filters, not > at the filters themselves. And with lavfi's design, this is not allowed. As is, the filter is scanning links that it does not even know are audio, let alone are in any way connected to it. > This scan and warn behavior was added to address a real issue. That is more or less the Transportation Security Administration's motto, and we all know what a catastrophe that is. Good intentions are not enough to make a solution correct, and this solution is not correct at all. > Consider these example cases: None of them involve anything remotely complicated. Regards,
applied, as dbd7a84c814161926e5f298eae1f5ea17082f814, with an additional check that AVFilterLink->type is AVMEDIA_TYPE_AUDIO before calling av_get_sample_fmt_name() on AVFilterLink->format. Thanks for pointing that out. I will look into disabling auto-conversions when the filter is used and removing the invasive scan. On Mon, Aug 8, 2016 at 5:16 AM, Nicolas George <george@nsup.org> wrote: > Le primidi 21 thermidor, an CCXXIV, Burt P. a écrit : >> Are you now talking about plans of the future or this specific case? > > Both. > >> As it is, automatic conversion is very helpful, for example from >> WavePack, which uses s16p. > > Apparently, not all of them are to your liking. FFmpeg can not guess which > ones. You could try to teach it, but it may prove tricky. The best solution > is to let the user choose, and that is exactly what disabling the automatic > conversion does. > > Note that it does not disable conversions, only automatic ones. It also does > not disable format negotiation. The only thing that changes is that the user > has to choose where the conversion happens. > >> This filter is only looking at the AVFilterLinks between filters, not >> at the filters themselves. > > And with lavfi's design, this is not allowed. As is, the filter is scanning > links that it does not even know are audio, let alone are in any way > connected to it. > >> This scan and warn behavior was added to address a real issue. > > That is more or less the Transportation Security Administration's motto, and > we all know what a catastrophe that is. Good intentions are not enough to > make a solution correct, and this solution is not correct at all. > >> Consider these example cases: > > None of them involve anything remotely complicated. > > Regards, > > -- > Nicolas George > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavfilter/af_hdcd.c b/libavfilter/af_hdcd.c index e4e37e2..ebfe0f1 100644 --- a/libavfilter/af_hdcd.c +++ b/libavfilter/af_hdcd.c @@ -1714,7 +1714,9 @@ static int config_input(AVFilterLink *inlink) { AVFilterLink *lk = inlink; while(lk != NULL) { AVFilterContext *nextf = lk->src; - if (lk->format != AV_SAMPLE_FMT_S16 || lk->sample_rate != 44100) { + int sfok = (lk->format == AV_SAMPLE_FMT_S16 || + lk->format == AV_SAMPLE_FMT_S16P); + if ( !sfok || lk->sample_rate != 44100) { av_log(ctx, AV_LOG_WARNING, "An input format is %s@%dHz at %s. It will truncated/resampled to s16@44100Hz.\n", av_get_sample_fmt_name(lk->format), lk->sample_rate, (nextf->name) ? nextf->name : "<unknown>"
Signed-off-by: Burt P <pburt0@gmail.com> --- libavfilter/af_hdcd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)