[FFmpeg-devel] [PATCH v6 2/4] avfilter/vf_framerate: if metadata lavfi.scd.mafd exists, we'll use it first

[lance.lmwang at gmail.com]

On Fri, May 15, 2020 at 11:50:08AM +0200, Paul B Mahol wrote:
> On 5/15/20, lance.lmwang at gmail.com <lance.lmwang at gmail.com> wrote:
> > On Fri, May 15, 2020 at 11:27:00AM +0200, Paul B Mahol wrote:
> >> On 5/15/20, lance.lmwang at gmail.com <lance.lmwang at gmail.com> wrote:
> >> > On Fri, May 15, 2020 at 10:27:35AM +0200, Paul B Mahol wrote:
> >> >> On 5/15/20, lance.lmwang at gmail.com <lance.lmwang at gmail.com> wrote:
> >> >> > On Thu, May 14, 2020 at 09:00:21PM +0200, Marton Balint wrote:
> >> >> >>
> >> >> >>
> >> >> >> On Thu, 14 May 2020, Marton Balint wrote:
> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> > On Thu, 14 May 2020, Nicolas George wrote:
> >> >> >> >
> >> >> >> > > Marton Balint (12020-05-14):
> >> >> >> > > > I am not a huge fan of this patch, mafd refers to a score
> >> >> >> > > > between this
> >> >> >> > frame
> >> >> >> > > > and the previous frame, we cannot ensure that there were no
> >> >> >> > > > additional
> >> >> >> > frame
> >> >> >> > > > processing between scdet and this filter which may have
> >> >> >> > > > duplicated
> >> >> >> > > > or
> >> >> >> > > > removed frames. So I'd rather not add this feature.
> >> >> >> > >
> >> >> >> > > It can only happen if the user has put filters in the middle. I
> >> >> >> > > move
> >> >> >> > > we
> >> >> >> > > trust users who insert such obscure filters to do what they want
> >> >> >> > > to,
> >> >> >> > > or
> >> >> >> > > to fix their issues if they have some.
> >> >> >> >
> >> >> >> > Fine, I am not blocking this if null pointer deref issues are
> >> >> >> > fixed.
> >> >> >> >
> >> >> >> > I think we can also assume that if the metadata exists, it will
> >> >> >> > contain
> >> >> >> > a valid number, so I suggest this code:
> >> >> >> >
> >> >> >> >         e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd",
> >> >> >> > NULL,
> >> >> >> > AV_DICT_MATCH_CASE);
> >> >> >> >         if (e_mafd) {
> >> >> >> >             mafd = strtod(e_mafd->value, NULL);
> >> >> >> >         } else {
> >> >> >> >             s->sad(crnt->data[0], crnt->linesize[0],
> >> >> >> > next->data[0],
> >> >> >> > next->linesize[0], crnt->width, crnt->height, &sad);
> >> >> >> >             emms_c();
> >> >> >> >             mafd = (double)sad * 100.0 / (crnt->width *
> >> >> >> > crnt->height)
> >> >> >> > /
> >> >> >> > (1 << s->bitdepth);
> >> >> >> >         }
> >> >> >>
> >> >> >> Why this patch was committed without a recent review???
> >> >> >
> >> >> > Sorry, I consider it's reviewed old patchset, but it seems it's
> >> >> > incomplete.
> >> >>
> >> >> Also it uses scd for metadata filter name, which is bad. It should use
> >> >> full filter name.
> >> >
> >> > Sorry, in fact I intentionally did not use the filter name. At that
> >> > time, I
> >> > considered
> >> > that there may be different filters for scene detection, but for filters
> >> > that use the metadata,
> >> > the processing method should be the same. So I didn't use the filter
> >> > name.
> >>
> >> That is really bad explanation, Different filters writting to same
> >> metadata entry is invalid.
> >
> > Maybe my thoughts isn't complete as I assume user should choose one scene
> > detect filter.
> > One example is facedetect, now we have opencv facedetect, in future, we may
> > add
> > dnn facedetect and even more, for example, the drawbox care for the face
> > detect
> > result instead of which filter detect I think. That's my consideration.
> 
> facedetect is not scene change detection.

It's my consideration only, I'll change it by your request if no other comments.
thx.

> 
> 
> >
> >
> >>
> >> >
> >> >
> >> >>
> >> >> >
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Marton
> >> >> >> _______________________________________________
> >> >> >> ffmpeg-devel mailing list
> >> >> >> ffmpeg-devel at ffmpeg.org
> >> >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >> >>
> >> >> >> To unsubscribe, visit link above, or email
> >> >> >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >> >> >
> >> >> > --
> >> >> > Thanks,
> >> >> > Limin Wang
> >> >> > _______________________________________________
> >> >> > ffmpeg-devel mailing list
> >> >> > ffmpeg-devel at ffmpeg.org
> >> >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >> >
> >> >> > To unsubscribe, visit link above, or email
> >> >> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >> >> _______________________________________________
> >> >> ffmpeg-devel mailing list
> >> >> ffmpeg-devel at ffmpeg.org
> >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >>
> >> >> To unsubscribe, visit link above, or email
> >> >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >> >
> >> > --
> >> > Thanks,
> >> > Limin Wang
> >> > _______________________________________________
> >> > ffmpeg-devel mailing list
> >> > ffmpeg-devel at ffmpeg.org
> >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >
> >> > To unsubscribe, visit link above, or email
> >> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >
> > --
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang