[FFmpeg-devel] [PATCH] libavcodec: QSV protect GPB code with CO3 define

[Li, Zhong]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Andreas Håkon
> Sent: Monday, April 22, 2019 8:21 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] libavcodec: QSV protect GPB code with
> CO3 define
> 
> 
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, 22 de April de 2019 13:33, Li, Zhong <zhong.li at intel.com>
> wrote:
> 
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > > Behalf Of Andreas Håkon via ffmpeg-devel
> > > Sent: Thursday, April 18, 2019 6:11 PM
> > > To: FFmpeg development discussions and patches
> > > ffmpeg-devel at ffmpeg.org
> > > Cc: Andreas Håkon andreas.hakon at protonmail.com
> > > Subject: [FFmpeg-devel] [PATCH] libavcodec: QSV protect GPB code
> > > with
> > > CO3 define
> > > Hi,
> > > In response to this ticket I provide the first part of the patch:
> > > https://trac.ffmpeg.org/ticket/7839
> > > This QSV_HAVE_GPB code needs to be protected by QSV_HAVE_CO3.
> > > Regards.
> > > A.H.
> >
> > Why it is must? It is impossible that QSV_HAVE_GPB is enabled but
> QSV_HAVE_CO3 is not enabled.
> > QSV_HAVE_GPB is enabled by MSDK API v1.18, but QSV_HAVE_CO3 is API
> V1.11.
> >
> 
> Hi Li,
> 
> > Why it is must?
> 
> Let me to explain why:
> 
> - The "#if QSV_HAVE_GPB" only appears two times inside
> "/libavcodec/qsvenc.c":
>   1.
> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/qsvenc.c#L75
> 6
>   2.
> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/qsvenc.c#L27
> 0
> 
> In the first occurrence (line #756) the code is protected by one "#if
> QSV_HAVE_CO3". This is necessary because if QSV_HAVE_CO3 is not enabled
> then the code fails when using "extco3.GPB". The original author (which I
> think was you) included the test with sound common sense.

I belive they are different. If you extend the MARCIO, they are actually:

Case 1:
#if QSV_VERSION_ATLEAST(1, 11)
        q->extco3.Header.BufferId      = MFX_EXTBUFF_CODING_OPTION3;
        q->extco3.Header.BufferSz      = sizeof(q->extco3);
#if QSV_VERSION_ATLEAST(1, 18)
        if (avctx->codec_id == AV_CODEC_ID_HEVC)
            q->extco3.GPB              = q->gpb ? MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF;
#endif
        q->extparam_internal[q->nb_extparam_internal++] = (mfxExtBuffer *)&q->extco3;
#endif

And Case2:

#if QSV_VERSION_ATLEAST(1, 18)
    if (avctx->codec_id == AV_CODEC_ID_HEVC)
        av_log(avctx, AV_LOG_VERBOSE,"GPB: %s\n", print_threestate(co3->GPB));
#endif

You must add "#if QSV_VERSION_ATLEAST(1, 11)"  for case 1, else " q->extco3.Header.BufferId = MFX_EXTBUFF_CODING_OPTION3;" is broken.
But you don't need change Case 2 to be:
#if QSV_VERSION_ATLEAST(1, 11)
#if QSV_VERSION_ATLEAST(1, 18)
    if (avctx->codec_id == AV_CODEC_ID_HEVC)
        av_log(avctx, AV_LOG_VERBOSE,"GPB: %s\n", print_threestate(co3->GPB));
#endif
#endif

> In the second occurrence (line #270) where my patch is applied, the code
> isn't protected. The reason is because this code is changed **after**. And
> the developer forgot to protect it.
> 
> So my patch is a simple fixing.
> 
> > It is impossible that QSV_HAVE_GPB is enabled but QSV_HAVE_CO3 is not
> enabled.
> > QSV_HAVE_GPB is enabled by MSDK API v1.18, but QSV_HAVE_CO3 is API
> V1.11.
> 
> This isn't true in all cases. As described in
> "https://trac.ffmpeg.org/ticket/7839"
> one current bug breaks the "mpeg2_qsv" encoder. One solution is to
> MANUALLY disable the QVBR. This can be done with a simple "#define
> QSV_HAVE_QVBR 0". Even if you compile with a recent version of the SDK >
> v1.11.
> 
> But the problem is then that this implies too to disable "QSV_HAVE_CO3".
> And doing this the code doesn't compile as this patch isn't applied.
> 
> So, the best solution is to merge this patch. It enables then the option to
> disable manually what you like, and the code compiles clean.
> 
> Please, accept the patch.

IMHO, your patch is only needed when disable "QSV_HAVE_CO3", but tiket#7839 is not root caused now.
I will consider to accept it when tiket#7839 is root caused.