On 31/10/18 11:29, Li, Zhong wrote:
>> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
>> Of Mark Thompson
>> Sent: Wednesday, October 31, 2018 7:40 AM
>> To: ffmpeg-devel@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-boun...@ffmpeg.org] On
>> Behalf
>>>> Of Mark Thompson
>>>> Sent: Tuesday, October 30, 2018 5:06 AM
>>>> To: ffmpeg-devel@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...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
