On 30 October 2015 at 09:59, Julien Isorce <j.iso...@samsung.com> wrote:
>
>
> -----Original Message-----
> From: Emil Velikov [mailto:emil.l.veli...@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.iso...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
