diff mbox

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

Message ID j8_J9HNIbQdAj7QPvmU4hfDu0GNLNNZr2aLY8E80aTk1EI8nCgYFRuiouFH9KeoGPMj4RVu5UqXgEx7BhGc-tKFblcTUeoJ6DoveyAjP1ls=@protonmail.com
State New
Headers show

Commit Message

Andreas Håkon April 23, 2019, 8:34 a.m. UTC 
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 23 de April de 2019 9:29, Andreas Håkon <andreas.hakon@protonmail.com> wrote:

> ‐‐‐‐‐‐‐ 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.
>
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Hi Li,

Here the updated patch.
I hope you agree with it!

Regards.
A.H.

---
From 211c6704e4e4272221fbc8c908b99307b11e1800 Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Tue, 23 Apr 2019 09:25:15 +0100
Subject: [PATCH] libavcodec: QSV protect GPB code with CO3 define [v2]

This patch protects some code using the member "co3" inside the qsvenc.c file.

The first block is a simple debug message, so it doesn't generates troubles.
The second block is an initialization using of the "extco3" for the QVBR mode.

After this patch it's safe to enable/disable some options in the source code
without triggering compilation errors.

Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com>
---
 libavcodec/qsvenc.c |    4 ++++
 1 file changed, 4 insertions(+)

Comments

Zhong Li April 29, 2019, 8:54 a.m. UTC | #1 
> > 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).


I don't think so.
It is pre-compile checking and should not be decided in FFmpeg level, indstead depended on MSDK API has defined it or not.
diff mbox

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index a03ab69..feedceb 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",
@@ -598,9 +600,11 @@  static int init_video_param(AVCodecContext *avctx, QSVEncContext *q)
         q->param.mfx.MaxKbps          = max_bitrate_kbps / brc_param_multiplier;
         q->param.mfx.BRCParamMultiplier = brc_param_multiplier;
 #if QSV_HAVE_QVBR
+#if QSV_HAVE_CO3
         if (q->param.mfx.RateControlMethod == MFX_RATECONTROL_QVBR)
             q->extco3.QVBRQuality = av_clip(avctx->global_quality, 0, 51);
 #endif
+#endif
         break;
     case MFX_RATECONTROL_CQP:
         quant = avctx->global_quality / FF_QP2LAMBDA;