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

[Christian König]

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) and there are the 
pipe_resource->width/height which are aligned so that the hardware can 
deal with them.

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 */
>>
>>
>