[FFmpeg-devel] [PATCH] avfilter/vf_stack: round down internal item heights

[FFmpeg-devel] [PATCH] avfilter/vf_stack: round down internal item heights

[Timo Rothenpieler]

Following situation:
Someone wants to stack two yuv420p frames of the size 3x3 pixel each.
Now the various AV_CEIL_RSHIFT() calculations calculate a height of 2
pixel for each items subsampled planes.
However, for example in case of a vstack, the output frames will have a
height of 6, so the subsampled planes one of 3.
When the filter now tries to stack two items with a rounded-up height of
2 into a frame with a height of 3, it'll write an entire extra line past
the end of a buffer.

This patch instead rounds down all the item heights, to avoid this
situation. It's not ideal either, since now lines might be missed. But
that is definitely preferable over writing past the end of the
bufferThis patch instead rounds down all the item heights, to avoid this
situation. It's not ideal either, since now lines might be missed. But
that is definitely preferable over writing past the end of the buffer.
---
 libavfilter/vf_stack.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/libavfilter/vf_stack.c b/libavfilter/vf_stack.c
index 2bb3d9b1d2..46e7d6b7f5 100644
--- a/libavfilter/vf_stack.c
+++ b/libavfilter/vf_stack.c
@@ -220,11 +220,11 @@ static int config_output(AVFilterLink *outlink)
                 return ret;
             }
 
-            item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
+            item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
             item->height[0] = item->height[3] = inlink->h;
 
             if (i) {
-                item->y[1] = item->y[2] = AV_CEIL_RSHIFT(height, s->desc->log2_chroma_h);
+                item->y[1] = item->y[2] = height / (1 << s->desc->log2_chroma_h);
                 item->y[0] = item->y[3] = height;
 
                 height += ctx->inputs[i]->h;
@@ -244,7 +244,7 @@ static int config_output(AVFilterLink *outlink)
                 return ret;
             }
 
-            item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
+            item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
             item->height[0] = item->height[3] = inlink->h;
 
             if (i) {
@@ -278,14 +278,14 @@ static int config_output(AVFilterLink *outlink)
                     return ret;
                 }
 
-                item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
+                item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
                 item->height[0] = item->height[3] = inlink->h;
 
                 if ((ret = av_image_fill_linesizes(item->x, inlink->format, inw)) < 0) {
                     return ret;
                 }
 
-                item->y[1] = item->y[2] = AV_CEIL_RSHIFT(inh, s->desc->log2_chroma_h);
+                item->y[1] = item->y[2] = inh / (1 << s->desc->log2_chroma_h);
                 item->y[0] = item->y[3] = inh;
                 inw += ctx->inputs[k]->w;
             }
@@ -322,7 +322,7 @@ static int config_output(AVFilterLink *outlink)
                 return ret;
             }
 
-            item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
+            item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
             item->height[0] = item->height[3] = inlink->h;
 
             p2 = arg;
@@ -370,7 +370,7 @@ static int config_output(AVFilterLink *outlink)
                 return ret;
             }
 
-            item->y[1] = item->y[2] = AV_CEIL_RSHIFT(inh, s->desc->log2_chroma_h);
+            item->y[1] = item->y[2] = inh / (1 << s->desc->log2_chroma_h);
             item->y[0] = item->y[3] = inh;
 
             width  = FFMAX(width,  inlink->w + inw);
-- 
2.34.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".

Re: [FFmpeg-devel] [PATCH] avfilter/vf_stack: round down internal item heights

[Timo Rothenpieler]

On 23/03/2024 16:30, Timo Rothenpieler wrote:
> Following situation:
> Someone wants to stack two yuv420p frames of the size 3x3 pixel each.
> Now the various AV_CEIL_RSHIFT() calculations calculate a height of 2
> pixel for each items subsampled planes.
> However, for example in case of a vstack, the output frames will have a
> height of 6, so the subsampled planes one of 3.
> When the filter now tries to stack two items with a rounded-up height of
> 2 into a frame with a height of 3, it'll write an entire extra line past
> the end of a buffer.
> 
> This patch instead rounds down all the item heights, to avoid this
> situation. It's not ideal either, since now lines might be missed. But
> that is definitely preferable over writing past the end of the
> bufferThis patch instead rounds down all the item heights, to avoid this
> situation. It's not ideal either, since now lines might be missed. But
> that is definitely preferable over writing past the end of the buffer.
> ---
>   libavfilter/vf_stack.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/libavfilter/vf_stack.c b/libavfilter/vf_stack.c
> index 2bb3d9b1d2..46e7d6b7f5 100644
> --- a/libavfilter/vf_stack.c
> +++ b/libavfilter/vf_stack.c
> @@ -220,11 +220,11 @@ static int config_output(AVFilterLink *outlink)
>                   return ret;
>               }
>   
> -            item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
> +            item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
>               item->height[0] = item->height[3] = inlink->h;
>   
>               if (i) {
> -                item->y[1] = item->y[2] = AV_CEIL_RSHIFT(height, s->desc->log2_chroma_h);
> +                item->y[1] = item->y[2] = height / (1 << s->desc->log2_chroma_h);
>                   item->y[0] = item->y[3] = height;
>   
>                   height += ctx->inputs[i]->h;
> @@ -244,7 +244,7 @@ static int config_output(AVFilterLink *outlink)
>                   return ret;
>               }
>   
> -            item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
> +            item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
>               item->height[0] = item->height[3] = inlink->h;
>   
>               if (i) {
> @@ -278,14 +278,14 @@ static int config_output(AVFilterLink *outlink)
>                       return ret;
>                   }
>   
> -                item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
> +                item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
>                   item->height[0] = item->height[3] = inlink->h;
>   
>                   if ((ret = av_image_fill_linesizes(item->x, inlink->format, inw)) < 0) {
>                       return ret;
>                   }
>   
> -                item->y[1] = item->y[2] = AV_CEIL_RSHIFT(inh, s->desc->log2_chroma_h);
> +                item->y[1] = item->y[2] = inh / (1 << s->desc->log2_chroma_h);
>                   item->y[0] = item->y[3] = inh;
>                   inw += ctx->inputs[k]->w;
>               }
> @@ -322,7 +322,7 @@ static int config_output(AVFilterLink *outlink)
>                   return ret;
>               }
>   
> -            item->height[1] = item->height[2] = AV_CEIL_RSHIFT(inlink->h, s->desc->log2_chroma_h);
> +            item->height[1] = item->height[2] = inlink->h / (1 << s->desc->log2_chroma_h);
>               item->height[0] = item->height[3] = inlink->h;
>   
>               p2 = arg;
> @@ -370,7 +370,7 @@ static int config_output(AVFilterLink *outlink)
>                   return ret;
>               }
>   
> -            item->y[1] = item->y[2] = AV_CEIL_RSHIFT(inh, s->desc->log2_chroma_h);
> +            item->y[1] = item->y[2] = inh / (1 << s->desc->log2_chroma_h);
>               item->y[0] = item->y[3] = inh;
>   
>               width  = FFMAX(width,  inlink->w + inw);

ping

I'm pretty sure this patch is not the correct solution.
But right now there is a pretty nasty heap overflow in the filter that 
should be addressed somehow.
_______________________________________________
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".