On Tue, 2018-10-30 at 09:49 +0000, Li, Zhong wrote: > > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On > > Behalf > > Of Mark Thompson > > Sent: Tuesday, October 30, 2018 5:06 AM > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr > > opiton > > > > On 25/10/18 13:36, Zhong Li wrote: > > > This option can be used to repect original input I/IDR frame > > > type. > > > > > > Signed-off-by: Zhong Li <zhong...@intel.com> > > > --- > > > libavcodec/qsvenc.c | 7 +++++++ > > > libavcodec/qsvenc.h | 2 ++ > > > 2 files changed, 9 insertions(+) > > > > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index > > > 948751d..e534dcf 100644 > > > --- a/libavcodec/qsvenc.c > > > +++ b/libavcodec/qsvenc.c > > > @@ -1192,6 +1192,13 @@ static int encode_frame(AVCodecContext > > > > *avctx, QSVEncContext *q, > > > if (qsv_frame) { > > > surf = &qsv_frame->surface; > > > enc_ctrl = &qsv_frame->enc_ctrl; > > > + > > > + if (q->forced_idr >= 0 && frame->pict_type == > > > > AV_PICTURE_TYPE_I) { > > > + enc_ctrl->FrameType = MFX_FRAMETYPE_I | > > > > MFX_FRAMETYPE_REF; > > > + if (q->forced_idr || frame->key_frame) > > > + enc_ctrl->FrameType |= MFX_FRAMETYPE_IDR; > > > + } else > > > + enc_ctrl->FrameType = MFX_FRAMETYPE_UNKNOWN; > > > } > > > > > > ret = av_new_packet(&new_pkt, q->packet_size); diff --git > > > a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index > > > 055b4a6..1f97f77 > > > 100644 > > > --- a/libavcodec/qsvenc.h > > > +++ b/libavcodec/qsvenc.h > > > @@ -87,6 +87,7 @@ > > > { "adaptive_i", "Adaptive I-frame placement", > > > > OFFSET(qsv.adaptive_i), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, > > 1, VE }, \ > > > { "adaptive_b", "Adaptive B-frame placement", > > > > OFFSET(qsv.adaptive_b), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, > > 1, VE }, \ > > > { "b_strategy", "Strategy to choose between I/P/B-frames", > > > > OFFSET(qsv.b_strategy), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, > > 1, VE }, \ > > > +{ "forced_idr", "Forcing I frames as IDR frames", > > > > OFFSET(qsv.forced_idr), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, > > 1, VE }, \ > > > > > > typedef int SetEncodeCtrlCB (AVCodecContext *avctx, > > > const AVFrame *frame, > > > > mfxEncodeCtrl* > > > enc_ctrl); @@ -168,6 +169,7 @@ typedef struct QSVEncContext > > > { #endif > > > char *load_plugins; > > > SetEncodeCtrlCB *set_encode_ctrl_cb; > > > + int forced_idr; > > > } QSVEncContext; > > > > > > int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q); > > > > > > > This seems confusing, because it doesn't match what forced_idr does > > in any > > other encoder. > > > > Checking of pict_type for AV_PICTURE_TYPE_I in order to get a key > > frame > > (of whatever kind) is always enabled if supported (many > > encoders). The > > "forced_idr" option to H.26[45] encoders (libx264, libx265) then > > forces that > > to be an IDR frame, not just an I frame. > > > > - Mark > > Yup, I know it doesn’t match other encoders such as > libx264/libx265/nvenc. > However, it is my intentional behavior. I believe current implement > in libx264/libx265 is not complete. > One case is ignored: user just want to keep the gop structure as > input, not to force all I frames as IDR frames. > So I have an idea: > Default value = -1, ignore the input gop structure.
I see the idea now, but I very much would like to see this at least somehow reflected in ffmpeg option documentation. To me your interpretation of -1, 0, 1 is quite non-intuitive. As you can see from my other comments I did not guess your intent from the patch. End-users will have even more problems with this option since a little of them will take care to look into the sources themselves. Besides, I am still feeling that you try to mix 2 different options together: one which will permit to follow (or not) input stream gop structure and another which forces IDR frames for each I frame. > 0: respect input gop structure but don't force I frame as IDR frame. > 1: force all I frame as IDR frame. > > Since this is a qsv encoder private option, I just changed qsv > implementation. > But I believe it should be a good to make other encoders follow such > a way with separated patches. > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
