| Message ID | dFnWD8oBgjaBU4MYTcf7Jnf4sgVtmz7qVtFIgVbJRp9tMde-dbhFfARUWp8HAWEgD92DDtjXBel4lgWo1qMlbhZDKObMO6UC9IpzRvu5kek=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
> 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.
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.
‐‐‐‐‐‐‐ 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. ---
‐‐‐‐‐‐‐ 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. ---
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.
> -----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.
‐‐‐‐‐‐‐ 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. ---
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",
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(+)