| Message ID | 1540470971-2838-1-git-send-email-zhong.li@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Oct 25, 2018 at 20:36:07 +0800, Zhong Li wrote:
> +{ "forced_idr", "Forcing I frames as IDR frames", OFFSET(qsv.forced_idr), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 1, VE }, \
ffmpeg uses imperative (mostly): "Force I frames as IDR frames".
Moritz
On Fri, 2018-10-26 at 13:46 +0200, Moritz Barsnick wrote: > On Thu, Oct 25, 2018 at 20:36:07 +0800, Zhong Li wrote: > > +{ "forced_idr", "Forcing I frames as IDR > > frames", OFFSET(qsv.forced_idr), AV_OPT_TYPE_INT, { > > .i64 = -1 }, -1, 1, VE }, \ > > ffmpeg uses imperative (mostly): "Force I frames as IDR frames". > Should not the option be named 'force_idr' as well? It makes better sense to me in that way... > Moritz > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 29/10/2018 20:51, Rogozhkin, Dmitry V wrote: > Should not the option be named 'force_idr' as well? It makes better > sense to me in that way... That would be inconsistent with the rest of the options for various encoders in FFmpeg, all named forced_idr. - Derek
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@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
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@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);
On Mon, 2018-10-29 at 20:54 +0000, Derek Buitenhuis wrote: > On 29/10/2018 20:51, Rogozhkin, Dmitry V wrote: > > Should not the option be named 'force_idr' as well? It makes better > > sense to me in that way... > > That would be inconsistent with the rest of the options for various > encoders > in FFmpeg, all named forced_idr. Ok. Better to be consistent with the option names across components. I have overlooked this in the help though I have tried to check. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, 2018-10-29 at 21:06 +0000, Mark Thompson wrote: > 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@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. Is there an option to disable/override this behavior? Will "- force_key_frames" do the trick? > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 29/10/18 21:29, Rogozhkin, Dmitry V wrote: > On Mon, 2018-10-29 at 21:06 +0000, Mark Thompson wrote: >> 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@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. > > Is there an option to disable/override this behavior? Will "- > force_key_frames" do the trick? Not in libavcodec; encoders always looks at pict_type (e.g. <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/libx264.c;h=d6367bf557953ec4e91d057d551848b3b2ba9d36;hb=HEAD#l300>). "-force_key_frames" is an option to the ffmpeg utility which sets pict_type dependent on some expression, making use of the above behaviour. - Mark
On Mon, 2018-10-29 at 21:34 +0000, Mark Thompson wrote: > On 29/10/18 21:29, Rogozhkin, Dmitry V wrote: > > On Mon, 2018-10-29 at 21:06 +0000, Mark Thompson wrote: > > > 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@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. > > > > Is there an option to disable/override this behavior? Will "- > > force_key_frames" do the trick? > > Not in libavcodec; encoders always looks at pict_type (e.g. > <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/libx264.c; > h=d6367bf557953ec4e91d057d551848b3b2ba9d36;hb=HEAD#l300>). Where gop structure is being set in SW encoders? I mean, is pict_type set by infrastructure according to -g and -bf options + some information on the incoming stream (passing thru I frames probably?) or pict_type is in a way recommendation to follow, but general gop structure is calculated/maintained inside the encoder? > > "-force_key_frames" is an option to the ffmpeg utility which sets > pict_type dependent on some expression, making use of the above > behaviour. > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.li@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. Since this is a qsv encoder private option, I just changed qsv implementation. But I believe it should be a good to make other encoders follow such a way with separated patches.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Moritz Barsnick > Sent: Friday, October 26, 2018 7:46 PM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton > > On Thu, Oct 25, 2018 at 20:36:07 +0800, Zhong Li wrote: > > +{ "forced_idr", "Forcing I frames as IDR frames", > OFFSET(qsv.forced_idr), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, > 1, VE }, \ > > ffmpeg uses imperative (mostly): "Force I frames as IDR frames". > > Moritz Agree. Will update.
> From: Rogozhkin, Dmitry V > Sent: Tuesday, October 30, 2018 5:07 AM > To: ffmpeg-devel@ffmpeg.org > Cc: Li, Zhong <zhong.li@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton > > 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@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. This was a case I had concern. I doubt the default initialization is always zero (you know MFX_FRAMETYPE_UNKNOWN is zero). Isn't it possible? Please check the regression case I fixed: https://patchwork.ffmpeg.org/patch/10517/ > > } > > > > 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? Please check my reply to Mark. And grep forced_idr implementation in nvenc. >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); I assume the reset of your comments have been replied by others. No?
On Tue, 2018-10-30 at 18:05 +0800, Li, Zhong wrote: > > > + } 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. > > This was a case I had concern. I doubt the default initialization is > always zero > (you know MFX_FRAMETYPE_UNKNOWN is zero). Isn't it possible? > Please check the regression case I fixed: https://patchwork.ffmpeg.or > g/patch/10517/ Patch 10517 deals with unitialized variable on a compilation level. As for the enc_ctrl I very much hope that ffmpeg-qsv code takes care to memset the mfcEncodeCtrl (as well as all other mediasdk structures) _before_ usage. I.e. there should be a code somewhere similar to: memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl)); If this is missing, there is a VERY big problem in the QSV code since indeed compiler may initialize structures to everything it wants and there will be very bad consequences. As for the usage of mfxEncodeCtrl, the idea is the following. User allocates this control and memsets it to 0. If this will be passed in that way mediasdk will change nothing to encode the frame. If user wants to change some parameter, it can do this changing only the parameter it wants to take effect. So, the "else" should really not be needed. Dmitry.
On Tue, 2018-10-30 at 09:49 +0000, Li, Zhong wrote: > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.li@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. I see the idea now, but I very much would like to see this at least somehow reflected in ffmpeg option documentation. To me your interpretation of -1, 0, 1 is quite non-intuitive. As you can see from my other comments I did not guess your intent from the patch. End-users will have even more problems with this option since a little of them will take care to look into the sources themselves. Besides, I am still feeling that you try to mix 2 different options together: one which will permit to follow (or not) input stream gop structure and another which forces IDR frames for each I frame. > 0: respect input gop structure but don't force I frame as IDR frame. > 1: force all I frame as IDR frame. > > Since this is a qsv encoder private option, I just changed qsv > implementation. > But I believe it should be a good to make other encoders follow such > a way with separated patches. > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 30/10/18 09:49, Li, Zhong wrote: >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.li@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
> From: Rogozhkin, Dmitry V > Sent: Wednesday, October 31, 2018 2:07 AM > To: Li, Zhong <zhong.li@intel.com>; ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton > > On Tue, 2018-10-30 at 18:05 +0800, Li, Zhong wrote: > > > > + } 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. > > > > This was a case I had concern. I doubt the default initialization is > > always zero (you know MFX_FRAMETYPE_UNKNOWN is zero). Isn't it > > possible? > > Please check the regression case I fixed: https://patchwork.ffmpeg.or > > g/patch/10517/ > > Patch 10517 deals with unitialized variable on a compilation level. As for the > enc_ctrl I very much hope that ffmpeg-qsv code takes care to memset the > mfcEncodeCtrl (as well as all other mediasdk structures) _before_ usage. I.e. > there should be a code somewhere similar to: > memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl)); If this is missing, there is a > VERY big problem in the QSV code since indeed compiler may initialize > structures to everything it wants and there will be very bad consequences. > > As for the usage of mfxEncodeCtrl, the idea is the following. User allocates > this control and memsets it to 0. If this will be passed in that way mediasdk > will change nothing to encode the frame. If user wants to change some > parameter, it can do this changing only the parameter it wants to take effect. > So, the "else" should really not be needed. > > Dmitry. Yup, we are on the same page now to avoid uninitialized value. Memset is a good idea, will update.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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-bounces@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.li@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 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. 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) Compare with x264/x265 forced_idr. I just add this case: "0: respect input gop structure but don't force I frame as IDR frame."
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Rogozhkin, Dmitry V > Sent: Wednesday, October 31, 2018 2:20 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton > > On Tue, 2018-10-30 at 09:49 +0000, Li, Zhong wrote: > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.li@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. > > I see the idea now, but I very much would like to see this at least somehow > reflected in ffmpeg option documentation. To me your interpretation of -1, 0, > 1 is quite non-intuitive. As you can see from my other comments I did not > guess your intent from the patch. End-users will have even more problems > with this option since a little of them will take care to look into the sources > themselves. Yup, that is a good point. Currently qsv options haven't been well-documented and well-organized. (See https://www.ffmpeg.org/ffmpeg-codecs.html#QSV-encoders ), thus should be refined and then new options documentation can be easily added. > > Besides, I am still feeling that you try to mix 2 different options > together: one which will permit to follow (or not) input stream gop structure > and another which forces IDR frames for each I frame. Let's continue this part in the discussion mail with Mark and then make a decision. > > > 0: respect input gop structure but don't force I frame as IDR frame. > > 1: force all I frame as IDR frame. > > > > Since this is a qsv encoder private option, I just changed qsv > > implementation. > > But I believe it should be a good to make other encoders follow such a > > way with separated patches.
On 31/10/18 11:29, Li, Zhong wrote: >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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-bounces@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.li@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
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 option can be used to repect original input I/IDR frame type. Signed-off-by: Zhong Li <zhong.li@intel.com> --- libavcodec/qsvenc.c | 7 +++++++ libavcodec/qsvenc.h | 2 ++ 2 files changed, 9 insertions(+)