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

[Reimar Döffinger]

On Fri, Feb 27, 2009 at 11:55:14AM +0100, Gwenole Beauchesne wrote:
> 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...

Unless (like MPlayer) what libavcodec does is actually _not_ the thing
he wants.
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.
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.

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

You have been counting wrong. Your patch has the mess in
libmpcodecs/vd.c
libmpcodecs/vd_ffmpeg.c
libmpcodecs/vf.h
libmpcodecs/vf_vo.c
libvo/video_out.h
libvo/vo_vaapi.c
That is certainly _not_ a single place.
And vd_ffmpeg is the translation wrapper between MPlayer and FFmpeg, if
FFmpeg design needs the application to know HW-accel specific stuff to
convert between an application-specific frame format and AVFrame then
the code for it _does_ belong in vd_ffmpeg IMO, ugly or not.

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