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

[Michael Niedermayer]

On Thu, Jun 20, 2013 at 11:29:25PM +0200, Laurent Aimar wrote:
> Hi,
> 
> Here is a first pass review.
> 

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

I also leave the style and names to others


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

I think there should be no cases with decoding failure and a returned
picture


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

i would say, no, or at least its pretty ugly


[...]

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

no, not valid


Thanks alot for your review


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130621/95b6839e/attachment.asc>