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

Submitted by Oliver Collyer via ffmpeg-devel on April 18, 2019, 10:11 a.m.

Details

Message ID dFnWD8oBgjaBU4MYTcf7Jnf4sgVtmz7qVtFIgVbJRp9tMde-dbhFfARUWp8HAWEgD92DDtjXBel4lgWo1qMlbhZDKObMO6UC9IpzRvu5kek=@protonmail.com
State New
Headers show

Commit Message

Oliver Collyer via ffmpeg-devel April 18, 2019, 10:11 a.m.
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.

---
From d43c81f5bba49e55ea867bd6afd2eef878dc0ad3 Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Thu, 18 Apr 2019 10:40:38 +0100
Subject: [PATCH] libavcodec: qsv protect gpb with co3

---
 libavcodec/qsvenc.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Zhong Li April 22, 2019, 11:33 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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@ffmpeg.org>

> Cc: Andreas Håkon <andreas.hakon@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.
mypopy@gmail.com April 22, 2019, 12:17 p.m.
On Mon, Apr 22, 2019 at 7:38 PM Li, Zhong <zhong.li@intel.com> wrote:
>
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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@ffmpeg.org>
> > Cc: Andreas Håkon <andreas.hakon@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.

In the ticket,  Andreas Håkon  pointed out that it is "only as a
temporary solution", so I think it's a workaround.
Andreas Håkon April 22, 2019, 12:20 p.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, 22 de April de 2019 13:33, Li, Zhong <zhong.li@intel.com> wrote:

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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@ffmpeg.org
> > Cc: Andreas Håkon andreas.hakon@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#L756
  2. https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/qsvenc.c#L270

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.

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.
Thank you!
A.H.


---
Andreas Håkon April 22, 2019, 12:42 p.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, 22 de April de 2019 14:17, mypopy@gmail.com <mypopy@gmail.com> wrote:

> On Mon, Apr 22, 2019 at 7:38 PM Li, Zhong zhong.li@intel.com wrote:
>
> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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@ffmpeg.org
> > > Cc: Andreas Håkon andreas.hakon@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.
>
> In the ticket, Andreas Håkon pointed out that it is "only as a
> temporary solution", so I think it's a workaround.

Hi mypopy,

My patch only resolves one part of the problem. It's not a temporary
solution, but a forgot in the original code.

I feel Mr. Zhong needs to fix the "mpeg2_qsv" encoder. But this simple
patch can help with the "temporary solution" of disable QVBR in the code.
In any case, this patch needs to be merged.

Regards.
A.H.

---
mypopy@gmail.com April 22, 2019, 1:13 p.m.
On Mon, Apr 22, 2019 at 8:42 PM Andreas Håkon
<andreas.hakon@protonmail.com> wrote:
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, 22 de April de 2019 14:17, mypopy@gmail.com <mypopy@gmail.com> wrote:
>
> > On Mon, Apr 22, 2019 at 7:38 PM Li, Zhong zhong.li@intel.com wrote:
> >
> > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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@ffmpeg.org
> > > > Cc: Andreas Håkon andreas.hakon@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.
> >
> > In the ticket, Andreas Håkon pointed out that it is "only as a
> > temporary solution", so I think it's a workaround.
>
> Hi mypopy,
>
> My patch only resolves one part of the problem. It's not a temporary
> solution, but a forgot in the original code.
>
> I feel Mr. Zhong needs to fix the "mpeg2_qsv" encoder. But this simple
> patch can help with the "temporary solution" of disable QVBR in the code.
> In any case, this patch needs to be merged.
>
> Regards.
> A.H.

Thanks the details and clarification, I've got the point for this patch.
Zhong Li April 23, 2019, 6:39 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Andreas Håkon

> Sent: Monday, April 22, 2019 8:21 PM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@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@intel.com>

> wrote:

> 

> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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@ffmpeg.org

> > > Cc: Andreas Håkon andreas.hakon@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.
Andreas Håkon April 23, 2019, 7:29 a.m.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 23 de April de 2019 8:39, Li, Zhong <zhong.li@intel.com> wrote:

>
> I belive they are different. If you extend the MARCIO, they are actually:
>
>
> 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.
>

Hi Li,

With all due respect.
You're assuming that:
 "QSV_VERSION_ATLEAST(1, 18)" > "QSV_VERSION_ATLEAST(1, 11)"
So,
 "QSV_VERSION_ATLEAST(1, 18)" implies that "QSV_VERSION_ATLEAST(1, 11)"

But the CODE doesn't say that!

The code uses:
 "#if QSV_HAVE_CO3"

And then ***every*** use of the "co3" member needs to be protected by this clausule.
Futhermore, this is true in every part of the source code in "qsvenc.c"... except at
two points (I need to review my patch, as I noticed another block not protected).

This is a must, even if "QSV_VERSION_ATLEAST(1, 18)" > "QSV_VERSION_ATLEAST(1, 11)"
Why?
Because the option "QSV_HAVE_CO3" can be rewriten (for example, disabled).

So the conclusion is that the code doesn't say "QSV_VERSION_ATLEAST(x)".

Anyway, thanks for discussing it. Stay tuned, while I prepare another patch
protecting all cases.

Regards.
A.H.

---

Patch hide | download patch | download mbox

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index a03ab69..9583d62 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -267,10 +267,12 @@  static void dump_video_param(AVCodecContext *avctx, QSVEncContext *q,
 #endif
 #endif
 
+#if QSV_HAVE_CO3
 #if QSV_HAVE_GPB
     if (avctx->codec_id == AV_CODEC_ID_HEVC)
         av_log(avctx, AV_LOG_VERBOSE,"GPB: %s\n", print_threestate(co3->GPB));
 #endif
+#endif
 
     if (avctx->codec_id == AV_CODEC_ID_H264) {
         av_log(avctx, AV_LOG_VERBOSE, "Entropy coding: %s; MaxDecFrameBuffering: %"PRIu16"\n",