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

[James Darnley]

On 2017-06-25 21:27, Michael Niedermayer 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:
>>
>>> 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
> 
> also potential future fate tests of simplemmx or other simd idcts
> require there to be a way to select a specific idct output
> 
> no strong oppinion on this ...

I admit I haven't considered whether I should be using this with
simplemmx.  I could change the code so that the new code isn't used for it.

If simplemmx is supposed to be its own algorithm available to anyone who
might wish to use it then I think that an error should occur when MMX is
not available.

Since the current behaviour is to have simple as the catch-all fallback
I will leave the code as is.  auto, simpleauto, simplemmx, and simple
will now all use the new code.

We can discuss these points all you want but I intend to push the
remaining 3 patches Soon(TM).  I have still not tried Gramner's
suggestion so you have some time to object and block.