[FFmpeg-devel] [PATCH] lavc/dxva2: add ffmpeg calling dxva2 APIs
Wei Gao
highgod0401 at gmail.com
Wed Jul 3 07:21:57 CEST 2013
sorry that this mail has not been replied for a long time, so I CC to fenrir
thanks
Best regards
2013/6/24 Wei Gao <highgod0401 at gmail.com>
> Hi,
>
> Thanks for your reply, some questions are as follows
>
> Thanks.
> Best regards
>
>
> 2013/6/21 Laurent Aimar <fenrir at elivagar.org>
>
>> Hi,
>>
>>
>>
>> > +
>> > +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 ?
>>
> I will remove it
>
>>
>> > + 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?
>
>>
>>
>> > +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 will add check the ret
>
>>
>> > + 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 ?
>>
> will remove it
>
>>
>> > + 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.
>>
> add codec->flush(avctx);, is it right?
>
>>
>> > + /* 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 ?
>
> 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?
>
>>
>
>
>> > + 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, will fix
>
>>
>> > + 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.
>
>>
>> > + 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
>
>>
>> > + }
>> > + }
>> > + 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.
>>
>> will remove it
>
>> > + /* 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 ?
>>
>> > + 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
>
> switch (h->sps.bit_depth_luma) {
> case 8:
> if (!CHROMA444(h) && !CHROMA422(h)) {
> // only this will H.264 decoder switch to hwaccel
> ret = 0;
> break;
> }
> default:
> av_log(avctx, AV_LOG_ERROR, "Unsupported file.\n");
> }
>
> > + 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.
>>
> OK
>
>> > + 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.
>>
>> > + return ret;
>> > +}
>> > +
>> > +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.
>
>>
>> > + /* 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?
>
>
>> > +
>> > + /* 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?
>
>>
>> > + avctx->slice_flags |= SLICE_FLAG_ALLOW_FIELD;
>> Do you need that ?
>>
>> > + /* 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
>
> the vda code is as follows:
>
> /* changes callback functions */
> avctx->get_format = get_format;
> avctx->get_buffer2 = get_buffer2;
> #if FF_API_GET_BUFFER
> // force the old get_buffer to be empty
> avctx->get_buffer = NULL;
> #endif
>
>
>
>> > + /* 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.
>
>>
>> Also, it would probably be better to disable any threading as it becomes
>> useless
>> and may break the decoding (I am not sure all the bugs have been fixed in
>> that
>> regards).
>>
>> > +
>> > +static void dxva2dec_flush(AVCodecContext *avctx)
>> > +{
>> > + DXVA2_DecoderContext *ctx = (DXVA2_DecoderContext
>> *)avctx->priv_data;
>> > + AVCodec *codec = ctx->codec;
>> > + return codec->flush(avctx);
>> > +}
>> > +#if CONFIG_H264_DXVA2_DECODER
>> > +AVCodec ff_h264_dxva2_decoder = {
>> > + .name = "h264_dxva2",
>> > + .type = AVMEDIA_TYPE_VIDEO,
>> > + .id = AV_CODEC_ID_H264,
>> > + .priv_data_size = sizeof(DXVA2_DecoderContext),
>> > + .init = dxva2dec_init,
>> > + .close = dxva2dec_close,
>> > + .decode = dxva2dec_decode,
>> > + .capabilities = CODEC_CAP_DELAY,
>> > + .flush = dxva2dec_flush,
>> > + .long_name = NULL_IF_CONFIG_SMALL("H.264 (DXVA2
>> acceleration)"),
>> > +};
>> > +#endif
>> > +
>> > +#if CONFIG_MPEG2VIDEO_DXVA2_DECODER
>> > +AVCodec ff_mpeg2video_dxva2_decoder = {
>> > + .name = "mpeg2video_dxva2",
>> > + .type = AVMEDIA_TYPE_VIDEO,
>> > + .id = AV_CODEC_ID_MPEG2VIDEO,
>> > + .priv_data_size = sizeof(DXVA2_DecoderContext),
>> > + .init = dxva2dec_init,
>> > + .close = dxva2dec_close,
>> > + .decode = dxva2dec_decode,
>> > + .capabilities = CODEC_CAP_DELAY,
>> > + .flush = dxva2dec_flush,
>> > + .long_name = NULL_IF_CONFIG_SMALL("MPEG2 Video (DXVA2
>> acceleration)"),
>> > +};
>> > +#endif
>> > +
>> > +#if CONFIG_VC1_DXVA2_DECODER
>> > +AVCodec ff_vc1_dxva2_decoder = {
>> > + .name = "vc1_dxva2",
>> > + .type = AVMEDIA_TYPE_VIDEO,
>> > + .id = AV_CODEC_ID_VC1,
>> > + .priv_data_size = sizeof(DXVA2_DecoderContext),
>> > + .init = dxva2dec_init,
>> > + .close = dxva2dec_close,
>> > + .decode = dxva2dec_decode,
>> > + .capabilities = CODEC_CAP_DELAY,
>> > + .flush = dxva2dec_flush,
>> > + .long_name = NULL_IF_CONFIG_SMALL("VC1 (DXVA2 acceleration)"),
>> > +};
>> > +#endif
>> > +
>> > +#if CONFIG_WMV3_DXVA2_DECODER
>> > +AVCodec ff_wmv3_dxva2_decoder = {
>> > + .name = "wmv3_dxva2",
>> > + .type = AVMEDIA_TYPE_VIDEO,
>> > + .id = AV_CODEC_ID_WMV3,
>> > + .priv_data_size = sizeof(DXVA2_DecoderContext),
>> > + .init = dxva2dec_init,
>> > + .close = dxva2dec_close,
>> > + .decode = dxva2dec_decode,
>> > + .capabilities = CODEC_CAP_DELAY,
>> > + .flush = dxva2dec_flush,
>> > + .long_name = NULL_IF_CONFIG_SMALL("WMV3 (DXVA2
>> acceleration)"),
>> > +};
>> > +#endif
>>
>> I will review the rest of the patch once the already commented code has
>> been
>> processed.
>>
>> Regards,
>>
>> --
>> fenrir
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
>
More information about the ffmpeg-devel
mailing list