x86: mpegvideo: Move mmx functions under HAVE_MMX_INLINE

Message ID 1373220621-61284-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö July 7, 2013, 6:10 p.m.
From: Michael Niedermayer <michaelni@gmx.at>

This avoids enabling these functions if --disable-mmx is specified.
In that setup, emms_c() doesn't do anything, which could lead to
clobbered fpu registers if these functions were to be used.
---
There's a number of other asm snippets under HAVE_INLINE_ASM
within the codebase that probably should be changed to the
corresponding instruction set defines as well.
---
 libavcodec/x86/mpegvideo.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Luca Barbato July 7, 2013, 6:56 p.m. | #1
On 07/07/2013 08:10 PM, Martin Storsjö wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
> 
> This avoids enabling these functions if --disable-mmx is specified.
> In that setup, emms_c() doesn't do anything, which could lead to
> clobbered fpu registers if these functions were to be used.

Don't we have a yasm version for it?


lu
Derek Buitenhuis July 7, 2013, 6:56 p.m. | #2
On 7/7/2013 2:56 PM, Luca Barbato wrote:
> Don't we have a yasm version for it?

We do: http://git.libav.org/?p=libav.git;a=blob;f=libavutil/x86/emms.asm;h=a6851acc9906fcf262aa951ace9a8756c16a7f30;hb=HEAD

- Derek
Martin Storsjö July 7, 2013, 6:58 p.m. | #3
On Sun, 7 Jul 2013, Derek Buitenhuis wrote:

> On 7/7/2013 2:56 PM, Luca Barbato wrote:
>> Don't we have a yasm version for it?
>
> We do: http://git.libav.org/?p=libav.git;a=blob;f=libavutil/x86/emms.asm;h=a6851acc9906fcf262aa951ace9a8756c16a7f30;hb=HEAD

Yes, which is only used within HAVE_MMX_EXTERNAL, which is disabled by 
--disable-mmx. So with --disable-mmx, we don't provide any implementation 
of emms_c(), while there still can be mmx assembly functions enabled that 
are only guarded within HAVE_INLINE_ASM.

// Martin
Diego Biurrun July 7, 2013, 6:58 p.m. | #4
On 2013-07-07 20:10, Martin Storsjö wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
>
> This avoids enabling these functions if --disable-mmx is specified.
> In that setup, emms_c() doesn't do anything, which could lead to
> clobbered fpu registers if these functions were to be used.
> ---
> There's a number of other asm snippets under HAVE_INLINE_ASM
> within the codebase that probably should be changed to the
> corresponding instruction set defines as well.
> ---
>   libavcodec/x86/mpegvideo.c |    8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

It seems pretty arbitrary to just change these two blocks.
I'd say either adjust everything or nothing.

Diego
Diego Biurrun July 7, 2013, 7 p.m. | #5
On 2013-07-07 20:58, Martin Storsjö wrote:
> On Sun, 7 Jul 2013, Derek Buitenhuis wrote:
>> On 7/7/2013 2:56 PM, Luca Barbato wrote:
>>> Don't we have a yasm version for it?
>>
>> We do:
>> http://git.libav.org/?p=libav.git;a=blob;f=libavutil/x86/emms.asm;h=a6851acc9906fcf262aa951ace9a8756c16a7f30;hb=HEAD
>
> Yes, which is only used within HAVE_MMX_EXTERNAL, which is disabled by
> --disable-mmx. So with --disable-mmx, we don't provide any
> implementation of emms_c(), while there still can be mmx assembly
> functions enabled that are only guarded within HAVE_INLINE_ASM.

That would be a bug in some of the init functions then; they should 
check for mmx.

Diego
Martin Storsjö July 7, 2013, 7:03 p.m. | #6
On Sun, 7 Jul 2013, Diego Biurrun wrote:

> On 2013-07-07 20:58, Martin Storsjö wrote:
>> On Sun, 7 Jul 2013, Derek Buitenhuis wrote:
>>> On 7/7/2013 2:56 PM, Luca Barbato wrote:
>>>> Don't we have a yasm version for it?
>>> 
>>> We do:
>>> http://git.libav.org/?p=libav.git;a=blob;f=libavutil/x86/emms.asm;h=a6851acc9906fcf262aa951ace9a8756c16a7f30;hb=HEAD
>> 
>> Yes, which is only used within HAVE_MMX_EXTERNAL, which is disabled by
>> --disable-mmx. So with --disable-mmx, we don't provide any
>> implementation of emms_c(), while there still can be mmx assembly
>> functions enabled that are only guarded within HAVE_INLINE_ASM.
>
> That would be a bug in some of the init functions then; they should check for 
> mmx.

Yes, and as this patch points out, there are such bugs. I did a cursory 
grep for other cases of HAVE_INLINE_ASM, and there's a bunch of them, not 
all of them quite as simple as this one to fix up.

// Martin

Patch

diff --git a/libavcodec/x86/mpegvideo.c b/libavcodec/x86/mpegvideo.c
index 68f3b62..22c66b3 100644
--- a/libavcodec/x86/mpegvideo.c
+++ b/libavcodec/x86/mpegvideo.c
@@ -26,7 +26,7 @@ 
 #include "libavcodec/mpegvideo.h"
 #include "dsputil_x86.h"
 
-#if HAVE_INLINE_ASM
+#if HAVE_MMX_INLINE
 
 static void dct_unquantize_h263_intra_mmx(MpegEncContext *s,
                                   int16_t *block, int n, int qscale)
@@ -552,11 +552,11 @@  static void  denoise_dct_sse2(MpegEncContext *s, int16_t *block){
     );
 }
 
-#endif /* HAVE_INLINE_ASM */
+#endif /* HAVE_MMX_INLINE */
 
 av_cold void ff_MPV_common_init_x86(MpegEncContext *s)
 {
-#if HAVE_INLINE_ASM
+#if HAVE_MMX_INLINE
     int mm_flags = av_get_cpu_flags();
 
     if (mm_flags & AV_CPU_FLAG_MMX) {
@@ -574,5 +574,5 @@  av_cold void ff_MPV_common_init_x86(MpegEncContext *s)
                 s->denoise_dct= denoise_dct_mmx;
         }
     }
-#endif /* HAVE_INLINE_ASM */
+#endif /* HAVE_MMX_INLINE */
 }