diff mbox

[FFmpeg-devel] Fix visual glitch with XvMC, caused by wrong idct permutation.

Message ID CABA=pqe2-h8HQt=NVENqgocxsMb-9PVxAoiP3d7yZD0tpnonyA@mail.gmail.com
State Superseded
Headers show

Commit Message

Ivan Kalvachev Oct. 8, 2017, 10:52 p.m. UTC 
In the past XvMC forced simple_idct since
it was using FF_IDCT_PERM_NONE.
However now we have SIMD variants of simple_idct that
are using FF_IDCT_PERM_TRANSPOSE and if they are selected
XvMC would get coefficients in the wrong order.

The patch creates new FF_IDCT_NONE that
is used only for this kind of hardware decoding
and that fallbacks to the old C only simple idct.

BTW,
If you have idea for a better name, tell me, I might change it.
I thought of FF_IDCT_HWACCEL_PASSTHROUGHT but it is too huge and ugly.
For some reason mpeg12 vdpau also uses the same idct, so using XVMC in
the name doesn't seem right. (I'm not sure why vdpau needs it...)

I also was thinking of using "-1" number for the define, but ...
I didn't want to risk with it.

Best Regards
   Ivan Kalvachev.

Comments

Ronald S. Bultje Oct. 9, 2017, 12:17 a.m. UTC | #1 
Hi,

On Sun, Oct 8, 2017 at 6:52 PM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
[..]

Indentation is off in the second hunk, can you fix that?

Ronald
Ivan Kalvachev Oct. 9, 2017, 10:46 a.m. UTC | #2 
On 10/9/17, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Sun, Oct 8, 2017 at 6:52 PM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
> [..]
>
> Indentation is off in the second hunk, can you fix that?

You want it 4 spaces to the right
or to start from the first position?

BTW, I think it would be better to use "127" number.
Ronald S. Bultje Oct. 9, 2017, 1:02 p.m. UTC | #3 
Hi,

On Mon, Oct 9, 2017 at 6:46 AM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:

> On 10/9/17, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> > On Sun, Oct 8, 2017 at 6:52 PM, Ivan Kalvachev <ikalvachev@gmail.com>
> wrote:
> > [..]
> >
> > Indentation is off in the second hunk, can you fix that?
>
> You want it 4 spaces to the right
>

Yes, please.

BTW, I think it would be better to use "127" number.
>

I don't really mind either way. The number 128 suggests it may have been
intended as a bitmask. Michael is probably better positioned to comment on
this.

Ronald
Michael Niedermayer Oct. 9, 2017, 6:12 p.m. UTC | #4 
On Mon, Oct 09, 2017 at 09:02:38AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Mon, Oct 9, 2017 at 6:46 AM, Ivan Kalvachev <ikalvachev@gmail.com> wrote:
> 
> > On 10/9/17, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> > > On Sun, Oct 8, 2017 at 6:52 PM, Ivan Kalvachev <ikalvachev@gmail.com>
> > wrote:
> > > [..]
> > >
> > > Indentation is off in the second hunk, can you fix that?
> >
> > You want it 4 spaces to the right
> >
> 
> Yes, please.
> 
> BTW, I think it would be better to use "127" number.
> >
> 
> I don't really mind either way. The number 128 suggests it may have been
> intended as a bitmask. Michael is probably better positioned to comment on
> this.

I dont really remember but i think 128 was choosen for ABI
compatibility with additions to it from libav. So it shuld no longer
matter what values are used on additions

[...]
diff mbox

Patch

From 88a5f15f8ea04a5fb4eb135e1e773f92bb56a6e0 Mon Sep 17 00:00:00 2001
From: Ivan Kalvachev <ikalvachev@gmail.com>
Date: Mon, 9 Oct 2017 01:25:00 +0300
Subject: [PATCH] Fix visual glitch with XvMC, caused by wrong idct
 permutation.

In the past XvMC forced simple_idct since
it was using FF_IDCT_PERM_NONE.
However now we have SIMD variants of simple_idct that
are using FF_IDCT_PERM_TRANSPOSE and if they are selected
XvMC would get coefficients in the wrong order.

The patch creates new FF_IDCT_NONE that
is used only for this kind of hardware decoding
and that fallbacks to the old C only simple idct.

Signed-off-by: Ivan Kalvachev <ikalvachev@gmail.com>
---
 libavcodec/avcodec.h   | 1 +
 libavcodec/idctdsp.c   | 1 +
 libavcodec/mpeg12dec.c | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 52cc5b0ca..ca0cac501 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3147,6 +3147,7 @@  typedef struct AVCodecContext {
 #define FF_IDCT_SIMPLEALPHA   23
 #endif
 #define FF_IDCT_SIMPLEAUTO    128
+#define FF_IDCT_NONE          129 /* Used by XvMC to extract IDCT coefficients with FF_IDCT_PERM_NONE */
 
     /**
      * bits per sample/pixel from the demuxer (needed for huffyuv).
diff --git a/libavcodec/idctdsp.c b/libavcodec/idctdsp.c
index d596aed1a..45b29d6b7 100644
--- a/libavcodec/idctdsp.c
+++ b/libavcodec/idctdsp.c
@@ -279,6 +279,7 @@  av_cold void ff_idctdsp_init(IDCTDSPContext *c, AVCodecContext *avctx)
                 c->perm_type = FF_IDCT_PERM_NONE;
 #endif /* CONFIG_FAANIDCT */
             } else { // accurate/default
+            /* Be sure FF_IDCT_NONE will select this one, since it uses FF_IDCT_PERM_NONE */
                 c->idct_put  = ff_simple_idct_put_8;
                 c->idct_add  = ff_simple_idct_add_8;
                 c->idct      = ff_simple_idct_8;
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 22c29c150..4e68be27f 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1217,7 +1217,7 @@  static void setup_hwaccel_for_pixfmt(AVCodecContext *avctx)
 #endif
         )
         if (avctx->idct_algo == FF_IDCT_AUTO)
-            avctx->idct_algo = FF_IDCT_SIMPLE;
+            avctx->idct_algo = FF_IDCT_NONE;
 
     if (avctx->hwaccel && avctx->pix_fmt == AV_PIX_FMT_XVMC) {
         Mpeg1Context *s1 = avctx->priv_data;
-- 
2.14.1