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

[Mark Thompson]

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?

>  #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