[FFmpeg-devel] [PATCH] swscale/x86/rgb2_rgb: Empty MMX state in ff_shuffle_bytes_2103_mmxext

[Andreas Rheinhardt]

Michael Niedermayer:
> On Mon, Aug 22, 2022 at 11:59:17PM +0200, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Fixes FATE-failures with the the filter-2xbr filter-3xbr filter-4xbr
>>> filter-ep2x filter-ep3x filter-hq2x filter-hq3x filter-hq4x
>>> filter-paletteuse-bayer filter-paletteuse-bayer0
>>> filter-paletteuse-nodither and filter-paletteuse-sierra2_4a tests
>>> when using 32bit x86 with CPUFLAGS ranging from "mmx+mmxext" to
>>> "mmx+mmxext+sse+sse2+sse3" (the relevant function is only overwritten
>>> when using SSSE3).
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>>> ---
>>>  libswscale/x86/rgb_2_rgb.asm | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libswscale/x86/rgb_2_rgb.asm b/libswscale/x86/rgb_2_rgb.asm
>>> index c695c61d5c..76ca1eec03 100644
>>> --- a/libswscale/x86/rgb_2_rgb.asm
>>> +++ b/libswscale/x86/rgb_2_rgb.asm
>>> @@ -104,6 +104,7 @@ jge .end
>>>      jl .loop_simd
>>>  
>>>  .end:
>>> +    emms
>>>      RET
>>>  
>>>  ;------------------------------------------------------------------------------
>>
>> I'd really love if someone with x86 assembly skills could look over this
>> trivial patch and confirm whether it is indeed correct. All I currently
>> know is that is works for me.
> 
> emms needs to be called between MMX and float code, as far outside of loops
> as possible
> that would suggest outside the for() loops in rgbToRgbWrapper() and any
> other code using it.

But there is another aspect that the above is missing: Namely that if
emms_c() is put outside of MMX functions, then it will be called even
when it is unnecessary. In this case it is unnecessary for all modern
CPUs, as this function is overridden when SSSE3 is available.

> 
> thats what we did and what is most efficient. One can make an argument that
> emms must be called before returning to C code when its needed. That though
> would imply also that all uses of emms_c() are wrong
> 

Well, e.g. the x64 psABI contains this clause: "The CPU shall be in x87
mode upon entry to a function. Therefore, every
 function that uses the MMX registers is required to issue an emms or femms
 instruction after using MMX registers, before returning or calling
another function."
So using emms_c() is ABI-incompliant. If I add an av_assert0_fpu() at
the beginning of av_log_default_callback (a function that may be
overridden by a user-defined callback that actually relies on us
conforming to the ABI), several FATE tests fail. I am sure that there
are lots of av_logs or other functions that are in parts of the code
where the CPU is not in x87 mode and that are just not executed in fate
because they are error logs.

- Andreas

PS: On the brighter side: fate.ffmpeg.org now contains three more green
boxes!