[FFmpeg-devel] [PATCH 5/6] x86/simple_idct: add explicit sse2 simple_idct_put/add versions.

[Ronald S. Bultje]

Hi,

On Tue, Apr 4, 2017 at 5:52 PM, Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Tue, Apr 04, 2017 at 12:48:17PM -0400, Ronald S. Bultje wrote:
> > These use the mmx IDCT, but sse2 put/add_pixels_clamped implementations.
> > This way we don't need to use the ff_put/add_pixels_clamped function
> > pointers.
> > ---
> >  libavcodec/x86/idctdsp_init.c | 10 ++++++++++
> >  libavcodec/x86/simple_idct.c  | 15 +++++++++++++--
> >  libavcodec/x86/simple_idct.h  |  3 +++
> >  3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/x86/idctdsp_init.c
> b/libavcodec/x86/idctdsp_init.c
> > index bcf7e5b..579c5e7 100644
> > --- a/libavcodec/x86/idctdsp_init.c
> > +++ b/libavcodec/x86/idctdsp_init.c
> > @@ -87,6 +87,16 @@ av_cold void ff_idctdsp_init_x86(IDCTDSPContext *c,
> AVCodecContext *avctx,
> >      }
> >
> >      if (ARCH_X86_64 && avctx->lowres == 0) {
> > +        if (!high_bit_depth &&
>
> > +            avctx->lowres == 0 &&
>
> this looks redundant
>
> otherwise should be ok
>
> thx


Thanks.

While looking, this patch may actually have a slightly theoretical issue.
Imagine that you're on a configuration where external asm is disabled but
inline asm is enabled. In our current system,
put/add_pixels_clamped_mmx/sse2 are yasm functions, but idct() is still
inline. So in this configuration, idct() is available, but idct_add/put are
not.

What do we set idct_permutation to?

(I know that in practice this configuration is basically a bug and so the
performance implications aren't that important, but I do believe it should
not crash; should we just move the assignment of idct under
HAVE_EXTERNAL_MMX - which is where idct_put/add would go - even though it
strictly speaking doesn't use external asm?)

Ronald