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

[Julien Isorce]

-----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.


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 ? ---

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

[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)

Julien

>>-Emil