| Message ID | 1589453053-8013-2-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v6,1/4] avfilter/vf_scdet: add filter to detect scene change | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Thu, 14 May 2020, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Reviewed-by: Paul B Mahol <onemda@gmail.com> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavfilter/vf_framerate.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c > index 6c8d01c..8d16998 100644 > --- a/libavfilter/vf_framerate.c > +++ b/libavfilter/vf_framerate.c > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next > > if (crnt->height == next->height && > crnt->width == next->width) { > + AVDictionaryEntry *e_mafd = NULL; > uint64_t sad; > - double mafd, diff; > + double mafd = HUGE_VAL, diff; > + char *tail = NULL; > > ff_dlog(ctx, "get_scene_score() process\n"); > - 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); > + e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE); > + if (e_mafd) > + mafd = strtod(e_mafd->value, &tail); > + if (*tail || mafd == HUGE_VAL) { Seems like null pointer dereference. 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. Regards, Marton > + 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); > + } > diff = fabs(mafd - s->prev_mafd); > ret = av_clipf(FFMIN(mafd, diff), 0, 100.0); > s->prev_mafd = mafd; > -- > 1.8.3.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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. Regards,
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); } Regards, Marton
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??? Thanks, Marton
On 5/14/20, Marton Balint <cus@passwd.hu> 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??? > Looks like repeating occurrence? Remove commit rights? > Thanks, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, 14 May 2020, Paul B Mahol wrote: > On 5/14/20, Marton Balint <cus@passwd.hu> 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??? >> > > Looks like repeating occurrence? Remove commit rights? > I have to agree. Limin's patches can be useful, but they always benefit from an extra set of eyes. Regards, Marton
On Thu, May 14, 2020 at 08:53:58PM +0200, 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. I try to keep the old logic, after it's single filter, we can improve the detection. > > > > 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); > } thanks, I'll try to reproduce the issue and test the code. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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. > > Thanks, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@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. > >> >> Thanks, >> Marton >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > -- > Thanks, > Limin Wang > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, May 15, 2020 at 10:27:35AM +0200, Paul B Mahol wrote: > On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@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. > > > > >> > >> Thanks, > >> Marton > >> _______________________________________________ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > >> To unsubscribe, visit link above, or email > >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > > > -- > > Thanks, > > Limin Wang > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote: > On Fri, May 15, 2020 at 10:27:35AM +0200, Paul B Mahol wrote: >> On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@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@ffmpeg.org >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> >> To unsubscribe, visit link above, or email >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> > >> > -- >> > Thanks, >> > Limin Wang >> > _______________________________________________ >> > ffmpeg-devel mailing list >> > ffmpeg-devel@ffmpeg.org >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > >> > To unsubscribe, visit link above, or email >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > -- > Thanks, > Limin Wang > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, May 15, 2020 at 11:27:00AM +0200, Paul B Mahol wrote: > On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote: > > On Fri, May 15, 2020 at 10:27:35AM +0200, Paul B Mahol wrote: > >> On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@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. > > > > > > >> > >> > > >> >> > >> >> Thanks, > >> >> Marton > >> >> _______________________________________________ > >> >> ffmpeg-devel mailing list > >> >> ffmpeg-devel@ffmpeg.org > >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> >> > >> >> To unsubscribe, visit link above, or email > >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> > > >> > -- > >> > Thanks, > >> > Limin Wang > >> > _______________________________________________ > >> > ffmpeg-devel mailing list > >> > ffmpeg-devel@ffmpeg.org > >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > > >> > To unsubscribe, visit link above, or email > >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> _______________________________________________ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > >> To unsubscribe, visit link above, or email > >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > > > -- > > Thanks, > > Limin Wang > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote: > On Fri, May 15, 2020 at 11:27:00AM +0200, Paul B Mahol wrote: >> On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote: >> > On Fri, May 15, 2020 at 10:27:35AM +0200, Paul B Mahol wrote: >> >> On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@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. > > >> >> > >> > >> >> >> >> > >> >> >> >> >> >> Thanks, >> >> >> Marton >> >> >> _______________________________________________ >> >> >> ffmpeg-devel mailing list >> >> >> ffmpeg-devel@ffmpeg.org >> >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> >> >> >> To unsubscribe, visit link above, or email >> >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >> > >> >> > -- >> >> > Thanks, >> >> > Limin Wang >> >> > _______________________________________________ >> >> > ffmpeg-devel mailing list >> >> > ffmpeg-devel@ffmpeg.org >> >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> > >> >> > To unsubscribe, visit link above, or email >> >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >> _______________________________________________ >> >> ffmpeg-devel mailing list >> >> ffmpeg-devel@ffmpeg.org >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> >> To unsubscribe, visit link above, or email >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> > >> > -- >> > Thanks, >> > Limin Wang >> > _______________________________________________ >> > ffmpeg-devel mailing list >> > ffmpeg-devel@ffmpeg.org >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > >> > To unsubscribe, visit link above, or email >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > -- > Thanks, > Limin Wang > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, May 15, 2020 at 11:50:08AM +0200, Paul B Mahol wrote: > On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote: > > On Fri, May 15, 2020 at 11:27:00AM +0200, Paul B Mahol wrote: > >> On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@gmail.com> wrote: > >> > On Fri, May 15, 2020 at 10:27:35AM +0200, Paul B Mahol wrote: > >> >> On 5/15/20, lance.lmwang@gmail.com <lance.lmwang@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@ffmpeg.org > >> >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> >> >> > >> >> >> To unsubscribe, visit link above, or email > >> >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> >> > > >> >> > -- > >> >> > Thanks, > >> >> > Limin Wang > >> >> > _______________________________________________ > >> >> > ffmpeg-devel mailing list > >> >> > ffmpeg-devel@ffmpeg.org > >> >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> >> > > >> >> > To unsubscribe, visit link above, or email > >> >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> >> _______________________________________________ > >> >> ffmpeg-devel mailing list > >> >> ffmpeg-devel@ffmpeg.org > >> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> >> > >> >> To unsubscribe, visit link above, or email > >> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> > > >> > -- > >> > Thanks, > >> > Limin Wang > >> > _______________________________________________ > >> > ffmpeg-devel mailing list > >> > ffmpeg-devel@ffmpeg.org > >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > > >> > To unsubscribe, visit link above, or email > >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >> _______________________________________________ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > >> To unsubscribe, visit link above, or email > >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > > > > -- > > Thanks, > > Limin Wang > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c index 6c8d01c..8d16998 100644 --- a/libavfilter/vf_framerate.c +++ b/libavfilter/vf_framerate.c @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next if (crnt->height == next->height && crnt->width == next->width) { + AVDictionaryEntry *e_mafd = NULL; uint64_t sad; - double mafd, diff; + double mafd = HUGE_VAL, diff; + char *tail = NULL; ff_dlog(ctx, "get_scene_score() process\n"); - 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); + e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE); + if (e_mafd) + mafd = strtod(e_mafd->value, &tail); + if (*tail || mafd == HUGE_VAL) { + 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); + } diff = fabs(mafd - s->prev_mafd); ret = av_clipf(FFMIN(mafd, diff), 0, 100.0); s->prev_mafd = mafd;