[FFmpeg-devel] [PATCH 1/9] avcodec/x86/simple_idct: Empty MMX state in ff_simple_idct_mmx

[FFmpeg-devel] [PATCH 1/9] avcodec/x86/simple_idct: Empty MMX state in ff_simple_idct_mmx

[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_simple_idct_mmx(), as this
function can by called by external users, because it is exported
via AVDCT (i.e. via avcodec_dct_init()). Without this, the following
fragment will assert (in i386):
    av_force_cpu_flags(AV_CPU_FLAG_MMX | AV_CPU_FLAG_MMXEXT);
    int16_t *blk = av_malloc(64 * sizeof(*blk));
    AVDCT *avdct = avcodec_dct_alloc();
    avcodec_dct_init(avdct);
    avdct->idct(blk);
    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>
---
 libavcodec/x86/simple_idct.asm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/x86/simple_idct.asm b/libavcodec/x86/simple_idct.asm
index dcf0da6df1..e3a29efc33 100644
--- a/libavcodec/x86/simple_idct.asm
+++ b/libavcodec/x86/simple_idct.asm
@@ -845,6 +845,7 @@ INIT_MMX mmx
 
 cglobal simple_idct, 1, 2, 8, 128, block, t0
     IDCT
+    emms
 RET
 
 INIT_XMM sse2
-- 
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/9] avcodec/mjpegenc_common: Don't check luma/chroma matrices unnecessarily

[Andreas Rheinhardt]

These matrices are only used for MJPEG, not for LJPEG.
So only check them for the former. This is in preparation
for removing said matrices from LJPEG altogether
(i.e. sending NULL matrices).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/mjpegenc_common.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c
index 98c464fc62..0076e94296 100644
--- a/libavcodec/mjpegenc_common.c
+++ b/libavcodec/mjpegenc_common.c
@@ -61,15 +61,13 @@ static void jpeg_table_header(AVCodecContext *avctx, PutBitContext *p,
                               ScanTable *intra_scantable,
                               uint16_t luma_intra_matrix[64],
                               uint16_t chroma_intra_matrix[64],
-                              int hsample[3], int use_slices)
+                              int hsample[3], int use_slices, int matrices_differ)
 {
     int i, j, size;
     uint8_t *ptr;
 
     if (m) {
-        int matrix_count = 1 + !!memcmp(luma_intra_matrix,
-                                        chroma_intra_matrix,
-                                        sizeof(luma_intra_matrix[0]) * 64);
+        int matrix_count = 1 + matrices_differ;
         if (m->force_duplicated_matrix)
             matrix_count = 2;
         /* quant matrixes */
@@ -285,9 +283,7 @@ void ff_mjpeg_encode_picture_header(AVCodecContext *avctx, PutBitContext *pb,
     const int lossless = !m;
     int hsample[4], vsample[4];
     int components = 3 + (avctx->pix_fmt == AV_PIX_FMT_BGRA);
-    int chroma_matrix = !!memcmp(luma_intra_matrix,
-                                 chroma_intra_matrix,
-                                 sizeof(luma_intra_matrix[0])*64);
+    int chroma_matrix;
 
     ff_mjpeg_init_hvsample(avctx, hsample, vsample);
 
@@ -299,9 +295,12 @@ void ff_mjpeg_encode_picture_header(AVCodecContext *avctx, PutBitContext *pb,
 
     jpeg_put_comments(avctx, pb, frame);
 
+    chroma_matrix = !lossless && !!memcmp(luma_intra_matrix,
+                                          chroma_intra_matrix,
+                                          sizeof(luma_intra_matrix[0]) * 64);
     jpeg_table_header(avctx, pb, m, intra_scantable,
                       luma_intra_matrix, chroma_intra_matrix, hsample,
-                      use_slices);
+                      use_slices, chroma_matrix);
 
     switch (avctx->codec_id) {
     case AV_CODEC_ID_MJPEG:  put_marker(pb, SOF0 ); 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 3/9] avcodec/ljpegenc: Remove unused IDCTDSPContext

[Andreas Rheinhardt]

It is basically write-only.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 configure             |  2 +-
 libavcodec/ljpegenc.c | 14 ++------------
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/configure b/configure
index 957b7fe13e..ab6ff27249 100755
--- a/configure
+++ b/configure
@@ -2869,7 +2869,7 @@ ipu_decoder_select="mpegvideodec"
 jpegls_decoder_select="mjpeg_decoder"
 jv_decoder_select="blockdsp"
 lagarith_decoder_select="llviddsp"
-ljpeg_encoder_select="idctdsp jpegtables"
+ljpeg_encoder_select="jpegtables"
 lscr_decoder_select="inflate_wrapper"
 magicyuv_decoder_select="llviddsp"
 magicyuv_encoder_select="llvidencdsp"
diff --git a/libavcodec/ljpegenc.c b/libavcodec/ljpegenc.c
index a708d71220..4b88218990 100644
--- a/libavcodec/ljpegenc.c
+++ b/libavcodec/ljpegenc.c
@@ -33,22 +33,16 @@
 #include "libavutil/frame.h"
 #include "libavutil/mem.h"
 #include "libavutil/opt.h"
-#include "libavutil/pixdesc.h"
 
 #include "avcodec.h"
 #include "codec_internal.h"
 #include "encode.h"
-#include "idctdsp.h"
 #include "jpegtables.h"
-#include "mathops.h"
 #include "mjpegenc_common.h"
 #include "mjpeg.h"
 
 typedef struct LJpegEncContext {
     AVClass *class;
-    IDCTDSPContext idsp;
-    ScanTable scantable;
-    uint16_t matrix[64];
 
     int vsample[4];
     int hsample[4];
@@ -240,8 +234,8 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
 
     init_put_bits(&pb, pkt->data, pkt->size);
 
-    ff_mjpeg_encode_picture_header(avctx, &pb, pict, NULL, &s->scantable,
-                                   s->pred, s->matrix, s->matrix, 0);
+    ff_mjpeg_encode_picture_header(avctx, &pb, pict, NULL, NULL,
+                                   s->pred, NULL, NULL, 0);
 
     header_bits = put_bits_count(&pb);
 
@@ -287,10 +281,6 @@ static av_cold int ljpeg_encode_init(AVCodecContext *avctx)
     if (!s->scratch)
         return AVERROR(ENOMEM);
 
-    ff_idctdsp_init(&s->idsp, avctx);
-    ff_init_scantable(s->idsp.idct_permutation, &s->scantable,
-                      ff_zigzag_direct);
-
     ff_mjpeg_init_hvsample(avctx, s->hsample, s->vsample);
 
     ff_mjpeg_build_huffman_codes(s->huff_size_dc_luminance,
-- 
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/9] avcodec/ljpegenc: Remove unnecessary emms_c()

[Andreas Rheinhardt]

This encoder does not use any DSP function at all.

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

diff --git a/libavcodec/ljpegenc.c b/libavcodec/ljpegenc.c
index 4b88218990..81c52a7c78 100644
--- a/libavcodec/ljpegenc.c
+++ b/libavcodec/ljpegenc.c
@@ -248,8 +248,6 @@ static int ljpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
     if (ret < 0)
         return ret;
 
-    emms_c();
-
     ff_mjpeg_escape_FF(&pb, header_bits >> 3);
     ff_mjpeg_encode_picture_trailer(&pb, header_bits);
 
-- 
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 5/9] avcodec/asvdec: Remove unnecessary emms_c()

[Andreas Rheinhardt]

This codec uses BswapDSP, BlockDSP and IDCTDSP.
The former never used MMX, the latter does not use it
for idct_put since bfb28b5ce89f3e950214b67ea95b45e3355c2caf
and BlockDSP does not use it since commit
ee551a21ddcbf81afe183d9489c534ee80f263a0.
Therefore this emms_c() is can be removed.

(It was actually always redundant, because its caller
(decode_simple_internal()) calls emms_c() itself afterwards.)

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

diff --git a/libavcodec/asvdec.c b/libavcodec/asvdec.c
index 7dafc115b3..be89544732 100644
--- a/libavcodec/asvdec.c
+++ b/libavcodec/asvdec.c
@@ -293,8 +293,6 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p,
 
     *got_frame = 1;
 
-    emms_c();
-
     return (get_bits_count(&a->gb) + 31) / 32 * 4;
 }
 
-- 
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 6/9] avcodec/me_cmp: Mark ff_square_tab as hidden

[Andreas Rheinhardt]

ff_square_tab is always used with an offset; if this table
is marked as hidden, the compiler can infer that it and
therefore also ff_square_tab + 256 have a fixed offset
from the code. This allows to avoid performing "+ 256"
at runtime by baking it into the offset from the code to
the table.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/me_cmp.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/me_cmp.h b/libavcodec/me_cmp.h
index c6de2d0061..90ea76c891 100644
--- a/libavcodec/me_cmp.h
+++ b/libavcodec/me_cmp.h
@@ -21,9 +21,11 @@
 
 #include <stdint.h>
 
+#include "libavutil/attributes_internal.h"
+
 #include "avcodec.h"
 
-extern const uint32_t ff_square_tab[512];
+extern const uint32_t attribute_visibility_hidden ff_square_tab[512];
 
 
 /* minimum alignment rules ;)
-- 
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 7/9] avcodec/vc2enc: Don't use bitcount when byte-aligned

[Andreas Rheinhardt]

(There is a small issue that is now being treated differently:
The earlier code would record a position in a buffer that
is being written to via put_bits(), then write data,
then overwrite the byte at the position recorded earlier
and only then flush the PutBitContext. In case there was
no writeout in the meantime, said flush would overwrite
what one has just written. This never happened in my tests,
but maybe it can happen. In this case this commit fixes
this issue by flushing before overwriting the old data.)

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

diff --git a/libavcodec/vc2enc.c b/libavcodec/vc2enc.c
index 5cb6e0d198..82d11462aa 100644
--- a/libavcodec/vc2enc.c
+++ b/libavcodec/vc2enc.c
@@ -233,7 +233,7 @@ static void encode_parse_info(VC2EncContext *s, enum DiracParseCodes pcode)
 
     align_put_bits(&s->pb);
 
-    cur_pos = put_bits_count(&s->pb) >> 3;
+    cur_pos = put_bytes_count(&s->pb, 0);
 
     /* Magic string */
     ff_put_string(&s->pb, "BBCD", 0);
@@ -746,7 +746,7 @@ static int encode_hq_slice(AVCodecContext *avctx, void *arg)
     /* Luma + 2 Chroma planes */
     for (p = 0; p < 3; p++) {
         int bytes_start, bytes_len, pad_s, pad_c;
-        bytes_start = put_bits_count(pb) >> 3;
+        bytes_start = put_bytes_count(pb, 0);
         put_bits(pb, 8, 0);
         for (level = 0; level < s->wavelet_depth; level++) {
             for (orientation = !!level; orientation < 4; orientation++) {
@@ -755,10 +755,10 @@ static int encode_hq_slice(AVCodecContext *avctx, void *arg)
                                quants[level][orientation]);
             }
         }
-        align_put_bits(pb);
-        bytes_len = (put_bits_count(pb) >> 3) - bytes_start - 1;
+        flush_put_bits(pb);
+        bytes_len = put_bytes_output(pb) - bytes_start - 1;
         if (p == 2) {
-            int len_diff = slice_bytes_max - (put_bits_count(pb) >> 3);
+            int len_diff = slice_bytes_max - put_bytes_output(pb);
             pad_s = FFALIGN((bytes_len + len_diff), s->size_scaler)/s->size_scaler;
             pad_c = (pad_s*s->size_scaler) - bytes_len;
         } else {
@@ -766,7 +766,6 @@ static int encode_hq_slice(AVCodecContext *avctx, void *arg)
             pad_c = (pad_s*s->size_scaler) - bytes_len;
         }
         pb->buf[bytes_start] = pad_s;
-        flush_put_bits(pb);
         /* vc2-reference uses that padding that decodes to '0' coeffs */
         memset(put_bits_ptr(pb), 0xFF, pad_c);
         skip_put_bytes(pb, pad_c);
-- 
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 8/9] avcodec/speedhqenc: Remove unnecessary headers

[Andreas Rheinhardt]

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

diff --git a/libavcodec/speedhqenc.h b/libavcodec/speedhqenc.h
index 5100bb2d34..0c52e6a380 100644
--- a/libavcodec/speedhqenc.h
+++ b/libavcodec/speedhqenc.h
@@ -31,10 +31,7 @@
 
 #include <stdint.h>
 
-#include "mjpeg.h"
-#include "mjpegenc_common.h"
 #include "mpegvideo.h"
-#include "put_bits.h"
 
 int  ff_speedhq_encode_init(MpegEncContext *s);
 void ff_speedhq_encode_close(MpegEncContext *s);
-- 
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 9/9] avcodec/mjpegenc_common: Don't flush unnecessarily

[Andreas Rheinhardt]

The PutBitContext has already been flushed a few lines above
and nothing has been written to it in the meantime.

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

diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c
index 0076e94296..c37c964931 100644
--- a/libavcodec/mjpegenc_common.c
+++ b/libavcodec/mjpegenc_common.c
@@ -423,7 +423,6 @@ void ff_mjpeg_escape_FF(PutBitContext *pb, int start)
 
     if(ff_count==0) return;
 
-    flush_put_bits(pb);
     skip_put_bytes(pb, ff_count);
 
     for(i=size-1; ff_count; i--){
-- 
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/9] avcodec/x86/simple_idct: Empty MMX state in ff_simple_idct_mmx

[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_simple_idct_mmx(), as this
> function can by called by external users, because it is exported
> via AVDCT (i.e. via avcodec_dct_init()). Without this, the following
> fragment will assert (in i386):
>     av_force_cpu_flags(AV_CPU_FLAG_MMX | AV_CPU_FLAG_MMXEXT);
>     int16_t *blk = av_malloc(64 * sizeof(*blk));
>     AVDCT *avdct = avcodec_dct_alloc();
>     avcodec_dct_init(avdct);
>     avdct->idct(blk);
>     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>
> ---
>  libavcodec/x86/simple_idct.asm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/x86/simple_idct.asm b/libavcodec/x86/simple_idct.asm
> index dcf0da6df1..e3a29efc33 100644
> --- a/libavcodec/x86/simple_idct.asm
> +++ b/libavcodec/x86/simple_idct.asm
> @@ -845,6 +845,7 @@ INIT_MMX mmx
>  
>  cglobal simple_idct, 1, 2, 8, 128, block, t0
>      IDCT
> +    emms
>  RET
>  
>  INIT_XMM sse2

Will apply 2-9 tomorrow unless there are objections. Feedback for #1 is
very welcome.

- 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/9] avcodec/x86/simple_idct: Empty MMX state in ff_simple_idct_mmx

[Michael Niedermayer]

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

On Thu, Oct 06, 2022 at 02:54:30AM +0200, Andreas Rheinhardt wrote:
> 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_simple_idct_mmx(), as this
> function can by called by external users, because it is exported
> via AVDCT (i.e. via avcodec_dct_init()). Without this, the following
> fragment will assert (in i386):
>     av_force_cpu_flags(AV_CPU_FLAG_MMX | AV_CPU_FLAG_MMXEXT);
>     int16_t *blk = av_malloc(64 * sizeof(*blk));
>     AVDCT *avdct = avcodec_dct_alloc();
>     avcodec_dct_init(avdct);
>     avdct->idct(blk);
>     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>
> ---
>  libavcodec/x86/simple_idct.asm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/x86/simple_idct.asm b/libavcodec/x86/simple_idct.asm
> index dcf0da6df1..e3a29efc33 100644
> --- a/libavcodec/x86/simple_idct.asm
> +++ b/libavcodec/x86/simple_idct.asm
> @@ -845,6 +845,7 @@ INIT_MMX mmx
>  
>  cglobal simple_idct, 1, 2, 8, 128, block, t0
>      IDCT
> +    emms
>  RET

as comments where requested

What speed impact does this have on old CPUs ?
can this be made compile time optional so someone building for an
old machiene can avoid the speedhit while ignoring the ABI (which
worked fine)
(this case would then require a 2nd function with the emms for AVDCT)

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates

[-- 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".