[FFmpeg-devel] [PATCH 1/4] avutil/x86/pixelutils: Empty MMX state in ff_pixelutils_sad_8x8_mmxext

[FFmpeg-devel] [PATCH 1/4] avutil/x86/pixelutils: Empty MMX state in ff_pixelutils_sad_8x8_mmxext

[Andreas Rheinhardt]

We currently mostly do not empty the MMX state in our MMX
DSP functions; instead we only do so before code that might
be using x87 code. This is a violation of the System V i386 ABI
(and maybe of other ABIs, too):
"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." (See 2.2.1 in [1])
This patch does not intend to change all these functions to abide
by the ABI; it only does so for ff_pixelutils_sad_8x8_mmxext, as this
function can by called by external users, because it is exported
via the pixelutils API. Without this, the following fragment will
assert (on x86/x64):
    uint8_t src1[8 * 8], src2[8 * 8];
    av_pixelutils_sad_fn fn = av_pixelutils_get_sad_fn(3, 3, 0, NULL);
    fn(src1, 8, src2, 8);
    av_assert0_fpu();

[1]: https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/intel386-psABI-1.1.pdf

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/x86/pixelutils.asm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavutil/x86/pixelutils.asm b/libavutil/x86/pixelutils.asm
index fbe9b45971..0bcccb51f5 100644
--- a/libavutil/x86/pixelutils.asm
+++ b/libavutil/x86/pixelutils.asm
@@ -43,6 +43,7 @@ cglobal pixelutils_sad_8x8, 4,4,0, src1, stride1, src2, stride2
     lea         src2q, [src2q + 2*stride2q]
 %endrep
     movd        eax, m2
+    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 2/4] avfilter/vf_mpdecimate: Remove emms_c

[Andreas Rheinhardt]

Unnecessary now that the pixelutils API abides by the ABI.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/vf_mpdecimate.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/libavfilter/vf_mpdecimate.c b/libavfilter/vf_mpdecimate.c
index baba1f183f..dab8941e46 100644
--- a/libavfilter/vf_mpdecimate.c
+++ b/libavfilter/vf_mpdecimate.c
@@ -24,7 +24,6 @@
  * Rich Felker.
  */
 
-#include "libavutil/emms.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/pixelutils.h"
@@ -142,13 +141,10 @@ static int decimate_frame(AVFilterContext *ctx,
                         cur->data[plane], cur->linesize[plane],
                         ref->data[plane], ref->linesize[plane],
                         AV_CEIL_RSHIFT(ref->width,  hsub),
-                        AV_CEIL_RSHIFT(ref->height, vsub))) {
-            emms_c();
+                        AV_CEIL_RSHIFT(ref->height, vsub)))
             return 0;
-        }
     }
 
-    emms_c();
     return 1;
 }
 
-- 
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 3/4] avfilter/vf_deshake: Remove unnecessary emms_c

[Andreas Rheinhardt]

Redundant since ea043cc53ed3506775ec6239ed5f8a20718b1098
(which made 16x16 no longer use MMX).

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

diff --git a/libavfilter/vf_deshake.c b/libavfilter/vf_deshake.c
index d44dd71eb0..cb062dd11f 100644
--- a/libavfilter/vf_deshake.c
+++ b/libavfilter/vf_deshake.c
@@ -53,7 +53,6 @@
 #include "internal.h"
 #include "video.h"
 #include "libavutil/common.h"
-#include "libavutil/emms.h"
 #include "libavutil/file_open.h"
 #include "libavutil/mem.h"
 #include "libavutil/opt.h"
@@ -178,7 +177,6 @@ static void find_block_motion(DeshakeContext *deshake, uint8_t *src1,
         mv->x = -1;
         mv->y = -1;
     }
-    emms_c();
     //av_log(NULL, AV_LOG_ERROR, "%d\n", smallest);
     //av_log(NULL, AV_LOG_ERROR, "Final: (%d, %d) = %d x %d\n", cx, cy, mv->x, mv->y);
 }
-- 
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 4/4] avfilter/deshake: Merge header into its only user

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/deshake.h    | 83 ----------------------------------------
 libavfilter/vf_deshake.c | 55 +++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 84 deletions(-)
 delete mode 100644 libavfilter/deshake.h

diff --git a/libavfilter/deshake.h b/libavfilter/deshake.h
deleted file mode 100644
index 406cbab2f6..0000000000
--- a/libavfilter/deshake.h
+++ /dev/null
@@ -1,83 +0,0 @@
-/*
- * Copyright (C) 2013 Wei Gao <weigao@multicorewareinc.com>
- * Copyright (C) 2013 Lenny Wang
- *
- * This file is part of FFmpeg.
- *
- * FFmpeg is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * FFmpeg is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with FFmpeg; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- */
-
-#ifndef AVFILTER_DESHAKE_H
-#define AVFILTER_DESHAKE_H
-
-#include "config.h"
-#include "avfilter.h"
-#include "transform.h"
-#include "libavutil/pixelutils.h"
-
-
-enum SearchMethod {
-    EXHAUSTIVE,        ///< Search all possible positions
-    SMART_EXHAUSTIVE,  ///< Search most possible positions (faster)
-    SEARCH_COUNT
-};
-
-typedef struct IntMotionVector {
-    int x;             ///< Horizontal shift
-    int y;             ///< Vertical shift
-} IntMotionVector;
-
-typedef struct MotionVector {
-    double x;             ///< Horizontal shift
-    double y;             ///< Vertical shift
-} MotionVector;
-
-typedef struct Transform {
-    MotionVector vec;     ///< Motion vector
-    double angle;         ///< Angle of rotation
-    double zoom;          ///< Zoom percentage
-} Transform;
-
-#define MAX_R 64
-
-typedef struct DeshakeContext {
-    const AVClass *class;
-    int counts[2*MAX_R+1][2*MAX_R+1]; /// < Scratch buffer for motion search
-    double *angles;            ///< Scratch buffer for block angles
-    unsigned angles_size;
-    AVFrame *ref;              ///< Previous frame
-    int rx;                    ///< Maximum horizontal shift
-    int ry;                    ///< Maximum vertical shift
-    int edge;                  ///< Edge fill method
-    int blocksize;             ///< Size of blocks to compare
-    int contrast;              ///< Contrast threshold
-    int search;                ///< Motion search method
-    av_pixelutils_sad_fn sad;  ///< Sum of the absolute difference function
-    Transform last;            ///< Transform from last frame
-    int refcount;              ///< Number of reference frames (defines averaging window)
-    FILE *fp;
-    Transform avg;
-    int cw;                    ///< Crop motion search to this box
-    int ch;
-    int cx;
-    int cy;
-    char *filename;            ///< Motion search detailed log filename
-    int opencl;
-    int (* transform)(AVFilterContext *ctx, int width, int height, int cw, int ch,
-                      const float *matrix_y, const float *matrix_uv, enum InterpolateMethod interpolate,
-                      enum FillMethod fill, AVFrame *in, AVFrame *out);
-} DeshakeContext;
-
-#endif /* AVFILTER_DESHAKE_H */
diff --git a/libavfilter/vf_deshake.c b/libavfilter/vf_deshake.c
index cb062dd11f..299814fd65 100644
--- a/libavfilter/vf_deshake.c
+++ b/libavfilter/vf_deshake.c
@@ -51,15 +51,68 @@
 
 #include "avfilter.h"
 #include "internal.h"
+#include "transform.h"
 #include "video.h"
 #include "libavutil/common.h"
 #include "libavutil/file_open.h"
 #include "libavutil/mem.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
+#include "libavutil/pixelutils.h"
 #include "libavutil/qsort.h"
 
-#include "deshake.h"
+
+enum SearchMethod {
+    EXHAUSTIVE,        ///< Search all possible positions
+    SMART_EXHAUSTIVE,  ///< Search most possible positions (faster)
+    SEARCH_COUNT
+};
+
+typedef struct IntMotionVector {
+    int x;             ///< Horizontal shift
+    int y;             ///< Vertical shift
+} IntMotionVector;
+
+typedef struct MotionVector {
+    double x;             ///< Horizontal shift
+    double y;             ///< Vertical shift
+} MotionVector;
+
+typedef struct Transform {
+    MotionVector vec;     ///< Motion vector
+    double angle;         ///< Angle of rotation
+    double zoom;          ///< Zoom percentage
+} Transform;
+
+#define MAX_R 64
+
+typedef struct DeshakeContext {
+    const AVClass *class;
+    int counts[2*MAX_R+1][2*MAX_R+1]; /// < Scratch buffer for motion search
+    double *angles;            ///< Scratch buffer for block angles
+    unsigned angles_size;
+    AVFrame *ref;              ///< Previous frame
+    int rx;                    ///< Maximum horizontal shift
+    int ry;                    ///< Maximum vertical shift
+    int edge;                  ///< Edge fill method
+    int blocksize;             ///< Size of blocks to compare
+    int contrast;              ///< Contrast threshold
+    int search;                ///< Motion search method
+    av_pixelutils_sad_fn sad;  ///< Sum of the absolute difference function
+    Transform last;            ///< Transform from last frame
+    int refcount;              ///< Number of reference frames (defines averaging window)
+    FILE *fp;
+    Transform avg;
+    int cw;                    ///< Crop motion search to this box
+    int ch;
+    int cx;
+    int cy;
+    char *filename;            ///< Motion search detailed log filename
+    int opencl;
+    int (* transform)(AVFilterContext *ctx, int width, int height, int cw, int ch,
+                      const float *matrix_y, const float *matrix_uv, enum InterpolateMethod interpolate,
+                      enum FillMethod fill, AVFrame *in, AVFrame *out);
+} DeshakeContext;
 
 #define OFFSET(x) offsetof(DeshakeContext, x)
 #define FLAGS AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_FILTERING_PARAM
-- 
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 1/4] avutil/x86/pixelutils: Empty MMX state in ff_pixelutils_sad_8x8_mmxext

[Henrik Gramner via ffmpeg-devel]

On Wed, Nov 1, 2023 at 10:44 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>  libavutil/x86/pixelutils.asm | 1 +
>  1 file changed, 1 insertion(+)

IIRC the emms instructions is quite slow on many systems, so if this
is the only pixelutils function using mmx it's probably better to just
rewrite it to use SSE2 instead (even if that means only using the
lower half of xmm registers).
_______________________________________________
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 1/4] avutil/x86/pixelutils: Empty MMX state in ff_pixelutils_sad_8x8_mmxext

[Andreas Rheinhardt]

Henrik Gramner via ffmpeg-devel:
> On Wed, Nov 1, 2023 at 10:44 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>  libavutil/x86/pixelutils.asm | 1 +
>>  1 file changed, 1 insertion(+)
> 
> IIRC the emms instructions is quite slow on many systems, so if this
> is the only pixelutils function using mmx it's probably better to just
> rewrite it to use SSE2 instead (even if that means only using the
> lower half of xmm registers).

Fixing it by rewriting it in a way that avoids MMX altogether is
possible, but this doesn't change the fact that all public functions
absolutely need to abide by the ABI.

- 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 1/4] avutil/x86/pixelutils: Empty MMX state in ff_pixelutils_sad_8x8_mmxext

[Andreas Rheinhardt]

Andreas Rheinhardt:
> We currently mostly do not empty the MMX state in our MMX
> DSP functions; instead we only do so before code that might
> be using x87 code. This is a violation of the System V i386 ABI
> (and maybe of other ABIs, too):
> "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." (See 2.2.1 in [1])
> This patch does not intend to change all these functions to abide
> by the ABI; it only does so for ff_pixelutils_sad_8x8_mmxext, as this
> function can by called by external users, because it is exported
> via the pixelutils API. Without this, the following fragment will
> assert (on x86/x64):
>     uint8_t src1[8 * 8], src2[8 * 8];
>     av_pixelutils_sad_fn fn = av_pixelutils_get_sad_fn(3, 3, 0, NULL);
>     fn(src1, 8, src2, 8);
>     av_assert0_fpu();
> 
> [1]: https://raw.githubusercontent.com/wiki/hjl-tools/x86-psABI/intel386-psABI-1.1.pdf
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/x86/pixelutils.asm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavutil/x86/pixelutils.asm b/libavutil/x86/pixelutils.asm
> index fbe9b45971..0bcccb51f5 100644
> --- a/libavutil/x86/pixelutils.asm
> +++ b/libavutil/x86/pixelutils.asm
> @@ -43,6 +43,7 @@ cglobal pixelutils_sad_8x8, 4,4,0, src1, stride1, src2, stride2
>      lea         src2q, [src2q + 2*stride2q]
>  %endrep
>      movd        eax, m2
> +    emms
>      RET
>  
>  ;-------------------------------------------------------------------------------

Will apply 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".