[FFmpeg-devel] [PATCH] Blackframe video filter now sets frame metadata accordingly.

[wm4]

On Fri, 11 Jul 2014 22:33:59 +0200
Stepan Bujnak <stepan.bujnak at gmail.com> wrote:

> 
> On 11.07.2014 22:25, wm4 wrote:
> > On Fri, 11 Jul 2014 22:13:54 +0200
> > Stepan Bujnak <stepan.bujnak at gmail.com> wrote:
> >
> >> the libavfilter/vf_blackframe.c filter now not only logs detected
> >> values, but also sets frame metadata. The metadata have prefix
> >> 'lavfi.blackframe.*'.
> >>
> >> Signed-off-by: Stepan Bujnak <stepan.bujnak at gmail.com>
> >> ---
> >>   libavfilter/vf_blackframe.c | 29 +++++++++++++++++++++++++----
> >>   1 file changed, 25 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavfilter/vf_blackframe.c b/libavfilter/vf_blackframe.c
> >> index 0eeda61..d89a25e 100644
> >> --- a/libavfilter/vf_blackframe.c
> >> +++ b/libavfilter/vf_blackframe.c
> >> @@ -58,6 +58,10 @@ static int query_formats(AVFilterContext *ctx)
> >>       return 0;
> >>   }
> >>   
> >> +#define SET_META(key, format, value) \
> >> +    snprintf(buf, sizeof(buf), format, value);  \
> >> +    av_dict_set(metadata, key, buf, 0)
> >> +
> >>   static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> >>   {
> >>       AVFilterContext *ctx = inlink->dst;
> >> @@ -65,6 +69,10 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> >>       int x, i;
> >>       int pblack = 0;
> >>       uint8_t *p = frame->data[0];
> >> +    AVDictionary **metadata;
> >> +    double t = -1;
> >> +    char type;
> >> +    char buf[32];
> >>   
> >>       for (i = 0; i < frame->height; i++) {
> >>           for (x = 0; x < inlink->w; x++)
> >> @@ -76,12 +84,25 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> >>           s->last_keyframe = s->frame;
> >>   
> >>       pblack = s->nblack * 100 / (inlink->w * inlink->h);
> >> -    if (pblack >= s->bamount)
> >> +    if (pblack >= s->bamount) {
> >> +        metadata = avpriv_frame_get_metadatap(frame);
> >> +        type = av_get_picture_type_char(frame->pict_type);
> >> +
> >> +        if (frame->pts != AV_NOPTS_VALUE) {
> >> +            t = frame->pts * av_q2d(inlink->time_base);
> >> +        }
> >> +
> >>           av_log(ctx, AV_LOG_INFO, "frame:%u pblack:%u pts:%"PRId64" t:%f "
> >>                  "type:%c last_keyframe:%d\n",
> >> -               s->frame, pblack, frame->pts,
> >> -               frame->pts == AV_NOPTS_VALUE ? -1 : frame->pts * av_q2d(inlink->time_base),
> >> -               av_get_picture_type_char(frame->pict_type), s->last_keyframe);
> >> +               s->frame, pblack, frame->pts, t, type, s->last_keyframe);
> >> +
> >> +        SET_META("lavfi.blackframe.frame", "%u", s->frame);
> >> +        SET_META("lavfi.blackframe.pblack", "%u", pblack);
> >> +        SET_META("lavfi.blackframe.pts", "%"PRId64, frame->pts);
> > It shouldn't be needed to set the PTS - the frame already has it.
> >
> >> +        SET_META("lavfi.blackframe.t", "%f", t);
> >> +        SET_META("lavfi.blackframe.type", "%c", type);
> >> +        SET_META("lavfi.blackframe.last_keyframe", "%d", s->last_keyframe);
> >> +    }
> >>   
> >>       s->frame++;
> >>       s->nblack = 0;
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> You are absolutely right. The same possibly applies to `t` and `type` 
> values, but how about `s->frame` and `s->last_frame`? Are those necessary?

Neither of them look particularly useful (and they're all redundant),
so IMHO they should be dropped too.