Re: [FFmpeg-devel] [PATCH] avcodec/mips/aaccoder_mips: Remove MIPS-specific aaccoder
Andreas Rheinhardt:
> ff_aac_coder_init_mips() modifies a static const structure of
> function pointers. This will crash if the binary uses relro
> and is a data race in any case.
>
> Furthermore it points to a maintainability issue: The
> AACCoefficientsEncoder structures have been constified
> in commit fd9212f2edfe9b107c3c08ba2df5fd2cba5ab9e3,
> a Libav commit merged in 318778de9ebec276cb9dfc65509231ca56590d13.
> Libav did not have the MIPS-specific AAC code and so this was
> fine for them; yet FFmpeg had them, but this was not recognized.
>
> Commit 75a099fc734a4ee2b1347d0a3d8c53d883b95174 points to another
> maintainability issue: Contrary to ordinary DSP code, this code
> here is way more complex and needs to be constantly kept in sync
> with the ordinary code which it mimicks and replaces. Said commit
> is the only commit actually changing aaccoder.c in the last few
> years and the same change has not been performed for the MIPS
> clone; before that, it even happened several times that the mips
> code was broken due to changes of the generic code (see commits
> 97437bd17a8c5d4135b2f3b1b299bd7bb72ce02c and
> de262d018d7d7d9c967af1dfd1b861c4b9eb2a60 or
> 860dbe0275e57cbf4228f3f653f872ff66ca596b or
> 933309a6ca0f18bf1d40e917fff455221f57fb4b or
> b65ffa316e377213c29736929beba584d0d80d7c). This might even lead
> to scenarios where someone changing non-dsp aacenc code would
> have to modify mips inline asm in order to keep them in sync.
> This is obviously a significant burden (if the AAC encoder were
> actively developed).
>
> Finally, the code does not even compile here due to errors like
> "Error: float register should be even, was 1".
>
> Signed-off-by: Andreas Rheinhardt
> ---
> libavcodec/aacenc.c |4 -
> libavcodec/aacenc.h |1 -
> libavcodec/mips/Makefile|1 -
> libavcodec/mips/aaccoder_mips.c | 2503 ---
> 4 files changed, 2509 deletions(-)
> delete mode 100644 libavcodec/mips/aaccoder_mips.c
>
> diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> index 3f99188be4..55fa307809 100644
> --- a/libavcodec/aacenc.c
> +++ b/libavcodec/aacenc.c
> @@ -1383,10 +1383,6 @@ static av_cold int aac_encode_init(AVCodecContext
> *avctx)
>
> ff_aacenc_dsp_init(>aacdsp);
>
> -#if HAVE_MIPSDSP
> -ff_aac_coder_init_mips(s);
> -#endif
> -
> ff_af_queue_init(avctx, >afq);
>
> return 0;
> diff --git a/libavcodec/aacenc.h b/libavcodec/aacenc.h
> index c18e828905..8899f90ac7 100644
> --- a/libavcodec/aacenc.h
> +++ b/libavcodec/aacenc.h
> @@ -241,7 +241,6 @@ typedef struct AACEncContext {
> } buffer;
> } AACEncContext;
>
> -void ff_aac_coder_init_mips(AACEncContext *c);
> void ff_quantize_band_cost_cache_init(struct AACEncContext *s);
>
>
> diff --git a/libavcodec/mips/Makefile b/libavcodec/mips/Makefile
> index 45c56e8ad9..50fe38a50e 100644
> --- a/libavcodec/mips/Makefile
> +++ b/libavcodec/mips/Makefile
> @@ -19,7 +19,6 @@ OBJS-$(CONFIG_AAC_DECODER)+=
> mips/aacdec_mips.o\
> mips/aacsbr_mips.o\
> mips/sbrdsp_mips.o\
> mips/aacpsdsp_mips.o
> -MIPSDSP-OBJS-$(CONFIG_AAC_ENCODER)+= mips/aaccoder_mips.o
> MIPSFPU-OBJS-$(CONFIG_AAC_ENCODER)+= mips/iirfilter_mips.o
> OBJS-$(CONFIG_HEVC_DECODER) += mips/hevcdsp_init_mips.o \
> mips/hevcpred_init_mips.o
> diff --git a/libavcodec/mips/aaccoder_mips.c b/libavcodec/mips/aaccoder_mips.c
> deleted file mode 100644
> index dd9661fbdd..00
> --- a/libavcodec/mips/aaccoder_mips.c
> +++ /dev/null
> @@ -1,2503 +0,0 @@
> -/*
> - * Copyright (c) 2012
> - * MIPS Technologies, Inc., California.
> - *
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - * 1. Redistributions of source code must retain the above copyright
> - *notice, this list of conditions and the following disclaimer.
> - * 2. Redistributions in binary form must reproduce the above copyright
> - *notice, this list of conditions and the following disclaimer in the
> - *documentation and/or other materials provided with the distribution.
> - * 3. Neither the name of the MIPS Technologies, Inc., nor the names of its
> - *contributors may be used to endorse or promote products derived from
> - *this software without specific prior written permission.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE MIPS TECHNOLOGIES, INC. ``AS IS'' AND
> - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> - * ARE DISCLAIMED. IN NO EVENT SHALL THE MIPS TECHNOLOGIES, INC. BE LIABLE
> - * FOR ANY DIRECT, INDIRECT,
Re: [FFmpeg-devel] [PATCH] avcodec/mips/aaccoder_mips: Remove MIPS-specific aaccoder
On Fri, 15 Mar 2024, at 02:20, Andreas Rheinhardt wrote: > ff_aac_coder_init_mips() modifies a static const structure of > function pointers. This will crash if the binary uses relro > and is a data race in any case. > > Furthermore it points to a maintainability issue: The > AACCoefficientsEncoder structures have been constified > in commit fd9212f2edfe9b107c3c08ba2df5fd2cba5ab9e3, > a Libav commit merged in 318778de9ebec276cb9dfc65509231ca56590d13. > Libav did not have the MIPS-specific AAC code and so this was > fine for them; yet FFmpeg had them, but this was not recognized. LGTM. -- Jean-Baptiste Kempf - President +33 672 704 734 https://jbkempf.com/ ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/mips/aaccoder_mips: Remove MIPS-specific aaccoder
Mar 15, 2024, 02:20 by andreas.rheinha...@outlook.com: > ff_aac_coder_init_mips() modifies a static const structure of > function pointers. This will crash if the binary uses relro > and is a data race in any case. > > Furthermore it points to a maintainability issue: The > AACCoefficientsEncoder structures have been constified > in commit fd9212f2edfe9b107c3c08ba2df5fd2cba5ab9e3, > a Libav commit merged in 318778de9ebec276cb9dfc65509231ca56590d13. > Libav did not have the MIPS-specific AAC code and so this was > fine for them; yet FFmpeg had them, but this was not recognized. > > Commit 75a099fc734a4ee2b1347d0a3d8c53d883b95174 points to another > maintainability issue: Contrary to ordinary DSP code, this code > here is way more complex and needs to be constantly kept in sync > with the ordinary code which it mimicks and replaces. Said commit > is the only commit actually changing aaccoder.c in the last few > years and the same change has not been performed for the MIPS > clone; before that, it even happened several times that the mips > code was broken due to changes of the generic code (see commits > 97437bd17a8c5d4135b2f3b1b299bd7bb72ce02c and > de262d018d7d7d9c967af1dfd1b861c4b9eb2a60 or > 860dbe0275e57cbf4228f3f653f872ff66ca596b or > 933309a6ca0f18bf1d40e917fff455221f57fb4b or > b65ffa316e377213c29736929beba584d0d80d7c). This might even lead > to scenarios where someone changing non-dsp aacenc code would > have to modify mips inline asm in order to keep them in sync. > This is obviously a significant burden (if the AAC encoder were > actively developed). > > Finally, the code does not even compile here due to errors like > "Error: float register should be even, was 1". > > Signed-off-by: Andreas Rheinhardt > --- > libavcodec/aacenc.c |4 - > libavcodec/aacenc.h |1 - > libavcodec/mips/Makefile|1 - > libavcodec/mips/aaccoder_mips.c | 2503 --- > 4 files changed, 2509 deletions(-) > delete mode 100644 libavcodec/mips/aaccoder_mips.c > I didn't even know we had one. Looks good to me. Most MIPS code except Loongson is unmaintained and largely untested, we should consider removing it in a few years. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] avcodec/mips/aaccoder_mips: Remove MIPS-specific aaccoder
ff_aac_coder_init_mips() modifies a static const structure of
function pointers. This will crash if the binary uses relro
and is a data race in any case.
Furthermore it points to a maintainability issue: The
AACCoefficientsEncoder structures have been constified
in commit fd9212f2edfe9b107c3c08ba2df5fd2cba5ab9e3,
a Libav commit merged in 318778de9ebec276cb9dfc65509231ca56590d13.
Libav did not have the MIPS-specific AAC code and so this was
fine for them; yet FFmpeg had them, but this was not recognized.
Commit 75a099fc734a4ee2b1347d0a3d8c53d883b95174 points to another
maintainability issue: Contrary to ordinary DSP code, this code
here is way more complex and needs to be constantly kept in sync
with the ordinary code which it mimicks and replaces. Said commit
is the only commit actually changing aaccoder.c in the last few
years and the same change has not been performed for the MIPS
clone; before that, it even happened several times that the mips
code was broken due to changes of the generic code (see commits
97437bd17a8c5d4135b2f3b1b299bd7bb72ce02c and
de262d018d7d7d9c967af1dfd1b861c4b9eb2a60 or
860dbe0275e57cbf4228f3f653f872ff66ca596b or
933309a6ca0f18bf1d40e917fff455221f57fb4b or
b65ffa316e377213c29736929beba584d0d80d7c). This might even lead
to scenarios where someone changing non-dsp aacenc code would
have to modify mips inline asm in order to keep them in sync.
This is obviously a significant burden (if the AAC encoder were
actively developed).
Finally, the code does not even compile here due to errors like
"Error: float register should be even, was 1".
Signed-off-by: Andreas Rheinhardt
---
libavcodec/aacenc.c |4 -
libavcodec/aacenc.h |1 -
libavcodec/mips/Makefile|1 -
libavcodec/mips/aaccoder_mips.c | 2503 ---
4 files changed, 2509 deletions(-)
delete mode 100644 libavcodec/mips/aaccoder_mips.c
diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
index 3f99188be4..55fa307809 100644
--- a/libavcodec/aacenc.c
+++ b/libavcodec/aacenc.c
@@ -1383,10 +1383,6 @@ static av_cold int aac_encode_init(AVCodecContext *avctx)
ff_aacenc_dsp_init(>aacdsp);
-#if HAVE_MIPSDSP
-ff_aac_coder_init_mips(s);
-#endif
-
ff_af_queue_init(avctx, >afq);
return 0;
diff --git a/libavcodec/aacenc.h b/libavcodec/aacenc.h
index c18e828905..8899f90ac7 100644
--- a/libavcodec/aacenc.h
+++ b/libavcodec/aacenc.h
@@ -241,7 +241,6 @@ typedef struct AACEncContext {
} buffer;
} AACEncContext;
-void ff_aac_coder_init_mips(AACEncContext *c);
void ff_quantize_band_cost_cache_init(struct AACEncContext *s);
diff --git a/libavcodec/mips/Makefile b/libavcodec/mips/Makefile
index 45c56e8ad9..50fe38a50e 100644
--- a/libavcodec/mips/Makefile
+++ b/libavcodec/mips/Makefile
@@ -19,7 +19,6 @@ OBJS-$(CONFIG_AAC_DECODER)+=
mips/aacdec_mips.o\
mips/aacsbr_mips.o\
mips/sbrdsp_mips.o\
mips/aacpsdsp_mips.o
-MIPSDSP-OBJS-$(CONFIG_AAC_ENCODER)+= mips/aaccoder_mips.o
MIPSFPU-OBJS-$(CONFIG_AAC_ENCODER)+= mips/iirfilter_mips.o
OBJS-$(CONFIG_HEVC_DECODER) += mips/hevcdsp_init_mips.o \
mips/hevcpred_init_mips.o
diff --git a/libavcodec/mips/aaccoder_mips.c b/libavcodec/mips/aaccoder_mips.c
deleted file mode 100644
index dd9661fbdd..00
--- a/libavcodec/mips/aaccoder_mips.c
+++ /dev/null
@@ -1,2503 +0,0 @@
-/*
- * Copyright (c) 2012
- * MIPS Technologies, Inc., California.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *notice, this list of conditions and the following disclaimer in the
- *documentation and/or other materials provided with the distribution.
- * 3. Neither the name of the MIPS Technologies, Inc., nor the names of its
- *contributors may be used to endorse or promote products derived from
- *this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE MIPS TECHNOLOGIES, INC. ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE MIPS TECHNOLOGIES, INC. BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
- * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
- * HOWEVER CAUSED AND ON ANY THEORY OF
