diff mbox series

[FFmpeg-devel] avfilter/vf_delogo: remove duplicated code

Message ID tencent_DB40CD28A00E4DF0CFFAD50CA745B8E91607@qq.com
State New
Headers show
Series [FFmpeg-devel] avfilter/vf_delogo: remove duplicated code
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Zhao Zhili Sept. 16, 2020, 5:09 a.m. UTC
From: Zhao Zhili <quinkblack@foxmail.com>

1. Remove the modification of x, y, w and h parameters since they
   are reset by filter_frame.
2. config_input leads to error out when logo area is outside of the
   frame, while filter_frame fix the parameters automatically. Make
   config_input don't return error to keep the logic consistent.
---
 libavfilter/vf_delogo.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

Comments

Zhao Zhili Sept. 26, 2020, 4:38 p.m. UTC | #1
Ping.

> On Sep 16, 2020, at 1:09 PM, quinkblack@foxmail.com wrote:
> 
> From: Zhao Zhili <quinkblack@foxmail.com>
> 
> 1. Remove the modification of x, y, w and h parameters since they
>   are reset by filter_frame.
> 2. config_input leads to error out when logo area is outside of the
>   frame, while filter_frame fix the parameters automatically. Make
>   config_input don't return error to keep the logic consistent.
> ---
> libavfilter/vf_delogo.c | 39 +++++++++++++++------------------------
> 1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/libavfilter/vf_delogo.c b/libavfilter/vf_delogo.c
> index 6069c30163..39f06512fa 100644
> --- a/libavfilter/vf_delogo.c
> +++ b/libavfilter/vf_delogo.c
> @@ -271,11 +271,6 @@ static av_cold int init(AVFilterContext *ctx)
>     av_log(ctx, AV_LOG_VERBOSE, "x:%d y:%d, w:%d h:%d band:%d show:%d\n",
>            s->x, s->y, s->w, s->h, s->band, s->show);
> 
> -    s->w += s->band*2;
> -    s->h += s->band*2;
> -    s->x -= s->band;
> -    s->y -= s->band;
> -
>     return 0;
> }
> 
> @@ -284,10 +279,21 @@ static int config_input(AVFilterLink *inlink)
>     DelogoContext *s = inlink->dst->priv;
> 
>     /* Check whether the logo area fits in the frame */
> -    if (s->x + (s->band - 1) < 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
> -        s->y + (s->band - 1) < 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
> -        av_log(s, AV_LOG_ERROR, "Logo area is outside of the frame.\n");
> -        return AVERROR(EINVAL);
> +    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
> +        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
> +        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
> +               " logo(x:%d y:%d w:%d h:%d), frame(%dx%d),"
> +               " auto set the area inside of the frame."
> +               " Note: x and y must be 1 at least.\n",
> +               s->x, s->y, s->w, s->h, inlink->w, inlink->h);
> +        if (s->x + (s->band - 1) <= 0)
> +            s->x = 1 + s->band;
> +        if (s->y + (s->band - 1) <= 0)
> +            s->y = 1 + s->band;
> +        if (s->x + s->w - (s->band*2 - 2) > inlink->w)
> +            s->w = inlink->w - s->x - (s->band*2 - 2);
> +        if (s->y + s->h - (s->band*2 - 2) > inlink->h)
> +            s->h = inlink->h - s->y - (s->band*2 - 2);
>     }
> 
>     return 0;
> @@ -313,21 +319,6 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *in)
>     s->w = av_expr_eval(s->w_pexpr, s->var_values, s);
>     s->h = av_expr_eval(s->h_pexpr, s->var_values, s);
> 
> -    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
> -        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
> -        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
> -               " auto set the area inside of the frame\n");
> -    }
> -
> -    if (s->x + (s->band - 1) <= 0)
> -        s->x = 1 + s->band;
> -    if (s->y + (s->band - 1) <= 0)
> -        s->y = 1 + s->band;
> -    if (s->x + s->w - (s->band*2 - 2) > inlink->w)
> -        s->w = inlink->w - s->x - (s->band*2 - 2);
> -    if (s->y + s->h - (s->band*2 - 2) > inlink->h)
> -        s->h = inlink->h - s->y - (s->band*2 - 2);
> -
>     ret = config_input(inlink);
>     if (ret < 0) {
>         av_frame_free(&in);
> -- 
> 2.17.1
> 
>
Paul B Mahol Sept. 26, 2020, 6:02 p.m. UTC | #2
On Sun, Sep 27, 2020 at 12:38:57AM +0800, Zhao Zhili wrote:
> Ping.
> 
> > On Sep 16, 2020, at 1:09 PM, quinkblack@foxmail.com wrote:
> > 
> > From: Zhao Zhili <quinkblack@foxmail.com>
> > 
> > 1. Remove the modification of x, y, w and h parameters since they
> >   are reset by filter_frame.
> > 2. config_input leads to error out when logo area is outside of the
> >   frame, while filter_frame fix the parameters automatically. Make
> >   config_input don't return error to keep the logic consistent.
> > ---

Is this causing overreads? Check with valgrind.

> > libavfilter/vf_delogo.c | 39 +++++++++++++++------------------------
> > 1 file changed, 15 insertions(+), 24 deletions(-)
> > 
> > diff --git a/libavfilter/vf_delogo.c b/libavfilter/vf_delogo.c
> > index 6069c30163..39f06512fa 100644
> > --- a/libavfilter/vf_delogo.c
> > +++ b/libavfilter/vf_delogo.c
> > @@ -271,11 +271,6 @@ static av_cold int init(AVFilterContext *ctx)
> >     av_log(ctx, AV_LOG_VERBOSE, "x:%d y:%d, w:%d h:%d band:%d show:%d\n",
> >            s->x, s->y, s->w, s->h, s->band, s->show);
> > 
> > -    s->w += s->band*2;
> > -    s->h += s->band*2;
> > -    s->x -= s->band;
> > -    s->y -= s->band;
> > -
> >     return 0;
> > }
> > 
> > @@ -284,10 +279,21 @@ static int config_input(AVFilterLink *inlink)
> >     DelogoContext *s = inlink->dst->priv;
> > 
> >     /* Check whether the logo area fits in the frame */
> > -    if (s->x + (s->band - 1) < 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
> > -        s->y + (s->band - 1) < 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
> > -        av_log(s, AV_LOG_ERROR, "Logo area is outside of the frame.\n");
> > -        return AVERROR(EINVAL);
> > +    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
> > +        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
> > +        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
> > +               " logo(x:%d y:%d w:%d h:%d), frame(%dx%d),"
> > +               " auto set the area inside of the frame."
> > +               " Note: x and y must be 1 at least.\n",
> > +               s->x, s->y, s->w, s->h, inlink->w, inlink->h);
> > +        if (s->x + (s->band - 1) <= 0)
> > +            s->x = 1 + s->band;
> > +        if (s->y + (s->band - 1) <= 0)
> > +            s->y = 1 + s->band;
> > +        if (s->x + s->w - (s->band*2 - 2) > inlink->w)
> > +            s->w = inlink->w - s->x - (s->band*2 - 2);
> > +        if (s->y + s->h - (s->band*2 - 2) > inlink->h)
> > +            s->h = inlink->h - s->y - (s->band*2 - 2);
> >     }
> > 
> >     return 0;
> > @@ -313,21 +319,6 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> >     s->w = av_expr_eval(s->w_pexpr, s->var_values, s);
> >     s->h = av_expr_eval(s->h_pexpr, s->var_values, s);
> > 
> > -    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
> > -        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
> > -        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
> > -               " auto set the area inside of the frame\n");
> > -    }
> > -
> > -    if (s->x + (s->band - 1) <= 0)
> > -        s->x = 1 + s->band;
> > -    if (s->y + (s->band - 1) <= 0)
> > -        s->y = 1 + s->band;
> > -    if (s->x + s->w - (s->band*2 - 2) > inlink->w)
> > -        s->w = inlink->w - s->x - (s->band*2 - 2);
> > -    if (s->y + s->h - (s->band*2 - 2) > inlink->h)
> > -        s->h = inlink->h - s->y - (s->band*2 - 2);
> > -
> >     ret = config_input(inlink);
> >     if (ret < 0) {
> >         av_frame_free(&in);
> > -- 
> > 2.17.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".
Zhao Zhili Sept. 27, 2020, 1:18 p.m. UTC | #3
> On Sep 27, 2020, at 2:02 AM, Paul B Mahol <onemda@gmail.com> wrote:
> 
> On Sun, Sep 27, 2020 at 12:38:57AM +0800, Zhao Zhili wrote:
>> Ping.
>> 
>>> On Sep 16, 2020, at 1:09 PM, quinkblack@foxmail.com wrote:
>>> 
>>> From: Zhao Zhili <quinkblack@foxmail.com>
>>> 
>>> 1. Remove the modification of x, y, w and h parameters since they
>>>  are reset by filter_frame.
>>> 2. config_input leads to error out when logo area is outside of the
>>>  frame, while filter_frame fix the parameters automatically. Make
>>>  config_input don't return error to keep the logic consistent.
>>> ---
> 
> Is this causing overreads? Check with valgrind.

The patch doesn’t add something new. I double checked with valgrind and asan
which didn’t show overreads.

> 
>>> libavfilter/vf_delogo.c | 39 +++++++++++++++------------------------
>>> 1 file changed, 15 insertions(+), 24 deletions(-)
>>> 
>>> diff --git a/libavfilter/vf_delogo.c b/libavfilter/vf_delogo.c
>>> index 6069c30163..39f06512fa 100644
>>> --- a/libavfilter/vf_delogo.c
>>> +++ b/libavfilter/vf_delogo.c
>>> @@ -271,11 +271,6 @@ static av_cold int init(AVFilterContext *ctx)
>>>    av_log(ctx, AV_LOG_VERBOSE, "x:%d y:%d, w:%d h:%d band:%d show:%d\n",
>>>           s->x, s->y, s->w, s->h, s->band, s->show);
>>> 
>>> -    s->w += s->band*2;
>>> -    s->h += s->band*2;
>>> -    s->x -= s->band;
>>> -    s->y -= s->band;
>>> -
>>>    return 0;
>>> }
>>> 
>>> @@ -284,10 +279,21 @@ static int config_input(AVFilterLink *inlink)
>>>    DelogoContext *s = inlink->dst->priv;
>>> 
>>>    /* Check whether the logo area fits in the frame */
>>> -    if (s->x + (s->band - 1) < 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
>>> -        s->y + (s->band - 1) < 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
>>> -        av_log(s, AV_LOG_ERROR, "Logo area is outside of the frame.\n");
>>> -        return AVERROR(EINVAL);
>>> +    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
>>> +        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
>>> +        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
>>> +               " logo(x:%d y:%d w:%d h:%d), frame(%dx%d),"
>>> +               " auto set the area inside of the frame."
>>> +               " Note: x and y must be 1 at least.\n",
>>> +               s->x, s->y, s->w, s->h, inlink->w, inlink->h);
>>> +        if (s->x + (s->band - 1) <= 0)
>>> +            s->x = 1 + s->band;
>>> +        if (s->y + (s->band - 1) <= 0)
>>> +            s->y = 1 + s->band;
>>> +        if (s->x + s->w - (s->band*2 - 2) > inlink->w)
>>> +            s->w = inlink->w - s->x - (s->band*2 - 2);
>>> +        if (s->y + s->h - (s->band*2 - 2) > inlink->h)
>>> +            s->h = inlink->h - s->y - (s->band*2 - 2);
>>>    }
>>> 
>>>    return 0;
>>> @@ -313,21 +319,6 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *in)
>>>    s->w = av_expr_eval(s->w_pexpr, s->var_values, s);
>>>    s->h = av_expr_eval(s->h_pexpr, s->var_values, s);
>>> 
>>> -    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
>>> -        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
>>> -        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
>>> -               " auto set the area inside of the frame\n");
>>> -    }
>>> -
>>> -    if (s->x + (s->band - 1) <= 0)
>>> -        s->x = 1 + s->band;
>>> -    if (s->y + (s->band - 1) <= 0)
>>> -        s->y = 1 + s->band;
>>> -    if (s->x + s->w - (s->band*2 - 2) > inlink->w)
>>> -        s->w = inlink->w - s->x - (s->band*2 - 2);
>>> -    if (s->y + s->h - (s->band*2 - 2) > inlink->h)
>>> -        s->h = inlink->h - s->y - (s->band*2 - 2);
>>> -
>>>    ret = config_input(inlink);
>>>    if (ret < 0) {
>>>        av_frame_free(&in);
>>> -- 
>>> 2.17.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".
> _______________________________________________
> 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 mbox series

Patch

diff --git a/libavfilter/vf_delogo.c b/libavfilter/vf_delogo.c
index 6069c30163..39f06512fa 100644
--- a/libavfilter/vf_delogo.c
+++ b/libavfilter/vf_delogo.c
@@ -271,11 +271,6 @@  static av_cold int init(AVFilterContext *ctx)
     av_log(ctx, AV_LOG_VERBOSE, "x:%d y:%d, w:%d h:%d band:%d show:%d\n",
            s->x, s->y, s->w, s->h, s->band, s->show);
 
-    s->w += s->band*2;
-    s->h += s->band*2;
-    s->x -= s->band;
-    s->y -= s->band;
-
     return 0;
 }
 
@@ -284,10 +279,21 @@  static int config_input(AVFilterLink *inlink)
     DelogoContext *s = inlink->dst->priv;
 
     /* Check whether the logo area fits in the frame */
-    if (s->x + (s->band - 1) < 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
-        s->y + (s->band - 1) < 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
-        av_log(s, AV_LOG_ERROR, "Logo area is outside of the frame.\n");
-        return AVERROR(EINVAL);
+    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
+        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
+        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
+               " logo(x:%d y:%d w:%d h:%d), frame(%dx%d),"
+               " auto set the area inside of the frame."
+               " Note: x and y must be 1 at least.\n",
+               s->x, s->y, s->w, s->h, inlink->w, inlink->h);
+        if (s->x + (s->band - 1) <= 0)
+            s->x = 1 + s->band;
+        if (s->y + (s->band - 1) <= 0)
+            s->y = 1 + s->band;
+        if (s->x + s->w - (s->band*2 - 2) > inlink->w)
+            s->w = inlink->w - s->x - (s->band*2 - 2);
+        if (s->y + s->h - (s->band*2 - 2) > inlink->h)
+            s->h = inlink->h - s->y - (s->band*2 - 2);
     }
 
     return 0;
@@ -313,21 +319,6 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *in)
     s->w = av_expr_eval(s->w_pexpr, s->var_values, s);
     s->h = av_expr_eval(s->h_pexpr, s->var_values, s);
 
-    if (s->x + (s->band - 1) <= 0 || s->x + s->w - (s->band*2 - 2) > inlink->w ||
-        s->y + (s->band - 1) <= 0 || s->y + s->h - (s->band*2 - 2) > inlink->h) {
-        av_log(s, AV_LOG_WARNING, "Logo area is outside of the frame,"
-               " auto set the area inside of the frame\n");
-    }
-
-    if (s->x + (s->band - 1) <= 0)
-        s->x = 1 + s->band;
-    if (s->y + (s->band - 1) <= 0)
-        s->y = 1 + s->band;
-    if (s->x + s->w - (s->band*2 - 2) > inlink->w)
-        s->w = inlink->w - s->x - (s->band*2 - 2);
-    if (s->y + s->h - (s->band*2 - 2) > inlink->h)
-        s->h = inlink->h - s->y - (s->band*2 - 2);
-
     ret = config_input(inlink);
     if (ret < 0) {
         av_frame_free(&in);