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

[fenrir at elivagar.org]

On Tue, Jul 16, 2013 at 03:40:48PM +0800, weixuan wang wrote:
> > +        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.
 Weird.

> *> > > +    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.*
 How could dxva2_ctx be NULL in this code ?

> > > > +        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? *
 I honestly don't recall which file you will have to look at.


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

Maybe it would be cleaner to use a new AVCodecContext, someone from the ffmpeg project
could have a look.

> > > > +
> > > > +    /* 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?*
 It's done implicitly by our implementation of get_format (ffmpeg_GetFormat)
(we delete our dxva handle and recreate one with the right parameters) and
get_buffer (ffmpeg_GetFrameBuf).


> > > > +    /* 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?*
 In VLC we overload the older get_buffer/reget_buffer/release_buffer. I
think adapting for get_buffer2 will be easy..

Regards,

-- 
fenrir