[FFmpeg-devel] [PATCH] AVHWAccel infrastructure v2

[Ivan Kalvachev]

On 9/26/09, Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> wrote:
> Hi,
>
> This patch updates the AVHWAccel infrastructure to make it possible to:
> - Get rid of the PixelFormat hacks
> - Get pixels back from GPU memory when the user requested it
> - Fallback implicitly to SW decoding
> - Handle multiple hardware accelerators (e.g. different chips in a
> single system)
> - Enable user-applications very easily
>
> As a bonus, it's backwards compatible with the v1 infrastructure.
>
> On a user point of view, everything is now controlled with
> HWAccelAttributes.
> - HW specific context that is initialized by the user (e.g. a VAAPI
> context: HWACCEL_ATTR_VAAPI_DISPLAY)
> - HWACCEL_ATTR_GET_PIXELS: controls whether the user wants decoded
> pixels in Picture.data[0-2] as usual (off by default for performance)
> - HWACCEL_ATTR_FORCE_ID: controls whether the user wants a specific HW
> accelerator
>
> No get_buffer() override. Nothing but AVCodecContext.hwaccel_attrs to
> be filled in. It's an array of uintptr_t that is HWACCEL_ATTR_NONE
> terminated.
>
> As a result, enabling ffplay for VAAPI was a very simple operation. On
> the MPlayer side, the usual VO (x11, xv, gl2) can be kept with very
> minimal changes too.

I don't like it.

I guess your goal is to separate completely the software and hardware decoding,
giving single separate api to work with. I can understand why this is desirable.

I see very few of the benefits you declare actually into the code.
IMHO, it only makes libavcodec bigger for no real gain at all,
adding code that is supposed to reside in the player.
And if I get things right, it also turns a read only structure into
user configurable one.
(v3 addresses one such case, but more remain).

1. You do remove get/release_buffer calls by creating new entries
alloc/free_surface in the AVHWAccel, then special case handling in
alloc/free_frame_buffer and adding big ugly hack into
avcodec_default_get_buffer() to allocate fake frames, so codecs won't
notice the difference.
This is the part I don't get, are these alloc/free from inside the
vaapi codec? I don't see them in the patch. The other option is that
they must be filled by the calling
application.
IMHO this is "clean up" makes bigger mess.
The init/close() entries usefulness is not illustrated in the
patch,either. (v3 removes the close...)

2. HWACCEL_CAP_GET_PIXELS is described as the option that would return the
pixels, but is actually used only on find_hwaccel().
In theory if lavcodecs is supposed to fetch rendered frame then it should
also incorporate direct calling of rendering routines. If these routines are
in the player application, then getting the surface into ram is better
done there.

3. The hwaccel_attribute are added. If I understand it correctly they
are filled by the calling application to control the codec selection/work.
This makes them abstract api into specific api of the general api
(attributes, hwaccel,avctx).  (v3 moves them out of hwaccel into
avctx).
IMHO too much abstraction. Also user would have only one of the api's
functional,
so there is not much sense in selection based on that, and the
application already
have api function that allows it to select that. As for the acceleration levels,
they are supposed to be present is specific order so the fastest are
selected first,
if there is any choice at all.
BTW what is Display doing there at all? (removed in v3).