[FFmpeg-devel,v2,6/8] avfilter/silencedetect: fix for ticket 6968 Fix missing log of silence_end at end of stream

Submitted by Gaullier Nicolas on Feb. 12, 2018, 9:48 a.m.

Details

Message ID 20180212094830.9304-6-nicolas.gaullier@arkena.com
State New
Headers show

Commit Message

Gaullier Nicolas Feb. 12, 2018, 9:48 a.m.
From: nicolas gaullier <nicolas.gaullier@arkena.com>

---
 libavfilter/af_silencedetect.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

Comments

Carl Eugen Hoyos Feb. 12, 2018, 1:52 p.m.
2018-02-12 10:48 GMT+01:00 Nicolas Gaullier <nicolas.gaullier@arkena.com>:
> From: nicolas gaullier <nicolas.gaullier@arkena.com>

Please mention ticket #6968 in the commit message if it is related.

Carl Eugen
Gaullier Nicolas Feb. 12, 2018, 3:33 p.m.
Here, the "fix for ticket 6968" should be the header, so I think the format is correct ? Everything behind  ("Fix missing...") should be the body of the commit message.
NB: thanks to you to have reported it previously, I was not aware of this ticket when I started work on this issue!

Nicolas Gaullier
-----Message d'origine-----
De : ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] De la part de Carl Eugen Hoyos
Envoyé : lundi 12 février 2018 14:52
À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Objet : Re: [FFmpeg-devel] [PATCH v2 6/8] avfilter/silencedetect: fix for ticket 6968 Fix missing log of silence_end at end of stream

2018-02-12 10:48 GMT+01:00 Nicolas Gaullier <nicolas.gaullier@arkena.com>:
> From: nicolas gaullier <nicolas.gaullier@arkena.com>


Please mention ticket #6968 in the commit message if it is related.

Carl Eugen
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Carl Eugen Hoyos Feb. 12, 2018, 9:42 p.m.
2018-02-12 16:33 GMT+01:00 Gaullier Nicolas <nicolas.gaullier@arkena.com>:
> Here, the "fix for ticket 6968" should be the header, so I think the format is correct ?

I simply missed it, but a better alternative is:
lavfi/silencedetect: Fix missing log at eos.

Fixes ticket #6968.

Carl Eugen
Michael Niedermayer Feb. 13, 2018, 3:26 p.m.
On Mon, Feb 12, 2018 at 10:48:28AM +0100, Nicolas Gaullier wrote:
> From: nicolas gaullier <nicolas.gaullier@arkena.com>
> 
> ---
>  libavfilter/af_silencedetect.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
> index 8973049fe5..723136c2a8 100644
> --- a/libavfilter/af_silencedetect.c
> +++ b/libavfilter/af_silencedetect.c
> @@ -41,7 +41,9 @@ typedef struct SilenceDetectContext {
>      int independant_channels;   ///< number of entries in following arrays (always 1 in mono mode)
>      int64_t *nb_null_samples;   ///< (array) current number of continuous zero samples
>      int64_t *start;             ///< (array) if silence is detected, this value contains the time of the first zero sample (default/unset = INT64_MIN)
> +    int64_t frame_end;          ///< pts of the end of the current frame (used to compute duration of silence at EOS)
>      int last_sample_rate;       ///< last sample rate to check for sample rate changes
> +    AVRational time_base;       ///< time_base
>  
>      void (*silencedetect)(struct SilenceDetectContext *s, AVFrame *insamples,
>                            int nb_samples, int64_t nb_samples_notify,
> @@ -92,13 +94,16 @@ static av_always_inline void update(SilenceDetectContext *s, AVFrame *insamples,
>          }
>      } else {
>          if (s->start[channel] > INT64_MIN) {
> -            int64_t end_pts = insamples->pts + av_rescale_q(current_sample / s->channels,
> -                    (AVRational){ 1, s->last_sample_rate }, time_base);
> +            int64_t end_pts = insamples ? insamples->pts + av_rescale_q(current_sample / s->channels,
> +                    (AVRational){ 1, s->last_sample_rate }, time_base)
> +                    : s->frame_end;
>              int64_t duration_ts = end_pts - s->start[channel];
> -            set_meta(insamples, s->mono ? channel + 1 : 0, "silence_end",
> -                    av_ts2timestr(end_pts, &time_base));
> -            set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
> -                    av_ts2timestr(duration_ts, &time_base));
> +            if (insamples) {
> +                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_end",

> +                        av_ts2timestr(end_pts, &time_base));
> +                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
> +                        av_ts2timestr(duration_ts, &stime_base));
                                                       ^
this does not build, and looks like a typo

[...]
Gaullier Nicolas Feb. 13, 2018, 5:19 p.m.
> +                        av_ts2timestr(duration_ts, &stime_base));
                                                       ^ this does not build, and looks like a typo

Sorry, this is ugly, I had indeed replaced manually two occurrences (s->time_base => &time_base) at the last minute just before sending the email...
By the way, at the end of this set of patch, I am wondering if a "somewhat cosmetic" patch which would now remove some 'time_base' local arguments by using the s->time_base from the context would be interesting to have the code little more consistent.

I send right now the fixed patch as v3 including carl eugen's feedback on commit msg.

Thank you
Nicolas Gaullier

Patch hide | download patch | download mbox

diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
index 8973049fe5..723136c2a8 100644
--- a/libavfilter/af_silencedetect.c
+++ b/libavfilter/af_silencedetect.c
@@ -41,7 +41,9 @@  typedef struct SilenceDetectContext {
     int independant_channels;   ///< number of entries in following arrays (always 1 in mono mode)
     int64_t *nb_null_samples;   ///< (array) current number of continuous zero samples
     int64_t *start;             ///< (array) if silence is detected, this value contains the time of the first zero sample (default/unset = INT64_MIN)
+    int64_t frame_end;          ///< pts of the end of the current frame (used to compute duration of silence at EOS)
     int last_sample_rate;       ///< last sample rate to check for sample rate changes
+    AVRational time_base;       ///< time_base
 
     void (*silencedetect)(struct SilenceDetectContext *s, AVFrame *insamples,
                           int nb_samples, int64_t nb_samples_notify,
@@ -92,13 +94,16 @@  static av_always_inline void update(SilenceDetectContext *s, AVFrame *insamples,
         }
     } else {
         if (s->start[channel] > INT64_MIN) {
-            int64_t end_pts = insamples->pts + av_rescale_q(current_sample / s->channels,
-                    (AVRational){ 1, s->last_sample_rate }, time_base);
+            int64_t end_pts = insamples ? insamples->pts + av_rescale_q(current_sample / s->channels,
+                    (AVRational){ 1, s->last_sample_rate }, time_base)
+                    : s->frame_end;
             int64_t duration_ts = end_pts - s->start[channel];
-            set_meta(insamples, s->mono ? channel + 1 : 0, "silence_end",
-                    av_ts2timestr(end_pts, &time_base));
-            set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
-                    av_ts2timestr(duration_ts, &time_base));
+            if (insamples) {
+                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_end",
+                        av_ts2timestr(end_pts, &time_base));
+                set_meta(insamples, s->mono ? channel + 1 : 0, "silence_duration",
+                        av_ts2timestr(duration_ts, &stime_base));
+            }
             if (s->mono)
                 av_log(s, AV_LOG_INFO, "channel: %d | ", channel);
             av_log(s, AV_LOG_INFO, "silence_end: %s | silence_duration: %s\n",
@@ -177,6 +182,9 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *insamples)
             s->nb_null_samples[c] = srate * s->nb_null_samples[c] / s->last_sample_rate;
         }
     s->last_sample_rate = srate;
+    s->time_base = inlink->time_base;
+    s->frame_end = insamples->pts + av_rescale_q(insamples->nb_samples,
+            (AVRational){ 1, s->last_sample_rate }, inlink->time_base);
 
     // TODO: document metadata
     s->silencedetect(s, insamples, nb_samples, nb_samples_notify,
@@ -218,6 +226,18 @@  static int query_formats(AVFilterContext *ctx)
     return ff_set_common_samplerates(ctx, formats);
 }
 
+static av_cold void uninit(AVFilterContext *ctx)
+{
+    SilenceDetectContext *s = ctx->priv;
+    int c;
+
+    for (c = 0; c < s->independant_channels; c++)
+        if (s->start[c] > INT64_MIN)
+            update(s, NULL, 0, c, 0, s->time_base);
+    av_freep(&s->nb_null_samples);
+    av_freep(&s->start);
+}
+
 static const AVFilterPad silencedetect_inputs[] = {
     {
         .name         = "default",
@@ -241,6 +261,7 @@  AVFilter ff_af_silencedetect = {
     .description   = NULL_IF_CONFIG_SMALL("Detect silence."),
     .priv_size     = sizeof(SilenceDetectContext),
     .query_formats = query_formats,
+    .uninit        = uninit,
     .inputs        = silencedetect_inputs,
     .outputs       = silencedetect_outputs,
     .priv_class    = &silencedetect_class,