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

weixuan wang batmanwwx at gmail.com
Mon Jul 22 05:16:40 CEST 2013


Hi,

This patch has not been reviewed for a long time.
Thanks for reviewing

Best regards


2013/7/18 weixuan wang <batmanwwx at gmail.com>

>
>
> ---------- Forwarded message ----------
> From: weixuan wang <batmanwwx at gmail.com>
> Date: 2013/7/16
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/dxva2: add ffmpeg calling dxva2
> APIs
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>
>
> *
>
>
> > +#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).*
>
> *I removed it.*
> *
>
> > +    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.*
>
> *We will add it in the next milestone.**
> *
> *
> > +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 ?
> *
>
> *The dxva2 decoder context needs this format to get the buffer. This
> format is valid. If i removed it, the decoder will run incorrectly. **
>
> > +        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.*
>
> *It is valid to check on the got_frame. If it is NULL, it will cause a
> fatal error.
> ***
> *
> > +        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 ?
> *
> *The dxva2 decoder context needs this format to get the buffer. This
> format is valid. If i removed it, the decoder will run incorrectly.
>
> > +    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.
> *
>
> *Ok, I used the UINT32_MAX instead of the “-1”.
> **
> > +        }
> > +    }*
> *
> > +    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.*
>
> *This initialization will reduce the warnings when compiling the code of
> the FFMPEG.
> *
> *
> > +    /* 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 ?
>
> *
>
> **
> *> +                    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.*
>
> *Ok, I will modify these codes to create a simpler and easier to read
> code.
> *
> *
> > +    avctx->slice_flags |= SLICE_FLAG_ALLOW_FIELD;
> Do you need that ?*
>
> *Yes, we need that. The mpeg2interlace will use this flag.
> **
> **Hi,*
>
> *
> On Mon, Jun 24, 2013 at 08:20:08PM +0800, Wei Gao wrote:*
>
> *> Thanks for your reply, some questions are as follows
> > *
>
> *> > > +    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.
> > > *
>
> *> Do you mean the  dxva2_ctx always be valid?*
>
> *I mean that it *seems* to me that dxva2_ctx can never be NULL. If it is
> so,
> then the test is useless and should be removed.*
>
> *I think the dxva2_ctx may be NULL, so I check it to make the code more
> robust.*
>
> *
> > > > +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.
> > > *
>
> *> add codec->flush(avctx);, is it right?*
>
> *From the point of view of a libavcodec user, that corresponds to a call
> to
> avcodec_flush_buffers. Inside AVCodecContext, I don't know.*
>
> *I have already added the codec->flush(avctx) to to ensure that all
> remain pictures are released before destroying the dxva2 context*
>
> *
> > > > +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 ?
> > *
>
> *> Sorry, I don't konw is there anyway to access the data, because I
> think the
> > format should be check.Can anyone give any advice?
> >
> > > *
>
> *> > > +            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 ?
> > > *
>
> *> I only konw this way to get the format code.*
>
> *Th issue here is that you use s->**mpeg_enc_ctx.gb*<http://mpeg_enc_ctx.gb/>
> * which is for the encoder. Use
> a local gb variable instead for example.*
>
> *Ok, I will modify it and use a local gb variable instead of the s->**
> mpeg_enc_ctx->gb* <http://mpeg_enc_ctx.gb/>**
>
> *
> > > > +                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.
> > > *
>
> *> sorry, I don't get what you mean*
>
> * I mean that the code could be simpler and cleaner if you
> refactor/modify it
> a bit.*
>
> *Ok, I will modify it to make the code simpler and cleaner.*
>
> *
>
> > > > +        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).
> > > *
>
> *> I reference the vda_h264_dec.c code, and it seems just check these
> things*
>
> *Yeah but the vda may or may not have the same constraints than dxva.*
>
> *Can you give me some details of the more checks? Which file in the MPC
> code I should use for reference? *
>
> *
>
> > > > +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 ?
> > > *
>
> *> Sorry, do you mean does the code can run? I tested on my computer, it
> can
> > run.*
>
> *I mean that it seems weird and may break some (but not all) valids way
> to use libavcodec. But I don't have the knowledge to be sure.*
>
> *I have already tried to modify the code of the hwcodec and the codec.
> But the code was necessary and can`t be removed.*
>
> *
> > > > +    /* 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.
> > > *
>
> *> > do you mean I should check the format during the decode?*
>
> *Yes, and ideally (dunno if it is feasible but I think so) switch between
> DXVA
> and software decoding.*
>
> *Ok, I will modify it and check the format during the 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.
> > > *
>
> *> OK, the dxva file?*
>
> * No, the vlc avcodec/video.c file. As you wrap the decoder, this file
> contains the same logics.*
>
> *I have already read the code in the vlc avcodec/video.c file and I
> didn`t find the code about the video dimension. Can you give me some
> details of the video dimension?*
>
> *
> > > > +    /* 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...
> > > *
>
> *> I reference the code of ffmpeg, and vda_h264_dec.c , it seems that the
> > release function is not valid in this version*
>
> * If you don't overload the release function you will leaks surfaces, the
> current code in dxva2 will try to workaround it, but it may creates broken
> pictures (it will depends on the streams). Have a look at the VLC code.*
>
> *I have already read the code of the VLC and I didn`t find the code about
> the overloading of the get_buffer2. Can you give me some details of the
> overloading of the get_buffer2? Which file I should read in the VLC code?*
>
> *
> > > > +    /* 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 ?
> > > *
>
> *> I reference the vda code. I test on my computer, it works.*
>
> *Same remark than before, I am not sure that it's always valid but I
> don't have
> the knowledge.*
>
> *I use the get_hw_soft_codec() function to set the ctx->codec to the
> right decoder depend on the avctx->codec_id. So the dxva2dec_close(avctx)
> will close the right decoder.*
>
> * *
>
> * *
> * i modified the code of the patch followwing your suggestions and still
> have some questions with some suggestions. i submit a new patch and please
> review it again.*
> **
> *thanks!*
>
>
> 2013/7/7 Michael Niedermayer <michaelni at gmx.at>
>
>>  On Sun, Jul 07, 2013 at 02:52:59PM +0200, fenrir at elivagar.org wrote:
>> > Hi,
>> >
>> > On Mon, Jun 24, 2013 at 08:20:08PM +0800, Wei Gao wrote:
>> > > Thanks for your reply, some questions are as follows
>> [...]
>> > > > > +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 ?
>> > > >
>> > > Sorry, do you mean does the code can run? I tested on my computer, it
>> can
>> > > run.
>> > I mean that it seems weird and may break some (but not all) valids way
>> > to use libavcodec. But I don't have the knowledge to be sure.
>>
>> i cant say it with 100% certainity without reviewing the whole patch
>> but i also think the code is wrong
>>
>>
>>
>> [...]
>>
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Observe your enemies, for they first find out your faults. -- Antisthenes
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
>
>


More information about the ffmpeg-devel mailing list