[FFmpeg-devel] [PATCH 3/4] lavfi/silencedetect: export silence info to metadata.

[Stefano Sabatini]

On date Thursday 2012-10-11 22:18:02 +0200, Clément Bœsch encoded:
> On Thu, Oct 11, 2012 at 11:55:05AM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2012-10-10 00:55:12 +0200, Clément Bœsch encoded:
> > > ---
> > >  libavfilter/af_silencedetect.c | 23 ++++++++++++++++++-----
> > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/libavfilter/af_silencedetect.c b/libavfilter/af_silencedetect.c
> > > index 2ea5d7f..7f1a8c4 100644
> > > --- a/libavfilter/af_silencedetect.c
> > > +++ b/libavfilter/af_silencedetect.c
> > > @@ -78,6 +78,12 @@ static av_cold int init(AVFilterContext *ctx, const char *args)
> > >      return 0;
> > >  }
> > >  
> > > +static char *get_meta_val(AVFilterBufferRef *insamples, const char *key)
> > 
> > static inline char *get_meta_val(const AVFilterBufferRef *insamples, const char *key) av_const

nit: get_metadata_val()

> OK for inline & const buf ref, but what's this av_const?

Feel free to discard, it says that the function has no side effects,
and thus code like this:
get_meta_val(...);

will issue a warning (statement has no effect).

> 
> > > +{
> > > +    AVDictionaryEntry *e = av_dict_get(insamples->metadata, key, NULL, 0);
> > > +    return e && e->value ? e->value : NULL;
> > > +}
> > > +
> > >  static int filter_samples(AVFilterLink *inlink, AVFilterBufferRef *insamples)
> > >  {
> > >      int i;
> > > @@ -103,16 +109,23 @@ static int filter_samples(AVFilterLink *inlink, AVFilterBufferRef *insamples)
> > >                      silence->nb_null_samples++;
> > >                      if (silence->nb_null_samples >= nb_samples_notify) {
> > >                          silence->start = insamples->pts - silence->duration / av_q2d(inlink->time_base);
> > > -                        av_log(silence, AV_LOG_INFO,
> > > -                               "silence_start: %s\n", av_ts2timestr(silence->start, &inlink->time_base));
> > > +                        av_dict_set(&insamples->metadata, "lavfi.silence_start",
> > > +                                    av_ts2timestr(silence->start, &inlink->time_base), 0);
> > > +                        av_log(silence, AV_LOG_INFO, "silence_start: %s\n",
> > > +                               get_meta_val(insamples, "lavfi.silence_start"));
> > >                      }
> > >                  }
> > >              } else {
> > > -                if (silence->start)
> > > +                if (silence->start) {
> > 
> > > +                    av_dict_set(&insamples->metadata, "lavfi.silence_end",
> > > +                                av_ts2timestr(silence->start, &inlink->time_base), 0);
> > 
> > end/start mismatch?
> 
> Yeah derp fixed locally, silence->start → insamples->pts.
> 
> > 
> > > +                    av_dict_set(&insamples->metadata, "lavfi.silence_duration",
> > > +                                av_ts2timestr(insamples->pts - silence->start, &inlink->time_base), 0);
> > >                      av_log(silence, AV_LOG_INFO,
> > >                             "silence_end: %s | silence_duration: %s\n",
> > > -                           av_ts2timestr(insamples->pts,                  &inlink->time_base),
> > > -                           av_ts2timestr(insamples->pts - silence->start, &inlink->time_base));
> > > +                           get_meta_val(insamples, "lavfi.silence_end"),
> > > +                           get_meta_val(insamples, "lavfi.silence_duration"));
> > > +                }
> > >                  silence->nb_null_samples = silence->start = 0;
> > >              }
> > >          }
> > 
> > Note: we should document this stuff in filters.texi, or people will
> > never realize the existence of this feature.
> > 
> 
> Yes but I was waiting for more filters to implement such features first:
> we might want to change the key formatting pretty soon (for example), or a
> few more things before it's stable. Also, we will likely need a new
> section to talk about the metadata, so I'd better wait a little for this
> if you don't mind.

OK.

> 
> > LGTM otherwise.

Still LGTM.
-- 
FFmpeg = Frenzy and Fast Maxi Philosophical Empowered Gem