[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