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

[Gwenole Beauchesne]

Hi,

On Mon, 9 Nov 2009, Michael Niedermayer wrote:

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

I think this was the smallest bits and, as you noticed hereunder, some 
enums/structs looked incomplete because they were part of other patches.

Besides, the patch did not remove the HW accel pixfmts so that to maintain 
some compatibility for now, and because I don't know what was the usual 
procedure for that since this would remove pixfmt in the middle of others, 
thus being an API/ABI change.

> [...]
>> @@ -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

I wanted an uintptr_t so that it was as large as a pointer because a 
pointer may be needed for certain attributes.

> [...]
>> +/** Identifies the hardware accelerator */
>> +enum HWAccelID {
>> +    HWACCEL_ID_NONE,
>> +    HWACCEL_ID_NB
>> +};
>
> That doesnt look complete, and thus makes a review hard

Others were:
HWACCEL_ID_VAAPI,
HWACCEL_ID_VDPAU,

>> +
>> +/** 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

/** Identifies the hardware accelerator entry-point. The entry-point 
defines the point where the hardware accelerator would actually handle the 
decoding operations */
?

> [...]
>> @@ -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  ...

get_format() deals with pixel formats. The aim was to get rid of HW accel 
pixel formats because some other HW accelerator will need the pixel format 
set to e.g. NV12 and we'd still need a way to identify this HW 
accelerator.

Besides, with this iteration of hwaccel, the pixel format for HW will have 
the same meaning as for SW: what pixel format the user actually wants for 
real. e.g. if he wants to get the HW decoded pixels back, pixfmt will 
identify this format, which will be the "standard" meaning, i.e. the same 
as in SW mode.

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

data[3] does contain that struct for the HW accelerator (vaapi_context, 
vdpau_render_state). However, for the default mode of operations, that is 
user wants to use hwaccel for decode+display, we don't have to allocate 
data[0-2] pixels. This saves memory in that case. However, making it NULL 
is possible but would require further checks in 
get_buffer()/release_buffer().

alloc_pixels is set if the user requested so through 
HWACCEL_ATTR_GET_PIXELS. We could keep FFmpeg allocating the pixels but 
doing so for several MBs won't be useful for the common case 
(decode+display), i.e. IMHO this would be a waste.

> [...]
>> +/** 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?

Because get_format() deals with pixel format which we (well, I) don't want 
in order to implement the following:
- Get pixels back from GPU memory when the user requested it => pixfmt 
will be used for the actual format chosen by the user
- Implement another HW accelerator that is a pure decoder: it returns NV12 
pixels.

Thanks,
Gwenole