diff mbox series

[FFmpeg-devel,v1,1/4] avfilter/af_loudnorm: Add file option for the measured stats

Message ID 20200409110720.6965-1-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v1,1/4] avfilter/af_loudnorm: Add file option for the measured stats | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Lance Wang April 9, 2020, 11:07 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/filters.texi          |  3 +++
 libavfilter/af_loudnorm.c | 54 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 55 insertions(+), 2 deletions(-)

Comments

Nicolas George April 9, 2020, 11:16 a.m. UTC | #1 
lance.lmwang@gmail.com (12020-04-09):
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  doc/filters.texi          |  3 +++
>  libavfilter/af_loudnorm.c | 54 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 55 insertions(+), 2 deletions(-)

NACK.

If we are doing something like that, we should be doing it in a way that
works for all filters that output statistics, not just one: common
options name and syntax, common code.

Doing it that way will only result in similar filters having different
features in a way that no user can understand.

Same for the compact output.

Regards,
Lance Wang April 9, 2020, 1:44 p.m. UTC | #2 
On Thu, Apr 09, 2020 at 01:16:16PM +0200, Nicolas George wrote:
> lance.lmwang@gmail.com (12020-04-09):
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  doc/filters.texi          |  3 +++
> >  libavfilter/af_loudnorm.c | 54 +++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 55 insertions(+), 2 deletions(-)
> 
> NACK.
> 
> If we are doing something like that, we should be doing it in a way that
> works for all filters that output statistics, not just one: common
> options name and syntax, common code.
> 
> Doing it that way will only result in similar filters having different
> features in a way that no user can understand.
> 
> Same for the compact output.

My patch set try to make the loudnorm filter easy to use for end user if they
want to process the volume in two pass. I have no clue how to make it common
yet.


> 
> Regards,
> 
> -- 
>   Nicolas George
Nicolas George April 9, 2020, 5:57 p.m. UTC | #3 
Limin Wang (12020-04-09):
> My patch set try to make the loudnorm filter easy to use for end user if they
> want to process the volume in two pass.

Yes: you are seeing a problem, and you want to fix it, that's fine.

But the problem you are seeing is part of a wider problem, and fixing
the part you see will make fixing the whole problem harder.

We did things like that many time, and it always caused problems. Let us
not do it again. Since your goal is "make things easier" and not "fix an
exploitable crash", we can take time to do things properly.

>					  I have no clue how to make it common
> yet.

Well, let us start discussing it right now.

There is JSON-specific code in af_loudnorm.c, there should not be, I
think we should start here.

My opinion is we should rework ffprobe's writers and move them to lavu.
That would be the first step.

What do you think?

Regards,
Kyle Swanson April 9, 2020, 10:07 p.m. UTC | #4 
Hi,

On Thu, Apr 9, 2020 at 10:57 AM Nicolas George <george@nsup.org> wrote:
> There is JSON-specific code in af_loudnorm.c, there should not be, I
> think we should start here.
>
> My opinion is we should rework ffprobe's writers and move them to lavu.
> That would be the first step.

Is there already a helper API for filters (or codecs) that output a
statistics file? If not, that's probably a pretty big undertaking. In
the codec case, there are probably stats files which we have no
control over, since they are written directly by some linked library.
As an aside, I've often thought about improving the 2-pass version of
this filter and not reuse the AGC on pass 2. A stats file would be a
requirement for this.

Thanks,
Kyle
Lance Wang April 10, 2020, 1:22 a.m. UTC | #5 
On Thu, Apr 09, 2020 at 07:57:14PM +0200, Nicolas George wrote:
> Limin Wang (12020-04-09):
> > My patch set try to make the loudnorm filter easy to use for end user if they
> > want to process the volume in two pass.
> 
> Yes: you are seeing a problem, and you want to fix it, that's fine.
> 
> But the problem you are seeing is part of a wider problem, and fixing
> the part you see will make fixing the whole problem harder.
> 
> We did things like that many time, and it always caused problems. Let us
> not do it again. Since your goal is "make things easier" and not "fix an
> exploitable crash", we can take time to do things properly.
> 
> >					  I have no clue how to make it common
> > yet.
> 
> Well, let us start discussing it right now.
> 
> There is JSON-specific code in af_loudnorm.c, there should not be, I
> think we should start here.
> 
> My opinion is we should rework ffprobe's writers and move them to lavu.
> That would be the first step.

If someone can rework the ffprobe writers with a helper function, I'm 
glad to use it. I'm not code maintainer, if no other developer review
my code and help to push it, then it's difficult to merge. The time cycle
is too long, so I have no plan to do such work.

> 
> What do you think?
> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas George April 10, 2020, 10:36 a.m. UTC | #6 
Kyle Swanson (12020-04-09):
> Is there already a helper API for filters (or codecs) that output a
> statistics file? If not, that's probably a pretty big undertaking. In

I do not think it is that big a task. It needs a convention for the name
of the option(s), a convention for the name of the file(s), and a few
functions to write and read them.

> the codec case, there are probably stats files which we have no
> control over, since they are written directly by some linked library.

Since they are out of our control, they should have no impact on our
development choices.

Regards,
Nicolas George April 10, 2020, 10:37 a.m. UTC | #7 
Limin Wang (12020-04-10):
> If someone can rework the ffprobe writers with a helper function, I'm 
> glad to use it. I'm not code maintainer, if no other developer review
> my code and help to push it, then it's difficult to merge. The time cycle
> is too long, so I have no plan to do such work.

Ok. This is something I was intending to work on anyway, and it seems
ripe. I will bring it to the mailing-list.

Regards,
diff mbox series

Patch

diff --git a/doc/filters.texi b/doc/filters.texi
index 3931d8d79e..738a40df4e 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -4285,6 +4285,9 @@  Options are true or false. Default is false.
 @item print_format
 Set print format for stats. Options are summary, json, or none.
 Default value is none.
+
+@item file, f
+Set file path for the measured stats
 @end table
 
 @section lowpass
diff --git a/libavfilter/af_loudnorm.c b/libavfilter/af_loudnorm.c
index 314b25fa39..bf530195e4 100644
--- a/libavfilter/af_loudnorm.c
+++ b/libavfilter/af_loudnorm.c
@@ -20,6 +20,8 @@ 
 
 /* http://k.ylo.ph/2016/04/04/loudnorm.html */
 
+#include "libavutil/avstring.h"
+#include "libavformat/avio.h"
 #include "libavutil/opt.h"
 #include "avfilter.h"
 #include "internal.h"
@@ -62,6 +64,9 @@  typedef struct LoudNormContext {
     int linear;
     int dual_mono;
     enum PrintFormat print_format;
+    AVIOContext* pb;
+    char *filename;
+    void (*print)(AVFilterContext *ctx, const char *msg, ...) av_printf_format(2, 3);
 
     double *buf;
     int buf_size;
@@ -119,6 +124,8 @@  static const AVOption loudnorm_options[] = {
     {     "none",         0,                                   0,                        AV_OPT_TYPE_CONST,   {.i64 =  NONE},     0,         0,  FLAGS, "print_format" },
     {     "json",         0,                                   0,                        AV_OPT_TYPE_CONST,   {.i64 =  JSON},     0,         0,  FLAGS, "print_format" },
     {     "summary",      0,                                   0,                        AV_OPT_TYPE_CONST,   {.i64 =  SUMMARY},  0,         0,  FLAGS, "print_format" },
+    { "file", "set file path for the measured stats",          OFFSET(filename),         AV_OPT_TYPE_STRING,  {.str=NULL},                       FLAGS },
+    { "f",    "set file path for the measured stats",          OFFSET(filename),         AV_OPT_TYPE_STRING,  {.str=NULL},                       FLAGS },
     { NULL }
 };
 
@@ -781,6 +788,30 @@  static int config_input(AVFilterLink *inlink)
     return 0;
 }
 
+static void print_log(AVFilterContext *ctx, const char *msg, ...)
+{
+    va_list va;
+
+    va_start(va, msg);
+    if (msg)
+        av_vlog(ctx, AV_LOG_INFO, msg, va);
+    va_end(va);
+}
+
+static void print_file(AVFilterContext *ctx, const char *msg, ...)
+{
+    LoudNormContext *s = ctx->priv;
+    va_list va;
+
+    va_start(va, msg);
+    if (msg) {
+        char buf[1024];
+        vsnprintf(buf, sizeof(buf), msg, va);
+        avio_write(s->pb, buf, av_strnlen(buf, sizeof(buf)));
+    }
+    va_end(va);
+}
+
 static av_cold int init(AVFilterContext *ctx)
 {
     LoudNormContext *s = ctx->priv;
@@ -799,6 +830,22 @@  static av_cold int init(AVFilterContext *ctx)
         }
     }
 
+    if (s->print_format != NONE && s->filename) {
+        s->print = print_file;
+    } else {
+        s->print = print_log;
+    }
+
+    if (s->filename) {
+        int ret = avio_open(&s->pb, s->filename, AVIO_FLAG_WRITE);
+        if (ret < 0) {
+            char buf[128];
+            av_strerror(ret, buf, sizeof(buf));
+            av_log(ctx, AV_LOG_ERROR, "Could not open %s: %s\n", s->filename, buf);
+            return ret;
+        }
+    }
+
     return 0;
 }
 
@@ -836,7 +883,7 @@  static av_cold void uninit(AVFilterContext *ctx)
         break;
 
     case JSON:
-        av_log(ctx, AV_LOG_INFO,
+        s->print(ctx,
             "\n{\n"
             "\t\"input_i\" : \"%.2f\",\n"
             "\t\"input_tp\" : \"%.2f\",\n"
@@ -863,7 +910,7 @@  static av_cold void uninit(AVFilterContext *ctx)
         break;
 
     case SUMMARY:
-        av_log(ctx, AV_LOG_INFO,
+        s->print(ctx,
             "\n"
             "Input Integrated:   %+6.1f LUFS\n"
             "Input True Peak:    %+6.1f dBTP\n"
@@ -899,6 +946,9 @@  end:
     av_freep(&s->limiter_buf);
     av_freep(&s->prev_smp);
     av_freep(&s->buf);
+
+    if (s->pb)
+        avio_closep(&s->pb);
 }
 
 static const AVFilterPad avfilter_af_loudnorm_inputs[] = {