Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
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
>>>>> ---
>>>>> 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 = _frame->surface;
>>>>> enc_ctrl = _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(_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 fra
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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 +, 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
> > > > ---
> > > > 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 = _frame->surface;
> > > > enc_ctrl = _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(_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
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> 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
> >>> ---
> >>> 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 = _frame->surface;
> >>> enc_ctrl = _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(_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
&g
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> From: Rogozhkin, Dmitry V > Sent: Wednesday, October 31, 2018 2:07 AM > To: Li, Zhong ; 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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
>>> ---
>>> 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 = _frame->surface;
>>> enc_ctrl = _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(_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
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
On Tue, 2018-10-30 at 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
> > > ---
> > > 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 = _frame->surface;
> > > enc_ctrl = _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(_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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> From: Rogozhkin, Dmitry V
> Sent: Tuesday, October 30, 2018 5:07 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Li, Zhong
> 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
> > ---
> > 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 = _frame->surface;
> > enc_ctrl = _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(_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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of Moritz Barsnick
> Sent: Friday, October 26, 2018 7:46 PM
> To: FFmpeg development discussions and patches
>
> 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> 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
> > ---
> > 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 = _frame->surface;
> > enc_ctrl = _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(_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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
On Mon, 2018-10-29 at 21:34 +, Mark Thompson wrote:
> On 29/10/18 21:29, Rogozhkin, Dmitry V wrote:
> > On Mon, 2018-10-29 at 21:06 +, 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
> > > > ---
> > > > 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 = _frame->surface;
> > > > enc_ctrl = _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(_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.
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
On 29/10/18 21:29, Rogozhkin, Dmitry V wrote:
> On Mon, 2018-10-29 at 21:06 +, 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
>>> ---
>>> 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 = _frame->surface;
>>> enc_ctrl = _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(_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.
).
"-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
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
On Mon, 2018-10-29 at 21:06 +, 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
> > ---
> > 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 = _frame->surface;
> > enc_ctrl = _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(_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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
On Mon, 2018-10-29 at 20:54 +, 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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
> ---
> 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 = _frame->surface;
> enc_ctrl = _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(_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);
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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
> ---
> 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 = _frame->surface;
> enc_ctrl = _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(_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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
