[FFmpeg-devel] [PATCH v2 3/6] lavc/qsvdec: Replace current parser with MFXVideoDECODE_DecodeHeader()
Li, Zhong
zhong.li at intel.com
Thu Feb 21 11:03:31 EET 2019
> From: Rogozhkin, Dmitry V
> Sent: Thursday, February 21, 2019 3:14 AM
> To: ffmpeg-devel at ffmpeg.org
> Cc: Li, Zhong <zhong.li at 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.li at 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;
More information about the ffmpeg-devel
mailing list