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

[Reimar Döffinger]

On Fri, Feb 27, 2009 at 01:11:37PM +0100, Gwenole Beauchesne wrote:
> On Fri, 27 Feb 2009, Reimar D?ffinger wrote:
>
>> The purpose of get_buffer is to use a buffer provided by the application
>> and let the application do its own buffer management however it wants
>> to.
>> Now, the problem why you want to do it this way seems to be that the
>> application otherwise would need a callback to FFmpeg to allocate
>> hwaccel_data.
>> The question is, why is that needed? The reason seems to be
>> "struct vaapi_render_state_private" which the application can not
>> allocate itself.
>
> You can see that vaapi_render_state_private as vdpau_render_state. 
> Currently, the application allocates those. But Michael wanted lavc to 
> handle that itself, so I was only suggesting patches to achieve this. Since 
> this yields mess, in your opinion, it's probably not a good idea then. i.e. 
> let's just stick to the current approach: have the application allocate the 
> *_render_state structs.

Well, I think Michael's consideration is that he wants to be able to
change the structs without breaking ABI.
However, even with your approach, only vaapi_render_state_private can be
extended without breaking ABI, vdpau_render_state can not be extended
(since that new element would refer to something within
vaapi_render_state_private in previous libavcodec versions).
Thus, I think you lose nothing when still letting the application allocate
vdpau_render_state, you'd just have to find a way to allocate
vaapi_render_state_private within FFmpeg.

>> Is this data in vaapi_render_state_private so very much
>> performance-critical that it absolutely must extend struct 
>> vaapi_render_state?
>> There could be a pointer to it in a "libavcodec-private" part of AVFrame 
>> and
>> there would be no issue at all because it is nothing the user ever needs 
>> to care about.
>> Or it could be a pointer in struct vaapi_render_state, though then you
>> can initialize it only _after_ calling get_buffer.
>
> And when/how the deallocation of the struct could be scheduled? You are not 
> guaranteed to no longer need the data after ::end_frame() since what 
> ff_draw_horiz_band() sends is not necessarily the surface that was just 
> rendered. So this has to be postponed to at least ::release_buffer().

I can't see the problem, FFmpeg can free it before calling
release_buffer, the code should be almost identical with what you
proposed, just that
>  pic->hwaccel_data = ff_alloc_vaapi_render_state();
>  r = s->avctx->get_buffer(s->avctx, (AVFrame*)pic);
becomes either
>  pic->hwaccel_data_priv = ff_alloc_vaapi_render_state_priv();
>  r = s->avctx->get_buffer(s->avctx, (AVFrame*)pic);
or
>  r = s->avctx->get_buffer(s->avctx, (AVFrame*)pic);
>  pic->hwaccel_data->priv = ff_alloc_vaapi_render_state_priv();

And for releasing
>  ff_free_vaapi_render_state(pic->hwaccel_data);
>  pic->hwaccel_data = NULL;
>  s->avctx->release_buffer(s->avctx, (AVFrame*)pic);
(uh, it seems bad/wrong to me that you first free hwaccel_data and then
call release_buffer, normally release/free functions should be mirrored
variants of the allocation) becomes either
>  s->avctx->release_buffer(s->avctx, (AVFrame*)pic);
>  ff_free_vaapi_render_state_priv(pic->hwaccel_data_priv);
>  pic->hwaccel_data_priv = NULL;
or
>  ff_free_vaapi_render_state_priv(pic->hwaccel_data->priv);
>  pic->hwaccel_data->priv = NULL;
>  s->avctx->release_buffer(s->avctx, (AVFrame*)pic);

Or am I missing any obvious issue with that? I would think it would need
only "cosmetic" changes to your code and in MPlayer only either adding a
hwaccel member to mp_image_t or misuse e.g. data[1] for it.

>>>> My question was why does the field _exist_ _at all_ in
>>>> vaapi_render_state, not why does the application allocate it.
>>>> I see that I was wrong though and VDPAU actually "needs" it because it
>>>> does the reference frame tracking directly with the surfaces and not the
>>>> AVFrames, and presumably it is the same with VAAPI. Well, I guess it
>>>> saves a bit of code in the user applications.
>>>
>>> Yes, how could an HW accelerator do reference frames tracking otherwise
>>> since it doesn't know about AVFrames?
>>
>> Well, the application could fill out (that part of) the info structs.
>> No, I don't claim that it's a good idea.
>
> This doesn't make sense. The application does not necessarily have access 
> to other Picture to retrieve the surface id back. Neither does it want to 
> duplicate the algorithms to perform this task.

Huh? It would have that info as AVFrame, it just would have to find the
*Surface for the AVFrame and put that in. The main reason I was thinking
about it is that I consider it quite ugly that you cannot include
vdapu.h and vaapi.h unconditionally because it depends on VDPAU/VAAPI
"internals", which could be avoided like that. It isn't any less ugly
though IMO, so I am not actually proposing it.