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

[Rogozhkin, Dmitry V]

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.
>      }
>  
>      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? 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);