[FFmpeg-devel] [PATCH] lavc/dxva2: add ffmpeg calling dxva2 APIs

Laurent Aimar fenrir at elivagar.org
Thu Jun 20 23:29:25 CEST 2013


Hi,

Here is a first pass review.

Remark: I did not comment on
 - the code style
 - the way the variables/structures are named
 - the build system part
I will let someone from the FFmpeg project to comment on it.

 
> +#if CONFIG_H264_DXVA2_DECODER
> +extern AVCodec ff_h264_decoder,ff_h264_dxva2_decoder;
> +#endif
> +
> +#if CONFIG_MPEG2VIDEO_DXVA2_DECODER
> +extern AVCodec ff_mpeg2video_decoder,ff_mpeg2video_dxva2_decoder;
> +#endif
> +
> +#if CONFIG_VC1_DXVA2_DECODER
> +extern AVCodec ff_vc1_decoder,ff_vc1_dxva2_decoder;
> +#endif
> +
> +#if CONFIG_WMV3_DXVA2_DECODER
> +extern AVCodec ff_wmv3_decoder, ff_wmv3_dxva2_decoder;
> +#endif
> +
> +typedef union {
> +#if CONFIG_H264_DXVA2_DECODER
> +    H264Context        h264ctx;
> +#endif
> +#if CONFIG_VC1_DXVA2_DECODER || CONFIG_WMV3_DXVA2_DECODER
> +    VC1Context         vc1ctx;
> +#endif
> +#if CONFIG_MPEG2VIDEO_DXVA2_DECODER
> +    Mpeg1Context       mpeg2videoctx;
> +#endif
> +}DecoderContext;
> +
> +typedef struct {
> +    DecoderContext          decoderctx;
> +    int                     initialized;
initialized seems useless (write only).

> +    enum AVPixelFormat      pix_fmt;
> +    int (*get_buffer2)(struct AVCodecContext *s, AVFrame *frame, int flags);
> +    AVCodec                 *hwcodec;
> +    AVCodec                 *codec;
> +    dxva2_context           dxva2_ctx;
> +} DXVA2_DecoderContext;
> +
> +static const enum AVPixelFormat dxva2_pixfmts[] = {
> +    AV_PIX_FMT_YUV420P,
> +    AV_PIX_FMT_NONE
> +};
It might be a good idea to also support NV12 output.

> +static void get_hw_soft_codec(struct AVCodecContext *avctx,DXVA2_DecoderContext *ctx)
> +{
> +    switch (avctx->codec_id) {
> +        #if CONFIG_H264_DXVA2_DECODER
> +        case AV_CODEC_ID_H264:
> +            ctx->hwcodec = &ff_h264_dxva2_decoder;
> +            ctx->codec   = &ff_h264_decoder;
> +            return;
> +        #endif
> +        #if CONFIG_MPEG2VIDEO_DXVA2_DECODER
> +        case AV_CODEC_ID_MPEG2VIDEO:
> +            ctx->hwcodec = &ff_mpeg2video_dxva2_decoder;
> +            ctx->codec   = &ff_mpeg2video_decoder;
> +            return;
> +        #endif
> +        #if CONFIG_VC1_DXVA2_DECODER
> +        case AV_CODEC_ID_VC1:
> +            ctx->hwcodec = &ff_vc1_dxva2_decoder;
> +            ctx->codec   = &ff_vc1_decoder;
> +            return;
> +        #endif
> +        #if CONFIG_WMV3_DXVA2_DECODER
> +        case AV_CODEC_ID_WMV3:
> +            ctx->hwcodec = &ff_wmv3_dxva2_decoder;
> +            ctx->codec   = &ff_wmv3_decoder;
> +            return;
> +        #endif
> +    }
> +}
> +
> +static int get_buffer2(struct AVCodecContext *avctx, AVFrame *pic, int flags)
> +{
> +    int ret;
> +    DXVA2_DecoderContext *ctx = (DXVA2_DecoderContext *)avctx->priv_data;
> +    dxva2_context *dxva2_ctx = &ctx->dxva2_ctx;
> +    avctx->pix_fmt = ctx->pix_fmt;
Why do you copy the pix_fmt here ? And is that valid ?

> +    ff_init_buffer_info(avctx, pic);
> +
> +    if ((ret = ctx->get_buffer2(avctx, pic, flags)) < 0) {
> +        return ret;
> +    }
> +    if (dxva2_ctx) {
I don't see how dxva2_ctx can be NULL.

> +        ff_get_dxva2_surface(dxva2_ctx, pic);
> +        return 0;
> +    } else {
> +        av_log(avctx, AV_LOG_ERROR, "No dxva2 context, get buffer failed\n");
> +        return AVERROR(EINVAL);
> +    }
> +}
> +
> +static enum PixelFormat get_format(AVCodecContext *p_context,
> +                                       const enum PixelFormat *pi_fmt)
> +{
> +     return AV_PIX_FMT_DXVA2_VLD;
> +}
> +
> +static int dxva2dec_decode(AVCodecContext *avctx, void *data, int *got_frame,
> +                                  AVPacket *avpkt)
> +{
> +    DXVA2_DecoderContext *ctx = (DXVA2_DecoderContext *)avctx->priv_data;
> +    AVFrame *pic = data;
> +    int ret;
> +    AVCodec *codec = ctx->codec;
> +    ret = codec->decode(avctx, data, got_frame, avpkt);
> +    if (*got_frame) {
I don't remember if it is valid to check on got_frame without first checking ret.

> +        pic->format = ctx->pix_fmt;
> +        ff_extract_dxva2(&ctx->dxva2_ctx, pic);
> +    }
> +    avctx->pix_fmt = ctx->pix_fmt;
Why do you copy the pix_fmt here too ? And is that valid ?

> +    return ret;
> +}
> +
> +static av_cold int dxva2dec_close(AVCodecContext *avctx)
> +{
> +    DXVA2_DecoderContext *ctx = avctx->priv_data;
> +    AVCodec *codec = ctx->codec;
> +    /* release buffers and decoder */
> +    ff_release_dxva2(&ctx->dxva2_ctx);
You probably need to flush the decoder first to ensure that all remaing pictures are
released before destroying the dxva2 context.

> +    /* close decoder */
> +    codec->close(avctx);
> +    return 0;
> +}
> +static int get_mpeg2_video_format(AVCodecContext *avctx)
> +{
> +    Mpeg1Context *s = avctx->priv_data;
> +    MpegEncContext *s2 = &s->mpeg_enc_ctx;
Is that a valid access when using the decoder part only ?

> +    const uint8_t *buf_ptr = avctx->extradata;
> +    const uint8_t *buf_end = avctx->extradata + avctx->extradata_size;
> +    int input_size, format = -1;
> +    if (avctx->extradata) {
> +        for (;;) {
> +            uint32_t start_code = -1;
Using UIN32_MAX would be cleaner IMHO.

> +            buf_ptr = avpriv_find_start_code(buf_ptr, buf_end, &start_code);
> +            input_size = buf_end - buf_ptr;
> +            if (EXT_START_CODE == start_code) {
> +                init_get_bits(&s2->gb, buf_ptr, input_size * 8);
Is it valid to use a get bit context private to the Mpeg1Context context ?

> +                switch (get_bits(&s2->gb, 4)) {
> +                    case 0x1:
> +                        skip_bits(&s2->gb, 9);
> +                        format = get_bits(&s2->gb, 2);
> +                        return format;
> +                }
> +            }
> +            if (input_size <= 0) {
> +                av_log(avctx, AV_LOG_ERROR,"get_mpeg2_video_format error\n");
> +                return format;
> +            }
It would be cleaner to move it before trying to use the buffer in case of a start
code match.

> +        }
> +    }
> +    av_log(avctx, AV_LOG_ERROR,"get_mpeg2_video_format error\n");
> +    return format;
> +}
> +
> +static int check_format(AVCodecContext *avctx)
> +{
> +    uint8_t *pout;
> +    int psize, index, ret = -1;
> +    H264Context *h;
> +    AVCodecParserContext *parser = NULL;
I think it's a useless initialization.

> +    /* check if support */
> +    switch (avctx->codec_id) {
> +    case AV_CODEC_ID_H264:
> +        /* init parser & parse file */
> +        parser = av_parser_init(avctx->codec->id);
> +        if (!parser) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to open parser.\n");
> +            break;
> +        }
> +        parser->flags = PARSER_FLAG_COMPLETE_FRAMES;
> +        index = av_parser_parse2(parser, avctx, &pout, &psize, NULL, 0, 0, 0, 0);
> +        if (index < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to parse this file.\n");
Weird error message.

> +            av_parser_close(parser);
> +        }
> +        h = parser->priv_data;
Is that valid when you just closed the parser in case of error ?

> +        if (8 == h->sps.bit_depth_luma) {
> +            if (!CHROMA444(h) && !CHROMA422(h)) {
> +                // only this will decoder switch to hwaccel
> +                //check the profile
> +                if ((FF_PROFILE_H264_BASELINE == h->sps.profile_idc) ||
> +                    (FF_PROFILE_H264_MAIN == h->sps.profile_idc) ||
> +                    (FF_PROFILE_H264_HIGH == h->sps.profile_idc)) {
If you really want to check the compatibility, you need more checks (you may
want to have a look at MPC code).
> +                    ret = 0;
> +                } else {
> +                    av_log(avctx, AV_LOG_ERROR, "Unsupported profile.\n");
> +                }
> +                av_parser_close(parser);
> +                break;
> +            }
> +        } else {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported file.\n");
Weird error message.
> +            av_parser_close(parser);
> +            break;
> +        }
You could probably factorized all the av_parser_close() calls.
> +        break;
> +    case AV_CODEC_ID_MPEG2VIDEO:
> +        if (CHROMA_420 == get_mpeg2_video_format(avctx)) {
> +            ret = 0;
> +            break;
> +        } else {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported file.\n");
Weird error message.
> +            break;
> +        }
> +    default:
> +        ret = 0;
> +        break;
> +    }
I think that returning directly when needed instead of using the ret variable
would create a simpler and easier to read code.

> +    return ret;
> +}
> +
> +static av_cold int dxva2dec_init(AVCodecContext *avctx)
> +{
> +    DXVA2_DecoderContext *ctx = (DXVA2_DecoderContext *)avctx->priv_data;
> +    dxva2_context *dxva2_ctx = (dxva2_context *)(&ctx->dxva2_ctx);
> +    AVCodec *hwcodec, *codec;
> +    int ret;
> +    ctx->initialized = 0;
> +    get_hw_soft_codec(avctx,ctx);
> +    hwcodec = ctx->hwcodec;
> +    codec = ctx->codec;
> +    /* init pix_fmts of codec */
> +    if (!hwcodec->pix_fmts) {
> +        hwcodec->pix_fmts = dxva2_pixfmts;
> +    }
I don't know how that works, could someone double check ?

> +    /* check if DXVA2 supports this file */
> +    if (check_format(avctx) < 0)
> +        return AVERROR(EINVAL);
I am not sure that's its acceptable. You may not have any extradata when the codec
is instantiated. In addition, the codec profile, or chroma type may change while
decoding.

> +
> +    /* init vda */
> +    memset(dxva2_ctx, 0, sizeof(dxva2_context));
> +    ret = ff_create_dxva2(avctx->codec_id, dxva2_ctx);
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "create dxva2 error\n");
> +        return AVERROR(EINVAL);
> +    }
> +    ctx->pix_fmt = avctx->get_format(avctx, avctx->codec->pix_fmts);
> +    ret = ff_setup_dxva2(dxva2_ctx, &avctx->hwaccel_context, &avctx->pix_fmt, avctx->width, avctx->height);
> +    if (ret < 0) {
> +        av_log(avctx,AV_LOG_ERROR,"error DXVA setup %d\n", ret);
> +        ret = AVERROR(EINVAL);
> +        goto failed;
> +    }
I am not sure that's its also acceptable. The video dimension may change while
decoding. You may have a look at VLC to see how we handle that.

> +    avctx->slice_flags |= SLICE_FLAG_ALLOW_FIELD;
Do you need that ?

> +    /* changes callback functions */
> +    ctx->get_buffer2 = avctx->get_buffer2;
> +    avctx->get_format = get_format;
Are you sure it is valid to basically ignore the get_format of the user ?

> +    avctx->get_buffer2 = get_buffer2;
You overload the get_buffer2 but I do not see you overloading the associated
release function. So it probably means you are not releasing correctly the dxva2
surface handle that get associated to the picture by get_buffer2...

> +    /* init decoder */
> +
> +    ret = codec->init(avctx);
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to open decoder.\n");
> +        goto failed;
> +    }
> +    ctx->initialized = 1;
> +    return 0;
> +failed:
> +    dxva2dec_close(avctx);
> +    return ret;
> +}
I have some doubt about the way the avctx context is shared by both decoder. Could
someone check that ?

Also, it would probably be better to disable any threading as it becomes useless
and may break the decoding (I am not sure all the bugs have been fixed in that
regards).

> +
> +static void dxva2dec_flush(AVCodecContext *avctx)
> +{
> +    DXVA2_DecoderContext *ctx = (DXVA2_DecoderContext *)avctx->priv_data;
> +    AVCodec *codec = ctx->codec;
> +    return codec->flush(avctx);
> +}
> +#if CONFIG_H264_DXVA2_DECODER
> +AVCodec ff_h264_dxva2_decoder = {
> +    .name           = "h264_dxva2",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_H264,
> +    .priv_data_size = sizeof(DXVA2_DecoderContext),
> +    .init           = dxva2dec_init,
> +    .close          = dxva2dec_close,
> +    .decode         = dxva2dec_decode,
> +    .capabilities   = CODEC_CAP_DELAY,
> +    .flush          = dxva2dec_flush,
> +    .long_name      = NULL_IF_CONFIG_SMALL("H.264 (DXVA2 acceleration)"),
> +};
> +#endif
> +
> +#if CONFIG_MPEG2VIDEO_DXVA2_DECODER
> +AVCodec ff_mpeg2video_dxva2_decoder = {
> +    .name           = "mpeg2video_dxva2",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_MPEG2VIDEO,
> +    .priv_data_size = sizeof(DXVA2_DecoderContext),
> +    .init           = dxva2dec_init,
> +    .close          = dxva2dec_close,
> +    .decode         = dxva2dec_decode,
> +    .capabilities   = CODEC_CAP_DELAY,
> +    .flush          = dxva2dec_flush,
> +    .long_name      = NULL_IF_CONFIG_SMALL("MPEG2 Video (DXVA2 acceleration)"),
> +};
> +#endif
> +
> +#if CONFIG_VC1_DXVA2_DECODER
> +AVCodec ff_vc1_dxva2_decoder = {
> +    .name           = "vc1_dxva2",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_VC1,
> +    .priv_data_size = sizeof(DXVA2_DecoderContext),
> +    .init           = dxva2dec_init,
> +    .close          = dxva2dec_close,
> +    .decode         = dxva2dec_decode,
> +    .capabilities   = CODEC_CAP_DELAY,
> +    .flush          = dxva2dec_flush,
> +    .long_name      = NULL_IF_CONFIG_SMALL("VC1 (DXVA2 acceleration)"),
> +};
> +#endif
> +
> +#if CONFIG_WMV3_DXVA2_DECODER
> +AVCodec ff_wmv3_dxva2_decoder = {
> +    .name           = "wmv3_dxva2",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_WMV3,
> +    .priv_data_size = sizeof(DXVA2_DecoderContext),
> +    .init           = dxva2dec_init,
> +    .close          = dxva2dec_close,
> +    .decode         = dxva2dec_decode,
> +    .capabilities   = CODEC_CAP_DELAY,
> +    .flush          = dxva2dec_flush,
> +    .long_name      = NULL_IF_CONFIG_SMALL("WMV3 (DXVA2 acceleration)"),
> +};
> +#endif

I will review the rest of the patch once the already commented code has been
processed.

Regards,

-- 
fenrir
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavc-dxva2-add-ffmpeg-calling-dxva2-APIs.patch
Type: application/octet-stream
Size: 60010 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130620/6fbc80ff/attachment.obj>


More information about the ffmpeg-devel mailing list