> From: Rogozhkin, Dmitry V > Sent: Thursday, February 21, 2019 3:14 AM > To: ffmpeg-devel@ffmpeg.org > Cc: Li, Zhong <zhong...@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH v2 3/6] lavc/qsvdec: Replace current > parser with MFXVideoDECODE_DecodeHeader() > > On Wed, 2019-02-20 at 10:58 +0800, Zhong Li wrote: > > Using MSDK parser can improve qsv decoder pass rate in some cases > > (E.g: > > sps declares a wrong level_idc, smaller than it should be). > > And it is necessary for adding new qsv decoders such as MJPEG and VP9 > > since current parser can't provide enough information. > > Actually using MFXVideoDECODE_DecodeHeader() was disscussed at > > https://ffmpeg.org/pipermail/ffmpeg-devel/2015-July/175734.html and > > merged as commit 1acb19d, but was overwritten when merged libav > > patches (commit: 1f26a23) without any explain. > > > > v2: split decode header from decode_init, and call it for everyframe > > to detect format/resoultion change. It can fix some regression issues > > such as hevc 10bits decoding. > > > > Signed-off-by: Zhong Li <zhong...@intel.com> > > --- > > libavcodec/qsvdec.c | 172 ++++++++++++++++++++++------------------ > > ---- > > libavcodec/qsvdec.h | 2 + > > 2 files changed, 90 insertions(+), 84 deletions(-) > > > > diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c index > > 4a0be811fb..efe054f5c5 100644 > > --- a/libavcodec/qsvdec.c > > +++ b/libavcodec/qsvdec.c > > @@ -120,19 +120,18 @@ static inline unsigned int qsv_fifo_size(const > > AVFifoBuffer* fifo) > > return av_fifo_size(fifo) / qsv_fifo_item_size(); > > } > > > > -static int qsv_decode_init(AVCodecContext *avctx, QSVContext *q) > > +static int qsv_decode_preinit(AVCodecContext *avctx, QSVContext *q, > > enum AVPixelFormat *pix_fmts, mfxVideoParam *param) > > { > > - const AVPixFmtDescriptor *desc; > > mfxSession session = NULL; > > int iopattern = 0; > > - mfxVideoParam param = { 0 }; > > - int frame_width = avctx->coded_width; > > - int frame_height = avctx->coded_height; > > int ret; > > > > - desc = av_pix_fmt_desc_get(avctx->sw_pix_fmt); > > - if (!desc) > > - return AVERROR_BUG; > > + ret = ff_get_format(avctx, pix_fmts); > > + if (ret < 0) { > > + q->orig_pix_fmt = avctx->pix_fmt = AV_PIX_FMT_NONE; > > + return ret; > > + } else > > + q->orig_pix_fmt = pix_fmts[1]; > > You rely on the way how pix_fmts will be declared in some other place. > This gives issues to understand the code for the beginners and hence > difficulty in code support. Can we somehow show what is actually going on > here? For example, declare pix_fmts something like: > enum qsv_formats { > QSV, > QSV_NV12, > QSV_MAX > } > > enum AVPixelFormat pix_fmts[QSV_MAX+1] = { > [QSV] = AV_PIX_FMT_QSV, > [QSV_NV12] = AV_PIX_FMT_NV12, > [QSV_MAX] = AV_PIX_FMT_NONE > } > > After that we could address like: > q->orig_pix_fmt = pix_fmts[QSV_NV12]; both highlighting what > we actually want to achieve and making sure that we will have less mistakes > supporting the list of fmts wish we reorder something.
QSV sw_pix_fmt is not always NV12. For 10bit clips, we just assume it is NV12, after decoder_header called, it will be updated as P010. > > > > if (!q->async_fifo) { > > q->async_fifo = av_fifo_alloc(q->async_depth * > > qsv_fifo_item_size()); @@ -170,48 +169,72 @@ static int > > qsv_decode_init(AVCodecContext *avctx, QSVContext *q) > > return ret; > > } > > > > - ret = ff_qsv_codec_id_to_mfx(avctx->codec_id); > > - if (ret < 0) > > - return ret; > > + param->IOPattern = q->iopattern; > > + param->AsyncDepth = q->async_depth; > > + param->ExtParam = q->ext_buffers; > > + param->NumExtParam = q->nb_ext_buffers; > > > > - param.mfx.CodecId = ret; > > - param.mfx.CodecProfile = ff_qsv_profile_to_mfx(avctx->codec_id, > > avctx->profile); > > - param.mfx.CodecLevel = avctx->level == > FF_LEVEL_UNKNOWN ? > > MFX_LEVEL_UNKNOWN : avctx->level; > > - > > - param.mfx.FrameInfo.BitDepthLuma = desc->comp[0].depth; > > - param.mfx.FrameInfo.BitDepthChroma = desc->comp[0].depth; > > - param.mfx.FrameInfo.Shift = desc->comp[0].depth > > 8; > > - param.mfx.FrameInfo.FourCC = q->fourcc; > > - param.mfx.FrameInfo.Width = frame_width; > > - param.mfx.FrameInfo.Height = frame_height; > > - param.mfx.FrameInfo.ChromaFormat = > MFX_CHROMAFORMAT_YUV420; > > - > > - switch (avctx->field_order) { > > - case AV_FIELD_PROGRESSIVE: > > - param.mfx.FrameInfo.PicStruct = > MFX_PICSTRUCT_PROGRESSIVE; > > - break; > > - case AV_FIELD_TT: > > - param.mfx.FrameInfo.PicStruct = > MFX_PICSTRUCT_FIELD_TFF; > > - break; > > - case AV_FIELD_BB: > > - param.mfx.FrameInfo.PicStruct = > MFX_PICSTRUCT_FIELD_BFF; > > - break; > > - default: > > - param.mfx.FrameInfo.PicStruct = > MFX_PICSTRUCT_UNKNOWN; > > - break; > > - } > > + return 0; > > + } > > + > > +static int qsv_decode_init(AVCodecContext *avctx, QSVContext *q, > > mfxVideoParam *param) > > +{ > > + int ret; > > > > - param.IOPattern = q->iopattern; > > - param.AsyncDepth = q->async_depth; > > - param.ExtParam = q->ext_buffers; > > - param.NumExtParam = q->nb_ext_buffers; > > + avctx->width = param->mfx.FrameInfo.CropW; > > + avctx->height = param->mfx.FrameInfo.CropH; > > + avctx->coded_width = param->mfx.FrameInfo.Width; > > + avctx->coded_height = param->mfx.FrameInfo.Height; > > + avctx->level = param->mfx.CodecLevel; > > + avctx->profile = param->mfx.CodecProfile; > > + avctx->field_order = ff_qsv_map_picstruct(param- > > >mfx.FrameInfo.PicStruct); > > + avctx->pix_fmt = ff_qsv_map_fourcc(param- > > >mfx.FrameInfo.FourCC); > > > > - ret = MFXVideoDECODE_Init(q->session, ¶m); > > + ret = MFXVideoDECODE_Init(q->session, param); > > if (ret < 0) > > return ff_qsv_print_error(avctx, ret, > > "Error initializing the > MFX video > > decoder"); > > > > - q->frame_info = param.mfx.FrameInfo; > > + q->frame_info = param->mfx.FrameInfo; > > + > > + return 0; > > +} > > + > > +static int qsv_decode_header(AVCodecContext *avctx, QSVContext *q, > > AVPacket *avpkt, enum AVPixelFormat *pix_fmts, mfxVideoParam *param) > > +{ > > + int ret; > > + > > + mfxBitstream bs = { { { 0 } } }; > > + > > + if (avpkt->size) { > > + bs.Data = avpkt->data; > > + bs.DataLength = avpkt->size; > > + bs.MaxLength = bs.DataLength; > > + bs.TimeStamp = avpkt->pts; > > + if (avctx->field_order == AV_FIELD_PROGRESSIVE) > > + bs.DataFlag |= > MFX_BITSTREAM_COMPLETE_FRAME; > > + } else > > + return AVERROR_INVALIDDATA; > > + > > + > > + if(!q->session) { > > + ret = qsv_decode_preinit(avctx, q, pix_fmts, param); > > + if (ret < 0) > > + return ret; > > + } > > + > > + ret = ff_qsv_codec_id_to_mfx(avctx->codec_id); > > + if (ret < 0) > > + return ret; > > + > > + param->mfx.CodecId = ret; > > + ret = MFXVideoDECODE_DecodeHeader(q->session, &bs, param); > > + if (MFX_ERR_MORE_DATA == ret) { > > + return AVERROR(EAGAIN); > > + } > > + if (ret < 0) > > + return ff_qsv_print_error(avctx, ret, > > + "Error decoding stream header"); > > > > return 0; > > } > > @@ -494,7 +517,11 @@ int ff_qsv_process_data(AVCodecContext *avctx, > > QSVContext *q, > > uint8_t *dummy_data; > > int dummy_size; > > int ret; > > - const AVPixFmtDescriptor *desc; > > + mfxVideoParam param = { 0 }; > > + static enum AVPixelFormat pix_fmts[3] = { AV_PIX_FMT_QSV, > > > + AV_PI > X_FMT_NV12, > > > + AV_PI > X_FMT_NONE }; > > + > > I am surprised to see 'static'. Why? > > > > > if (!q->avctx_internal) { > > q->avctx_internal = avcodec_alloc_context3(NULL); @@ > -521,15 > > +548,13 @@ int ff_qsv_process_data(AVCodecContext *avctx, > QSVContext > > *q, > > pkt->data, pkt->size, pkt->pts, pkt->dts, > > pkt->pos); > > > > - avctx->field_order = q->parser->field_order; > > /* TODO: flush delayed frames on reinit */ > > - if (q->parser->format != q->orig_pix_fmt || > > - FFALIGN(q->parser->coded_width, 16) != FFALIGN(avctx- > > >coded_width, 16) || > > - FFALIGN(q->parser->coded_height, 16) != FFALIGN(avctx- > > >coded_height, 16)) { > > - enum AVPixelFormat pix_fmts[3] = { AV_PIX_FMT_QSV, > > > - AV_PIX_F > MT_NONE, > > > - AV_PIX_F > MT_NONE }; > > - enum AVPixelFormat qsv_format; > > + > > + ret = qsv_decode_header(avctx, q, pkt, pix_fmts, ¶m); > > + > > + if (ret >= 0 && (q->orig_pix_fmt != > > ff_qsv_map_fourcc(param.mfx.FrameInfo.FourCC) || > > + avctx->coded_width != param.mfx.FrameInfo.Width || > > + avctx->coded_height != param.mfx.FrameInfo.Height)) { > > AVPacket zero_pkt = {0}; > > > > if (q->buffered_count) { > > @@ -538,45 +563,23 @@ int ff_qsv_process_data(AVCodecContext > *avctx, > > QSVContext *q, > > q->buffered_count--; > > return qsv_decode(avctx, q, frame, got_frame, > &zero_pkt); > > } > > - > > q->reinit_flag = 0; > > > > - qsv_format = ff_qsv_map_pixfmt(q->parser->format, &q- > > >fourcc); > > - if (qsv_format < 0) { > > - av_log(avctx, AV_LOG_ERROR, > > - "Decoding pixel format '%s' is not > supported\n", > > - av_get_pix_fmt_name(q->parser->format)); > > - ret = AVERROR(ENOSYS); > > - goto reinit_fail; > > - } > > - > > - q->orig_pix_fmt = q->parser->format; > > - avctx->pix_fmt = pix_fmts[1] = qsv_format; > > - avctx->width = q->parser->width; > > - avctx->height = q->parser->height; > > - avctx->coded_width = FFALIGN(q->parser->coded_width, > 16); > > - avctx->coded_height = FFALIGN(q->parser->coded_height, > 16); > > - avctx->level = q->avctx_internal->level; > > - avctx->profile = q->avctx_internal->profile; > > - > > - ret = ff_get_format(avctx, pix_fmts); > > + avctx->pix_fmt = pix_fmts[1] = > > ff_qsv_map_fourcc(param.mfx.FrameInfo.FourCC); > > Here you modify pix_fmts which is declared static. If someone will use QSV > decoder few times in the same application in parallel he might experience a > race condition, isn't it? Good question. Declare it as static is just for avoiding unnecessary reinit a session. Take hevc 10bits clips as example, we assume pix_fmts[1] is NV12 and then init a session and decode_header, after that, we know it is not NV12 but is P010, Then we need to reinit a session. If it is static, this reinit action is only needed for the first decoding call, else we need to reinit for every frame. However, you gave me a good remind to give another solution: remove the static keywords, and set pix_fmts[1] = q->orig_pix_fmt (and relative code should be refined too). > > + avctx->coded_width = param.mfx.FrameInfo.Width; > > + avctx->coded_height = param.mfx.FrameInfo.Height; > > + > > + ret = qsv_decode_preinit(avctx, q, pix_fmts, ¶m); > > if (ret < 0) > > goto reinit_fail; > > + q->initialized = 0; > > + } > > > > - avctx->pix_fmt = ret; > > - > > - desc = av_pix_fmt_desc_get(avctx->pix_fmt); > > - if (!desc) > > - goto reinit_fail; > > - > > - if (desc->comp[0].depth > 8) { > > - avctx->coded_width > = FFALIGN(q->parser->coded_width, > > 32); > > - avctx->coded_height = > FFALIGN(q->parser->coded_height, > > 32); > > - } > > - > > - ret = qsv_decode_init(avctx, q); > > + if (!q->initialized) { > > + ret = qsv_decode_init(avctx, q, ¶m); > > if (ret < 0) > > goto reinit_fail; > > + q->initialized = 1; > > } > > > > return qsv_decode(avctx, q, frame, got_frame, pkt); @@ -589,4 > > +592,5 @@ reinit_fail: > > void ff_qsv_decode_flush(AVCodecContext *avctx, QSVContext *q) > > { > > q->orig_pix_fmt = AV_PIX_FMT_NONE; > > + q->initialized = 0; > > } > > diff --git a/libavcodec/qsvdec.h b/libavcodec/qsvdec.h index > > 111536caba..4812fb2a6b 100644 > > --- a/libavcodec/qsvdec.h > > +++ b/libavcodec/qsvdec.h > > @@ -63,6 +63,8 @@ typedef struct QSVContext { > > uint32_t fourcc; > > mfxFrameInfo frame_info; > > > > + int initialized; > > + > > // options set by the caller > > int async_depth; > > int iopattern; _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel