diff mbox

[FFmpeg-devel] Patch for vf_fillborders.c – Bug?

Message ID 5a70adc0-d86f-bc89-b749-5285d63c68fb@CoSoCo.de
State New
Headers show

Commit Message

Ulf Zibis March 6, 2019, 11:52 p.m. UTC 
Hi,

I think there is a bug in vf_fillborders.c 16 bit routines.

When using memset or memcopy, I think, correct linesize instead
s->planewidth[p] should be used.
When using arrray syntax, I think, correct s->planewidth[p] instead
linesize should be used.

See my proposed patch.

-Ulf

Comments

Michael Niedermayer March 7, 2019, 11:53 p.m. UTC | #1 
On Thu, Mar 07, 2019 at 12:52:32AM +0100, Ulf Zibis wrote:
> Hi,
> 
> I think there is a bug in vf_fillborders.c 16 bit routines.
> 
> When using memset or memcopy, I think, correct linesize instead
> s->planewidth[p] should be used.
> When using arrray syntax, I think, correct s->planewidth[p] instead
> linesize should be used.
> 
> See my proposed patch.
> 
> -Ulf
> 

>  .gitignore                   |    2 
>  libavfilter/vf_fillborders.c |  125 ++++++++++++++++++++-----------------------
>  2 files changed, 63 insertions(+), 64 deletions(-)
> c9197f5c0215e05633f5a09071cc6ba87e74a757  vf_fillborders_linesize_in_16-bit.patch
> From f3513f992e0b5595f2644257b92fdea6189592de Mon Sep 17 00:00:00 2001
> From: Ulf Zibis <Ulf.Zibis@CoSoCo.de>
> Date: 07.03.2019, 00:34:51
> 
> Correct usage of linesize and width in 16 bit depth routines.
> 

> diff --git a/.gitignore b/.gitignore
> index 0e57cb0..7819c84 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -36,3 +36,5 @@
>  /lcov/
>  /src
>  /mapfile
> +/nbproject
> +/debug

this shouldnt be in the patch


[...]
Ulf Zibis March 8, 2019, 9:49 a.m. UTC | #2 
Hi Michael,

Am 08.03.19 um 00:53 schrieb Michael Niedermayer:
> On Thu, Mar 07, 2019 at 12:52:32AM +0100, Ulf Zibis wrote:
>> diff --git a/.gitignore b/.gitignore
>> index 0e57cb0..7819c84 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -36,3 +36,5 @@
>>  /lcov/
>>  /src
>>  /mapfile
>> +/nbproject
>> +/debug
> this shouldnt be in the patch

Thanks for your note.

/nbproject is automatically created by NetBeans IDE I'm using to develop.
I think this ignore tag could be useful for other developers using
NetBeans IDE.

/debug is a private folder for my testing.
Can some other developer please give me a practical hint how to deal
with private folders not to appear in GIT patches?

-Ulf
Tobias Rapp March 8, 2019, 9:59 a.m. UTC | #3 
On 08.03.2019 10:49, Ulf Zibis wrote:
> [...]
> Can some other developer please give me a practical hint how to deal
> with private folders not to appear in GIT patches?

I'm using .git/info/exclude to ignore files that are only found within 
my private developing environment.

Regards,
Tobias
Ulf Zibis March 8, 2019, 1:11 p.m. UTC | #4 
Am 08.03.19 um 10:59 schrieb Tobias Rapp:
> On 08.03.2019 10:49, Ulf Zibis wrote:
>> [...]
>> Can some other developer please give me a practical hint how to deal
>> with private folders not to appear in GIT patches?
>
> I'm using .git/info/exclude to ignore files that are only found within
> my private developing environment.
This is a valuable hint, thanks. But  I suspect it is easy to manage
ignores project-wise with this approach.

Are there other possibilities which are directly project-bounded?

-Ulf
Moritz Barsnick March 8, 2019, 8:26 p.m. UTC | #5 
On Fri, Mar 08, 2019 at 14:11:30 +0100, Ulf Zibis wrote:
> >> Can some other developer please give me a practical hint how to deal
> >> with private folders not to appear in GIT patches?
[...]
> Are there other possibilities which are directly project-bounded?

Hoe about not committing them in the first place? (Don't use "git
commit -A", but instead carefully inspect everything "git status"
offers you.) Or, if you must, for your development desire, then commit
them in a separate commit which you then don't include in your exported
patch(es).

I don't know about Netbeans, but e.g. Qt Creator offers me to add its
projects files to a detected revision control, but I can click "no
thanks" (or un-add them later, before committing).

Moritz
Ulf Zibis March 9, 2019, 12:44 p.m. UTC | #6 
Am 08.03.19 um 21:26 schrieb Moritz Barsnick:
>> Are there other possibilities which are directly project-bounded?
> Hoe about not committing them in the first place? (Don't use "git
> commit -A", but instead carefully inspect everything "git status"
> offers you.) Or, if you must, for your development desire, then commit
> them in a separate commit which you then don't include in your exported
> patch(es).
>
> I don't know about Netbeans, but e.g. Qt Creator offers me to add its
> projects files to a detected revision control, but I can click "no
> thanks" (or un-add them later, before committing).

I don't see this in NetBeans, but I've found a solution. Thanks for your hints!

I was in error about the interpretion of .git/info/exclude. I thought it was meant as path in $HOME folder. In the project's folder it does what I want, thanks Tobias.

-Ulf
diff mbox

Patch

From f3513f992e0b5595f2644257b92fdea6189592de Mon Sep 17 00:00:00 2001
From: Ulf Zibis <Ulf.Zibis@CoSoCo.de>
Date: 07.03.2019, 00:34:51

Correct usage of linesize and width in 16 bit depth routines.

diff --git a/.gitignore b/.gitignore
index 0e57cb0..7819c84 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,3 +36,5 @@ 
 /lcov/
 /src
 /mapfile
+/nbproject
+/debug
diff --git a/libavfilter/vf_fillborders.c b/libavfilter/vf_fillborders.c
index 1344587..019cdff 100644
--- a/libavfilter/vf_fillborders.c
+++ b/libavfilter/vf_fillborders.c
@@ -89,25 +89,27 @@ 
     for (p = 0; p < s->nb_planes; p++) {
         uint8_t *ptr = frame->data[p];
         int linesize = frame->linesize[p];
+        int nb_leftbytes = s->borders[p].left * linesize / s->planewidth[p];
+        int nb_rightbytes = s->borders[p].right * linesize / s->planewidth[p];
 
         for (y = s->borders[p].top; y < s->planeheight[p] - s->borders[p].bottom; y++) {
             memset(ptr + y * linesize,
-                   *(ptr + y * linesize + s->borders[p].left),
-                   s->borders[p].left);
-            memset(ptr + y * linesize + s->planewidth[p] - s->borders[p].right,
-                   *(ptr + y * linesize + s->planewidth[p] - s->borders[p].right - 1),
-                   s->borders[p].right);
+                   *(ptr + y * linesize + nb_leftbytes),
+                   nb_leftbytes);
+            memset(ptr + y * linesize + linesize - nb_rightbytes,
+                   *(ptr + y * linesize + linesize - nb_rightbytes - 1),
+                   nb_rightbytes);
         }
 
         for (y = 0; y < s->borders[p].top; y++) {
             memcpy(ptr + y * linesize,
-                   ptr + s->borders[p].top * linesize, s->planewidth[p]);
+                   ptr + s->borders[p].top * linesize, linesize);
         }
 
         for (y = s->planeheight[p] - s->borders[p].bottom; y < s->planeheight[p]; y++) {
             memcpy(ptr + y * linesize,
                    ptr + (s->planeheight[p] - s->borders[p].bottom - 1) * linesize,
-                   s->planewidth[p]);
+                   linesize);
         }
     }
 }
@@ -118,28 +120,28 @@ 
 
     for (p = 0; p < s->nb_planes; p++) {
         uint16_t *ptr = (uint16_t *)frame->data[p];
-        int linesize = frame->linesize[p] / 2;
+        int linesize = frame->linesize[p];
 
         for (y = s->borders[p].top; y < s->planeheight[p] - s->borders[p].bottom; y++) {
             for (x = 0; x < s->borders[p].left; x++) {
-                ptr[y * linesize + x] =  *(ptr + y * linesize + s->borders[p].left);
+                ptr[y * s->planewidth[p] + x] =  *(ptr + y * s->planewidth[p] + s->borders[p].left);
             }
 
             for (x = 0; x < s->borders[p].right; x++) {
-                ptr[y * linesize + s->planewidth[p] - s->borders[p].right + x] =
-                   *(ptr + y * linesize + s->planewidth[p] - s->borders[p].right - 1);
+                ptr[y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right + x] =
+                   *(ptr + y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right - 1);
             }
         }
 
         for (y = 0; y < s->borders[p].top; y++) {
             memcpy(ptr + y * linesize,
-                   ptr + s->borders[p].top * linesize, s->planewidth[p] * 2);
+                   ptr + s->borders[p].top * linesize, linesize);
         }
 
         for (y = s->planeheight[p] - s->borders[p].bottom; y < s->planeheight[p]; y++) {
             memcpy(ptr + y * linesize,
                    ptr + (s->planeheight[p] - s->borders[p].bottom - 1) * linesize,
-                   s->planewidth[p] * 2);
+                   linesize);
         }
     }
 }
@@ -154,25 +156,25 @@ 
 
         for (y = s->borders[p].top; y < s->planeheight[p] - s->borders[p].bottom; y++) {
             for (x = 0; x < s->borders[p].left; x++) {
-                ptr[y * linesize + x] = ptr[y * linesize + s->borders[p].left * 2 - 1 - x];
+                ptr[y * s->planewidth[p] + x] = ptr[y * s->planewidth[p] + s->borders[p].left * 2 - 1 - x];
             }
 
             for (x = 0; x < s->borders[p].right; x++) {
-                ptr[y * linesize + s->planewidth[p] - s->borders[p].right + x] =
-                    ptr[y * linesize + s->planewidth[p] - s->borders[p].right - 1 - x];
+                ptr[y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right + x] =
+                    ptr[y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right - 1 - x];
             }
         }
 
         for (y = 0; y < s->borders[p].top; y++) {
             memcpy(ptr + y * linesize,
                    ptr + (s->borders[p].top * 2 - 1 - y) * linesize,
-                   s->planewidth[p]);
+                   linesize);
         }
 
         for (y = 0; y < s->borders[p].bottom; y++) {
             memcpy(ptr + (s->planeheight[p] - s->borders[p].bottom + y) * linesize,
                    ptr + (s->planeheight[p] - s->borders[p].bottom - 1 - y) * linesize,
-                   s->planewidth[p]);
+                   linesize);
         }
     }
 }
@@ -183,29 +185,29 @@ 
 
     for (p = 0; p < s->nb_planes; p++) {
         uint16_t *ptr = (uint16_t *)frame->data[p];
-        int linesize = frame->linesize[p] / 2;
+        int linesize = frame->linesize[p];
 
         for (y = s->borders[p].top; y < s->planeheight[p] - s->borders[p].bottom; y++) {
             for (x = 0; x < s->borders[p].left; x++) {
-                ptr[y * linesize + x] = ptr[y * linesize + s->borders[p].left * 2 - 1 - x];
+                ptr[y * s->planewidth[p] + x] = ptr[y * s->planewidth[p] + s->borders[p].left * 2 - 1 - x];
             }
 
             for (x = 0; x < s->borders[p].right; x++) {
-                ptr[y * linesize + s->planewidth[p] - s->borders[p].right + x] =
-                    ptr[y * linesize + s->planewidth[p] - s->borders[p].right - 1 - x];
+                ptr[y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right + x] =
+                    ptr[y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right - 1 - x];
             }
         }
 
         for (y = 0; y < s->borders[p].top; y++) {
             memcpy(ptr + y * linesize,
                    ptr + (s->borders[p].top * 2 - 1 - y) * linesize,
-                   s->planewidth[p] * 2);
+                   linesize);
         }
 
         for (y = 0; y < s->borders[p].bottom; y++) {
             memcpy(ptr + (s->planeheight[p] - s->borders[p].bottom + y) * linesize,
                    ptr + (s->planeheight[p] - s->borders[p].bottom - 1 - y) * linesize,
-                   s->planewidth[p] * 2);
+                   linesize);
         }
     }
 }
@@ -216,21 +218,23 @@ 
 
     for (p = 0; p < s->nb_planes; p++) {
         uint8_t *ptr = frame->data[p];
-        uint8_t fill = s->fill[p];
         int linesize = frame->linesize[p];
+        int nb_leftbytes = s->borders[p].left * linesize / s->planewidth[p];
+        int nb_rightbytes = s->borders[p].right * linesize / s->planewidth[p];
+        uint8_t fill = s->fill[p];
 
         for (y = s->borders[p].top; y < s->planeheight[p] - s->borders[p].bottom; y++) {
-            memset(ptr + y * linesize, fill, s->borders[p].left);
-            memset(ptr + y * linesize + s->planewidth[p] - s->borders[p].right, fill,
-                   s->borders[p].right);
+            memset(ptr + y * linesize, fill, nb_leftbytes);
+            memset(ptr + y * linesize + linesize - nb_rightbytes, fill,
+                   nb_rightbytes);
         }
 
         for (y = 0; y < s->borders[p].top; y++) {
-            memset(ptr + y * linesize, fill, s->planewidth[p]);
+            memset(ptr + y * linesize, fill, linesize);
         }
 
         for (y = s->planeheight[p] - s->borders[p].bottom; y < s->planeheight[p]; y++) {
-            memset(ptr + y * linesize, fill, s->planewidth[p]);
+            memset(ptr + y * linesize, fill, linesize);
         }
     }
 }
@@ -242,27 +246,26 @@ 
     for (p = 0; p < s->nb_planes; p++) {
         uint16_t *ptr = (uint16_t *)frame->data[p];
         uint16_t fill = s->fill[p] << (s->depth - 8);
-        int linesize = frame->linesize[p] / 2;
 
         for (y = s->borders[p].top; y < s->planeheight[p] - s->borders[p].bottom; y++) {
             for (x = 0; x < s->borders[p].left; x++) {
-                ptr[y * linesize + x] = fill;
+                ptr[y * s->planewidth[p] + x] = fill;
             }
 
             for (x = 0; x < s->borders[p].right; x++) {
-                ptr[y * linesize + s->planewidth[p] - s->borders[p].right + x] = fill;
+                ptr[y * s->planewidth[p] + s->planewidth[p] - s->borders[p].right + x] = fill;
             }
         }
 
         for (y = 0; y < s->borders[p].top; y++) {
             for (x = 0; x < s->planewidth[p]; x++) {
-                ptr[y * linesize + x] = fill;
+                ptr[y * s->planewidth[p] + x] = fill;
             }
         }
 
         for (y = s->planeheight[p] - s->borders[p].bottom; y < s->planeheight[p]; y++) {
             for (x = 0; x < s->planewidth[p]; x++) {
-                ptr[y * linesize + x] = fill;
+                ptr[y * s->planewidth[p] + x] = fill;
             }
         }
     }
@@ -281,44 +284,38 @@ 
 {
     AVFilterContext *ctx = inlink->dst;
     FillBordersContext *s = ctx->priv;
+    if (inlink->w < s->left + s->right ||
+            inlink->w <= s->left ||
+            inlink->w <= s->right ||
+            inlink->h < s->top + s->bottom ||
+            inlink->h <= s->top ||
+            inlink->h <= s->bottom ||
+            inlink->w < s->left * 2 ||
+            inlink->w < s->right * 2 ||
+            inlink->h < s->top * 2 ||
+            inlink->h < s->bottom * 2) {
+        av_log(ctx, AV_LOG_ERROR, "Borders are bigger than input frame size.\n");
+        return AVERROR(EINVAL);
+        }
+
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
 
     s->nb_planes = desc->nb_components;
     s->depth = desc->comp[0].depth;
 
-    s->planeheight[1] = s->planeheight[2] = AV_CEIL_RSHIFT(inlink->h, desc->log2_chroma_h);
     s->planeheight[0] = s->planeheight[3] = inlink->h;
-    s->planewidth[1]  = s->planewidth[2]  = AV_CEIL_RSHIFT(inlink->w, desc->log2_chroma_w);
+    s->planeheight[1] = s->planeheight[2] = AV_CEIL_RSHIFT(inlink->h, desc->log2_chroma_h);
     s->planewidth[0]  = s->planewidth[3]  = inlink->w;
+    s->planewidth[1]  = s->planewidth[2]  = AV_CEIL_RSHIFT(inlink->w, desc->log2_chroma_w);
 
-    s->borders[0].left   = s->borders[3].left = s->left;
-    s->borders[0].right  = s->borders[3].right = s->right;
-    s->borders[0].top    = s->borders[3].top = s->top;
+    s->borders[0].left   = s->borders[3].left   = s->left;
+    s->borders[1].left   = s->borders[2].left   = s->left >> desc->log2_chroma_w;
+    s->borders[0].right  = s->borders[3].right  = s->right;
+    s->borders[1].right  = s->borders[2].right  = s->right >> desc->log2_chroma_w;
+    s->borders[0].top    = s->borders[3].top    = s->top;
+    s->borders[1].top    = s->borders[2].top    = s->top >> desc->log2_chroma_h;
     s->borders[0].bottom = s->borders[3].bottom = s->bottom;
-
-    s->borders[1].left   = s->left >> desc->log2_chroma_w;
-    s->borders[1].right  = s->right >> desc->log2_chroma_w;
-    s->borders[1].top    = s->top >> desc->log2_chroma_h;
-    s->borders[1].bottom = s->bottom >> desc->log2_chroma_h;
-
-    s->borders[2].left   = s->left >> desc->log2_chroma_w;
-    s->borders[2].right  = s->right >> desc->log2_chroma_w;
-    s->borders[2].top    = s->top >> desc->log2_chroma_h;
-    s->borders[2].bottom = s->bottom >> desc->log2_chroma_h;
-
-    if (inlink->w < s->left + s->right ||
-        inlink->w <= s->left ||
-        inlink->w <= s->right ||
-        inlink->h < s->top + s->bottom ||
-        inlink->h <= s->top ||
-        inlink->h <= s->bottom ||
-        inlink->w < s->left * 2 ||
-        inlink->w < s->right * 2 ||
-        inlink->h < s->top * 2 ||
-        inlink->h < s->bottom * 2) {
-        av_log(ctx, AV_LOG_ERROR, "Borders are bigger than input frame size.\n");
-        return AVERROR(EINVAL);
-    }
+    s->borders[1].bottom = s->borders[2].bottom = s->bottom >> desc->log2_chroma_h;
 
     switch (s->mode) {
     case FM_SMEAR:  s->fillborders = s->depth <= 8 ? smear_borders8  : smear_borders16;  break;