[FFmpeg-devel] [PATCH] HWAccel infrastructure

Michael Niedermayer michaelni
Tue Feb 17 19:46:32 CET 2009


On Tue, Feb 17, 2009 at 05:34:22PM +0100, Gwenole Beauchesne wrote:
> Hi,
>
> This patch implements a hardware acceleration infrastructure for FFmpeg. It 
> works for both VA API and VDPAU. As an extra benefit, this also fixes a 
> VDPAU HW decoding bug with some MPEG-2 bitstreams I have here.
>
> The user is expected to override AVCodecContext::get_format() to select the 
> proper hardware accelerator. The pix_fmts[] arg is a list constructed from 
> registered AVHWAccelCodec (allcodecs.c: REGISTER_HWACCEL_CODEC()).
>
> As a side note, and in order to illustrate this, an mplayer -va arg 
> controls what accelerator to use actually. Then get_format() checks that 
> user (human) requested accelerator and only selects the matching IMGFMT.
>
> AVHWAccelCodec::pix_fmt is no longer an array so that to implement HW 
> acceleration case 3 (as "specified" by Reimar earlier). That is, 
> avctx->hwaccel_codec->pix_fmt will stick to the HW accelerated pixel format 
> whereas avctx->pix_fmt could be YV12 if the user wants to readback pixels 
> in that format. Otherwise, both are equal by default.
>
> WDYT?

[...]
> @@ -2360,6 +2367,26 @@ typedef struct AVCodec {
>  } AVCodec;
>  
>  /**
> + * AVHWAccelCodec.
> + */
> +typedef struct AVHWAccelCodec {

I would prefer it to be named AVHWAccel


> +    enum CodecType type;
> +    enum CodecID id;

> +    enum PixelFormat pix_fmt;


> +    /**
> +     * Hardware accelerated codec capabilities.
> +     * see FF_HWACCEL_CODEC_CAP_*
> +     */
> +    int capabilities;

> +    int (*start_frame)(AVCodecContext *avctx);
> +    int (*start_slice)(AVCodecContext *avctx, const uint8_t *buffer, uint32_t offset);
> +    int (*decode_slice)(AVCodecContext *avctx);
> +    int (*end_slice)(AVCodecContext *avctx, const uint8_t *buffer_end);
> +    int (*end_frame)(AVCodecContext *avctx);

these must be documented,noone could write an implementation without it
being documented


> +    struct AVHWAccelCodec *next;
> +} AVHWAccelCodec;
> +
> +/**
>   * four components are given, that's all.
>   * the last component is alpha
>   */




> @@ -3158,4 +3185,66 @@ int av_parse_video_frame_rate(AVRational *frame_rate, const char *str);
>  #define AVERROR_NOENT       AVERROR(ENOENT)  /**< No such file or directory. */
>  #define AVERROR_PATCHWELCOME    -MKTAG('P','A','W','E') /**< Not yet implemented in FFmpeg. Patches welcome. */
>  
> +/**
> + * Registers the hardware accelerated codec \p codec.
> + */
> +void av_register_hwaccel_codec(AVHWAccelCodec *codec);
> +

> +/**
> + * If c is NULL, returns the first registered hardware accelerated codec,
> + * if c is non-NULL, returns the next registered hardware accelerated
> + * codec after c, or NULL if c is the last one.
> + */
> +AVHWAccelCodec *av_hwaccel_codec_next(AVHWAccelCodec *c);
> +
> +/**
> + * Decodes one slice through the hardware accelerator.
> + *
> + * This is a helper function that simply calls AVHWAccelCodec
> + * functions start_slice(), decode_slice(), end_slice().
> + *
> + * @param avctx the codec context
> + * @param[in] buf the slice, or frame, data buffer base
> + * @param[in] buf_offset the offset to the actual slice data
> + * @param[in] buf_size the size of the slice data buffer in bytes
> + * @return zero if successful, a negative value otherwise
> + */
> +int av_hwaccel_decode_slice(AVCodecContext *avctx, const uint8_t *buf, uint32_t buf_offset, uint32_t buf_size);

i suspect this is not supposed to be called by the user, thus
it does not belong in a public header.
Also private API is ff_ public is av_


> +
> +/**
> + * Decodes one frame through the hardware accelerator.
> + *
> + * This is a helper function that simply calls AVHWAccelCodec
> + * functions start_frame(), start_slice(), decode_slice(),
> + * end_slice(), end_frame().
> + *
> + * @param avctx the codec context
> + * @param[in] buf the encoded frame bitstream buffer
> + * @param[in] buf_size the size of the bitstream buffer in bytes
> + * @return zero if successful, a negative value otherwise
> + */
> +int av_hwaccel_decode_picture(AVCodecContext *avctx, const uint8_t *buf, uint32_t buf_size);

same


> +
> +/**
> + * Finds the hardware accelerator to use for the codec \p codec.
> + *
> + * This function first determines the set of hardware accelerators
> + * matching \p codec. Then, it hands out all their pixel formats to
> + * the AVCodecContext get_format() function which decides of the right
> + * format to use.
> + *
> + * @param avctx the codec context
> + * @param codec the codec to match
> + * @return the hardware accelerated codec, or NULL if none was found.
> + */
> +AVHWAccelCodec *av_find_hwaccel_codec(AVCodecContext *avctx, AVCodec *codec);

this surely is a poor name for the function, its really querring the user app
also it doesnt belong in a public header ...


> +
> +/**
> + * Find the hardware accelerator implementing the pixel format \p pix_fmt.
> + *
> + * @param pix_fmt the format to match
> + * @return the hardware accelerated codec, or NULL if none was found.
> + */
> +AVHWAccelCodec *av_find_hwaccel_codec_by_pix_fmt(enum PixelFormat pix_fmt);

pix_fmts do not neccesarily implicate the codec_id, its just what current hw
accels do. Besides its not logic if the pix_fmt implictes the codec_id


[...]
> @@ -7322,6 +7329,8 @@ static void execute_decode_slices(H264Context *h, int context_count){
>      H264Context *hx;
>      int i;
>  
> +    if (s->avctx->hwaccel_codec)
> +        return;
>      if(s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU)
>          return;

also once this stuff is applied i want the old VDPAU droped ASAP


[...]
> @@ -2386,6 +2403,21 @@ static int decode_chunks(AVCodecContext *avctx,
>                      return -1;
>                  }
>  
> +                if (avctx->hwaccel_codec) {
> +                    const uint8_t *curr_slice = buf_ptr - 4;
> +                    const uint8_t *next_slice;
> +                    start_code = -1;

> +                    next_slice = ff_find_start_code(buf_ptr + 2, buf_end, &start_code);

are you doing another pass over the bitstream? if so this is unacceptable



> +                    if (next_slice < buf_end)
> +                        next_slice -= 4;
> +                    s2->resync_mb_x= s2->mb_x;
> +                    s2->resync_mb_y= s2->mb_y= mb_y;

> +                    if (av_hwaccel_decode_slice(avctx, buf, curr_slice - buf, next_slice - curr_slice) < 0)
> +                        return -1;
> +                     buf_ptr = next_slice; /* don't traverse the whole slice again in ff_find_start_code() */
> +                     break;

your indention is not looking good ...


[...]
> +    assert(codec->start_frame);
> +    assert(codec->end_frame);
> +    if (codec->decode_slice) {
> +        assert(codec->start_slice);
> +        assert(codec->end_slice);
> +    }

no, assert cannot be used to check user provided parameters
either dont check or check properly with error messages and error
return but i suspect checking is overkill for this


[...]
> +AVHWAccelCodec *av_find_hwaccel_codec(AVCodecContext *avctx, AVCodec *codec)
> +{
> +    AVHWAccelCodec *c;
> +    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 (c = first_hwaccel_codec; c != NULL; c = c->next) {

!= NULL is superflous in C


> +        if (c->type == codec->type && c->id == codec->id)
> +            pix_fmts[n_pix_fmts++] = c->pix_fmt;
> +    }
> +    if (n_pix_fmts == 0)
> +        return NULL;
> +    pix_fmts[n_pix_fmts] = PIX_FMT_NONE;
> +

> +    if ((pix_fmt = avctx->get_format(avctx, pix_fmts)) != PIX_FMT_NONE)
> +        return av_find_hwaccel_codec_by_pix_fmt(pix_fmt);

i dont see why a PIX_FMT_NONE check is needed here


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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20090217/774da85e/attachment.pgp>



More information about the ffmpeg-devel mailing list