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

[Mark Thompson]

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