[FFmpeg-devel] [PATCH] lavfi: add blackdetect filter

[Stefano Sabatini]

On date Saturday 2012-03-03 00:54:40 +0100, Clément Bœsch encoded:
> On Fri, Mar 02, 2012 at 04:46:20PM +0100, Stefano Sabatini wrote:
> > Address trac ticket #901.
> > ---
> >  doc/filters.texi             |   50 ++++++++++
> >  libavfilter/Makefile         |    1 +
> >  libavfilter/allfilters.c     |    1 +
> >  libavfilter/vf_blackdetect.c |  209 ++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 261 insertions(+), 0 deletions(-)
> >  create mode 100644 libavfilter/vf_blackdetect.c
> > 
> > diff --git a/doc/filters.texi b/doc/filters.texi
> > index 238401a..b5e8954 100644
> > --- a/doc/filters.texi
> > +++ b/doc/filters.texi
> > @@ -761,6 +761,56 @@ video, use the command:
> >  ass=sub.ass
> >  @end example
> >  
> > + at section blackdetect
> > +
> 
> Global comment: couldn't we have a "unified color" detector instead? So we
> could detect bright frames for instance, or full green ones.

This filter is optimized for working on the luma plane.

I could easily factorize the code and create a dual whitedetect
filter, do you think that would be useful? (Alternatively the user can
still invert the video with negate).

For a more general approach check the fish filter on the archive.

[...]
> > + at item pixel_black_th, pix_th
> > +Set the threshold for considering a pixel "black".
> > +
> > +Express the maximum pixel luminance value for which a pixel is
> > +considered "black". The value is scaled according to the pixel format
> > +luminance range, following the equation below:
> > + at example
> > + at var{absolute_threshold} = @var{minimum_luminance_value} + @var{pixel_black_th} * @var{luminance_range}
> > + at end example
> > +
> 
> The luminance_range is a nice thing; you might want to comment on it just
> a bit.
> 

Extended.

[...]
> > +typedef struct {
> > +    const AVClass *class;
> > +    double  min_black_duration_time; ///< minimum duration of detected black, in seconds
> > +    double  min_black_duration;      ///< minimum duration of detected black, in seconds
> 
> One of the comment looks wrong

Fixed.

> > +    int64_t black_start;             ///< pts start time of the first black picture
> > +    int64_t black_end;               ///< pts end time of the last black picture
> > +    int black_started;
> > +
> > +    double       picture_black_ratio_th;
> > +    double       pixel_black_th;
> > +    unsigned int pixel_black_th_i;
> > +
> > +    unsigned int frame_count;       ///< frame number
> > +    unsigned int nb_black_pixels;   ///< number of black pixels counted so far
> > +} BlackDetectContext;
> > +
> > +#define OFFSET(x) offsetof(BlackDetectContext, x)
> > +static const AVOption blackdetect_options[] = {
> > +    { "d",                  "set minimum detected black duration in seconds",  OFFSET(min_black_duration_time), AV_OPT_TYPE_DOUBLE, {.dbl=2}, 0, FLT_MAX},
> > +    { "min_black_duration", "set minimum detected black duration in seconds",  OFFSET(min_black_duration_time), AV_OPT_TYPE_DOUBLE, {.dbl=2}, 0, FLT_MAX},
> 
> Why not just "duration" for the long version? These option are in the
> filter string scope, I don't think they need a long namespace like this.

I prefer to keep consistency with the internal variable, then there is
the short version or we could add an alias.

> 
> Also, why FLT_MAX and not DBL_MAX? (which BTW is used in only one single
> place).

I applied the zombie copy-paste algorithm, didn't think about that.
 
> > +    { "picture_black_ratio_th", "set the picture black ratio threshold",  OFFSET(picture_black_ratio_th),  AV_OPT_TYPE_DOUBLE, {.dbl=.98}, 0, 1},
> > +    { "pic_th",                 "set the picture black ratio threshold", OFFSET(picture_black_ratio_th), AV_OPT_TYPE_DOUBLE,   {.dbl=.98}, 0, 1},
> 
> Weird align around OFFSET()
> 
[...]
> > +static int config_input(AVFilterLink *inlink)
> > +{
> > +    AVFilterContext *ctx = inlink->dst;
> > +    BlackDetectContext *blackdetect = ctx->priv;
> > +
> > +    blackdetect->min_black_duration =
> > +        blackdetect->min_black_duration_time / av_q2d(inlink->time_base);
> > +
> > +    blackdetect->pixel_black_th_i = ff_fmt_is_in(inlink->format, yuvj_formats) ?
> > +        blackdetect->pixel_black_th * 255 :
> > +        16 + blackdetect->pixel_black_th * (235 - 16);
> 
> This is all about the luminance range, right?

Added a comment.

> > +
> > +    av_log(blackdetect, AV_LOG_INFO,
> > +           "min_black_duration:%s pixel_black_th:%f pixel_black_th_i:%d picture_black_ratio_th:%f\n",
> > +           av_ts2timestr(blackdetect->min_black_duration, &inlink->time_base),
> > +           blackdetect->pixel_black_th, blackdetect->pixel_black_th_i,
> > +           blackdetect->picture_black_ratio_th);
> > +    return 0;
> > +}
> > +
> > +static void draw_slice(AVFilterLink *inlink, int y, int h, int slice_dir)
> > +{
> > +    AVFilterContext *ctx = inlink->dst;
> > +    BlackDetectContext *blackdetect = ctx->priv;
> > +    AVFilterBufferRef *picref = inlink->cur_buf;
> > +    int x, i;
> > +    uint8_t *p = picref->data[0] + y * picref->linesize[0];
> > +
> 
> const uint8_t *p?

Changed.
-- 
FFmpeg = Forgiving & Fabulous Multimedia Prodigious Enhancing Governor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavfi-add-blackdetect-filter.patch
Type: text/x-diff
Size: 12076 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120303/24fe2b13/attachment.bin>