[FFmpeg-devel] framepool width and potential patch for bug 9873

[FFmpeg-devel] framepool width and potential patch for bug 9873

[Brendan Hack]

Hi,

I reported https://trac.ffmpeg.org/ticket/9873 last week and have since 
been digging into the code to see if I can get a patch together. I've 
found the underlying issue in ff_frame_pool_video_init at line 77 of 
libavfilter/framepool.c:

         ret = av_image_fill_linesizes(pool->linesize, pool->format,
                                       FFALIGN(pool->width, align));

When creating frames for frei0r filtering an align value of 16 gets 
passed in. This works fine for widths like 400 where FFALIGN just 
returns 400 since it's a multiple of 16. But for values like 1800 it 
pushes the width up to 1808 which then generates linesizes for that 
width which breaks the frei0r requirement that the stride matches width 
* 4 since width is now too large.

Initially I put together a patch that adds an extra parameter to 
ff_frame_pool_video_init that tells it just to use the width directly 
and not try to align it. This seemed a bit awkward though, especially 
when I tried to figure our why the code is even trying to make the width 
a multiple of the align value (which is effectively what it's doing) to 
start with. I can't make sense of why it's calling FFALIGN there at all 
and checking the commit history didn't provide any hints. If anything I 
would have thought that you would want to increase the width so that the 
final line size would be aligned, EG: FFALIGN(pool->width*4, align)/4, 
rather than the width itself.

In any case I have patches that work both ways, either an extra flag to 
not do the aligned width or the FFALIGN(pool->width*4, align)/4 fix. The 
former I'm confident of but am not so sure about the latter since I 
don't understand why it's there to start with, nor whether it may break 
other filters. The fate tests all pass with it though on my system. It 
will certainly work with frei0r though since it requires width to be a 
multiple of 8 which will always give a line size that's aligned to 16.

Any advice on which one I should submit would be great.

thanks

Brendan


_______________________________________________
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] framepool width and potential patch for bug 9873

[Timo Rothenpieler]

On 20.08.2022 13:21, Brendan Hack wrote:
> Hi,
> 
> I reported https://trac.ffmpeg.org/ticket/9873 last week and have since 
> been digging into the code to see if I can get a patch together. I've 
> found the underlying issue in ff_frame_pool_video_init at line 77 of 
> libavfilter/framepool.c:
> 
>          ret = av_image_fill_linesizes(pool->linesize, pool->format,
>                                        FFALIGN(pool->width, align));
> 
> When creating frames for frei0r filtering an align value of 16 gets 
> passed in. This works fine for widths like 400 where FFALIGN just 
> returns 400 since it's a multiple of 16. But for values like 1800 it 
> pushes the width up to 1808 which then generates linesizes for that 
> width which breaks the frei0r requirement that the stride matches width 
> * 4 since width is now too large.

Not sure what you mean. The code you quoted does not touch width. It 
just bumps up the linesize, to align the start of every line on the 
desired align value.

> Initially I put together a patch that adds an extra parameter to 
> ff_frame_pool_video_init that tells it just to use the width directly 
> and not try to align it. This seemed a bit awkward though, especially 

That parameter already exists. It's called the align.

> when I tried to figure our why the code is even trying to make the width 
> a multiple of the align value (which is effectively what it's doing) to 
> start with. I can't make sense of why it's calling FFALIGN there at all 
> and checking the commit history didn't provide any hints. If anything I 
> would have thought that you would want to increase the width so that the 
> final line size would be aligned, EG: FFALIGN(pool->width*4, align)/4, 
> rather than the width itself.
> 
> In any case I have patches that work both ways, either an extra flag to 
> not do the aligned width or the FFALIGN(pool->width*4, align)/4 fix. The 
> former I'm confident of but am not so sure about the latter since I 
> don't understand why it's there to start with, nor whether it may break 

You mean you don't understand the concept of aligning lines?
The whole point is that a lot of hardware instructions require the data 
they work on to be aligned properly. Hence the linesize needs to be 
larger than width, to make sure every line starts well aligned.

> other filters. The fate tests all pass with it though on my system. It 
> will certainly work with frei0r though since it requires width to be a 
> multiple of 8 which will always give a line size that's aligned to 16.
> 
> Any advice on which one I should submit would be great.

If frei0r tells it to align it to 16 bytes, but then fails because it 
expects it to be packed without gaps, it's just passing the wrong 
alignment, and should pass 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] framepool width and potential patch for bug 9873

[Timo Rothenpieler]

On 20.08.2022 14:27, Brendan Hack wrote:
> Oh, I think I've got the wrong end of the stick here in regards to what 
> align does in ff_frame_pool_video_init. Frei0r only needs the start of 
> the buffer to be aligned to 16 bytes. It doesn't need each _line_ to be 
> explicitly aligned to 16 bytes since its requirement that the width be a 
> multiple of 8 enforces that (since every 8 pixels is 32 bytes).

Now that sounds like the align should be 8, not 16 or 1, in order for 
frames with odd sizes still work?
_______________________________________________
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] framepool width and potential patch for bug 9873

[Paul B Mahol]

On Sat, Aug 20, 2022 at 2:30 PM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 20.08.2022 14:27, Brendan Hack wrote:
> > Oh, I think I've got the wrong end of the stick here in regards to what
> > align does in ff_frame_pool_video_init. Frei0r only needs the start of
> > the buffer to be aligned to 16 bytes. It doesn't need each _line_ to be
> > explicitly aligned to 16 bytes since its requirement that the width be a
> > multiple of 8 enforces that (since every 8 pixels is 32 bytes).
>
> Now that sounds like the align should be 8, not 16 or 1, in order for
> frames with odd sizes still work?
>

IIRC michael have wrote fix for frei0r filter, dunno if its merged.


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