[FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton

[Li, Zhong]

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Wednesday, October 31, 2018 7:40 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> 
> On 30/10/18 09:49, Li, Zhong wrote:
> >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> >> Of Mark Thompson
> >> Sent: Tuesday, October 30, 2018 5:06 AM
> >> To: ffmpeg-devel at 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.li at 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.
> > 0: respect input gop structure but don't force I frame as IDR frame.
> > 1: force all I frame as IDR frame.
> 
> This sounds like two independent options.  One is the "forced-idr" option
> implemented by several other encoders (notably libx264, which is the most
> commonly-used external encoder), which looks like a sensible addition to me.
> The second is an "ignore user-set pict_type" option, which I don't understand
> the need for at all - it's never set unless the user deliberately wants to use
> that feature (e.g. by using the force_key_frames option in the ffmpeg utility),
> so why would you want to have a special way to override that?
> 
> Thanks,
> 
> - Mark
I may miss something. The default case (forced_idr = -1) is do nothing, ignoring the input I frames. 
Which is same as default case of x264/x265/nvenc forced_idr.  
Vaapi encoder is different from others, there is no forced_idr option but an internal variable named force_idr, always set IDR if the input frame is I frame. (Please correct me if I am wrong)

Compare with x264/x265 forced_idr. I just add this case: "0: respect input gop structure but don't force I frame as IDR frame."