[FFmpeg-devel] [PATCH] lavc/dxva2: add ffmpeg calling dxva2 APIs
weixuan wang
batmanwwx at gmail.com
Tue Jul 16 09:40:48 CEST 2013
*
> +#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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavc-dxva2-add-ffmpeg-calling-dxva2.patch
Type: application/octet-stream
Size: 59839 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130716/0bf080cc/attachment.obj>
More information about the ffmpeg-devel
mailing list