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

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

[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@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
 
 ;------------------------------------------------------------------------------
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 02/10] avcodec/wmalosslessdec: Remove unnecessary emms_c()

[Andreas Rheinhardt]

Possible since 6feea076e98512d78c8d735509ab6b5e9a71ca1c.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/wmalosslessdec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
index 614b6135f5..6ba9f04d25 100644
--- a/libavcodec/wmalosslessdec.c
+++ b/libavcodec/wmalosslessdec.c
@@ -783,7 +783,6 @@ static void revert_cdlms ## bits (WmallDecodeCtx *s, int ch, \
             s->channel_residues[ch][icoef] = input; \
         } \
     } \
-    if (bits <= 16) emms_c(); \
 }
 
 CD_LMS(16, WMALL_COEFF_PAD_SIZE)
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 02/10] avcodec/wmalosslessdec: Remove unnecessary emms_c()

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Possible since 6feea076e98512d78c8d735509ab6b5e9a71ca1c.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/wmalosslessdec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
> index 614b6135f5..6ba9f04d25 100644
> --- a/libavcodec/wmalosslessdec.c
> +++ b/libavcodec/wmalosslessdec.c
> @@ -783,7 +783,6 @@ static void revert_cdlms ## bits (WmallDecodeCtx *s, int ch, \
>              s->channel_residues[ch][icoef] = input; \
>          } \
>      } \
> -    if (bits <= 16) emms_c(); \
>  }
>  
>  CD_LMS(16, WMALL_COEFF_PAD_SIZE)

Will apply the rest of this patchset tomorrow unless there are objections.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 03/10] avcodec/takdec: Remove unnecessary emms_c()

[Andreas Rheinhardt]

Possible since 3d716d38abdae1982e84e30becb57458244656bd.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/takdec.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/libavcodec/takdec.c b/libavcodec/takdec.c
index 3e53401328..6f4cc92e88 100644
--- a/libavcodec/takdec.c
+++ b/libavcodec/takdec.c
@@ -502,8 +502,6 @@ static int decode_subframe(TAKDecContext *s, int32_t *decoded,
             memcpy(s->residues, &s->residues[y], 2 * filter_order);
     }
 
-    emms_c();
-
     return 0;
 }
 
@@ -660,8 +658,6 @@ static int decorrelate(TAKDecContext *s, int c1, int c2, int length)
 
             memmove(s->residues, &s->residues[tmp], 2 * filter_order);
         }
-
-        emms_c();
         break;
     }
     }
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 04/10] avcodec/jpeglsenc: Remove unnecessary emms_c()

[Andreas Rheinhardt]

This encoder does not use any MMX anywhere.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/jpeglsenc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libavcodec/jpeglsenc.c b/libavcodec/jpeglsenc.c
index ba9355225b..8f3197e687 100644
--- a/libavcodec/jpeglsenc.c
+++ b/libavcodec/jpeglsenc.c
@@ -413,8 +413,6 @@ static int encode_picture_ls(AVCodecContext *avctx, AVPacket *pkt,
     /* End of image */
     put_marker_byteu(&pb, EOI);
 
-    emms_c();
-
     av_shrink_packet(pkt, bytestream2_tell_p(&pb));
     *got_packet = 1;
     return 0;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 05/10] avcodec/ffv1(dec|enc): Remove unnecessary emms_c()

[Andreas Rheinhardt]

These codecs do not use MMX at all.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/ffv1dec.c | 2 --
 libavcodec/ffv1enc.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 4fe1738dea..fd549c7913 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -360,8 +360,6 @@ static int decode_slice(AVCodecContext *c, void *arg)
         }
     }
 
-    emms_c();
-
     ff_thread_report_progress(&f->picture, si, 0);
 
     return 0;
diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c
index 90593fbaf1..b939871664 100644
--- a/libavcodec/ffv1enc.c
+++ b/libavcodec/ffv1enc.c
@@ -1076,7 +1076,6 @@ retry:
     } else {
         ret = encode_rgb_frame(fs, planes, width, height, p->linesize);
     }
-    emms_c();
 
     if (ret < 0) {
         av_assert0(fs->slice_coding_mode == 0);
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 06/10] avcodec/apedec: Remove unnecessary emms_c()

[Andreas Rheinhardt]

Possible since 6feea076e98512d78c8d735509ab6b5e9a71ca1c.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/apedec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
index 26183067a8..e7baa2e77f 100644
--- a/libavcodec/apedec.c
+++ b/libavcodec/apedec.c
@@ -1577,7 +1577,6 @@ static int ape_decode_frame(AVCodecContext *avctx, AVFrame *frame,
         ape_unpack_mono(s, blockstodecode);
     else
         ape_unpack_stereo(s, blockstodecode);
-    emms_c();
 
     if (s->error) {
         s->samples=0;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 07/10] avcodec/4xm: Remove unnecessary and redundat emms_c()

[Andreas Rheinhardt]

It is unnecessary since ee551a21ddcbf81afe183d9489c534ee80f263a0;
but it was redundant even before that, because decode_simple_internal()
calls emms_c().

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/4xm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libavcodec/4xm.c b/libavcodec/4xm.c
index 8941d715fa..67cd7d8af9 100644
--- a/libavcodec/4xm.c
+++ b/libavcodec/4xm.c
@@ -964,8 +964,6 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *picture,
 
     *got_frame = 1;
 
-    emms_c();
-
     return buf_size;
 }
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 08/10] avcodec/loongarch/cabac, vp9dsp_loongarch: Add missing headers

[Andreas Rheinhardt]

Fixes checkheaders on loongarch.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/loongarch/cabac.h            | 1 +
 libavcodec/loongarch/vp9dsp_loongarch.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/libavcodec/loongarch/cabac.h b/libavcodec/loongarch/cabac.h
index e1c946fe16..f896ccf138 100644
--- a/libavcodec/loongarch/cabac.h
+++ b/libavcodec/loongarch/cabac.h
@@ -25,6 +25,7 @@
 #ifndef AVCODEC_LOONGARCH_CABAC_H
 #define AVCODEC_LOONGARCH_CABAC_H
 
+#include "libavutil/attributes.h"
 #include "libavcodec/cabac.h"
 #include "config.h"
 
diff --git a/libavcodec/loongarch/vp9dsp_loongarch.h b/libavcodec/loongarch/vp9dsp_loongarch.h
index 3cc918a18c..9fe4294d50 100644
--- a/libavcodec/loongarch/vp9dsp_loongarch.h
+++ b/libavcodec/loongarch/vp9dsp_loongarch.h
@@ -22,6 +22,9 @@
 #ifndef AVCODEC_LOONGARCH_VP9DSP_LOONGARCH_H
 #define AVCODEC_LOONGARCH_VP9DSP_LOONGARCH_H
 
+#include <stddef.h>
+#include <stdint.h>
+
 #define VP9_8TAP_LOONGARCH_LSX_FUNC(SIZE, type, type_idx)                    \
 void ff_put_8tap_##type##_##SIZE##h_lsx(uint8_t *dst, ptrdiff_t dststride,   \
                                         const uint8_t *src,                  \
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 09/10] avformat/os_support: Include stdint.h for int64_t

[Andreas Rheinhardt]

Fixes checkheaders for Windows targets.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/os_support.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/os_support.h b/libavformat/os_support.h
index b419efa408..f2ff38e23b 100644
--- a/libavformat/os_support.h
+++ b/libavformat/os_support.h
@@ -42,6 +42,7 @@
 
 #ifdef _WIN32
 #  include <fcntl.h>
+#  include <stdint.h>
 #  ifdef lseek
 #   undef lseek
 #  endif
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 10/10] avutil/mem_internal: Fix headers

[Andreas Rheinhardt]

Including avassert.h is unnecessary since commit
786be70e28fe739b8e49893fa13ae4652a68d1ea.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/x86/h264_qpel.c | 1 +
 libavutil/mem_internal.h   | 3 ++-
 libavutil/tx.c             | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavcodec/x86/h264_qpel.c b/libavcodec/x86/h264_qpel.c
index b9b1edcd9e..2df4c11f82 100644
--- a/libavcodec/x86/h264_qpel.c
+++ b/libavcodec/x86/h264_qpel.c
@@ -20,6 +20,7 @@
  */
 
 #include "libavutil/attributes.h"
+#include "libavutil/avassert.h"
 #include "libavutil/cpu.h"
 #include "libavutil/mem_internal.h"
 #include "libavutil/x86/asm.h"
diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h
index ed846aac52..955e31a698 100644
--- a/libavutil/mem_internal.h
+++ b/libavutil/mem_internal.h
@@ -25,7 +25,8 @@
 
 #include <stdint.h>
 
-#include "avassert.h"
+#include "attributes.h"
+#include "macros.h"
 #include "mem.h"
 #include "version.h"
 
diff --git a/libavutil/tx.c b/libavutil/tx.c
index e6fcf9f451..c90ca509f5 100644
--- a/libavutil/tx.c
+++ b/libavutil/tx.c
@@ -16,6 +16,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include "avassert.h"
 #include "cpu.h"
 #include "qsort.h"
 #include "bprint.h"
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

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@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.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2038 bytes --]

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@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. 

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

Above assumes iam not missing something

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@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!
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2376 bytes --]

On Tue, Aug 23, 2022 at 07:28:19PM +0200, Andreas Rheinhardt wrote:
> 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@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.

If you dont like that,
dont call it when its not needed or call it a few hundread times unnecessary
like your patch does.
or write only code that doesnt need emms 
maybe there are more options ...

thx

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

Michael Niedermayer:
> On Tue, Aug 23, 2022 at 07:28:19PM +0200, Andreas Rheinhardt wrote:
>> 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@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.
> 
> If you dont like that,
> dont call it when its not needed or call it a few hundread times unnecessary
> like your patch does.
> or write only code that doesnt need emms 
> maybe there are more options ...
> 

If emms_c() is used as now outside of MMX functions, then a "dont call
it when its not needed" would involve a check and would therefore still
incur cost for users who don't use this. Also it is unclear how such a
check would even look like given that one can use av_force_cpu_flags().
See also 55fc2c5a892c50feb1b9a8f55b74ec6594755ddb.
This patch also only calls it a few hundred times unnecessarily if one
runs this without SSSE3. CPUs without SSSE3 are ancient today. For the
non-ancient CPUs, using emms_c() adds an EMMS.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3404 bytes --]

On Tue, Aug 23, 2022 at 08:09:09PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Tue, Aug 23, 2022 at 07:28:19PM +0200, Andreas Rheinhardt wrote:
> >> 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@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.
> > 
> > If you dont like that,
> > dont call it when its not needed or call it a few hundread times unnecessary
> > like your patch does.
> > or write only code that doesnt need emms 
> > maybe there are more options ...
> > 
> 
> If emms_c() is used as now outside of MMX functions, then a "dont call
> it when its not needed" would involve a check and would therefore still
> incur cost for users who don't use this. Also it is unclear how such a
> check would even look like given that one can use av_force_cpu_flags().
> See also 55fc2c5a892c50feb1b9a8f55b74ec6594755ddb.
> This patch also only calls it a few hundred times unnecessarily if one
> runs this without SSSE3. CPUs without SSSE3 are ancient today. For the
> non-ancient CPUs, using emms_c() adds an EMMS.

do whatever you prefer.
The best solution depends on assumptions.
The impact is biggest on old CPUs where EMMS is also a slow instruction
But as you say these are ancient today. very small impact on many vs
small to moderate impact on a today rare setup
the worst is if the bug is left open and time is wasted on bikesheding

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

Michael Niedermayer:
> On Tue, Aug 23, 2022 at 08:09:09PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Tue, Aug 23, 2022 at 07:28:19PM +0200, Andreas Rheinhardt wrote:
>>>> 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@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.
>>>
>>> If you dont like that,
>>> dont call it when its not needed or call it a few hundread times unnecessary
>>> like your patch does.
>>> or write only code that doesnt need emms 
>>> maybe there are more options ...
>>>
>>
>> If emms_c() is used as now outside of MMX functions, then a "dont call
>> it when its not needed" would involve a check and would therefore still
>> incur cost for users who don't use this. Also it is unclear how such a
>> check would even look like given that one can use av_force_cpu_flags().
>> See also 55fc2c5a892c50feb1b9a8f55b74ec6594755ddb.
>> This patch also only calls it a few hundred times unnecessarily if one
>> runs this without SSSE3. CPUs without SSSE3 are ancient today. For the
>> non-ancient CPUs, using emms_c() adds an EMMS.
> 
> do whatever you prefer.
> The best solution depends on assumptions.
> The impact is biggest on old CPUs where EMMS is also a slow instruction
> But as you say these are ancient today. very small impact on many vs
> small to moderate impact on a today rare setup
> the worst is if the bug is left open and time is wasted on bikesheding
> 

Given that Lynne already approved this on IRC, I have already applied it
as de33506e4b3e3362095aab167ad8bb87c1bd9488.
Several of your FATE-boxes are now green: E.g.
https://fate.ffmpeg.org/history.cgi?slot=x86_32-debian-kfreebsd-gcc-4.4-cpuflags-sse
Rejoice!

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".