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

[Michael Niedermayer]

On Tue, Apr 04, 2017 at 06:13:30PM -0400, Ronald S. Bultje wrote:
> 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?

idct_permutation must match the idcts, the idcts must match each
other permutation wise

for internal / extern, we could sidestep this by using a internal
clamped with internal idcts


> (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?)
>

if the idct depends on external asm then it would need to be under
HAVE_EXTERNAL_*, you are correct. Also that is ugly


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170405/13458898/attachment.sig>