[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 Rogozhkin, Dmitry V
> Sent: Wednesday, October 31, 2018 2:20 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> 
> On Tue, 2018-10-30 at 09:49 +0000, 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.
> 
> 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.

Yup, that is a good point. Currently qsv options haven't been well-documented and well-organized.
(See https://www.ffmpeg.org/ffmpeg-codecs.html#QSV-encoders ), thus should be refined and then new options documentation can be easily added.

> 
> 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.

Let's continue this part in the discussion mail with Mark and then make a decision. 

> 
> > 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.