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

[Reimar Döffinger]

On Fri, Feb 27, 2009 at 11:17:53AM +0100, Gwenole Beauchesne wrote:
> On Fri, 27 Feb 2009, Reimar D?ffinger wrote:
>> I consider it an abomination to have half of the AVFrame data allocated
>> in one place and the other half in another place and somewhen we'd then
>> add some other allocation in yet another place.
>
> So, you'd prefer make alloc_picture() include all the get_buffer() + 
> hwaccel_data allocation? You'd also have to note that free_picture() is 
> called at the end of the bitstream, so alloc_picture()/free_picture() are 
> not symetrically called. That's why I wanted to merge 
> s->avctx->release_buffer() into a single place.

No, get_buffer allocates the memory, why do you want to allocate
somewhere else in addition, that was kind of my question?

> Michael suggested to request the user to chain to a default get_buffer() 
> provided by FFmpeg but I fear they wouldn't do it in practise.

Well, then they'd have to reimplement part of it (they probably will
have to anyway), what is the problem with that? As long as our users
_can_ write clean code everything is ok, it's not our job to force them
to do that...

>> Also about the MPlayer part: IMO this is a giant hack. Since it is a
>> hack anyway, it should not be spread all over MPlayer, just set
>> mpi->priv to something that contains all the information and make
>> vd_ffmpeg analyze the internals and fill in whatever needs to be filled
>> it in AVFrame.
>
> Setting mpi->priv to the HW accel data and let vd_ffmpeg parse and 
> recompose it into the hwaccel_data on a per-accelerator basis is not a very 
> clean approach either.

No, but at least the mess doesn't get sprinkled over the place.
My rule is either make it clean or make sure it is as much in one place
as possible.

>> Btw. why is "surface" in vaapi_render_state, is that pure lazyness/bad
>> design as in XvMC and VDPAU or does FFmpeg actually use it?
>
> Yes. I will check if I can allocate it in ff_alloc_vaapi_render_state() 
> instead. So that user only has to set va_context to something sensible 
> enough.

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.