| Message ID | 1540470971-2838-4-git-send-email-zhong.li@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Oct 25, 2018 at 20:36:10 +0800, Zhong Li wrote: > RepeatPPS is enabled by default in mfx. It is not necessary mostly and > wasting encoding bits. > Add an option to control it and disable it by default. Could this change in behavior surprise anyone? If it's okay to change, perhaps at least bump lavc's micro version? Moritz
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Moritz Barsnick > Sent: Friday, October 26, 2018 7:49 PM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to set > h264 pps for every frame > > On Thu, Oct 25, 2018 at 20:36:10 +0800, Zhong Li wrote: > > RepeatPPS is enabled by default in mfx. It is not necessary mostly and > > wasting encoding bits. > > Add an option to control it and disable it by default. > > Could this change in behavior surprise anyone? If it's okay to change, > perhaps at least bump lavc's micro version? > > Moritz I doubt how many people would like to repeat every frame's pps. Most codecs (x264/x265, and nvenc) won't do that by default. Bumping lavc micro version should be a good suggestion.
On 30/10/18 09:21, Li, Zhong wrote: >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Moritz Barsnick >> Sent: Friday, October 26, 2018 7:49 PM >> To: FFmpeg development discussions and patches >> <ffmpeg-devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to set >> h264 pps for every frame >> >> On Thu, Oct 25, 2018 at 20:36:10 +0800, Zhong Li wrote: >>> RepeatPPS is enabled by default in mfx. It is not necessary mostly and >>> wasting encoding bits. >>> Add an option to control it and disable it by default. >> >> Could this change in behavior surprise anyone? If it's okay to change, >> perhaps at least bump lavc's micro version? >> >> Moritz > > I doubt how many people would like to repeat every frame's pps. > Most codecs (x264/x265, and nvenc) won't do that by default. Huh, right. I feel like I must be missing something here - why would anyone ever want this option enabled? It doesn't even help with seeking to arbitrary points in streams without global headers because you don't have the SPS as well. If there isn't some hidden reason for this, it sounds fine to just switch it off completely without an option to reenable. Thanks, - Mark
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Wednesday, October 31, 2018 8:15 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to set > h264 pps for every frame > > On 30/10/18 09:21, Li, Zhong wrote: > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > >> Of Moritz Barsnick > >> Sent: Friday, October 26, 2018 7:49 PM > >> To: FFmpeg development discussions and patches > >> <ffmpeg-devel@ffmpeg.org> > >> Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to > >> set > >> h264 pps for every frame > >> > >> On Thu, Oct 25, 2018 at 20:36:10 +0800, Zhong Li wrote: > >>> RepeatPPS is enabled by default in mfx. It is not necessary mostly > >>> and wasting encoding bits. > >>> Add an option to control it and disable it by default. > >> > >> Could this change in behavior surprise anyone? If it's okay to > >> change, perhaps at least bump lavc's micro version? > >> > >> Moritz > > > > I doubt how many people would like to repeat every frame's pps. > > Most codecs (x264/x265, and nvenc) won't do that by default. > > Huh, right. I feel like I must be missing something here - why would anyone > ever want this option enabled? It doesn't even help with seeking to > arbitrary points in streams without global headers because you don't have > the SPS as well. > > If there isn't some hidden reason for this, it sounds fine to just switch it off > completely without an option to reenable. Two reasons I prefer to have an option to disable it: 1. Keep compatibility with previous behavior though the default case has been changed. Since someone is surprised by current change as previous comment, would be a bigger surprise if totally disable it? 2. I am not very good at video streaming, but I guess in some rare cases of streaming, repeating pps maybe helpful (pps and sps can be transferred separated. Some pps are dropped won't cause the whole GOP can't decodable). It is possible?
On 31/10/18 11:00, Li, Zhong wrote: >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Mark Thompson >> Sent: Wednesday, October 31, 2018 8:15 AM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to set >> h264 pps for every frame >> >> On 30/10/18 09:21, Li, Zhong wrote: >>>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On >> Behalf >>>> Of Moritz Barsnick >>>> Sent: Friday, October 26, 2018 7:49 PM >>>> To: FFmpeg development discussions and patches >>>> <ffmpeg-devel@ffmpeg.org> >>>> Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to >>>> set >>>> h264 pps for every frame >>>> >>>> On Thu, Oct 25, 2018 at 20:36:10 +0800, Zhong Li wrote: >>>>> RepeatPPS is enabled by default in mfx. It is not necessary mostly >>>>> and wasting encoding bits. >>>>> Add an option to control it and disable it by default. >>>> >>>> Could this change in behavior surprise anyone? If it's okay to >>>> change, perhaps at least bump lavc's micro version? >>>> >>>> Moritz >>> >>> I doubt how many people would like to repeat every frame's pps. >>> Most codecs (x264/x265, and nvenc) won't do that by default. >> >> Huh, right. I feel like I must be missing something here - why would anyone >> ever want this option enabled? It doesn't even help with seeking to >> arbitrary points in streams without global headers because you don't have >> the SPS as well. >> >> If there isn't some hidden reason for this, it sounds fine to just switch it off >> completely without an option to reenable. > > Two reasons I prefer to have an option to disable it: > 1. Keep compatibility with previous behavior though the default case has been changed. > Since someone is surprised by current change as previous comment, would be a bigger surprise if totally disable it? > 2. I am not very good at video streaming, but I guess in some rare cases of streaming, repeating pps maybe helpful > (pps and sps can be transferred separated. Some pps are dropped won't cause the whole GOP can't decodable). > It is possible? I can't think of any case where this would make sense, since the extradata cases always carry the two parameter set types together. (Though maybe someone else can think of one?) Anyway, the compatibility argument is reasonable so I won't argue further. > RepeatPPS is enabled by default in mfx. It is not necessary mostly and > wasting encoding bits. > Add an option to control it and disable it by default. > > Signed-off-by: Zhong Li <zhong.li@intel.com> > --- > libavcodec/qsvenc.c | 5 ++--- > libavcodec/qsvenc.h | 2 ++ > libavcodec/qsvenc_h264.c | 2 ++ > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c > index ad432fc..fffca65 100644 > --- a/libavcodec/qsvenc.c > +++ b/libavcodec/qsvenc.c > @@ -648,11 +648,10 @@ FF_ENABLE_DEPRECATION_WARNINGS > q->extco2.Trellis = q->trellis; > #endif > > -#if QSV_HAVE_LA_DS > +#if QSV_VERSION_ATLEAST(1, 8) > q->extco2.LookAheadDS = q->look_ahead_downsampling; > -#endif > + q->extco2.RepeatPPS = q->repeat_pps ? MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF; > > -#if QSV_HAVE_BREF_TYPE While the #if changes look technically correct (they all resolve to the 1.8 version), changing the unrelated conditions doesn't feel like it should be done in this patch. If you want to clean that up then maybe a separate patch? > #if FF_API_PRIVATE_OPT > FF_DISABLE_DEPRECATION_WARNINGS > if (avctx->b_frame_strategy >= 0) > diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h > index 1f97f77..9aebc2a 100644 > --- a/libavcodec/qsvenc.h > +++ b/libavcodec/qsvenc.h > @@ -162,6 +162,8 @@ typedef struct QSVEncContext { > int int_ref_qp_delta; > int recovery_point_sei; > > + int repeat_pps; > + > int a53_cc; > > #if QSV_HAVE_MF > diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c > index ac7023e..893a737 100644 > --- a/libavcodec/qsvenc_h264.c > +++ b/libavcodec/qsvenc_h264.c > @@ -155,6 +155,8 @@ static const AVOption options[] = { > { "auto" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_MF_AUTO }, INT_MIN, INT_MAX, VE, "mfmode" }, > #endif > > + { "repeat_pps", "repeat pps for every frame", OFFSET(qsv.repeat_pps), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE }, Should be AV_OPT_TYPE_BOOL. > + > { NULL }, > }; > > -- > 2.7.4 > Thanks, - Mark
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Thursday, November 1, 2018 6:30 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to set > h264 pps for every frame > > On 31/10/18 11:00, Li, Zhong wrote: > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > >> Of Mark Thompson > >> Sent: Wednesday, October 31, 2018 8:15 AM > >> To: ffmpeg-devel@ffmpeg.org > >> Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to > >> set > >> h264 pps for every frame > >> > >> On 30/10/18 09:21, Li, Zhong wrote: > >>>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > >> Behalf > >>>> Of Moritz Barsnick > >>>> Sent: Friday, October 26, 2018 7:49 PM > >>>> To: FFmpeg development discussions and patches > >>>> <ffmpeg-devel@ffmpeg.org> > >>>> Subject: Re: [FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option > >>>> to set > >>>> h264 pps for every frame > >>>> > >>>> On Thu, Oct 25, 2018 at 20:36:10 +0800, Zhong Li wrote: > >>>>> RepeatPPS is enabled by default in mfx. It is not necessary mostly > >>>>> and wasting encoding bits. > >>>>> Add an option to control it and disable it by default. > >>>> > >>>> Could this change in behavior surprise anyone? If it's okay to > >>>> change, perhaps at least bump lavc's micro version? > >>>> > >>>> Moritz > >>> > >>> I doubt how many people would like to repeat every frame's pps. > >>> Most codecs (x264/x265, and nvenc) won't do that by default. > >> > >> Huh, right. I feel like I must be missing something here - why would > >> anyone ever want this option enabled? It doesn't even help with > >> seeking to arbitrary points in streams without global headers because > >> you don't have the SPS as well. > >> > >> If there isn't some hidden reason for this, it sounds fine to just > >> switch it off completely without an option to reenable. > > > > Two reasons I prefer to have an option to disable it: > > 1. Keep compatibility with previous behavior though the default case has > been changed. > > Since someone is surprised by current change as previous comment, > would be a bigger surprise if totally disable it? > > 2. I am not very good at video streaming, but I guess in some rare cases of > streaming, repeating pps maybe helpful > > (pps and sps can be transferred separated. Some pps are dropped won't > cause the whole GOP can't decodable). > > It is possible? > > I can't think of any case where this would make sense, since the extradata > cases always carry the two parameter set types together. (Though maybe > someone else can think of one?) > > Anyway, the compatibility argument is reasonable so I won't argue further. > > > > RepeatPPS is enabled by default in mfx. It is not necessary mostly and > > wasting encoding bits. > > Add an option to control it and disable it by default. > > > > Signed-off-by: Zhong Li <zhong.li@intel.com> > > --- > > libavcodec/qsvenc.c | 5 ++--- > > libavcodec/qsvenc.h | 2 ++ > > libavcodec/qsvenc_h264.c | 2 ++ > > 3 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index > > ad432fc..fffca65 100644 > > --- a/libavcodec/qsvenc.c > > +++ b/libavcodec/qsvenc.c > > @@ -648,11 +648,10 @@ FF_ENABLE_DEPRECATION_WARNINGS > > q->extco2.Trellis = q->trellis; #endif > > > > -#if QSV_HAVE_LA_DS > > +#if QSV_VERSION_ATLEAST(1, 8) > > q->extco2.LookAheadDS = > q->look_ahead_downsampling; > > -#endif > > + q->extco2.RepeatPPS = q->repeat_pps ? > MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF; > > > > -#if QSV_HAVE_BREF_TYPE > > While the #if changes look technically correct (they all resolve to the 1.8 > version), changing the unrelated conditions doesn't feel like it should be > done in this patch. If you want to clean that up then maybe a separate > patch? Splitting to a separated patch is a good suggestion. Will update. > > > #if FF_API_PRIVATE_OPT > > FF_DISABLE_DEPRECATION_WARNINGS > > if (avctx->b_frame_strategy >= 0) diff --git > > a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index 1f97f77..9aebc2a > > 100644 > > --- a/libavcodec/qsvenc.h > > +++ b/libavcodec/qsvenc.h > > @@ -162,6 +162,8 @@ typedef struct QSVEncContext { > > int int_ref_qp_delta; > > int recovery_point_sei; > > > > + int repeat_pps; > > + > > int a53_cc; > > > > #if QSV_HAVE_MF > > diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c index > > ac7023e..893a737 100644 > > --- a/libavcodec/qsvenc_h264.c > > +++ b/libavcodec/qsvenc_h264.c > > @@ -155,6 +155,8 @@ static const AVOption options[] = { > > { "auto" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = > MFX_MF_AUTO }, INT_MIN, INT_MAX, VE, "mfmode" }, > > #endif > > > > + { "repeat_pps", "repeat pps for every frame", > > + OFFSET(qsv.repeat_pps), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE }, > > Should be AV_OPT_TYPE_BOOL. Yup, will update. Thanks
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index ad432fc..fffca65 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -648,11 +648,10 @@ FF_ENABLE_DEPRECATION_WARNINGS q->extco2.Trellis = q->trellis; #endif -#if QSV_HAVE_LA_DS +#if QSV_VERSION_ATLEAST(1, 8) q->extco2.LookAheadDS = q->look_ahead_downsampling; -#endif + q->extco2.RepeatPPS = q->repeat_pps ? MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF; -#if QSV_HAVE_BREF_TYPE #if FF_API_PRIVATE_OPT FF_DISABLE_DEPRECATION_WARNINGS if (avctx->b_frame_strategy >= 0) diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index 1f97f77..9aebc2a 100644 --- a/libavcodec/qsvenc.h +++ b/libavcodec/qsvenc.h @@ -162,6 +162,8 @@ typedef struct QSVEncContext { int int_ref_qp_delta; int recovery_point_sei; + int repeat_pps; + int a53_cc; #if QSV_HAVE_MF diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c index ac7023e..893a737 100644 --- a/libavcodec/qsvenc_h264.c +++ b/libavcodec/qsvenc_h264.c @@ -155,6 +155,8 @@ static const AVOption options[] = { { "auto" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_MF_AUTO }, INT_MIN, INT_MAX, VE, "mfmode" }, #endif + { "repeat_pps", "repeat pps for every frame", OFFSET(qsv.repeat_pps), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE }, + { NULL }, };
RepeatPPS is enabled by default in mfx. It is not necessary mostly and wasting encoding bits. Add an option to control it and disable it by default. Signed-off-by: Zhong Li <zhong.li@intel.com> --- libavcodec/qsvenc.c | 5 ++--- libavcodec/qsvenc.h | 2 ++ libavcodec/qsvenc_h264.c | 2 ++ 3 files changed, 6 insertions(+), 3 deletions(-)