[FFmpeg-devel,v1,1/4] avfilter/vf_framerate: add flags none to disable scene change detection if necessary

Submitted by lance.lmwang@gmail.com on Sept. 21, 2019, 2:49 p.m.

Details

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

Commit Message

lance.lmwang@gmail.com Sept. 21, 2019, 2:49 p.m.
From: Limin Wang <lance.lmwang@gmail.com>

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

Comments

Marton Balint Sept. 22, 2019, 6:54 p.m.
On Sat, 21 Sep 2019, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> doc/filters.texi           |  2 ++
> libavfilter/vf_framerate.c | 17 +++++++++++------
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index bbfdad4..fce4ef4 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -10637,6 +10637,8 @@ Specify flags influencing the filter process.
> Available value for @var{flags} is:
> 
> @table @option
> +@item none
> +Disable scene change detection

This is only true if no additional flags are added later. The reason of 
using a flags option is future extensibility, so adding such a constant 
kind of defeats that purpose. It is also uneeded, "none" is a a named 
constant which you can always use to explicitly set no flags.

So this patch seems uneeded.

Regards,
Marton

> @item scene_change_detect, scd
> Enable scene change detection using the value of the option @var{scene}.
> This flag is enabled by default.
> diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> index 06e463e..99faf75 100644
> --- a/libavfilter/vf_framerate.c
> +++ b/libavfilter/vf_framerate.c
> @@ -44,7 +44,9 @@
> #define OFFSET(x) offsetof(FrameRateContext, x)
> #define V AV_OPT_FLAG_VIDEO_PARAM
> #define F AV_OPT_FLAG_FILTERING_PARAM
> +#define FRAMERATE_FLAG_NONE 00
> #define FRAMERATE_FLAG_SCD 01
> +#define CONST(name, help, val, unit) { name, help, 0, AV_OPT_TYPE_CONST, {.i64=val}, 0, 0, V|F, unit }
> 
> static const AVOption framerate_options[] = {
>     {"fps",                 "required output frames per second rate", OFFSET(dest_frame_rate), AV_OPT_TYPE_VIDEO_RATE, {.str="50"},             0,       INT_MAX, V|F },
> @@ -53,9 +55,10 @@ static const AVOption framerate_options[] = {
>     {"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 },
> 
> -    {"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" },
> -    {"scd",                 "enable scene change detection",          0,                       AV_OPT_TYPE_CONST,    {.i64=FRAMERATE_FLAG_SCD}, INT_MIN, INT_MAX, V|F, "flags" },
> +    {"flags",               "set flags", OFFSET(flags),  AV_OPT_TYPE_FLAGS, {.i64=FRAMERATE_FLAG_SCD}, FRAMERATE_FLAG_NONE, FRAMERATE_FLAG_SCD, V|F, "flags" },
> +        CONST("none",                 "disable scene change detection",      FRAMERATE_FLAG_NONE,    "flags"),
> +        CONST("scene_change_detect",  "enable scene change detection",       FRAMERATE_FLAG_SCD,     "flags"),
> +        CONST("scd",                  "enable scene change detection",       FRAMERATE_FLAG_SCD,     "flags"),
>
>     {NULL}
> };
> @@ -301,9 +304,11 @@ static int config_input(AVFilterLink *inlink)
>     s->bitdepth = pix_desc->comp[0].depth;
>     s->vsub = pix_desc->log2_chroma_h;
> 
> -    s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
> -    if (!s->sad)
> -        return AVERROR(EINVAL);
> +    if ((s->flags & FRAMERATE_FLAG_SCD)) {
> +        s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
> +        if (!s->sad)
> +            return AVERROR(EINVAL);
> +    }
>
>     s->srce_time_base = inlink->time_base;
> 
> -- 
> 2.6.4
>
> _______________________________________________
> 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. 23, 2019, 1:29 a.m.
On Sun, Sep 22, 2019 at 08:54:36PM +0200, Marton Balint wrote:
> 
> 
> On Sat, 21 Sep 2019, lance.lmwang@gmail.com wrote:
> 
> >From: Limin Wang <lance.lmwang@gmail.com>
> >
> >Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >---
> >doc/filters.texi           |  2 ++
> >libavfilter/vf_framerate.c | 17 +++++++++++------
> >2 files changed, 13 insertions(+), 6 deletions(-)
> >
> >diff --git a/doc/filters.texi b/doc/filters.texi
> >index bbfdad4..fce4ef4 100644
> >--- a/doc/filters.texi
> >+++ b/doc/filters.texi
> >@@ -10637,6 +10637,8 @@ Specify flags influencing the filter process.
> >Available value for @var{flags} is:
> >
> >@table @option
> >+@item none
> >+Disable scene change detection
> 
> This is only true if no additional flags are added later. The reason
> of using a flags option is future extensibility, so adding such a
> constant kind of defeats that purpose. It is also uneeded, "none" is
> a a named constant which you can always use to explicitly set no
> flags.
> 
> So this patch seems uneeded.

Marton, thanks for your review, the none flags is for debug purpose, 
without sceencut detection, the result will consistent. So it's
necessary to have none flags to turn it off. 


> 
> Regards,
> Marton
> 
> >@item scene_change_detect, scd
> >Enable scene change detection using the value of the option @var{scene}.
> >This flag is enabled by default.
> >diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> >index 06e463e..99faf75 100644
> >--- a/libavfilter/vf_framerate.c
> >+++ b/libavfilter/vf_framerate.c
> >@@ -44,7 +44,9 @@
> >#define OFFSET(x) offsetof(FrameRateContext, x)
> >#define V AV_OPT_FLAG_VIDEO_PARAM
> >#define F AV_OPT_FLAG_FILTERING_PARAM
> >+#define FRAMERATE_FLAG_NONE 00
> >#define FRAMERATE_FLAG_SCD 01
> >+#define CONST(name, help, val, unit) { name, help, 0, AV_OPT_TYPE_CONST, {.i64=val}, 0, 0, V|F, unit }
> >
> >static const AVOption framerate_options[] = {
> >    {"fps",                 "required output frames per second rate", OFFSET(dest_frame_rate), AV_OPT_TYPE_VIDEO_RATE, {.str="50"},             0,       INT_MAX, V|F },
> >@@ -53,9 +55,10 @@ static const AVOption framerate_options[] = {
> >    {"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 },
> >
> >-    {"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" },
> >-    {"scd",                 "enable scene change detection",          0,                       AV_OPT_TYPE_CONST,    {.i64=FRAMERATE_FLAG_SCD}, INT_MIN, INT_MAX, V|F, "flags" },
> >+    {"flags",               "set flags", OFFSET(flags),  AV_OPT_TYPE_FLAGS, {.i64=FRAMERATE_FLAG_SCD}, FRAMERATE_FLAG_NONE, FRAMERATE_FLAG_SCD, V|F, "flags" },
> >+        CONST("none",                 "disable scene change detection",      FRAMERATE_FLAG_NONE,    "flags"),
> >+        CONST("scene_change_detect",  "enable scene change detection",       FRAMERATE_FLAG_SCD,     "flags"),
> >+        CONST("scd",                  "enable scene change detection",       FRAMERATE_FLAG_SCD,     "flags"),
> >
> >    {NULL}
> >};
> >@@ -301,9 +304,11 @@ static int config_input(AVFilterLink *inlink)
> >    s->bitdepth = pix_desc->comp[0].depth;
> >    s->vsub = pix_desc->log2_chroma_h;
> >
> >-    s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
> >-    if (!s->sad)
> >-        return AVERROR(EINVAL);
> >+    if ((s->flags & FRAMERATE_FLAG_SCD)) {
> >+        s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
> >+        if (!s->sad)
> >+            return AVERROR(EINVAL);
> >+    }
> >
> >    s->srce_time_base = inlink->time_base;
> >
> >-- 
> >2.6.4
> >
> >_______________________________________________
> >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".
Marton Balint Sept. 23, 2019, 9:18 p.m.
On Mon, 23 Sep 2019, Limin Wang wrote:

> On Sun, Sep 22, 2019 at 08:54:36PM +0200, Marton Balint wrote:
>> 
>> 
>> On Sat, 21 Sep 2019, lance.lmwang@gmail.com wrote:
>> 
>> >From: Limin Wang <lance.lmwang@gmail.com>
>> >
>> >Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> >---
>> >doc/filters.texi           |  2 ++
>> >libavfilter/vf_framerate.c | 17 +++++++++++------
>> >2 files changed, 13 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/doc/filters.texi b/doc/filters.texi
>> >index bbfdad4..fce4ef4 100644
>> >--- a/doc/filters.texi
>> >+++ b/doc/filters.texi
>> >@@ -10637,6 +10637,8 @@ Specify flags influencing the filter process.
>> >Available value for @var{flags} is:
>> >
>> >@table @option
>> >+@item none
>> >+Disable scene change detection
>> 
>> This is only true if no additional flags are added later. The reason
>> of using a flags option is future extensibility, so adding such a
>> constant kind of defeats that purpose. It is also uneeded, "none" is
>> a a named constant which you can always use to explicitly set no
>> flags.
>> 
>> So this patch seems uneeded.
>
> Marton, thanks for your review, the none flags is for debug purpose, 
> without sceencut detection, the result will consistent. So it's
> necessary to have none flags to turn it off.

You can change flags using the +flag or -flag syntax. You don't need a 
separate constant for this.

Regards,
Marton
lance.lmwang@gmail.com Sept. 24, 2019, 12:03 a.m.
On Mon, Sep 23, 2019 at 11:18:01PM +0200, Marton Balint wrote:
> 
> 
> On Mon, 23 Sep 2019, Limin Wang wrote:
> 
> >On Sun, Sep 22, 2019 at 08:54:36PM +0200, Marton Balint wrote:
> >>
> >>
> >>On Sat, 21 Sep 2019, lance.lmwang@gmail.com wrote:
> >>
> >>>From: Limin Wang <lance.lmwang@gmail.com>
> >>>
> >>>Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>---
> >>>doc/filters.texi           |  2 ++
> >>>libavfilter/vf_framerate.c | 17 +++++++++++------
> >>>2 files changed, 13 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/doc/filters.texi b/doc/filters.texi
> >>>index bbfdad4..fce4ef4 100644
> >>>--- a/doc/filters.texi
> >>>+++ b/doc/filters.texi
> >>>@@ -10637,6 +10637,8 @@ Specify flags influencing the filter process.
> >>>Available value for @var{flags} is:
> >>>
> >>>@table @option
> >>>+@item none
> >>>+Disable scene change detection
> >>
> >>This is only true if no additional flags are added later. The reason
> >>of using a flags option is future extensibility, so adding such a
> >>constant kind of defeats that purpose. It is also uneeded, "none" is
> >>a a named constant which you can always use to explicitly set no
> >>flags.
> >>
> >>So this patch seems uneeded.
> >
> >Marton, thanks for your review, the none flags is for debug
> >purpose, without sceencut detection, the result will consistent.
> >So it's
> >necessary to have none flags to turn it off.
> 
> You can change flags using the +flag or -flag syntax. You don't need
> a separate constant for this.
OK, thank for the hint, I don't know it's supporot -flag with one only
although I have used flag+flag before. Any comments for the other
patches, if ok, I'll update the patch and remove this one.

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

Patch hide | download patch | download mbox

diff --git a/doc/filters.texi b/doc/filters.texi
index bbfdad4..fce4ef4 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -10637,6 +10637,8 @@  Specify flags influencing the filter process.
 Available value for @var{flags} is:
 
 @table @option
+@item none
+Disable scene change detection
 @item scene_change_detect, scd
 Enable scene change detection using the value of the option @var{scene}.
 This flag is enabled by default.
diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
index 06e463e..99faf75 100644
--- a/libavfilter/vf_framerate.c
+++ b/libavfilter/vf_framerate.c
@@ -44,7 +44,9 @@ 
 #define OFFSET(x) offsetof(FrameRateContext, x)
 #define V AV_OPT_FLAG_VIDEO_PARAM
 #define F AV_OPT_FLAG_FILTERING_PARAM
+#define FRAMERATE_FLAG_NONE 00
 #define FRAMERATE_FLAG_SCD 01
+#define CONST(name, help, val, unit) { name, help, 0, AV_OPT_TYPE_CONST, {.i64=val}, 0, 0, V|F, unit }
 
 static const AVOption framerate_options[] = {
     {"fps",                 "required output frames per second rate", OFFSET(dest_frame_rate), AV_OPT_TYPE_VIDEO_RATE, {.str="50"},             0,       INT_MAX, V|F },
@@ -53,9 +55,10 @@  static const AVOption framerate_options[] = {
     {"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 },
 
-    {"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" },
-    {"scd",                 "enable scene change detection",          0,                       AV_OPT_TYPE_CONST,    {.i64=FRAMERATE_FLAG_SCD}, INT_MIN, INT_MAX, V|F, "flags" },
+    {"flags",               "set flags", OFFSET(flags),  AV_OPT_TYPE_FLAGS, {.i64=FRAMERATE_FLAG_SCD}, FRAMERATE_FLAG_NONE, FRAMERATE_FLAG_SCD, V|F, "flags" },
+        CONST("none",                 "disable scene change detection",      FRAMERATE_FLAG_NONE,    "flags"),
+        CONST("scene_change_detect",  "enable scene change detection",       FRAMERATE_FLAG_SCD,     "flags"),
+        CONST("scd",                  "enable scene change detection",       FRAMERATE_FLAG_SCD,     "flags"),
 
     {NULL}
 };
@@ -301,9 +304,11 @@  static int config_input(AVFilterLink *inlink)
     s->bitdepth = pix_desc->comp[0].depth;
     s->vsub = pix_desc->log2_chroma_h;
 
-    s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
-    if (!s->sad)
-        return AVERROR(EINVAL);
+    if ((s->flags & FRAMERATE_FLAG_SCD)) {
+        s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
+        if (!s->sad)
+            return AVERROR(EINVAL);
+    }
 
     s->srce_time_base = inlink->time_base;