[Mesa-dev] [PATCH v4 3/9] st/va: implement VaCreateSurfaces2 and VaQuerySurfaceAttributes

[Emil Velikov]

On 30 October 2015 at 09:59, Julien Isorce <j.isorce at samsung.com> wrote:
>
>
> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
> Sent: 29 October 2015 19:04
> To: Julien Isorce
> Cc: ML mesa-dev
> Subject: Re: [Mesa-dev] [PATCH v4 3/9] st/va: implement VaCreateSurfaces2 and VaQuerySurfaceAttributes
>
> On 29 October 2015 at 17:40, Julien Isorce <j.isorce at samsung.com> wrote:
>> +
>> +VAStatus
>> +vlVaCreateSurfaces2(VADriverContextP ctx, unsigned int format,
>> +                    unsigned int width, unsigned int height,
>> +                    VASurfaceID *surfaces, unsigned int num_surfaces,
>> +                    VASurfaceAttrib *attrib_list, unsigned int
>> +num_attribs) {
> [snip]
>> +    if (VA_RT_FORMAT_YUV400 != format &&
>>>Please drop this format.
>
> ok
>
>> +        VA_RT_FORMAT_YUV420 != format &&
>> +        VA_RT_FORMAT_YUV422 != format &&
>> +        VA_RT_FORMAT_YUV444 != format &&
>> +        VA_RT_FORMAT_RGB32  != format) {
>>>Whereas RGB32, if it works, is by sheer luck. Because ...
>
> [snip]
>> +
>> +    memset(&templat, 0, sizeof(templat));
>> +
> [snip]
>> +    if (format != VA_RT_FORMAT_RGB32)
>> +       templat.chroma_format = ChromaToPipe(format);
>> +
>>>chroma_format defaults to 0 (thanks to memset above) which likely maps to PIPE_VIDEO_CHROMA_FORMAT_400 from the pipe_video_chroma_format enum. With >>most drivers (incl the 'software implementation' in aux/vl) only handle 420/422/444.
>
> For nouveau it works because: it reaches:
>
> nouveau_vp3_video_buffer_create
> if (templat->buffer_format != PIPE_FORMAT_NV12) then vl_video_buffer_create (by chance does not rely on chroma 400 :) )
> but mostly relies on "pipe_format" (instead of chroma_format) and support PIPE_FORMAT_YV12, NV12, RGBA, BGRA, YUYV, UYVY.
>
Yes it seems that it should just work, despite the explicit
checks/handling of non 420/422 formats.

Upon closer look we have some duplication around the chroma_format
(and related *video_buffer_create) code.

Here is a wild list for those interested, likely only myself :-P
 - Consolidate all the format = foo, width/height /= 2 from
vl_create_mpeg12_decoder, vl_video_buffer_template,
vlVaVideoSurfaceSize, vlVaVideoSurfaceSize
 - Add some extra format (number of planes) checks in the above.
 - s/chroma_format/chroma_format_idc/ for radeons ?
 - Kill off NOUVEAU_RESOURCE_FLAG_LINEAR, directly use PIPE_BIND_LINEAR
 - Check for BIND_LINEAR|CURSOR|etc in nv30_mt_choose_storage_type,
similar to nv50/nvc0
 - Remove 'usage' and 'array_size' arguments from
vl_video_buffer_template() vl_video_buffer_create_ex()
 - Add 'bind' (nouveau + some radeons want BIND_LINEAR), and(?)
'flags' (nouveau)
 - Replace open-coded versions of vl_video_buffer_template(),
vl_video_buffer_sampler_view_planes(),
vl_video_buffer_sampler_view_components() in nouveau -
*_video_buffer_create().
 - Similar to above but for _buffer_destroy().
 - Nuke nouveau_video_buffer in favour of vl_video_buffer.
 - Subclass {nv84,nouveau_vp3}_video_buffer around the vl one, or move
the extra bits elsewhere nuke them and use
vl_video_buffer_create_ex{,2} directly ?


>
> What should I do then because:
>
> enum pipe_video_chroma_format
> {
>    PIPE_VIDEO_CHROMA_FORMAT_400,
>    PIPE_VIDEO_CHROMA_FORMAT_420,
>    PIPE_VIDEO_CHROMA_FORMAT_422,
>    PIPE_VIDEO_CHROMA_FORMAT_444
> };
>
>  --- Should I add PIPE_VIDEO_CHROMA_FORMAT_NONE ? ---
>
FORMAT_NONE sounds fine be me. Then again seems like Christian already
pushed the updated version.

> Also to compare with libva:
>
> /** attribute value for VAConfigAttribRTFormat */
> #define VA_RT_FORMAT_YUV420     0x00000001
> #define VA_RT_FORMAT_YUV422     0x00000002
> #define VA_RT_FORMAT_YUV444     0x00000004
> #define VA_RT_FORMAT_YUV411     0x00000008
> #define VA_RT_FORMAT_YUV400     0x00000010
> #define VA_RT_FORMAT_RGB16      0x00010000
> #define VA_RT_FORMAT_RGB32      0x00020000
> /* RGBP covers RGBP and BGRP fourcc */
> #define VA_RT_FORMAT_RGBP       0x00100000
> #define VA_RT_FORMAT_PROTECTED  0x80000000
>
For the rest... let's cross the bridge as we get there.

> [snip]
>> +no_res:
>> +   for (i = 0; i < num_surfaces; i++) {
>> +      if (surfaces[i] != VA_INVALID_ID)
>>>Should have caught this one sooner - this looks odd for couple of reasons.
>>> - vlVaDestroySurfaces removes a total of i surfaces. Thus we don't need a loop here.
>>> - handle_table_add() returns 0 on error and 0xffffffff
>>>(VA_INVALID_ID) is a valid handle.
>
> Ah right thx a lot ! (Actually the existing code was ok)
>
Heh error paths area always pain to test :)

Thanks
Emil

P.S. If you want to be extra nice (by no means a requirement), keep
the revision log within the commit message.