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

[Gwenole Beauchesne]

On Fri, 27 Feb 2009, Reimar D?ffinger wrote:

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

And how would you do the chaining to an avcodec_default_get_buffer() (or 
whatever it's called)? If the user calls it, you turn out to do exactly 
what you didn't want: "allocate somewhere else in addition" (call back 
into lavc). Whereas with the approach I posted, he doesn't need to care, 
lavc does it already, not need for him to call another function to do the 
exact same thing...

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

Doing extra work in vd_ffmpeg would have the (undesired) effect you 
describe. Since hwaccel_data is VO specific data, having the VO bind the 
thing itself makes sure it's a single place. Moving that to vd_ffmpeg 
makes it two places: retreive the surface id/va_context in vo, then copy 
it to the hwaccel_data depending on the HW accelerator in use? That is 
mess.

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

Yes, how could an HW accelerator do reference frames tracking otherwise 
since it doesn't know about AVFrames?