[FFmpeg-devel,v1] avfilter/vf_framerate: make the scene change detection calculation method consistent with f_select filter

Submitted by lance.lmwang@gmail.com on Sept. 12, 2019, 10:22 a.m.

Details

Message ID 20190912102259.16630-1-lance.lmwang@gmail.com
State New
Headers show

Commit Message

lance.lmwang@gmail.com Sept. 12, 2019, 10:22 a.m.
From: Limin Wang <lance.lmwang@gmail.com>

Have been tested by fate-suite/svq3/Vertical400kbit.sorenson3.mov and the
testing results are consistent.

If it is acceptable, I want to move get_scene_score() to the public function and
make the scene change detection calculation method of the relevant module consistent.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/filters.texi           | 4 ++--
 libavfilter/vf_framerate.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Marton Balint Sept. 12, 2019, 5:55 p.m.
On Thu, 12 Sep 2019, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Have been tested by fate-suite/svq3/Vertical400kbit.sorenson3.mov and the
> testing results are consistent.
>
> If it is acceptable, I want to move get_scene_score() to the public function and
> make the scene change detection calculation method of the relevant module consistent.
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> doc/filters.texi           | 4 ++--
> libavfilter/vf_framerate.c | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 9d500e44a9..1c2e86b635 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -10626,10 +10626,10 @@ the default is @code{240}.
> 
> @item scene
> Specify the level at which a scene change is detected as a value between
> -0 and 100 to indicate a new scene; a low value reflects a low
> +0 and 1.0 to indicate a new scene; a low value reflects a low

You cannot change the scale of a user facing parameter, that would break
existing command lines.

> probability for the current frame to introduce a new scene, while a higher
> value means the current frame is more likely to be one.
> -The default is @code{8.2}.
> +The default is @code{0.25}.

You should not change the default unless you have a very good reason, as 
it would change existing behaviour.

> 
> @item flags
> Specify flags influencing the filter process.
> diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> index 06e463e4d7..def3f19f55 100644
> --- a/libavfilter/vf_framerate.c
> +++ b/libavfilter/vf_framerate.c
> @@ -51,7 +51,7 @@ static const AVOption framerate_options[] = {
>
>     {"interp_start",        "point to start linear interpolation",    OFFSET(interp_start),    AV_OPT_TYPE_INT,      {.i64=15},                 0,       255,     V|F },
>     {"interp_end",          "point to end linear interpolation",      OFFSET(interp_end),      AV_OPT_TYPE_INT,      {.i64=240},                0,       255,     V|F },
> -    {"scene",               "scene change level",                     OFFSET(scene_score),     AV_OPT_TYPE_DOUBLE,   {.dbl=8.2},                0,       INT_MAX, V|F },
> +    {"scene",               "scene change level",                     OFFSET(scene_score),     AV_OPT_TYPE_DOUBLE,   {.dbl=0.25},               0,       1.0,     V|F },
>
>     {"flags",               "set flags",                              OFFSET(flags),           AV_OPT_TYPE_FLAGS,    {.i64=1},                  0,       INT_MAX, V|F, "flags" },
>     {"scene_change_detect", "enable scene change detection",          0,                       AV_OPT_TYPE_CONST,    {.i64=FRAMERATE_FLAG_SCD}, INT_MIN, INT_MAX, V|F, "flags" },
> @@ -77,9 +77,9 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
>         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);
> +        mafd = (double)sad / (crnt->width * crnt->height) / (1ULL << (s->bitdepth - 8));

No, dividing by bitdepth-8 is completely wrong. It is wrong in f_select, 
but we could not change it there as that would have changed the scale of a 
user facing option.

Regards,
Marton


>         diff = fabs(mafd - s->prev_mafd);
> -        ret  = av_clipf(FFMIN(mafd, diff), 0, 100.0);
> +        ret  = av_clipf(FFMIN(mafd, diff) / 100., 0, 1);
>         s->prev_mafd = mafd;
>     }
>     ff_dlog(ctx, "get_scene_score() result is:%f\n", ret);
> -- 
> 2.21.0
>
> _______________________________________________
> 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".
lance.lmwang@gmail.com Sept. 12, 2019, 11:31 p.m.
On Thu, Sep 12, 2019 at 07:55:30PM +0200, Marton Balint wrote:
> 
> 
> On Thu, 12 Sep 2019, lance.lmwang@gmail.com wrote:
> 
> >From: Limin Wang <lance.lmwang@gmail.com>
> >
> >Have been tested by fate-suite/svq3/Vertical400kbit.sorenson3.mov and the
> >testing results are consistent.
> >
> >If it is acceptable, I want to move get_scene_score() to the public function and
> >make the scene change detection calculation method of the relevant module consistent.
> >
> >Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >---
> >doc/filters.texi           | 4 ++--
> >libavfilter/vf_framerate.c | 6 +++---
> >2 files changed, 5 insertions(+), 5 deletions(-)
> >
> >diff --git a/doc/filters.texi b/doc/filters.texi
> >index 9d500e44a9..1c2e86b635 100644
> >--- a/doc/filters.texi
> >+++ b/doc/filters.texi
> >@@ -10626,10 +10626,10 @@ the default is @code{240}.
> >
> >@item scene
> >Specify the level at which a scene change is detected as a value between
> >-0 and 100 to indicate a new scene; a low value reflects a low
> >+0 and 1.0 to indicate a new scene; a low value reflects a low
> 
> You cannot change the scale of a user facing parameter, that would break
> existing command lines.

Yes, I'm afraid of it when I'm change it. Then it's impossible to make
consistent.
> 
> >probability for the current frame to introduce a new scene, while a higher
> >value means the current frame is more likely to be one.
> >-The default is @code{8.2}.
> >+The default is @code{0.25}.
> 
> You should not change the default unless you have a very good
> reason, as it would change existing behaviour.
> 
> >
> >@item flags
> >Specify flags influencing the filter process.
> >diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> >index 06e463e4d7..def3f19f55 100644
> >--- a/libavfilter/vf_framerate.c
> >+++ b/libavfilter/vf_framerate.c
> >@@ -51,7 +51,7 @@ static const AVOption framerate_options[] = {
> >
> >    {"interp_start",        "point to start linear interpolation",    OFFSET(interp_start),    AV_OPT_TYPE_INT,      {.i64=15},                 0,       255,     V|F },
> >    {"interp_end",          "point to end linear interpolation",      OFFSET(interp_end),      AV_OPT_TYPE_INT,      {.i64=240},                0,       255,     V|F },
> >-    {"scene",               "scene change level",                     OFFSET(scene_score),     AV_OPT_TYPE_DOUBLE,   {.dbl=8.2},                0,       INT_MAX, V|F },
> >+    {"scene",               "scene change level",                     OFFSET(scene_score),     AV_OPT_TYPE_DOUBLE,   {.dbl=0.25},               0,       1.0,     V|F },
> >
> >    {"flags",               "set flags",                              OFFSET(flags),           AV_OPT_TYPE_FLAGS,    {.i64=1},                  0,       INT_MAX, V|F, "flags" },
> >    {"scene_change_detect", "enable scene change detection",          0,                       AV_OPT_TYPE_CONST,    {.i64=FRAMERATE_FLAG_SCD}, INT_MIN, INT_MAX, V|F, "flags" },
> >@@ -77,9 +77,9 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> >        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);
> >+        mafd = (double)sad / (crnt->width * crnt->height) / (1ULL << (s->bitdepth - 8));
> 
> No, dividing by bitdepth-8 is completely wrong. It is wrong in
> f_select, but we could not change it there as that would have
> changed the scale of a user facing option.
> 
I have changed vf_minterpolate filter, it divides (crnt->width * crnt->height * 3) which I think it's wrong, 
so I consider to make them using consistent method.  If we need to keep the user option unchanged, then 
it's impossible to do it. I'll submit the change for review.


> Regards,
> Marton
> 
> 
> >        diff = fabs(mafd - s->prev_mafd);
> >-        ret  = av_clipf(FFMIN(mafd, diff), 0, 100.0);
> >+        ret  = av_clipf(FFMIN(mafd, diff) / 100., 0, 1);
> >        s->prev_mafd = mafd;
> >    }
> >    ff_dlog(ctx, "get_scene_score() result is:%f\n", ret);
> >-- 
> >2.21.0
> >
> >_______________________________________________
> >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".

Patch hide | download patch | download mbox

diff --git a/doc/filters.texi b/doc/filters.texi
index 9d500e44a9..1c2e86b635 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -10626,10 +10626,10 @@  the default is @code{240}.
 
 @item scene
 Specify the level at which a scene change is detected as a value between
-0 and 100 to indicate a new scene; a low value reflects a low
+0 and 1.0 to indicate a new scene; a low value reflects a low
 probability for the current frame to introduce a new scene, while a higher
 value means the current frame is more likely to be one.
-The default is @code{8.2}.
+The default is @code{0.25}.
 
 @item flags
 Specify flags influencing the filter process.
diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
index 06e463e4d7..def3f19f55 100644
--- a/libavfilter/vf_framerate.c
+++ b/libavfilter/vf_framerate.c
@@ -51,7 +51,7 @@  static const AVOption framerate_options[] = {
 
     {"interp_start",        "point to start linear interpolation",    OFFSET(interp_start),    AV_OPT_TYPE_INT,      {.i64=15},                 0,       255,     V|F },
     {"interp_end",          "point to end linear interpolation",      OFFSET(interp_end),      AV_OPT_TYPE_INT,      {.i64=240},                0,       255,     V|F },
-    {"scene",               "scene change level",                     OFFSET(scene_score),     AV_OPT_TYPE_DOUBLE,   {.dbl=8.2},                0,       INT_MAX, V|F },
+    {"scene",               "scene change level",                     OFFSET(scene_score),     AV_OPT_TYPE_DOUBLE,   {.dbl=0.25},               0,       1.0,     V|F },
 
     {"flags",               "set flags",                              OFFSET(flags),           AV_OPT_TYPE_FLAGS,    {.i64=1},                  0,       INT_MAX, V|F, "flags" },
     {"scene_change_detect", "enable scene change detection",          0,                       AV_OPT_TYPE_CONST,    {.i64=FRAMERATE_FLAG_SCD}, INT_MIN, INT_MAX, V|F, "flags" },
@@ -77,9 +77,9 @@  static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
         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);
+        mafd = (double)sad / (crnt->width * crnt->height) / (1ULL << (s->bitdepth - 8));
         diff = fabs(mafd - s->prev_mafd);
-        ret  = av_clipf(FFMIN(mafd, diff), 0, 100.0);
+        ret  = av_clipf(FFMIN(mafd, diff) / 100., 0, 1);
         s->prev_mafd = mafd;
     }
     ff_dlog(ctx, "get_scene_score() result is:%f\n", ret);