[FFmpeg-devel] [PATCH][VAAPI][2/6] Add common data structures and helpers

[Gwenole Beauchesne]

On Thu, 26 Feb 2009, Michael Niedermayer wrote:

> On Thu, Feb 26, 2009 at 02:20:34PM +0100, Gwenole Beauchesne wrote:
>> On Thu, 26 Feb 2009, Michael Niedermayer wrote:
>>
>>> i think the struct placed in data[0] should be designed so that it does not
>>> need any vaapi headers, that is contain pointers to structs instead of
>>> vaapi structs.
>>> then this "data[0] struct" can just be placed in avcodec.h and vaapi.h wont
>>> be needed.
>>
>> It's hard because VADisplay, VAConfigID, VAContextID, VASurfaceID are not
>> structs but either void * (VADisplay) or unsigned int (VA*ID). I think
>
> neither void * nor unsigned int would need a #include va.h

Though they are primitive types, a change in the VA API definitions would 
mess the things up if we want to maintain several versions of the API. 
Highly unlikely, but it's a case that might happen.

>>>> +/** \brief Allocate a new VA API render state structure. */
>>>> +struct vaapi_render_state *av_alloc_vaapi_render_state(void);
>>>
>>> do i understand correctly that the use provided get_buffer() is called and
>>> then the user calls av_alloc_vaapi_render_state()?
>>> this is ugly
>>> either lavc should already call av_alloc_vaapi_render_state() before
>>> get_buffer()
>>> or it should be done in default_get_buffer() and the users get_buffer() should
>>> call default_get_buffer()
>>> or allocate it by avpicture_alloc()
>>> i dunno which of these would be cleanest ...
>>
>> I agree that we shall not depend on a specific AVFrame::data[0] to stuff
>> the HW accelerator data in. We should not delegate the call to
>> default_get_buffer() to the user as he would certainly forget about it.
>>
>
>> Why not use an hwaccel_data member for it? Though, in that case, data[0]
>> shall not be NULL for correct operation of h264.
>
> iam ok with that.

The problem is then what to fill in data[0]. It cannot be NULL, it cannot 
be "constant" (for all allocs) either. If the HW accelerator cannot handle 
surface pixels readback, this shall be valid. If it does support that, it 
would have allocated data[0-2] accordingly and those allocations naturally 
yield different pointers, so no problem in that case.

I see two solutions:

- Make data[0] a linearly increasing ID just to make sure any allocated 
AVFrame::data[0] is different

- Replace: h->default_ref_list[0][i].data[0] == 
h->default_ref_list[1][i].data[0] with some ff_compare_picture() which 
handles hwaccel_data. However, this could be a frequent function call. So, 
make it static inline in mpegvideo_common.h?

>> Did you mean alloc_picture() in mpegvideo.c? It doesn't seem
>> avpicture_alloc() to be used anywhere but imgconvert.c.
>
> i meant avpicture_alloc() but it was just a idea after 1sec thinking

I don't see how to use it then.