[FFmpeg-devel] [PATCH 10/11] avcodec/x86: add an 8-bit simple IDCT function based on the x86-64 high depth functions

Ronald S. Bultje rsbultje at gmail.com
Mon Jun 26 16:03:04 EEST 2017


Hi,

On Sun, Jun 25, 2017 at 3:27 PM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> On Sat, Jun 24, 2017 at 06:30:26PM -0400, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Sat, Jun 24, 2017 at 3:27 PM, Michael Niedermayer
> <michael at niedermayer.cc
> > > wrote:
> >
> > > On Mon, Jun 19, 2017 at 05:11:03PM +0200, James Darnley wrote:
> > > > Includes add/put functions
> > > >
> > > > Rounding contributed by Ronald S. Bultje
> > > > ---
> > > >  libavcodec/tests/x86/dct.c                |  2 +
> > > >  libavcodec/x86/idctdsp_init.c             | 23 ++++++++
> > > >  libavcodec/x86/simple_idct.h              |  9 +++
> > > >  libavcodec/x86/simple_idct10.asm          | 92
> > > +++++++++++++++++++++++++++++++
> > > >  libavcodec/x86/simple_idct10_template.asm |  6 +-
> > > >  5 files changed, 130 insertions(+), 2 deletions(-)
> > >
> > > Sorry for the delay, testing this took me a bit longer than expected
> > > as many files change slightly and looking at differences manually
> > > is manual work ...
> > >
> >
> > Understood, and thanks for taking the time to do the testing.
> >
> >
> > > This patch changes the default IDCT on x86(64), which is intended IIUC
> > > It also changes the IDCT when simplemmx is set
> > >
> > > but on x86-32 simplemmx does after this patch not produce the same
> > > result as simplemmx on x86-64.
> > >
> > > iam not sure but
> > > maybe the changed code should enable on FF_IDCT_SIMPLE instead of
> > > FF_IDCT_SIMPLEMMX ?
> > > whats your oppinion on this ?
> > > the next patch would add FF_IDCT_SIMPLE but it also leaves
> > > FF_IDCT_SIMPLEMMX
> >
> >
> >  That's a good point, I also considered that question (not so much the
> > 32bit vs. 64bit, but the mmx vs. sse2). The question is basically what
> > simplemmx means. Is it the exact binary result of the mmx function? Or is
> > it a way of saying "almost simple, but with some rounding diffs because
> > mmx"?
> >
> > If the second, then simple is a superset of simplemmx. If the first, then
> > we should remove simplemmx from the list of "supported" idcts for the
> > sse2/avx functions. I have no preference (I assumed it meant the first),
> > but if you'd prefer to use the second meaning, then that's an easy
> > modification to make and it won't practically have any impact for most
> use
> > cases I think...
>
> I didnt think about meaning, rather more about practice.
> if someone reports any issue using "simplemmx" and bitexact and
> that fails to be reproduced it could be confusing.
> This is especially plausible when the bug is not idct rounding but
> a bug in a later stage just triggered by specific output from the idct
>

Agreed. I'll leave it to James to decide on a final approach since it's his
patch. :-). It sounds like we're both fine with either approach.


> also potential future fate tests of simplemmx or other simd idcts
> require there to be a way to select a specific idct output


This is true, but don't forget that -cpuflags can also be used to cycle
between the various -idct flavours. (This requires some trial and error,
but it's not that many cpuflag-combinations.)

Ronald


More information about the ffmpeg-devel mailing list