[FFmpeg-devel] [PATCH] HWAccel infrastructure (take 3)

[Michael Niedermayer]

On Wed, Feb 18, 2009 at 01:13:58PM +0100, Gwenole Beauchesne wrote:
> On Wed, 18 Feb 2009, Diego Biurrun wrote:
>
>> This header should #include its requirements to at least pass 'make
>> checkheaders'.  In this case I think it means stdint.h and avcodec.h.
>
> Done, though it's probably desirable to not #include "avcodec.h" from it.
>
> New patch attached:
> - Break long decls
> - Drop avcodec_internal.h, use internal.h instead

[...]
>  } AVCodec;
>  
>  /**
> + * AVHWAccel.
> + */
> +typedef struct AVHWAccel {
> +    enum CodecType type;
> +    enum CodecID id;
> +    enum PixelFormat pix_fmt;
> +    /**
> +     * Hardware accelerated codec capabilities.
> +     * see FF_HWACCEL_CODEC_CAP_*
> +     */
> +    int capabilities;
> +    struct AVHWAccel *next;

> +    /**
> +     * Called at the beginning of each frame.
> +     *
> +     * Meaningful frame information (codec specific) is guaranteed to
> +     * be parsed at this point. This function is mandatory.
> +     *
> +     * @param avctx the codec context
> +     * @return zero if successful, a negative value otherwise
> +     */
> +    int (*start_frame)(AVCodecContext *avctx);

this should have a buf, buf_size for the frame


> +    /**
> +     * Called at the beginning of each slice.
> +     *
> +     * libavcodec will try its best to provide a single \p buf base at
> +     * each call. However, this is not guaranteed for all codecs but
> +     * for MPEG-1/2. This function is mandatory only if end_slice()
> +     * is implemented.
> +     *
> +     * @param avctx the codec context
> +     * @param buf the slice, or frame, data buffer base
> +     * @param buf_offset the offset to the actual slice data
> +     * @return zero if successful, a negative value otherwise
> +     */
> +    int (*start_slice)(AVCodecContext *avctx, const uint8_t *buf, uint32_t buf_offset);

hmm this seems poorly designed
first, it needs a buf_size or you will segfault
second buf / buf_offset can be replaced by buf or why not?


> +    /**
> +     * Called to decode a single slice.
> +     *
> +     * Meaningful slice information (codec specific) is guaranteed to
> +     * be parsed at this point. This function is optional. Should this
> +     * be implemented, so shall start_slice() and end_slice().
> +     *
> +     * @return zero if successful, a negative value otherwise
> +     */
> +    int (*decode_slice)(AVCodecContext *avctx);

as decode_slice is always called after start_slice
and start_slice is always called before decode_slice this is unneeded


> +    /**
> +     * Called at the end of each slice.
> +     *
> +     * This function is mandatory only if decode_slice() is implemented.
> +     *
> +     * @param avctx the codec context
> +     * @param buf_end the pointer to the end of the current slice
> +     * @return zero if successful, a negative value otherwise
> +     */
> +    int (*end_slice)(AVCodecContext *avctx, const uint8_t *buf_end);
> +    /**
> +     * Called at the end of each frame.
> +     *
> +     * The whole picture is parsed at this point and can now be sent
> +     * to the hardware accelerator. This function is mandatory.
> +     *
> +     * @param avctx the codec context
> +     * @return zero if successful, a negative value otherwise
> +     */
> +    int (*end_frame)(AVCodecContext *avctx);
> +} AVHWAccel;

please add a "const char name"


[...]
> diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> index 409661e..63361a3 100644
> --- a/libavcodec/mpegvideo.c
> +++ b/libavcodec/mpegvideo.c
> @@ -950,7 +950,8 @@ void MPV_frame_end(MpegEncContext *s)
>      //just to make sure that all data is rendered.
>      if(CONFIG_MPEG_XVMC_DECODER && s->avctx->xvmc_acceleration){

>          ff_xvmc_field_end(s);
> -    }else if(!(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
> +    }else if(!s->avctx->hwaccel
> +       && !(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
>         && s->unrestricted_mv

    }else if(!(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
+      !s->avctx->hwaccel
       && s->unrestricted_mv


[...]
> +int ff_hwaccel_decode_picture(AVCodecContext *avctx, const uint8_t *buffer, uint32_t size)
> +{
> +    if (avctx->hwaccel->start_frame(avctx) < 0)
> +        return -1;
> +    if (ff_hwaccel_decode_slice(avctx, buffer, 0, size) < 0)
> +        return -1;
> +    if (avctx->hwaccel->end_frame(avctx) < 0)
> +        return -1;
> +    return 0;
> +}
> +

> +AVHWAccel *ff_query_hwaccel_codec(AVCodecContext *avctx, AVCodec *codec)

please pass codec_id instead of AVCodec


> +{
> +    AVHWAccel *hwaccel;
> +    AVHWAccel *hwaccels[PIX_FMT_NB] = { NULL, };
> +    enum PixelFormat pix_fmts[PIX_FMT_NB + 1];
> +    int pix_fmt, n_pix_fmts = 0;
> +
> +    /* The user shall override ::get_format() with something sensible enough */
> +    if (!avctx->get_format || avctx->get_format == avcodec_default_get_format)
> +        return NULL;
> +
> +    for (hwaccel = first_hwaccel; hwaccel; hwaccel = hwaccel->next) {

> +        if (hwaccel->type == codec->type && hwaccel->id == codec->id) {

you dont need to check the type, the id implicates the type


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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090218/d37fcfef/attachment.pgp>