[Mesa-dev] [PATCH] radeonsi/uvd: fix interlaced video buffer height alignment

[Leo Liu]

On 09/12/2017 11:37 AM, Christian König wrote:
> Am 12.09.2017 um 17:32 schrieb Leo Liu:
>>
>>
>> On 09/12/2017 11:23 AM, Christian König wrote:
>>> I don't think this is correct. A long long time ago I've came up 
>>> with this because the firmware didn't liked what you proposed below.
>> Since this change only affects 720p video, so I did quite a bit tests 
>> on 720p video dec/enc, and haven't see any problem.
>>
>> Can you point out some failed case?
>
> No, I briefly remember the issue was with some low res MPEG2 stream, 
> but no idea which one exactly.
>
>>
>>>
>>> Instead we should rather fix the scaler to use the original 
>>> width/height of the video buffer and not the adjusted width/height 
>>> of the resources.
>> To be honest, I don't really want to touch here, that's why I sent 
>> this patch alone for RFC, and also why I was got workaround for OMX 
>> one year ago, but now encounter the same again, and it seems not easy 
>> to fix on the blit, since it's scaling, not like OMX case, I can use 
>> src rect for dst.
>
> I think the problem is simply that you use the wrong variables.
>
> See there are video_buffer->width/heigth which are the original values 
> the application requested (IIRC) 

The height used from va/postproc.c is from video buffer.

static const VARectangle *
vlVaRegionDefault(const VARectangle *region, struct pipe_video_buffer *buf,
           VARectangle *def)
{
    if (region)
       return region;

    def->x = 0;
    def->y = 0;
    def->width = buf->width;
    def->height = buf->height;

    return def;
}


The problem is:

In si_uvd.c

struct pipe_video_buffer *si_video_buffer_create(struct pipe_context *pipe,
                          const struct pipe_video_buffer *tmpl)
{
struct pipe_video_buffer template;

template.height = align(tmpl->height / array_size, VL_MACROBLOCK_HEIGHT);


The original info with right height in the tmpl, and that's my first 
thought to deal with the issue.

but when you keep looking to the code, the tmpl got wiped out, and leave 
a new template with 32 aligned height.

The video buffer was created based on this new template.


> and there are the pipe_resource->width/height which are aligned so 
> that the hardware can deal with them.

Video buffer and pipe buffer are same, they both got aligned.


Regards,
Leo


>
> So while scaling we need to look at video_buffer->width/height instead 
> of the resource width/height to figure out the destination area of the 
> blit.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> Leo
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 12.09.2017 um 15:56 schrieb Leo Liu:
>>>> In code:
>>>>     template.height = align(tmpl->height / array_size, 
>>>> VL_MACROBLOCK_HEIGHT);
>>>>     ...
>>>>     template.height *= array_size;
>>>>
>>>> It turns out the height will be aligned with 2*VL_MACROBLOCK_HEIGHT.
>>>> The problematic case for example is when VA-API postproc scaling with
>>>> blit between interlaced buffers, if the size is 720 in height, it will
>>>> be actually scaled to 736 in height, so the scaled video will crop out
>>>> the extra 16 lines of height.
>>>>
>>>> Another example is when deint with 720p video from interlaced buffer
>>>> to progressive buffer. This problem happened on OMX, and got 
>>>> workaround
>>>> with patch:
>>>>
>>>> (0c374a777 st/omx/dec: set dst rect to match src size
>>>>
>>>> When creating interlaced video buffer, hegith set to 
>>>> "template.height =
>>>> align(tmpl->height/ array_size, VL_MACROBLOCK_HEIGHT);", and we use
>>>> "template.height *= array_size;" for the buffer height, so it actually
>>>> aligned with 32. With progressive video buffer it still aligned 
>>>> with 16,
>>>> thus causing different height between interlaced buffer and 
>>>> progressive
>>>> buffer for 4K (height=2160), and 720p (height=720).
>>>>
>>>> When transcode the video, this will cause the 16 lines corruption
>>>> at the bottom of the encode video.)
>>>>
>>>> Signed-off-by: Leo Liu <leo.liu at amd.com>
>>>> ---
>>>>   src/gallium/drivers/radeonsi/si_uvd.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/gallium/drivers/radeonsi/si_uvd.c 
>>>> b/src/gallium/drivers/radeonsi/si_uvd.c
>>>> index 2441ad248c..e4c55c20e1 100644
>>>> --- a/src/gallium/drivers/radeonsi/si_uvd.c
>>>> +++ b/src/gallium/drivers/radeonsi/si_uvd.c
>>>> @@ -62,7 +62,7 @@ struct pipe_video_buffer 
>>>> *si_video_buffer_create(struct pipe_context *pipe,
>>>>       array_size = tmpl->interlaced ? 2 : 1;
>>>>       template = *tmpl;
>>>>       template.width = align(tmpl->width, VL_MACROBLOCK_WIDTH);
>>>> -    template.height = align(tmpl->height / array_size, 
>>>> VL_MACROBLOCK_HEIGHT);
>>>> +    template.height = align(tmpl->height, VL_MACROBLOCK_HEIGHT) / 
>>>> array_size;
>>>>         vl_video_buffer_template(&templ, &template, 
>>>> resource_formats[0], 1, array_size, PIPE_USAGE_DEFAULT, 0);
>>>>       /* TODO: get tiling working */
>>>
>>>
>>
>