> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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-boun...@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-boun...@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...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
