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

[Leo Liu]

On 09/12/2017 10:24 AM, Emil Velikov wrote:
> Hi Leo,
>
> On 12 September 2017 at 14:56, Leo Liu <leo.liu at amd.com> wrote:
>> 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
>>
> With this patch in, can/should we revert this patch?
The OMX code has been changed. the alternative for "Revert patch OMX" 
will be followed along with other patches in a set.

This alignment patch will not be committed alone. Most likely Sending it 
alone is for "RFC", it would be better to add RFC before I sent.

> Looks like this should have a a fixes and/or a stable tag.
Yes. It's not currently affecting OMX, since the workaround for OMX,  
but it affects VA-API scaling for interlaced buffers.


>
>> 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.)
>>
> Humble suggestion for alternative, simpler, commit message.
Received. Thanks.

>
> "The current code (snippet below), incorrectly calculates the height
> of the video.
> Namely: the alignment happens before the division, thus the height
> will effectively be aligned to array_size*VL_MACROBLOCK_HEIGHT.
>
>     template.height = align(tmpl->height / array_size, VL_MACROBLOCK_HEIGHT);
>
> So when array_size != 1 (aka interleaved buffer) the resulting height is wrong.
> For example: when resolutions is 720, we'll end up with 736.
>
> Adjust the order to align first and then divide.
>
> Note: earlier commit 0c374a777 ("st/omx/dec: set dst rect to match src
> size") worked around this issue.
> With this fix in, we can remove (XXX: or keep) it because $REASON.
>
> Fixes: ???
> Cc: mesa-stable at lists.freedesktop.org
> "
>
> With the above,
> Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>

Leo

> Emil