diff mbox

[FFmpeg-devel] libavcodec/qsvenc: fix mpeg2 encoding

Message ID p6WqcMUW1S2yikEGI0iRjzS_jdtitkwXec69KGWsWxzxlnKmKK7TROQ5ct4XagzicDqItE-FVbZDfP7hZXTaQy61CY7OMqfOe5ztzm-ZEas=@protonmail.com
State New
Headers show

Commit Message

Andreas Håkon May 14, 2019, 3:59 p.m. UTC 
Fixes bug #7839
https://trac.ffmpeg.org/ticket/7839

Supersedes:
#12935 - https://patchwork.ffmpeg.org/patch/12935/
#12872 - https://patchwork.ffmpeg.org/patch/12872/

Regards.
A.H.

---
From 49aa7d21026051dc353a8658509d2ba81520bf78 Mon Sep 17 00:00:00 2001
From: Andreas Hakon <andreas.hakon@protonmail.com>
Date: Tue, 14 May 2019 16:42:08 +0100
Subject: [PATCH] libavcodec/qsvenc: fix mpeg2 encoding

Commit "lavc/qsvenc: enable QVBR mode" generated a regression with the MPEG-2
QSV HW Encoder on the Windows platform. See trac #7839.

This patch fixes the problem by running a runtime check during initialization.
When using the MPEG-2 encoder the extended CO3 buffer is not added to the context.

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

Comments

Zhong Li May 15, 2019, 10:23 a.m. UTC | #1 
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Andreas Håkon

> Sent: Wednesday, May 15, 2019 12:00 AM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@ffmpeg.org>

> Subject: [FFmpeg-devel] [PATCH] libavcodec/qsvenc: fix mpeg2 encoding

> 

> Fixes bug #7839

> https://trac.ffmpeg.org/ticket/7839

> 

> Supersedes:

> #12935 - https://patchwork.ffmpeg.org/patch/12935/

> #12872 - https://patchwork.ffmpeg.org/patch/12872/

> 

> Regards.

> A.H.


It was said https://patchwork.ffmpeg.org/patch/12935/ could not fix #7839, how this one can work?
Andreas Håkon May 15, 2019, 10:38 a.m. UTC | #2 
Hi,

> > Subject: [FFmpeg-devel] [PATCH] libavcodec/qsvenc: fix mpeg2 encoding
> > Fixes bug #7839
> > https://trac.ffmpeg.org/ticket/7839
> > Supersedes:
> > #12935 - https://patchwork.ffmpeg.org/patch/12935/
> > #12872 - https://patchwork.ffmpeg.org/patch/12872/
> > Regards.
> > A.H.
>
> It was saidhttps://patchwork.ffmpeg.org/patch/12935/ could not fix #7839, how this one can work?


I compiled your patch and doesn't work.
My path works.

The difference?
In addition to a few extra little checks, It seems that the
QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails every time in
Windows (almost in my environment).

Futhermore, it has more sense to apply the check inside the
conditional preprocessing of #if QSV_HAVE_CO3 and not outside.

As a summary: I confirm that my patch works. And it's based on your
proposal. So please comment positively to merge it.

Thank you!
A.H.
Zhong Li May 15, 2019, 11:13 a.m. UTC | #3 
> > > Subject: [FFmpeg-devel] [PATCH] libavcodec/qsvenc: fix mpeg2

> > > encoding Fixes bug #7839

> > > https://trac.ffmpeg.org/ticket/7839

> > > Supersedes:

> > > #12935 - https://patchwork.ffmpeg.org/patch/12935/

> > > #12872 - https://patchwork.ffmpeg.org/patch/12872/

> > > Regards.

> > > A.H.

> >

> > It was saidhttps://patchwork.ffmpeg.org/patch/12935/ could not fix

> #7839, how this one can work?

> 

> 

> I compiled your patch and doesn't work.

> My path works.


Glad to know it.

> The difference?

> In addition to a few extra little checks, It seems that the

> QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails every time in

> Windows (almost in my environment).


If QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) always equal to zero, then co3 won't be set, 
If codec is MPEG2, co3 won't be set too.
I can't see any difference for MPEG2 case.

> Futhermore, it has more sense to apply the check inside the conditional

> preprocessing of #if QSV_HAVE_CO3 and not outside.


Make sense, but no difference for run-time result? 

> As a summary: I confirm that my patch works. And it's based on your

> proposal. So please comment positively to merge it.


It is closer to be merged, but need to confirm which exact line of code make difference.
(Your verification on Windows was appreciated.)

> Thank you!

> A.H.
Andreas Håkon May 15, 2019, 12:54 p.m. UTC | #4 
Hi,


> > The difference?
> > In addition to a few extra little checks, It seems that the
> > QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails every time in
> > Windows (almost in my environment).
>
> If QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) always equal to zero, then co3 won't be set,
> If codec is MPEG2, co3 won't be set too.
> I can't see any difference for MPEG2 case.

Your assumption about QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) is not real.
This depends on the run-time driver used.


> > Futhermore, it has more sense to apply the check inside the conditional
> > preprocessing of #if QSV_HAVE_CO3 and not outside.
>
> Make sense, but no difference for run-time result?

I prefer to make the code as more legible as possible.
That's the reason for the other additions of #if QSV_HAVE_CO3.


> > As a summary: I confirm that my patch works. And it's based on your
> > proposal. So please comment positively to merge it.
>
> It is closer to be merged, but need to confirm which exact line of code make difference.
> (Your verification on Windows was appreciated.)

As I pointed, the QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails in Windows.
This version runs without problems and resolves the bug (in Windows).

So, I suggest to accept this patch.

Regards.
A.H.

---
Zhong Li May 15, 2019, 3:31 p.m. UTC | #5 
> -----Original Message-----

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

> Of Andreas Håkon

> Sent: Wednesday, May 15, 2019 8:55 PM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH] libavcodec/qsvenc: fix mpeg2 encoding

> 

> Hi,

> 

> 

> > > The difference?

> > > In addition to a few extra little checks, It seems that the

> > > QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails every time in

> > > Windows (almost in my environment).

> >

> > If QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) always equal to zero,

> > then co3 won't be set, If codec is MPEG2, co3 won't be set too.

> > I can't see any difference for MPEG2 case.

> 

> Your assumption about QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) is

> not real.

> This depends on the run-time driver used.

> 

> 

> > > Futhermore, it has more sense to apply the check inside the

> > > conditional preprocessing of #if QSV_HAVE_CO3 and not outside.

> >

> > Make sense, but no difference for run-time result?

> 

> I prefer to make the code as more legible as possible.

> That's the reason for the other additions of #if QSV_HAVE_CO3.

> 

> 

> > > As a summary: I confirm that my patch works. And it's based on your

> > > proposal. So please comment positively to merge it.

> >

> > It is closer to be merged, but need to confirm which exact line of code

> make difference.

> > (Your verification on Windows was appreciated.)

> 

> As I pointed, the QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails

> in Windows.

> This version runs without problems and resolves the bug (in Windows).


Well, let me to explain with more detail:

V1: 
if (avctx->codec_id != AV_CODEC_ID_MPEG2VIDEO &&
> QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11)) 


For mpeg2 case, it is always equal to "if (0)" , right? 

V2:
if (avctx->codec_id != AV_CODEC_ID_MPEG2VIDEO)

For mpeg2 case, it is equal to "if (0)" too, right?

Thus why I asked what the difference between V1 and V2 was.
Andreas Håkon May 15, 2019, 3:46 p.m. UTC | #6 
> > >
> > > It is closer to be merged, but need to confirm which exact line of code
> > > make difference.
> > > (Your verification on Windows was appreciated.)
> >
> > As I pointed, the QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11) check fails
> > in Windows.
> > This version runs without problems and resolves the bug (in Windows).
>
> Well, let me to explain with more detail:
>
> V1:
> if (avctx->codec_id != AV_CODEC_ID_MPEG2VIDEO &&
>
> > QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 11))
>
> For mpeg2 case, it is always equal to "if (0)" , right?
>
> V2:
> if (avctx->codec_id != AV_CODEC_ID_MPEG2VIDEO)
>
> For mpeg2 case, it is equal to "if (0)" too, right?
>
> Thus why I asked what the difference between V1 and V2 was.


I'm sorry! Now I understand what you're saying. It's my fault.
Well then... I feel like you're right.

However, I can only comment that the first time I compiled your patch
it doesn't work. And for this reason I created this one.
And now I can confirm that it resolves the problem.

Please feel free to approve whatever you think is best. But something has
to be done, because the MPEG-2 QSV encoder in Windows is broken.

Regards.
A.H.

---
diff mbox

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index a03ab69..9d26db0 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;
@@ -751,13 +755,16 @@  FF_ENABLE_DEPRECATION_WARNINGS
 #endif
         }
 #if QSV_HAVE_CO3
-        q->extco3.Header.BufferId      = MFX_EXTBUFF_CODING_OPTION3;
-        q->extco3.Header.BufferSz      = sizeof(q->extco3);
+        if (avctx->codec_id != AV_CODEC_ID_MPEG2VIDEO ) {
+            av_log(avctx,AV_LOG_DEBUG,"Initializing the MFX context with the Extended Coding Option 3 (extco3)\n");
+            q->extco3.Header.BufferId      = MFX_EXTBUFF_CODING_OPTION3;
+            q->extco3.Header.BufferSz      = sizeof(q->extco3);
 #if QSV_HAVE_GPB
-        if (avctx->codec_id == AV_CODEC_ID_HEVC)
-            q->extco3.GPB              = q->gpb ? MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF;
+            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;
+            q->extparam_internal[q->nb_extparam_internal++] = (mfxExtBuffer *)&q->extco3;
+        }
 #endif
     }