[vlc-devel] [PATCH] [RFC] picture: integrate the 2 extra lines in the height padding

[Steve Lhomme]

Le 05/04/2018 à 10:05, Rémi Denis-Courmont a écrit :
> Le 5 avril 2018 09:55:25 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>> This is a rebased/reworked version of
>> https://patches.videolan.org/patch/19128/
>>
>> Also since this is done in picture_Setup() I wonder if we should also
>> update the i_width/i_height of the picture accordingly. Right now a
>> picture that has its height modified for padding has its plane lines
>> correct but the height reported doesn't match. Leading to potential
>> misdetection of padding/alignment.
>>
>>
>> Le 05/04/2018 à 08:51, Steve Lhomme a écrit :
>>> No need for extra lines when there's already padding. Plus it may
>> break the
>>> alignment of the secondary planes.
>>> ---
>>>    src/misc/picture.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/misc/picture.c b/src/misc/picture.c
>>> index 7506e47725..7ce1170702 100644
>>> --- a/src/misc/picture.c
>>> +++ b/src/misc/picture.c
>>> @@ -133,17 +133,17 @@ int picture_Setup( picture_t *p_picture, const
>> video_format_t *restrict fmt )
>>>    
>>>        unsigned width, height;
>>>    
>>> +    /* Hack: append two scan lines for some SIMD assembler */
>>> +    if (unlikely(add_overflow(fmt->i_height, 2 * i_ratio_h,
>> &height)))
>>> +        return VLC_EGENERIC;
>>> +
>>>        if (unlikely(add_overflow(fmt->i_width, i_modulo_w - 1,
>> &width))
>>> -     || unlikely(add_overflow(fmt->i_height, i_modulo_h - 1,
>> &height)))
>>> +     || unlikely(add_overflow(height, i_modulo_h - 1, &height)))
>>>            return VLC_EGENERIC;
>>>    
>>>        width = width / i_modulo_w * i_modulo_w;
>>>        height = height / i_modulo_h * i_modulo_h;
>>>    
>>> -    /* Hack: append two scan lines for some SIMD assembler */
>>> -    if (unlikely(add_overflow(height, 2 * i_ratio_h, &height)))
>>> -        return VLC_EGENERIC;
>>> -
>>>        /* plane_t uses 'int'. */
>>>        if (unlikely(width > INT_MAX) || unlikely(height > INT_MAX))
>>>            return VLC_EGENERIC;
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> This works if SIMD simply assumes two extra lines after visible lines. But it does not work if SIMD assumes two extra lines after the aligned visible lines. This could be an algorithm operating on macro blocks and expecting some extra padding...
>
> Basically, this patch assumes that either lines count must be over-aligned or there must be extra lines, but not both for any given algorithm. Not that I'd know if there is or ever will be such a case in underlying libraries.

Yes, and that's also a problem. We keep on misaligning every buffer (we 
do the alignment and then add 2 lines, breaking the i_modulo_h) for some 
hypothetical case that may not exist anymore. For the ASM code I've 
looked at/modified it was always possible to stop processing after the 
visible lines. I'm not sure if there's anything that requires that anymore.

And since with 4.0 modules might each handle their pool, the ones that 
need more specific alignment may deal with it themselves.

>
> Regardless, I don't see how this fixes anything. The alignment of next planes should not be dependent on the number of lines of the previous planes. IMO, all planes should be individually page-aligned.

This is the case for QSV
http://git.videolan.org/?p=vlc.git;a=blob;f=modules/codec/qsv.c;h=0f865dd4ab748fdc4cb2e8685b31547597ed0c55;hb=HEAD#l761

Due to internal optimizations they require that the UV planes follow 
extactly the Y plane for NV12 textures and that they are aligned to 16 
or 32 pixels. There might be cases where this assert doesn't work if the 
added 2 lines don't fall in a bigger 16/32 rounding.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180405/24f15abc/attachment.html>