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

[Mark Thompson]

On 31/10/18 11:29, Li, Zhong wrote:
>> 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?
> 
> 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.  

No it isn't.

>From libavcodec/libx264.c:

> static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
>                       int *got_packet)
> {
>     ...
>     if (frame) {
>         ...
>         switch (frame->pict_type) {
>         case AV_PICTURE_TYPE_I:
>             x4->pic.i_type = x4->forced_idr > 0 ? X264_TYPE_IDR
>                                                 : X264_TYPE_KEYFRAME;
>             break;
>         case AV_PICTURE_TYPE_P:
>             x4->pic.i_type = X264_TYPE_P;
>             break;
>         case AV_PICTURE_TYPE_B:
>             x4->pic.i_type = X264_TYPE_B;
>             break;
>         default:
>             x4->pic.i_type = X264_TYPE_AUTO;
>             break;
>         }

The input pict_type is always used, and forced_idr indicates that a forced intra frame must be 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)

Yeah, VAAPI just supports the force-frame feature in the simplest possible way, giving you an IDR frame (well, or codec-dependent equivalent - KEY for VP9, etc.) if you ask for anything intra.

Thanks,

- Mark