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? Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
