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

[Li, Zhong]

> From: Rogozhkin, Dmitry V
> Sent: Tuesday, October 30, 2018 5:07 AM
> To: ffmpeg-devel at ffmpeg.org
> Cc: Li, Zhong <zhong.li at intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> 
> On Thu, 2018-10-25 at 20:36 +0800, 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;
> 
> Hm. There is another option "-force_key_frames" which looks unhandled
> here. At least I don't understand "|| frame->key_frame"...
> 
> 
> > +        } else
> > +            enc_ctrl->FrameType = MFX_FRAMETYPE_UNKNOWN;
> 
> "else" block don't make much sense to me. You eventually already had
> enc_ctrl structure passed to the encoder. Thus, it should be initialized to
> default (already). And you don't make anything specific/new in the "else".
> From my perspective "else" just obscures the code and should be dropped.

This was a case I had concern. I doubt the default initialization is always zero 
(you know MFX_FRAMETYPE_UNKNOWN is zero). Isn't it possible? 
Please check the regression case I fixed: https://patchwork.ffmpeg.org/patch/10517/ 

> >      }
> >
> >      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 },                         \
> 
> As pointed out in other mail, I think this should be "force_idr" option and
> "Force I frames as IDR frames" as an option description. Secondly, why
> there are 3 values accepted: -1, 0, 1? I can understand 1 as enable the
> feature and 0 as don't enable, but what is -1? 

Please check my reply to Mark. And grep forced_idr implementation in nvenc.

>Also, how the option correlates with "-force_key_frames"?
> 
> From this perspective shouldn't the code be:
> 
> { "force_idr", "Force I frames as IDR
> frames", OFFSET(qsv.force_idr), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE },
> 
> if (frame->pict_type == AV_PICTURE_TYPE_I && (frame->key_frame || q-
> >force_idr)) {
>    enc_ctrl->FrameType = MFX_FRAMETYPE_I | MFX_FRAMETYPE_REF;
>    if (q->force_idr)
>       enc_ctrl->FrameType |= MFX_FRAMETYPE_IDR; }
> 
> This assumes that frame->key_frame corresponds to "-force_key_frames"
> in which I am not quite sure...
> 
> >
> >  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;
> 
> int force_idr;
> 
> if agreed on the above...
> 
> >  } QSVEncContext;
> >
> >  int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q);

I assume the reset of your comments have been replied by others. No?