| Message ID | 20220628121400.20444-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] fftools/ffmpeg_filter: Fix audio_drift_threshold check | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/configure_x86 | warning | Failed to apply patch |
| andriy/configure_armv7_RPi4 | warning | Failed to apply patch |
Variant 2 is less bad, but the whole check seems hacky to me, since it seems to make assumptions about swr defaults Won't setting this unconditionally have the same effect?
On Thu, Jun 30, 2022 at 10:55:46AM +0200, Anton Khirnov wrote: > Variant 2 is less bad, but the whole check seems hacky to me, since it > seems to make assumptions about swr defaults > > Won't setting this unconditionally have the same effect? it has the same effect but its not so nice to the user to recommand extra arguments which make no difference thx [...]
Quoting Michael Niedermayer (2022-07-01 21:53:00) > On Thu, Jun 30, 2022 at 10:55:46AM +0200, Anton Khirnov wrote: > > Variant 2 is less bad, but the whole check seems hacky to me, since it > > seems to make assumptions about swr defaults > > > > Won't setting this unconditionally have the same effect? > > it has the same effect but its not so nice to the user to recommand extra > arguments which make no difference Sorry, I don't follow. What is recommending any arguments to the user here?
On Sat, Jul 02, 2022 at 10:38:26AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-07-01 21:53:00) > > On Thu, Jun 30, 2022 at 10:55:46AM +0200, Anton Khirnov wrote: > > > Variant 2 is less bad, but the whole check seems hacky to me, since it > > > seems to make assumptions about swr defaults > > > > > > Won't setting this unconditionally have the same effect? > > > > it has the same effect but its not so nice to the user to recommand extra > > arguments which make no difference > > Sorry, I don't follow. What is recommending any arguments to the user > here? i meant this thing here: ./ffmpeg -i matrixbench_mpeg2.mpg -async 1 -f null - 2>&1 | grep async -async is forwarded to lavfi similarly to -af aresample=async=1:min_hard_comp=0.100000:first_pts=0. vs. -async is forwarded to lavfi similarly to -af aresample=async=1:first_pts=0. [...]
Quoting Michael Niedermayer (2022-07-02 14:56:50) > On Sat, Jul 02, 2022 at 10:38:26AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2022-07-01 21:53:00) > > > On Thu, Jun 30, 2022 at 10:55:46AM +0200, Anton Khirnov wrote: > > > > Variant 2 is less bad, but the whole check seems hacky to me, since it > > > > seems to make assumptions about swr defaults > > > > > > > > Won't setting this unconditionally have the same effect? > > > > > > it has the same effect but its not so nice to the user to recommand extra > > > arguments which make no difference > > > > Sorry, I don't follow. What is recommending any arguments to the user > > here? > > i meant this thing here: > ./ffmpeg -i matrixbench_mpeg2.mpg -async 1 -f null - 2>&1 | grep async > > -async is forwarded to lavfi similarly to -af aresample=async=1:min_hard_comp=0.100000:first_pts=0. > > vs. > > -async is forwarded to lavfi similarly to -af aresample=async=1:first_pts=0. I don't see a problem - why would the user care how it is forwarded?
On Tue, Jul 05, 2022 at 06:58:28PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-07-02 14:56:50) > > On Sat, Jul 02, 2022 at 10:38:26AM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2022-07-01 21:53:00) > > > > On Thu, Jun 30, 2022 at 10:55:46AM +0200, Anton Khirnov wrote: > > > > > Variant 2 is less bad, but the whole check seems hacky to me, since it > > > > > seems to make assumptions about swr defaults > > > > > > > > > > Won't setting this unconditionally have the same effect? > > > > > > > > it has the same effect but its not so nice to the user to recommand extra > > > > arguments which make no difference > > > > > > Sorry, I don't follow. What is recommending any arguments to the user > > > here? > > > > i meant this thing here: > > ./ffmpeg -i matrixbench_mpeg2.mpg -async 1 -f null - 2>&1 | grep async > > > > -async is forwarded to lavfi similarly to -af aresample=async=1:min_hard_comp=0.100000:first_pts=0. > > > > vs. > > > > -async is forwarded to lavfi similarly to -af aresample=async=1:first_pts=0. > > I don't see a problem - why would the user care how it is forwarded? The user may want to perform the equivalent operation inside a filter chain/graph or with a tool different from ffmpeg or even via the API its really a very minor thing if these defaults are displayed too, it just feels a bit cleaner to skip them thx [...]
Quoting Michael Niedermayer (2022-07-06 00:36:25) > On Tue, Jul 05, 2022 at 06:58:28PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2022-07-02 14:56:50) > > > On Sat, Jul 02, 2022 at 10:38:26AM +0200, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2022-07-01 21:53:00) > > > > > On Thu, Jun 30, 2022 at 10:55:46AM +0200, Anton Khirnov wrote: > > > > > > Variant 2 is less bad, but the whole check seems hacky to me, since it > > > > > > seems to make assumptions about swr defaults > > > > > > > > > > > > Won't setting this unconditionally have the same effect? > > > > > > > > > > it has the same effect but its not so nice to the user to recommand extra > > > > > arguments which make no difference > > > > > > > > Sorry, I don't follow. What is recommending any arguments to the user > > > > here? > > > > > > i meant this thing here: > > > ./ffmpeg -i matrixbench_mpeg2.mpg -async 1 -f null - 2>&1 | grep async > > > > > > -async is forwarded to lavfi similarly to -af aresample=async=1:min_hard_comp=0.100000:first_pts=0. > > > > > > vs. > > > > > > -async is forwarded to lavfi similarly to -af aresample=async=1:first_pts=0. > > > > I don't see a problem - why would the user care how it is forwarded? > > The user may want to perform the equivalent operation inside a filter chain/graph > or with a tool different from ffmpeg or even via the API > its really a very minor thing if these defaults are displayed too, it just > feels a bit cleaner to skip them Then the correct thing to do is retrieve the swr default value via the AVOption API. Though IMO it's extra complexity for marginal gain. I also wonder if this option should exist at all, given that it does nothing but set a swr option. It is also global, so you can't have different values for different streams.
diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c index 0845c631a5..a204c4f65f 100644 --- a/fftools/ffmpeg_filter.c +++ b/fftools/ffmpeg_filter.c @@ -18,6 +18,7 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <float.h> #include <stdint.h> #include "ffmpeg.h" @@ -891,7 +892,7 @@ static int configure_input_audio_filter(FilterGraph *fg, InputFilter *ifilter, char args[256] = {0}; av_strlcatf(args, sizeof(args), "async=%d", audio_sync_method); - if (audio_drift_threshold != 0.1) + if (fabs(audio_drift_threshold - 0.1) > FLT_EPSILON) av_strlcatf(args, sizeof(args), ":min_hard_comp=%f", audio_drift_threshold); if (!fg->reconfiguration) av_strlcatf(args, sizeof(args), ":first_pts=0");
Variant 2 Found-by: mkver Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- fftools/ffmpeg_filter.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)