[FFmpeg-devel] [PATCH 4/5] lavc/qsvenc: add an option to set h264 pps for every frame

[Li, Zhong]

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Thursday, November 1, 2018 6:30 AM
> To: ffmpeg-devel at 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 at ffmpeg.org] On
> Behalf
> >> Of Mark Thompson
> >> Sent: Wednesday, October 31, 2018 8:15 AM
> >> To: ffmpeg-devel at 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 at ffmpeg.org] On
> >> Behalf
> >>>> Of Moritz Barsnick
> >>>> Sent: Friday, October 26, 2018 7:49 PM
> >>>> To: FFmpeg development discussions and patches
> >>>> <ffmpeg-devel at 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 at 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