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

On Sun, 15 Nov 2009, Michael Niedermayer wrote:

...

...

[...]

...

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

If avcodec_decode_video() outputs NV12, get_format() would select NV12 and the user application would not even need to know if that was sw or hw decoded. if it does not output a normal pixel format then the userapp has to support that pixel format that is support the hw decoding counterpart

thus really, a hardware accelerator that returns NV12 needs no changes at all not even any of the previous accelerator API additions

of course a user app should be able to force sw or hw too by some means ...

Yes, this what I wanted to do with the hwaccel attributes. Even a way to force a specific hw accelerator if several ones are installed on the system.

...

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

could it be that this problem arises from using [3] instead of [0] for the struct? (note i dont know if using [0] would not lead to more mess elsewhere ...)

I want to keep data[0-2] as the actual pixel data, when needed, and make data[3] hwaccel specific data. The hack presented in this hunk is only meaningful to save memory. Of course, we could keep all of them allocated but I doubt this is desired. Regards, Gwenole.

On Fri, Nov 13, 2009 at 04:50:11PM +0100, Gwenole Beauchesne wrote:

On Mon, 9 Nov 2009, Michael Niedermayer wrote:

...

[...]

...

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

Do you mean I should use AVOptions directly + add an FF_OPT_TYPE_POINTER? The problem is I don't see any codec using av_get_*() so I don't think that was what you meant or is there another API to access AVOptions?

i mean + int my_flags; } AVCodecContext; [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is not what we do, but why we do it that matters.

On Mon, Jan 04, 2010 at 02:09:51PM +0100, Gwenole Beauchesne wrote:

Hi,

On Sun, 15 Nov 2009, Michael Niedermayer wrote:

...

On Fri, Nov 13, 2009 at 04:50:11PM +0100, Gwenole Beauchesne wrote:

...

On Mon, 9 Nov 2009, Michael Niedermayer wrote:

...

[...]

...

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

Do you mean I should use AVOptions directly + add an FF_OPT_TYPE_POINTER? The problem is I don't see any codec using av_get_*() so I don't think that was what you meant or is there another API to access AVOptions?

i mean

+ int my_flags; } AVCodecContext;

In the new model I wanted to use, hwaccel_context was to be solely maintained by libavcodec. However, it still needed a way to get some hwaccel context initialized by the user-application. e.g. a VADisplay (void *), an LPDIRECTXVIDEODECODER, etc.

I felt interesting to pass this information through hwaccel attrs along with other int attributes.

We have existing means to pass information, that is through AVCodecContext and using AVOption where appropriate As well as using AVFrame instead of AVCodecContext where its per frame data I really dont want every developer to invent a new way to pass his data It would be an unimagineable complex mess to work with [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time.

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

...

...

In the new model I wanted to use, hwaccel_context was to be solely maintained by libavcodec. However, it still needed a way to get some hwaccel context initialized by the user-application. e.g. a VADisplay (void *), an LPDIRECTXVIDEODECODER, etc.

I felt interesting to pass this information through hwaccel attrs along with other int attributes.

We have existing means to pass information, that is through AVCodecContext and using AVOption where appropriate As well as using AVFrame instead of AVCodecContext where its per frame data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably [...]

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

/**

diff --git a/libavcodec/hwaccel.h b/libavcodec/hwaccel.h new file mode 100644 index 0000000..d40186d --- /dev/null +++ b/libavcodec/hwaccel.h @@ -0,0 +1,48 @@ +/* + * Hardware Acceleration API + * + * Copyright (C) 2008-2009 Splitted-Desktop Systems + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVCODEC_HWACCEL_H +#define AVCODEC_HWACCEL_H + +/** Identifies the hardware accelerator */ +enum HWAccelID { + HWACCEL_ID_NONE, + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API + HWACCEL_ID_NB +};

this needs a any or auto for autodetection (which should be 0) also HWACCEL_ID_NB needs a comment that it is not part of the public API/ABI or we can not add another hwaccel except that iam ok with this & adding hwaccel_id to AVCodecContext if people from the player side have no objections

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

as said previously this is ambigous HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC to software i think it would be better to completely list the steps that are accelerated, maybe even as flags like: HWACCEL_BITSTREAM 1 HWACCEL_IDCT 2 HWACCEL_MC 4 HWACCEL_LOOPFILTR 8

+ +/** + * The hardware accelerator can fetch the pixels from the decoded + * frames. If the user requested it, avcodec_default_get_buffer() + * will then allocate the data buffers. + */ +#define HWACCEL_CAP_GET_PIXELS 0x00000001

might need a AV prefix [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope

Hi, Ping. v7 also attached to replace {alloc,free}_surface() hooks with the generic {get,release}_buffer(). If the user implements his own get_buffer() function, he will have to call the hwaccel's one too to initialize data[3] properly. Regards, Gwenole. On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

Hi,

It seems I didn't reply to this one or the message got lost because I had a v5 lying around for some time. Here is a v6 anyway, that intersects with the tidsp needs.

On Mon, 22 Feb 2010, Michael Niedermayer wrote:

...

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

...

...

In the new model I wanted to use, hwaccel_context was to be solely maintained by libavcodec. However, it still needed a way to get some hwaccel context initialized by the user-application. e.g. a VADisplay (void *), an LPDIRECTXVIDEODECODER, etc.

I felt interesting to pass this information through hwaccel attrs along with other int attributes.

We have existing means to pass information, that is through AVCodecContext and using AVOption where appropriate As well as using AVFrame instead of AVCodecContext where its per frame data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

...

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably

[...]

...

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

...

...

+/** Identifies the hardware accelerator */ +enum HWAccelID { + HWACCEL_ID_NONE, + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API + HWACCEL_ID_NB +};

this needs a any or auto for autodetection (which should be 0)

I finally forgot about autodetection because it's too complex to handle at the FFmpeg level and delegated that to the player level. In particular, HWAccel context may depend on upper level info, like an X display...

...

...

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

as said previously this is ambigous

HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC to software

Actually, I didn't find the exact meaning. But from a pipeline point of view, this would mean XXX and above. i.e. VLD and above, motion compensation and above. Flagging those would lead to more player work (well, just OR'ing more flags).

I tried to express that in the comment.

...

...

+/** + * The hardware accelerator can fetch the pixels from the decoded + * frames. If the user requested it, avcodec_default_get_buffer() + * will then allocate the data buffers. + */ +#define HWACCEL_CAP_GET_PIXELS 0x00000001

might need a AV prefix

OK.

Regards, Gwenole.

Hi, Here is a new patch that applies to current SVN tree with some further cosmetics (comments) added. Regards, Gwenole. On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

It seems I didn't reply to this one or the message got lost because I had a v5 lying around for some time. Here is a v6 anyway, that intersects with the tidsp needs.

On Mon, 22 Feb 2010, Michael Niedermayer wrote:

...

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

...

...

In the new model I wanted to use, hwaccel_context was to be solely maintained by libavcodec. However, it still needed a way to get some hwaccel context initialized by the user-application. e.g. a VADisplay (void *), an LPDIRECTXVIDEODECODER, etc.

I felt interesting to pass this information through hwaccel attrs along with other int attributes.

We have existing means to pass information, that is through AVCodecContext and using AVOption where appropriate As well as using AVFrame instead of AVCodecContext where its per frame data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

...

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably

[...]

...

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

Those functions are also used to help user implement AVCodecContext.get_buffer() and release_buffer() if he wants to do so manually. e.g. by default, AVCodecContext.get_buffer() will point to avcodec_default_get_buffer(). If a user overrides that function, he will lose automatic data[3] handling. So, two possible cases: 1) He really knows how to do so, so he allocates the HW surface himself 2) He can delegate that to FFmpeg that will know how to do that just fine too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), etc.

...

...

+/** Identifies the hardware accelerator */ +enum HWAccelID { + HWACCEL_ID_NONE, + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API + HWACCEL_ID_NB +};

this needs a any or auto for autodetection (which should be 0)

I finally forgot about autodetection because it's too complex to handle at the FFmpeg level and delegated that to the player level. In particular, HWAccel context may depend on upper level info, like an X display...

...

...

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

as said previously this is ambigous

HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC to software

Actually, I didn't find the exact meaning. But from a pipeline point of view, this would mean XXX and above. i.e. VLD and above, motion compensation and above. Flagging those would lead to more player work (well, just OR'ing more flags).

I tried to express that in the comment.

...

...

+/** + * The hardware accelerator can fetch the pixels from the decoded + * frames. If the user requested it, avcodec_default_get_buffer() + * will then allocate the data buffers. + */ +#define HWACCEL_CAP_GET_PIXELS 0x00000001

might need a AV prefix

OK.

Regards, Gwenole.

Hi, Le 29 oct. 10 ? 23:45, Michael Niedermayer a ?crit :

On Fri, Oct 29, 2010 at 02:52:41PM +0200, Gwenole Beauchesne wrote:

...

Hi,

Here is a new patch that applies to current SVN tree with some further cosmetics (comments) added.

Regards, Gwenole.

On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

...

It seems I didn't reply to this one or the message got lost because I had a v5 lying around for some time. Here is a v6 anyway, that intersects with the tidsp needs.

On Mon, 22 Feb 2010, Michael Niedermayer wrote:

...

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

...

> In the new model I wanted to use, hwaccel_context was to be > solely > maintained by libavcodec. However, it still needed a way to > get some > hwaccel context initialized by the user-application. e.g. a > VADisplay > (void > *), an LPDIRECTXVIDEODECODER, etc. > > I felt interesting to pass this information through hwaccel > attrs along > with other int attributes.

We have existing means to pass information, that is through AVCodecContext and using AVOption where appropriate As well as using AVFrame instead of AVCodecContext where its per frame data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

...

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display- dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably

[...]

...

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

Those functions are also used to help user implement AVCodecContext.get_buffer() and release_buffer() if he wants to do so manually.

e.g. by default, AVCodecContext.get_buffer() will point to avcodec_default_get_buffer(). If a user overrides that function, he will lose automatic data[3] handling. So, two possible cases: 1) He really knows how to do so, so he allocates the HW surface himself 2) He can delegate that to FFmpeg that will know how to do that just fine too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), etc.

the user can call the default get/release buffer. And if the user doesnt know how to allocate the surface what is he doing with the functions thats their purpose. Your explanations are so incomplete and terse that i cannot really comment. Because i do not know what problem this solves or where this problem would arrise

I want the default avcodec_default_get_buffer() allocate HW surfaces (Picture.data[3]) itself the same way it would allocate memory for SW decoding (Picture.data[0-2]). Since the way a HW surface is allocated is HW-dependent, this needs per-accelerator hooks. There are two cases to consider: (a) The user requested the decoded pixels back to Picture.data[0-2] (AV_HWACCEL_CAP_GET_PIXELS), thus we need that along with the Picture.data[3]. (b) The user wants to retain the decoded pixels in GPU memory and avoid a round-trip GPU -> CPU -> GPU (for presentation). Thus, data[0-2] doesn't need to be allocated but Picture.data[3] must still be allocated, the HW surface. In the V6 revision of the patch, I proposed a way to account for both. i.e. if HW accelerator is enabled, only allocate the Picture.data[0-2] if the user requested for it through the AV_HWACCEL_CAP_GET_PIXELS flag. Otherwise, don't allocate anything in there. This can save up to 50 MB of memory. e.g. 16x 1080p reference frames. Now, imagine the user wants to use his own version of get_buffer() to e.g. manage reordered_opaque or whatever other field. In his get_buffer() for SW decoding, he would initially have: /* SW */ pic->type = FF_BUFFER_TYPE_USER; pic->reordered_opaque = avctx->reordered_opaque; // allocate pic->data[0-2] planes /* HW: case (a) */ pic->type = FF_BUFFER_TYPE_USER; pic->reordered_opaque = avctx->reordered_opaque; // allocate pic->data[0-2] planes with his own memory if (avctx->hwaccel && avctx->hwaccel->alloc_surface) pic->data[3] = avctx->hwaccel->alloc_surface(avctx); /* HW: case (b) */ pic->type = FF_BUFFER_TYPE_USER; pic->reordered_opaque = avctx->reordered_opaque; if (avctx->hwaccel) { pic->data[0..2] = NULL; pic->data[3] = avctx->hwaccel->alloc_surface(avctx); } else { // allocate pic->data[0-2] planes with his own memory } Now, if we don't have the {alloc/free}_surface() functions, we would need to: /* HW: case (b) */ pic->type = FF_BUFFER_TYPE_USER; pic->reordered_opaque = avctx->reordered_opaque; if (avctx->hwaccel) { switch (avctx->hwaccel_id) { case HWACCEL_ID_API_1: pic->data[3] = allocate_hw_surface_for_API_1(avctx); break; case HWACCEL_ID_API_2: pic->data[3] = allocate_hw_surface_for_API_2(avctx); break; // ... } } else { // allocate pic->data[0-2] planes with his own memory } i.e. mass code duplication accross players with plenty of cases to chose (and implement) for the favorite API the end-user might want. As Reimar pointed out, the end effect of hwaccel should be that only as few as 10 lines are needed in the user applications to actually use hwaccel. Regards, Gwenole.

On Fri, Oct 29, 2010 at 02:52:41PM +0200, Gwenole Beauchesne wrote:

Hi,

Here is a new patch that applies to current SVN tree with some further cosmetics (comments) added.

Regards, Gwenole.

On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

...

It seems I didn't reply to this one or the message got lost because I had a v5 lying around for some time. Here is a v6 anyway, that intersects with the tidsp needs.

On Mon, 22 Feb 2010, Michael Niedermayer wrote:

...

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

...

...

In the new model I wanted to use, hwaccel_context was to be solely maintained by libavcodec. However, it still needed a way to get some hwaccel context initialized by the user-application. e.g. a VADisplay (void *), an LPDIRECTXVIDEODECODER, etc.

I felt interesting to pass this information through hwaccel attrs along with other int attributes.

We have existing means to pass information, that is through AVCodecContext and using AVOption where appropriate As well as using AVFrame instead of AVCodecContext where its per frame data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

...

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably

[...]

...

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

Those functions are also used to help user implement AVCodecContext.get_buffer() and release_buffer() if he wants to do so manually.

e.g. by default, AVCodecContext.get_buffer() will point to avcodec_default_get_buffer(). If a user overrides that function, he will lose automatic data[3] handling. So, two possible cases: 1) He really knows how to do so, so he allocates the HW surface himself 2) He can delegate that to FFmpeg that will know how to do that just fine too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), etc.

...

...

...

+/** Identifies the hardware accelerator */ +enum HWAccelID { + HWACCEL_ID_NONE, + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API + HWACCEL_ID_NB +};

this needs a any or auto for autodetection (which should be 0)

I finally forgot about autodetection because it's too complex to handle at the FFmpeg level and delegated that to the player level. In particular, HWAccel context may depend on upper level info, like an X display...

...

...

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

as said previously this is ambigous

HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC to software

Actually, I didn't find the exact meaning. But from a pipeline point of view, this would mean XXX and above. i.e. VLD and above, motion compensation and above. Flagging those would lead to more player work (well, just OR'ing more flags).

I tried to express that in the comment.

...

...

+/** + * The hardware accelerator can fetch the pixels from the decoded + * frames. If the user requested it, avcodec_default_get_buffer() + * will then allocate the data buffers. + */ +#define HWACCEL_CAP_GET_PIXELS 0x00000001

might need a AV prefix

OK.

Regards, Gwenole.

Makefile | 2 - avcodec.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- hwaccel.h | 55 +++++++++++++++++++++++++++++++++++++ internal.h | 14 +++++++++ mpegvideo.c | 4 ++ options.c | 2 + utils.c | 52 +++++++++++++++++++++++++++++++++++ 7 files changed, 213 insertions(+), 3 deletions(-) e890e0f1046043c9a28d3a64d9380146b0316139 ffmpeg.hwaccel.v2.8.patch commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43 Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> Date: Fri Oct 29 06:16:55 2010 +0200

Update AVHWAccel infrastructure to v2.

diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 385ae02..1b64768 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak NAME = avcodec FFLIBS = avutil avcore

-HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h

OBJS = allcodecs.o \ audioconvert.o \ diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 705259e..137f485 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -29,6 +29,7 @@ #include <errno.h> #include "libavutil/avutil.h" #include "libavutil/cpu.h" +#include "libavcodec/hwaccel.h"

#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 93 @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

@@ -2753,6 +2754,27 @@ typedef struct AVCodecContext { * - decoding: unused */ int slices; + + /** + * User-requested hardware accelerator. See HWACCEL_ID_xxx. + * - encoding: unused + * - decoding: Set by user + */ + enum HWAccelID hwaccel_id;

could you remind me why this is needed compared to the apparently more flexible system with pixel formats?

+ + /** + * Hardware accelerator configuration flags. See See AV_HWACCEL_CAP_xxx. + * - encoding: unused + * - decoding: Set by user + */ + unsigned int hwaccel_flags;

+ + /** + * Hardware accelerator private context. + * - encoding: unused + * - decoding: Set by libavcodec + */ + void *hwaccel_context_private;

This looks like it can be merged with hwaccel_context [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway

Hi, Ping^3.2. ;-) On Tue, 2 Nov 2010, Gwenole Beauchesne wrote:

Hi,

On Sat, 30 Oct 2010, Michael Niedermayer wrote:

...

...

On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

...

It seems I didn't reply to this one or the message got lost because I had a v5 lying around for some time. Here is a v6 anyway, that intersects with the tidsp needs.

On Mon, 22 Feb 2010, Michael Niedermayer wrote:

...

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

>> In the new model I wanted to use, hwaccel_context was to be solely >> maintained by libavcodec. However, it still needed a way to get some >> hwaccel context initialized by the user-application. e.g. a VADisplay >> (void >> *), an LPDIRECTXVIDEODECODER, etc. >> >> I felt interesting to pass this information through hwaccel attrs >> along >> with other int attributes. > > We have existing means to pass information, that is through > AVCodecContext > and using AVOption where appropriate > As well as using AVFrame instead of AVCodecContext where its per frame > data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

...

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably

[...]

...

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

Those functions are also used to help user implement AVCodecContext.get_buffer() and release_buffer() if he wants to do so manually.

e.g. by default, AVCodecContext.get_buffer() will point to avcodec_default_get_buffer(). If a user overrides that function, he will lose automatic data[3] handling. So, two possible cases: 1) He really knows how to do so, so he allocates the HW surface himself 2) He can delegate that to FFmpeg that will know how to do that just fine too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), etc.

...

...

...

+/** Identifies the hardware accelerator */ +enum HWAccelID { + HWACCEL_ID_NONE, + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API + HWACCEL_ID_NB +};

this needs a any or auto for autodetection (which should be 0)

I finally forgot about autodetection because it's too complex to handle at the FFmpeg level and delegated that to the player level. In particular, HWAccel context may depend on upper level info, like an X display...

...

...

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

as said previously this is ambigous

HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC to software

Actually, I didn't find the exact meaning. But from a pipeline point of view, this would mean XXX and above. i.e. VLD and above, motion compensation and above. Flagging those would lead to more player work (well, just OR'ing more flags).

I tried to express that in the comment.

...

...

+/** + * The hardware accelerator can fetch the pixels from the decoded + * frames. If the user requested it, avcodec_default_get_buffer() + * will then allocate the data buffers. + */ +#define HWACCEL_CAP_GET_PIXELS 0x00000001

might need a AV prefix

OK.

Regards, Gwenole.

...

Makefile | 2 - avcodec.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- hwaccel.h | 55 +++++++++++++++++++++++++++++++++++++ internal.h | 14 +++++++++ mpegvideo.c | 4 ++ options.c | 2 + utils.c | 52 +++++++++++++++++++++++++++++++++++ 7 files changed, 213 insertions(+), 3 deletions(-) e890e0f1046043c9a28d3a64d9380146b0316139 ffmpeg.hwaccel.v2.8.patch commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43 Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> Date: Fri Oct 29 06:16:55 2010 +0200

Update AVHWAccel infrastructure to v2.

diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 385ae02..1b64768 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak NAME = avcodec FFLIBS = avutil avcore

-HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h

OBJS = allcodecs.o \ audioconvert.o \ diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 705259e..137f485 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -29,6 +29,7 @@ #include <errno.h> #include "libavutil/avutil.h" #include "libavutil/cpu.h" +#include "libavcodec/hwaccel.h"

#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 93 @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

...

@@ -2753,6 +2754,27 @@ typedef struct AVCodecContext { * - decoding: unused */ int slices; + + /** + * User-requested hardware accelerator. See HWACCEL_ID_xxx. + * - encoding: unused + * - decoding: Set by user + */ + enum HWAccelID hwaccel_id;

could you remind me why this is needed compared to the apparently more flexible system with pixel formats?

Before hwaccel v1, we had: PIX_FMT_<hwaccel>_<codec>. One <hwaccel> could easily have ~5 codecs. If you have 2 <haccel>, you get 10 pixel formats. Besides, you can have <codec> supported at different entrypoint. e.g. VLD, MoComp, IDCT. Thus, we could have combinatorial explosion.

With hwaccel v1, we factored this with PIX_FMT_<hwaccel>_<entrypoint>, the <codec> information became irrelevant. <entrypoint> was reduced to typically 1 to 3. What does a pixel format with a <codec> name in it actually mean anyway?

This is better, but this was still silly. A pixel format defines... well, a pixel format. If the user wants to extract the decoded pixels from the GPU, he requires so with AV_HWACCEL_CAP_GET_PIXELS. The pixel format of those needs to be expressed the usual way, e.g. avctx->pix_fmt == PIX_FMT_YUV420P. How would this have been expressed if avctx->pix_fmt was PIX_FMT_<hwaccel>_<entrypoint>?

Besides, from an implementation point of view (in libavcodec), the pre-v1 and v1 way of using pixel formats becomes tedious to handle.

We currently have hooks called through:

if (avctx->hwaccel) avctx->hwaccel->decode_slice(...);

this needs to become: if (avctx->hwaccel && <is entrypoint VLD>) avctx->hwaccel->decode_slice(...);

so that other entry-points could be managed easily.

With the v1 way, <is entrypoint == VLD> becomes a big evil switch: return avctx->pix_fmt == PIX_FMT_hwaccel1_VLD ||?avctx->pix_fmt == PIX_FMT_hwaccel2_VLD || avcxtx->pix_fmt == PIX_FMT_hwaccel3_VLD, etc.

Horrible, isn't it?

In the v2 proposal, we just need to do: if (avctx->hwaccel && avctx->hwaccel->entrypoint == HWACCEL_ENTRYPOINT_VLD) avctx->hwaccel->decode_slice(...);

...

...

+ /** + * Hardware accelerator configuration flags. See See AV_HWACCEL_CAP_xxx. + * - encoding: unused + * - decoding: Set by user + */ + unsigned int hwaccel_flags;

...

+ + /** + * Hardware accelerator private context. + * - encoding: unused + * - decoding: Set by libavcodec + */ + void *hwaccel_context_private;

This looks like it can be merged with hwaccel_context

OK, the initial intent was to have private contents to a private struct. I reckon that, should this be needed, a priv_data field in the relevant HW accel *_context would be better instead, akin to AVCodecContext.

So, I have attached a new patch with field removed. This still works for me.

Thanks, Gwenole.

On Tue, Nov 02, 2010 at 11:05:35AM +0100, Gwenole Beauchesne wrote:

Hi,

On Sat, 30 Oct 2010, Michael Niedermayer wrote:

...

...

On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

...

It seems I didn't reply to this one or the message got lost because I had a v5 lying around for some time. Here is a v6 anyway, that intersects with the tidsp needs.

On Mon, 22 Feb 2010, Michael Niedermayer wrote:

...

On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Mon, 4 Jan 2010, Michael Niedermayer wrote:

>> In the new model I wanted to use, hwaccel_context was to be solely >> maintained by libavcodec. However, it still needed a way to get some >> hwaccel context initialized by the user-application. e.g. a VADisplay >> (void >> *), an LPDIRECTXVIDEODECODER, etc. >> >> I felt interesting to pass this information through hwaccel attrs along >> with other int attributes. > > We have existing means to pass information, that is through > AVCodecContext > and using AVOption where appropriate > As well as using AVFrame instead of AVCodecContext where its per frame > data

OK, I nuked the hwaccel attrs and replaced it with AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I left hwaccel_id as is to match existing practise (pix_fmt et al.).

I haven't tested with MPlayer yet but this new approach still works with my modified ffplay.

So, HW accelerator selection works as is: - Set AVCodecContext.hwaccel_id to the desired accelerator - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user wants AVFrame.data[0-2] populated with YV12 or NV12 pixels

This means we can handle multiple accelerators automatically at once. Anyway, this lived in an ideal world without practical use since the user-application would still have to handle the HW accelerator specifically.

Regards, Gwenole.

]...]

...

@@ -2497,7 +2498,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

hunk ok probably

[...]

...

+ /** + * Called in codec initialization. + * + * This is used to initialize the AVCodecContext.hwaccel_context + * hardware accelerator context. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*init)(AVCodecContext *avctx); + + /** + * Called in codec finalization. + * + * This is used to clean-up any data from the hardware accelerator + * context. Should this function be implemented, it shall reset + * AVCodecContext.hwaccel_context to NULL. + * + * @param avctx the codec context + * @return zero if successful, a negative value otherwise + */ + int (*close)(AVCodecContext *avctx); + + /** + * Called at the beginning of each frame to allocate a HW surface. + * + * The returned surface will be stored in Picture.data[3]. + * + * @param avctx the codec context + * @param surface the pointer to the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); + + /** + * Called to free the HW surface that was allocated with alloc_surface(). + * + * @param avctx the codec context + * @param surface the HW accelerator surface + * @return zero if successful, a negative value otherwise + */ + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); } AVHWAccel;

iam not in favor of these. What are these good for? Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

Those functions are also used to help user implement AVCodecContext.get_buffer() and release_buffer() if he wants to do so manually.

e.g. by default, AVCodecContext.get_buffer() will point to avcodec_default_get_buffer(). If a user overrides that function, he will lose automatic data[3] handling. So, two possible cases: 1) He really knows how to do so, so he allocates the HW surface himself 2) He can delegate that to FFmpeg that will know how to do that just fine too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), etc.

...

...

...

+/** Identifies the hardware accelerator */ +enum HWAccelID { + HWACCEL_ID_NONE, + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API + HWACCEL_ID_NB +};

this needs a any or auto for autodetection (which should be 0)

I finally forgot about autodetection because it's too complex to handle at the FFmpeg level and delegated that to the player level. In particular, HWAccel context may depend on upper level info, like an X display...

...

...

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

as said previously this is ambigous

HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC to software

Actually, I didn't find the exact meaning. But from a pipeline point of view, this would mean XXX and above. i.e. VLD and above, motion compensation and above. Flagging those would lead to more player work (well, just OR'ing more flags).

I tried to express that in the comment.

...

...

+/** + * The hardware accelerator can fetch the pixels from the decoded + * frames. If the user requested it, avcodec_default_get_buffer() + * will then allocate the data buffers. + */ +#define HWACCEL_CAP_GET_PIXELS 0x00000001

might need a AV prefix

OK.

Regards, Gwenole.

...

Makefile | 2 - avcodec.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- hwaccel.h | 55 +++++++++++++++++++++++++++++++++++++ internal.h | 14 +++++++++ mpegvideo.c | 4 ++ options.c | 2 + utils.c | 52 +++++++++++++++++++++++++++++++++++ 7 files changed, 213 insertions(+), 3 deletions(-) e890e0f1046043c9a28d3a64d9380146b0316139 ffmpeg.hwaccel.v2.8.patch commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43 Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> Date: Fri Oct 29 06:16:55 2010 +0200

Update AVHWAccel infrastructure to v2.

diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 385ae02..1b64768 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak NAME = avcodec FFLIBS = avutil avcore

-HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h

OBJS = allcodecs.o \ audioconvert.o \ diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 705259e..137f485 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -29,6 +29,7 @@ #include <errno.h> #include "libavutil/avutil.h" #include "libavutil/cpu.h" +#include "libavcodec/hwaccel.h"

#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 93 @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

...

@@ -2753,6 +2754,27 @@ typedef struct AVCodecContext { * - decoding: unused */ int slices; + + /** + * User-requested hardware accelerator. See HWACCEL_ID_xxx. + * - encoding: unused + * - decoding: Set by user + */ + enum HWAccelID hwaccel_id;

could you remind me why this is needed compared to the apparently more flexible system with pixel formats?

Before hwaccel v1, we had: PIX_FMT_<hwaccel>_<codec>. One <hwaccel> could easily have ~5 codecs. If you have 2 <haccel>, you get 10 pixel formats. Besides, you can have <codec> supported at different entrypoint. e.g. VLD, MoComp, IDCT. Thus, we could have combinatorial explosion.

With hwaccel v1, we factored this with PIX_FMT_<hwaccel>_<entrypoint>, the <codec> information became irrelevant. <entrypoint> was reduced to typically 1 to 3. What does a pixel format with a <codec> name in it actually mean anyway?

This is better, but this was still silly. A pixel format defines... well, a pixel format. If the user wants to extract the decoded pixels from the GPU, he requires so with AV_HWACCEL_CAP_GET_PIXELS. The pixel format of those needs to be expressed the usual way, e.g. avctx->pix_fmt == PIX_FMT_YUV420P. How would this have been expressed if avctx->pix_fmt was PIX_FMT_<hwaccel>_<entrypoint>?

"A pixel format defines... well, a pixel format." thats a useless circular definition a pixel format defines the structuring of data in AVFrame.data[] that likely differs between 2 hw accelerator APIs, so should allow selecting hw accelerators

Besides, from an implementation point of view (in libavcodec), the pre-v1 and v1 way of using pixel formats becomes tedious to handle.

We currently have hooks called through:

if (avctx->hwaccel) avctx->hwaccel->decode_slice(...);

this needs to become: if (avctx->hwaccel && <is entrypoint VLD>) avctx->hwaccel->decode_slice(...);

so that other entry-points could be managed easily.

With the v1 way, <is entrypoint == VLD> becomes a big evil switch: return avctx->pix_fmt == PIX_FMT_hwaccel1_VLD ||?avctx->pix_fmt == PIX_FMT_hwaccel2_VLD || avcxtx->pix_fmt == PIX_FMT_hwaccel3_VLD, etc.

Horrible, isn't it?

pix_fmt_descriptor[pix_fmt].flags & FLAG_VLD doesnt look horrible to me [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu

On Wed, Nov 10, 2010 at 11:01:11AM +0100, Gwenole Beauchesne wrote:

Hi,

On Tue, 9 Nov 2010, Michael Niedermayer wrote:

...

On Tue, Nov 02, 2010 at 11:05:35AM +0100, Gwenole Beauchesne wrote:

...

On Sat, 30 Oct 2010, Michael Niedermayer wrote:

...

...

On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:

...

>> So, HW accelerator selection works as is: >> - Set AVCodecContext.hwaccel_id to the desired accelerator >> - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user >> wants AVFrame.data[0-2] populated with YV12 or NV12 pixels >> >> This means we can handle multiple accelerators automatically at once. >> Anyway, this lived in an ideal world without practical use since the >> user-application would still have to handle the HW accelerator >> specifically. > > [...] >> + /** >> + * Called in codec initialization. >> + * >> + * This is used to initialize the AVCodecContext.hwaccel_context >> + * hardware accelerator context. >> + * >> + * @param avctx the codec context >> + * @return zero if successful, a negative value otherwise >> + */ >> + int (*init)(AVCodecContext *avctx); >> + >> + /** >> + * Called in codec finalization. >> + * >> + * This is used to clean-up any data from the hardware accelerator >> + * context. Should this function be implemented, it shall reset >> + * AVCodecContext.hwaccel_context to NULL. >> + * >> + * @param avctx the codec context >> + * @return zero if successful, a negative value otherwise >> + */ >> + int (*close)(AVCodecContext *avctx); >> + >> + /** >> + * Called at the beginning of each frame to allocate a HW surface. >> + * >> + * The returned surface will be stored in Picture.data[3]. >> + * >> + * @param avctx the codec context >> + * @param surface the pointer to the HW accelerator surface >> + * @return zero if successful, a negative value otherwise >> + */ >> + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); >> + >> + /** >> + * Called to free the HW surface that was allocated with >> alloc_surface(). >> + * >> + * @param avctx the codec context >> + * @param surface the HW accelerator surface >> + * @return zero if successful, a negative value otherwise >> + */ >> + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); >> } AVHWAccel; > > iam not in favor of these. What are these good for? > Why are they needed?

This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * instead of the void *.

Those functions are also used to help user implement AVCodecContext.get_buffer() and release_buffer() if he wants to do so manually.

e.g. by default, AVCodecContext.get_buffer() will point to avcodec_default_get_buffer(). If a user overrides that function, he will lose automatic data[3] handling. So, two possible cases: 1) He really knows how to do so, so he allocates the HW surface himself 2) He can delegate that to FFmpeg that will know how to do that just fine too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), etc.

...

>> +/** Identifies the hardware accelerator */ >> +enum HWAccelID { >> + HWACCEL_ID_NONE, >> + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API >> + HWACCEL_ID_NB >> +}; > > this needs a any or auto for autodetection (which should be 0)

I finally forgot about autodetection because it's too complex to handle at the FFmpeg level and delegated that to the player level. In particular, HWAccel context may depend on upper level info, like an X display...

>> + >> +/** Identifies the hardware accelerator entry-point */ >> +enum HWAccelEntrypoint { >> + HWACCEL_ENTRYPOINT_VLD = 1, ///< Variable-length decoding >> + HWACCEL_ENTRYPOINT_IDCT, ///< Inverse discrete cosine transform >> + HWACCEL_ENTRYPOINT_MOCO, ///< Motion compensation >> + HWACCEL_ENTRYPOINT_NB >> +}; > > as said previously this is ambigous > > HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC > to software

Actually, I didn't find the exact meaning. But from a pipeline point of view, this would mean XXX and above. i.e. VLD and above, motion compensation and above. Flagging those would lead to more player work (well, just OR'ing more flags).

I tried to express that in the comment.

...

Makefile | 2 - avcodec.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- hwaccel.h | 55 +++++++++++++++++++++++++++++++++++++ internal.h | 14 +++++++++ mpegvideo.c | 4 ++ options.c | 2 + utils.c | 52 +++++++++++++++++++++++++++++++++++ 7 files changed, 213 insertions(+), 3 deletions(-) e890e0f1046043c9a28d3a64d9380146b0316139 ffmpeg.hwaccel.v2.8.patch commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43 Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> Date: Fri Oct 29 06:16:55 2010 +0200

Update AVHWAccel infrastructure to v2.

diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 385ae02..1b64768 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak NAME = avcodec FFLIBS = avutil avcore

-HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h

OBJS = allcodecs.o \ audioconvert.o \ diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 705259e..137f485 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -29,6 +29,7 @@ #include <errno.h> #include "libavutil/avutil.h" #include "libavutil/cpu.h" +#include "libavcodec/hwaccel.h"

#define LIBAVCODEC_VERSION_MAJOR 52 #define LIBAVCODEC_VERSION_MINOR 93 @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext { * provided by the user. In that case, this holds display-dependent * data FFmpeg cannot instantiate itself. Please refer to the * FFmpeg HW accelerator documentation to know how to fill this - * is. e.g. for VA API, this is a struct vaapi_context. + * is. e.g. for VA API, this is a VADisplay. * - encoding: unused * - decoding: Set by user */

...

@@ -2753,6 +2754,27 @@ typedef struct AVCodecContext { * - decoding: unused */ int slices; + + /** + * User-requested hardware accelerator. See HWACCEL_ID_xxx. + * - encoding: unused + * - decoding: Set by user + */ + enum HWAccelID hwaccel_id;

could you remind me why this is needed compared to the apparently more flexible system with pixel formats?

Before hwaccel v1, we had: PIX_FMT_<hwaccel>_<codec>. One <hwaccel> could easily have ~5 codecs. If you have 2 <haccel>, you get 10 pixel formats. Besides, you can have <codec> supported at different entrypoint. e.g. VLD, MoComp, IDCT. Thus, we could have combinatorial explosion.

With hwaccel v1, we factored this with PIX_FMT_<hwaccel>_<entrypoint>, the <codec> information became irrelevant. <entrypoint> was reduced to typically 1 to 3. What does a pixel format with a <codec> name in it actually mean anyway?

This is better, but this was still silly. A pixel format defines... well, a pixel format. If the user wants to extract the decoded pixels from the GPU, he requires so with AV_HWACCEL_CAP_GET_PIXELS. The pixel format of those needs to be expressed the usual way, e.g. avctx->pix_fmt == PIX_FMT_YUV420P. How would this have been expressed if avctx->pix_fmt was PIX_FMT_<hwaccel>_<entrypoint>?

"A pixel format defines... well, a pixel format." thats a useless circular definition

a pixel format defines the structuring of data in AVFrame.data[] that likely differs between 2 hw accelerator APIs, so should allow selecting hw accelerators

And how do you define and select an HW accelerator that outputs NV12 or I420 frames? Or other HW accelerators that the user requested decoded pixels back to .data[0-2]?

easy, pix_fmt = PIX_FMT_NV12 its fully transparent then the user app does not even need to know a hwaccel is used

Would you introduce another level with the actual pixel format. e.g. PIX_FMT_YUV420P_<hwaccel>_<entrypoint>?

...

...

Besides, from an implementation point of view (in libavcodec), the pre-v1 and v1 way of using pixel formats becomes tedious to handle.

We currently have hooks called through:

if (avctx->hwaccel) avctx->hwaccel->decode_slice(...);

this needs to become: if (avctx->hwaccel && <is entrypoint VLD>) avctx->hwaccel->decode_slice(...);

so that other entry-points could be managed easily.

With the v1 way, <is entrypoint == VLD> becomes a big evil switch: return avctx->pix_fmt == PIX_FMT_hwaccel1_VLD ||?avctx->pix_fmt == PIX_FMT_hwaccel2_VLD || avcxtx->pix_fmt == PIX_FMT_hwaccel3_VLD, etc.

Horrible, isn't it?

pix_fmt_descriptor[pix_fmt].flags & FLAG_VLD doesnt look horrible to me

if (avctx->hwaccel && avctx->hwaccel->entrypoint == HWACCEL_ENTRYPOINT_VLD) looks better, less indirections, and keeps things on a single line. :)

if you put entrypoint in hwaccel you need a seperate AVHWAccel for each entrypoint. and then you could just do if(avctx->hwaccel && avctx->hwaccel->decode_slice) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates

On Wed, Nov 10, 2010 at 04:18:45PM +0100, Gwenole Beauchesne wrote:

Hi,

On Wed, 10 Nov 2010, Michael Niedermayer wrote:

...

On Wed, Nov 10, 2010 at 11:01:11AM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Tue, 9 Nov 2010, Michael Niedermayer wrote:

...

On Tue, Nov 02, 2010 at 11:05:35AM +0100, Gwenole Beauchesne wrote:

...

On Sat, 30 Oct 2010, Michael Niedermayer wrote:

...

> On Mon, 20 Sep 2010, Gwenole Beauchesne wrote: > >>>> So, HW accelerator selection works as is: >>>> - Set AVCodecContext.hwaccel_id to the desired accelerator >>>> - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user >>>> wants AVFrame.data[0-2] populated with YV12 or NV12 pixels >>>> >>>> This means we can handle multiple accelerators automatically at once. >>>> Anyway, this lived in an ideal world without practical use since the >>>> user-application would still have to handle the HW accelerator >>>> specifically. >>> >>> [...] >>>> + /** >>>> + * Called in codec initialization. >>>> + * >>>> + * This is used to initialize the AVCodecContext.hwaccel_context >>>> + * hardware accelerator context. >>>> + * >>>> + * @param avctx the codec context >>>> + * @return zero if successful, a negative value otherwise >>>> + */ >>>> + int (*init)(AVCodecContext *avctx); >>>> + >>>> + /** >>>> + * Called in codec finalization. >>>> + * >>>> + * This is used to clean-up any data from the hardware accelerator >>>> + * context. Should this function be implemented, it shall reset >>>> + * AVCodecContext.hwaccel_context to NULL. >>>> + * >>>> + * @param avctx the codec context >>>> + * @return zero if successful, a negative value otherwise >>>> + */ >>>> + int (*close)(AVCodecContext *avctx); >>>> + >>>> + /** >>>> + * Called at the beginning of each frame to allocate a HW surface. >>>> + * >>>> + * The returned surface will be stored in Picture.data[3]. >>>> + * >>>> + * @param avctx the codec context >>>> + * @param surface the pointer to the HW accelerator surface >>>> + * @return zero if successful, a negative value otherwise >>>> + */ >>>> + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); >>>> + >>>> + /** >>>> + * Called to free the HW surface that was allocated with >>>> alloc_surface(). >>>> + * >>>> + * @param avctx the codec context >>>> + * @param surface the HW accelerator surface >>>> + * @return zero if successful, a negative value otherwise >>>> + */ >>>> + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); >>>> } AVHWAccel; >>> >>> iam not in favor of these. What are these good for? >>> Why are they needed? >> >> This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) >> from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * >> instead of the void *. > > Those functions are also used to help user implement > AVCodecContext.get_buffer() and release_buffer() if he wants to do so > manually. > > e.g. by default, AVCodecContext.get_buffer() will point to > avcodec_default_get_buffer(). If a user overrides that function, he will > lose automatic data[3] handling. So, two possible cases: > 1) He really knows how to do so, so he allocates the HW surface himself > 2) He can delegate that to FFmpeg that will know how to do that just fine > too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), > etc. > >>>> +/** Identifies the hardware accelerator */ >>>> +enum HWAccelID { >>>> + HWACCEL_ID_NONE, >>>> + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API >>>> + HWACCEL_ID_NB >>>> +}; >>> >>> this needs a any or auto for autodetection (which should be 0) >> >> I finally forgot about autodetection because it's too complex to handle >> at the FFmpeg level and delegated that to the player level. In >> particular, HWAccel context may depend on upper level info, like an X >> display... >> >>>> + >>>> +/** Identifies the hardware accelerator entry-point */ >>>> +enum HWAccelEntrypoint { >>>> + HWACCEL_ENTRYPOINT_VLD = 1, ///< Variable-length decoding >>>> + HWACCEL_ENTRYPOINT_IDCT, ///< Inverse discrete cosine transform >>>> + HWACCEL_ENTRYPOINT_MOCO, ///< Motion compensation >>>> + HWACCEL_ENTRYPOINT_NB >>>> +}; >>> >>> as said previously this is ambigous >>> >>> HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC >>> to software >> >> Actually, I didn't find the exact meaning. But from a pipeline point of >> view, this would mean XXX and above. i.e. VLD and above, motion >> compensation and above. Flagging those would lead to more player work >> (well, just OR'ing more flags). >> >> I tried to express that in the comment.

> Makefile | 2 - > avcodec.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > hwaccel.h | 55 +++++++++++++++++++++++++++++++++++++ > internal.h | 14 +++++++++ > mpegvideo.c | 4 ++ > options.c | 2 + > utils.c | 52 +++++++++++++++++++++++++++++++++++ > 7 files changed, 213 insertions(+), 3 deletions(-) > e890e0f1046043c9a28d3a64d9380146b0316139 ffmpeg.hwaccel.v2.8.patch > commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43 > Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> > Date: Fri Oct 29 06:16:55 2010 +0200 > > Update AVHWAccel infrastructure to v2. > > diff --git a/libavcodec/Makefile b/libavcodec/Makefile > index 385ae02..1b64768 100644 > --- a/libavcodec/Makefile > +++ b/libavcodec/Makefile > @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak > NAME = avcodec > FFLIBS = avutil avcore > > -HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h > +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h > > OBJS = allcodecs.o \ > audioconvert.o \ > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index 705259e..137f485 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -29,6 +29,7 @@ > #include <errno.h> > #include "libavutil/avutil.h" > #include "libavutil/cpu.h" > +#include "libavcodec/hwaccel.h" > > #define LIBAVCODEC_VERSION_MAJOR 52 > #define LIBAVCODEC_VERSION_MINOR 93 > @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext { > * provided by the user. In that case, this holds display-dependent > * data FFmpeg cannot instantiate itself. Please refer to the > * FFmpeg HW accelerator documentation to know how to fill this > - * is. e.g. for VA API, this is a struct vaapi_context. > + * is. e.g. for VA API, this is a VADisplay. > * - encoding: unused > * - decoding: Set by user > */

> @@ -2753,6 +2754,27 @@ typedef struct AVCodecContext { > * - decoding: unused > */ > int slices; > + > + /** > + * User-requested hardware accelerator. See HWACCEL_ID_xxx. > + * - encoding: unused > + * - decoding: Set by user > + */ > + enum HWAccelID hwaccel_id;

could you remind me why this is needed compared to the apparently more flexible system with pixel formats?

Before hwaccel v1, we had: PIX_FMT_<hwaccel>_<codec>. One <hwaccel> could easily have ~5 codecs. If you have 2 <haccel>, you get 10 pixel formats. Besides, you can have <codec> supported at different entrypoint. e.g. VLD, MoComp, IDCT. Thus, we could have combinatorial explosion.

With hwaccel v1, we factored this with PIX_FMT_<hwaccel>_<entrypoint>, the <codec> information became irrelevant. <entrypoint> was reduced to typically 1 to 3. What does a pixel format with a <codec> name in it actually mean anyway?

This is better, but this was still silly. A pixel format defines... well, a pixel format. If the user wants to extract the decoded pixels from the GPU, he requires so with AV_HWACCEL_CAP_GET_PIXELS. The pixel format of those needs to be expressed the usual way, e.g. avctx->pix_fmt == PIX_FMT_YUV420P. How would this have been expressed if avctx->pix_fmt was PIX_FMT_<hwaccel>_<entrypoint>?

"A pixel format defines... well, a pixel format." thats a useless circular definition

a pixel format defines the structuring of data in AVFrame.data[] that likely differs between 2 hw accelerator APIs, so should allow selecting hw accelerators

And how do you define and select an HW accelerator that outputs NV12 or I420 frames? Or other HW accelerators that the user requested decoded pixels back to .data[0-2]?

easy, pix_fmt = PIX_FMT_NV12 its fully transparent then the user app does not even need to know a hwaccel is used

Actually, this is not that easy. The hwaccel can require the user-initialized, in a system-dependent way, hwaccel_context.

why? this is duplicated code in each application using lavc, isnt it?

The user may also want to explicitly require a specific hwaccel to use. How do you express those?

like MMX/SSE/Altivec

You have several cases to consider:

(Kind of hwaccel) K1: the hwaccel decodes to opaque HW surfaces. i.e. the user can't access to the decoded pixels directly but through some explicit readback function. K2: the hwaccel decodes to SW provided memory. i.e. this memory can be exposed directly to Picture.data[0-2].

(Where the user wants the decoded pixels) L1: closest to the GPU L2: accessible through the CPU only (AV_HWACCEL_CAP_GET_PIXELS)

(What hwaccel the user specifically wants) H1 .. Hn.

In v1, we currently have the following code flow: lavc: avctx->pix_fmt = get_format() user: <selects hwaccel and initializes hwaccel_context> lavc: avctx->hwaccel = ff_find_hwaccel(avctx->codec->id, avctx->pix_fmt);

This scenario doesn't work for K2 hwaccels because an hwaccel_context needs to be provided and/or user wants to choose H1..Hn specifically through get_format().

...

...

Would you introduce another level with the actual pixel format. e.g. PIX_FMT_YUV420P_<hwaccel>_<entrypoint>?

...

...

Besides, from an implementation point of view (in libavcodec), the pre-v1 and v1 way of using pixel formats becomes tedious to handle.

We currently have hooks called through:

if (avctx->hwaccel) avctx->hwaccel->decode_slice(...);

this needs to become: if (avctx->hwaccel && <is entrypoint VLD>) avctx->hwaccel->decode_slice(...);

so that other entry-points could be managed easily.

With the v1 way, <is entrypoint == VLD> becomes a big evil switch: return avctx->pix_fmt == PIX_FMT_hwaccel1_VLD ||?avctx->pix_fmt == PIX_FMT_hwaccel2_VLD || avcxtx->pix_fmt == PIX_FMT_hwaccel3_VLD, etc.

Horrible, isn't it?

pix_fmt_descriptor[pix_fmt].flags & FLAG_VLD doesnt look horrible to me

if (avctx->hwaccel && avctx->hwaccel->entrypoint == HWACCEL_ENTRYPOINT_VLD) looks better, less indirections, and keeps things on a single line. :)

if you put entrypoint in hwaccel you need a seperate AVHWAccel for each entrypoint. and then you could just do if(avctx->hwaccel && avctx->hwaccel->decode_slice)

I'd prefer lavc to ensure the implementor did not do anything insane. If we make all hooks optional without proper check or notification of error (segfault or assert()), the implementor could miss some functions to add.

e.g. VLD requires decode_slice(), IDCT & MoComp require decode_mb(). I would like to ensure that in VLD mode, only decode_slice() is implemented, and in IDCT & MoComp mode, only decode_mb() is implemented.

Hmmm, we could check this in the ff_init_hwaccel() though.

agree and yes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Good people do not need laws to tell them to act responsibly, while bad people will find a way around the laws. -- Plato

On Tue, Nov 23, 2010 at 10:27:32PM +0100, Gwenole Beauchesne wrote:

Hi,

On Wed, 10 Nov 2010, Michael Niedermayer wrote:

...

On Wed, Nov 10, 2010 at 04:18:45PM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Wed, 10 Nov 2010, Michael Niedermayer wrote:

...

On Wed, Nov 10, 2010 at 11:01:11AM +0100, Gwenole Beauchesne wrote:

...

Hi,

On Tue, 9 Nov 2010, Michael Niedermayer wrote:

...

On Tue, Nov 02, 2010 at 11:05:35AM +0100, Gwenole Beauchesne wrote: > > On Sat, 30 Oct 2010, Michael Niedermayer wrote: > >>> On Mon, 20 Sep 2010, Gwenole Beauchesne wrote: >>> >>>>>> So, HW accelerator selection works as is: >>>>>> - Set AVCodecContext.hwaccel_id to the desired accelerator >>>>>> - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user >>>>>> wants AVFrame.data[0-2] populated with YV12 or NV12 pixels >>>>>> >>>>>> This means we can handle multiple accelerators automatically at once. >>>>>> Anyway, this lived in an ideal world without practical use since the >>>>>> user-application would still have to handle the HW accelerator >>>>>> specifically. >>>>> >>>>> [...] >>>>>> + /** >>>>>> + * Called in codec initialization. >>>>>> + * >>>>>> + * This is used to initialize the AVCodecContext.hwaccel_context >>>>>> + * hardware accelerator context. >>>>>> + * >>>>>> + * @param avctx the codec context >>>>>> + * @return zero if successful, a negative value otherwise >>>>>> + */ >>>>>> + int (*init)(AVCodecContext *avctx); >>>>>> + >>>>>> + /** >>>>>> + * Called in codec finalization. >>>>>> + * >>>>>> + * This is used to clean-up any data from the hardware accelerator >>>>>> + * context. Should this function be implemented, it shall reset >>>>>> + * AVCodecContext.hwaccel_context to NULL. >>>>>> + * >>>>>> + * @param avctx the codec context >>>>>> + * @return zero if successful, a negative value otherwise >>>>>> + */ >>>>>> + int (*close)(AVCodecContext *avctx); >>>>>> + >>>>>> + /** >>>>>> + * Called at the beginning of each frame to allocate a HW surface. >>>>>> + * >>>>>> + * The returned surface will be stored in Picture.data[3]. >>>>>> + * >>>>>> + * @param avctx the codec context >>>>>> + * @param surface the pointer to the HW accelerator surface >>>>>> + * @return zero if successful, a negative value otherwise >>>>>> + */ >>>>>> + int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface); >>>>>> + >>>>>> + /** >>>>>> + * Called to free the HW surface that was allocated with >>>>>> alloc_surface(). >>>>>> + * >>>>>> + * @param avctx the codec context >>>>>> + * @param surface the HW accelerator surface >>>>>> + * @return zero if successful, a negative value otherwise >>>>>> + */ >>>>>> + void (*free_surface)(AVCodecContext *avctx, uint8_t *surface); >>>>>> } AVHWAccel; >>>>> >>>>> iam not in favor of these. What are these good for? >>>>> Why are they needed? >>>> >>>> This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.) >>>> from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t * >>>> instead of the void *. >>> >>> Those functions are also used to help user implement >>> AVCodecContext.get_buffer() and release_buffer() if he wants to do so >>> manually. >>> >>> e.g. by default, AVCodecContext.get_buffer() will point to >>> avcodec_default_get_buffer(). If a user overrides that function, he will >>> lose automatic data[3] handling. So, two possible cases: >>> 1) He really knows how to do so, so he allocates the HW surface himself >>> 2) He can delegate that to FFmpeg that will know how to do that just fine >>> too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(), >>> etc. >>> >>>>>> +/** Identifies the hardware accelerator */ >>>>>> +enum HWAccelID { >>>>>> + HWACCEL_ID_NONE, >>>>>> + HWACCEL_ID_VAAPI, ///< Video Acceleration (VA) API >>>>>> + HWACCEL_ID_NB >>>>>> +}; >>>>> >>>>> this needs a any or auto for autodetection (which should be 0) >>>> >>>> I finally forgot about autodetection because it's too complex to handle >>>> at the FFmpeg level and delegated that to the player level. In >>>> particular, HWAccel context may depend on upper level info, like an X >>>> display... >>>> >>>>>> + >>>>>> +/** Identifies the hardware accelerator entry-point */ >>>>>> +enum HWAccelEntrypoint { >>>>>> + HWACCEL_ENTRYPOINT_VLD = 1, ///< Variable-length decoding >>>>>> + HWACCEL_ENTRYPOINT_IDCT, ///< Inverse discrete cosine transform >>>>>> + HWACCEL_ENTRYPOINT_MOCO, ///< Motion compensation >>>>>> + HWACCEL_ENTRYPOINT_NB >>>>>> +}; >>>>> >>>>> as said previously this is ambigous >>>>> >>>>> HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC >>>>> to software >>>> >>>> Actually, I didn't find the exact meaning. But from a pipeline point of >>>> view, this would mean XXX and above. i.e. VLD and above, motion >>>> compensation and above. Flagging those would lead to more player work >>>> (well, just OR'ing more flags). >>>> >>>> I tried to express that in the comment. >> >>> Makefile | 2 - >>> avcodec.h | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> hwaccel.h | 55 +++++++++++++++++++++++++++++++++++++ >>> internal.h | 14 +++++++++ >>> mpegvideo.c | 4 ++ >>> options.c | 2 + >>> utils.c | 52 +++++++++++++++++++++++++++++++++++ >>> 7 files changed, 213 insertions(+), 3 deletions(-) >>> e890e0f1046043c9a28d3a64d9380146b0316139 ffmpeg.hwaccel.v2.8.patch >>> commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43 >>> Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com> >>> Date: Fri Oct 29 06:16:55 2010 +0200 >>> >>> Update AVHWAccel infrastructure to v2. >>> >>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile >>> index 385ae02..1b64768 100644 >>> --- a/libavcodec/Makefile >>> +++ b/libavcodec/Makefile >>> @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak >>> NAME = avcodec >>> FFLIBS = avutil avcore >>> >>> -HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h >>> +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h >>> >>> OBJS = allcodecs.o \ >>> audioconvert.o \ >>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >>> index 705259e..137f485 100644 >>> --- a/libavcodec/avcodec.h >>> +++ b/libavcodec/avcodec.h >>> @@ -29,6 +29,7 @@ >>> #include <errno.h> >>> #include "libavutil/avutil.h" >>> #include "libavutil/cpu.h" >>> +#include "libavcodec/hwaccel.h" >>> >>> #define LIBAVCODEC_VERSION_MAJOR 52 >>> #define LIBAVCODEC_VERSION_MINOR 93 >>> @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext { >>> * provided by the user. In that case, this holds display-dependent >>> * data FFmpeg cannot instantiate itself. Please refer to the >>> * FFmpeg HW accelerator documentation to know how to fill this >>> - * is. e.g. for VA API, this is a struct vaapi_context. >>> + * is. e.g. for VA API, this is a VADisplay. >>> * - encoding: unused >>> * - decoding: Set by user >>> */ >> >>> @@ -2753,6 +2754,27 @@ typedef struct AVCodecContext { >>> * - decoding: unused >>> */ >>> int slices; >>> + >>> + /** >>> + * User-requested hardware accelerator. See HWACCEL_ID_xxx. >>> + * - encoding: unused >>> + * - decoding: Set by user >>> + */ >>> + enum HWAccelID hwaccel_id; >> >> could you remind me why this is needed compared to the apparently more flexible >> system with pixel formats? > > Before hwaccel v1, we had: PIX_FMT_<hwaccel>_<codec>. One <hwaccel> could > easily have ~5 codecs. If you have 2 <haccel>, you get 10 pixel formats. > Besides, you can have <codec> supported at different entrypoint. e.g. > VLD, MoComp, IDCT. Thus, we could have combinatorial explosion. > > With hwaccel v1, we factored this with PIX_FMT_<hwaccel>_<entrypoint>, > the <codec> information became irrelevant. <entrypoint> was reduced to > typically 1 to 3. What does a pixel format with a <codec> name in it > actually mean anyway? > > This is better, but this was still silly. A pixel format defines... well, > a pixel format. If the user wants to extract the decoded pixels from the > GPU, he requires so with AV_HWACCEL_CAP_GET_PIXELS. The pixel format of > those needs to be expressed the usual way, e.g. avctx->pix_fmt == > PIX_FMT_YUV420P. How would this have been expressed if avctx->pix_fmt was > PIX_FMT_<hwaccel>_<entrypoint>?

"A pixel format defines... well, a pixel format." thats a useless circular definition

a pixel format defines the structuring of data in AVFrame.data[] that likely differs between 2 hw accelerator APIs, so should allow selecting hw accelerators

And how do you define and select an HW accelerator that outputs NV12 or I420 frames? Or other HW accelerators that the user requested decoded pixels back to .data[0-2]?

easy, pix_fmt = PIX_FMT_NV12 its fully transparent then the user app does not even need to know a hwaccel is used

Actually, this is not that easy. The hwaccel can require the user-initialized, in a system-dependent way, hwaccel_context.

why? this is duplicated code in each application using lavc, isnt it?

The HW decoder handle initialization code can be display-dependent, or some other obscure factors that may involve user feedback or notifying him some actions to take manually. The former case is for VA-API/VDPAU like API that construct a display-independent device ID (opaque) from a display-dependent one. The latter case is for another API that nowadays allows only a single application to use the globally shared resources. e.g. if the BCM decoder is already used by another application, the player can gracefully notify the user about it.

Besides, the user code for this initialization is rather small and generally reduces to a single line.

So we have the options A. We use existing APIs and avoid code duplication in the user APP, but this might for some cases require 1 line of system dependant code in lavc for initialization. B. We introduce a new API, require all apps to duplicate initialization code require all apps to have code for 2 entirely different apis to select cpu and hw accel features. Require all apps to support 2 different apis to select the output format. I really think its obvious which way is better, oddly you seem to keep arguing the other way Either you see a problem that iam not seeing or you arent seeing the problems i see

...

...

The user may also want to explicitly require a specific hwaccel to use. How do you express those?

like MMX/SSE/Altivec

Well, this one is based on an extra dsp_mask field in AVCodecContext. Thus, I also suggest an extra hwaccel_id field in AVCodecContext to allow this. :)

i dont mind having a seperate bitmask for on cpu and one for off cpu accelerations. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin