[FFmpeg-devel] [PATCH] AVHWAccel infrastructure v2 (take 3)

[Michael Niedermayer]

On Mon, Sep 28, 2009 at 03:19:47PM +0200, Gwenole Beauchesne wrote:
> On Sat, 26 Sep 2009, Gwenole Beauchesne wrote:
>
>> Le 26 sept. 09 ? 09:28, Gwenole Beauchesne a ?crit :
>>
>>> 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

Can these be split into seperate patches?


[...]
> @@ -2526,6 +2527,14 @@ typedef struct AVCodecContext {
>       * - decoding: Set by libavcodec
>       */
>       enum AVChromaLocation chroma_sample_location;
> +
> +    /**
> +     * Hardware accelerator configuration attributes.
> +     * The array is terminated by HWACCEL_ATTR_NONE.
> +     * - encoding: unused
> +     * - decoding: Set by user
> +     */
> +    const uintptr_t *hwaccel_attrs;
>  } AVCodecContext;
>  
>  /**

uintptr_t is an optional type in C
and i really have a bad feeling about using it in public API, i bet this
will break several compilers

and this way of passing information/attributes is very odd and there is
no support in AVOptions to deal with that
so i really think a int someflags would be better


[...]
> +/** Identifies the hardware accelerator */
> +enum HWAccelID {
> +    HWACCEL_ID_NONE,
> +    HWACCEL_ID_NB
> +};

That doesnt look complete, and thus makes a review hard


> +
> +/** Identifies the hardware accelerator entry-point */
> +enum HWAccelEntrypoint {
> +    HWACCEL_ENTRYPOINT_VLD = 1, ///< Variable-length decoding
> +    HWACCEL_ENTRYPOINT_IZZ,     ///< Inverse zig-zag scan decoder
> +    HWACCEL_ENTRYPOINT_IDCT,    ///< Inverse discrete cosine transform
> +    HWACCEL_ENTRYPOINT_MOCO,    ///< Motion compensation
> +    HWACCEL_ENTRYPOINT_NB
> +};

The documentation is too terse


[...]
> @@ -248,9 +249,29 @@ int avcodec_default_get_buffer(AVCodecContext *s, AVFrame *pic){
>          }
>      }
>  
> +    if (s->hwaccel &&
> +        !ff_find_hwaccel_attribute(s, HWACCEL_ATTR_GET_PIXELS, &alloc_pixels))
> +        alloc_pixels = 0; /* SW surfaces are not needed */
> +

We have an existing API to let the user application choose the output
format (get_format())
your patch does this instead without explaining why the existing API is
not used  ...



>      if(buf->base[0]){
>          pic->age= *picture_number - buf->last_pic_num;
>          buf->last_pic_num= *picture_number;
> +    }else if(s->hwaccel && !alloc_pixels){
> +        pic->age= 256*256*256*64;
> +        buf->last_pic_num= -256*256*256*64;
> +        memset(buf->linesize, 0, sizeof(buf->linesize));
> +        memset(buf->base, 0, sizeof(buf->base));
> +        memset(buf->data, 0, sizeof(buf->data));
> +        /*
> +         * Allocate a dummy buffer so that we don't have to worry
> +         * about hwaccel checks afterwards in this get_buffer() /
> +         * release_buffer() machinery.
> +         */
> +        buf->base[0] = av_malloc(16);
> +        buf->data[0] = buf->base[0];
> +        buf->width   = s->width;
> +        buf->height  = s->height;
> +        buf->pix_fmt = s->pix_fmt;

i would say base/data either contain pixels or could contain some struct
representing a surface of the hwaccelerator
why is neither of these the case?


[...]
> +/** Returns a hardware accelerator for the specified codec */
> +static AVHWAccel *find_hwaccel(AVCodecContext *avctx, enum CodecID codec_id)
> +{
> +    AVHWAccel *hwaccel = NULL, *best = NULL;
> +    uintptr_t hwaccel_id, get_pixels;
> +
> +    static const int score[] = {
> +        [HWACCEL_ENTRYPOINT_MOCO] = 1,
> +        [HWACCEL_ENTRYPOINT_IDCT] = 2,
> +        [HWACCEL_ENTRYPOINT_IZZ]  = 3,
> +        [HWACCEL_ENTRYPOINT_VLD]  = 4
> +    };
> +
> +    if (!ff_find_hwaccel_attribute(avctx, HWACCEL_ATTR_FORCE_ID, &hwaccel_id))
> +        hwaccel_id = HWACCEL_ID_NONE;
> +
> +    if (!ff_find_hwaccel_attribute(avctx, HWACCEL_ATTR_GET_PIXELS, &get_pixels))
> +        get_pixels = 0;
> +
> +    while ((hwaccel = av_hwaccel_next(hwaccel))) {
> +        if (hwaccel_id != HWACCEL_ID_NONE && hwaccel->hwaccel_id != hwaccel_id)
> +            continue;
> +        if (get_pixels && !(hwaccel->capabilities & HWACCEL_CAP_GET_PIXELS))
> +            continue;
> +        if (hwaccel->id == codec_id &&
> +            (!best || score[best->entrypoint] < score[hwaccel->entrypoint]))
> +            best = hwaccel;
> +    }
> +    return best;
> +}

the formats given to the user app in get_format() are ordered per preferance
this again bypasses existing API without comment
why?


[..]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091109/cfb47195/attachment.pgp>