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

[fenrir at elivagar.org]

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.

> > > +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.

> > > +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 which is for the encoder. Use
a local gb variable instead for example.

> > > +                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.


> > > +        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.


> > > +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.

> > > +    /* 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.

> > > +
> > > +    /* 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.

> > > +    /* 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.

> > > +    /* 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.

Regards,

-- 
fenrir