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

[Paul B Mahol]

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.

>
>
>>
>> >
>> >>
>> >> 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".