[FFmpeg-devel] [PATCH] avcodec/decode: Reset MMX state for receive_frame decoders, too

[FFmpeg-devel] [PATCH] avcodec/decode: Reset MMX state for receive_frame decoders, too

[Andreas Rheinhardt]

FFmpeg's assembly code currently does not abide by the
plattform-specific ABIs wrt its handling of the X86 MMX flag:
Resetting the MMX state is deferred to avoid doing it multiple times
instead of ensuring that the CPU is in floating point state
upon return from any function.

Furthermore, resetting said state is sometimes done generically,
namely for all the decoders using the ordinary decode callback;
yet this is not done for the decoders using the receive_frame API.

This led to problems when MJPEG (and the MJPEG-based decoders)
were switched to the receive_frame API in commit
e9a2a8777317d91af658f774c68442ac4aa726ec, because ff_mjpeg_decode_sos()
only resets the MMX state on success, not on failure.
Such issues are probably still possible with SMVJPEG, which still
uses the receive_frame API. See issue #10210.

This commit therefore also resets the MMX state for
the receive_frame API to avoid any more surprises of this sort.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Of course, I am in favour of actually abiding by the ABI,
even if it would cost performance; notice that there are
currently scenarios where emms_c() is called unnecessarily
(it is called generically and therefore even for codecs
that don't need it; furthermore, for some codecs it depends upon
the CPU flags used whether MMX code is in use, so it is unnecessary
for newer CPUs).

 libavcodec/decode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index d1ba7f167f..22a0f8eb25 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -547,6 +547,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
 
     if (codec->cb_type == FF_CODEC_CB_TYPE_RECEIVE_FRAME) {
         ret = codec->cb.receive_frame(avctx, frame);
+        emms_c();
     } else
         ret = decode_simple_receive_frame(avctx, frame);
 
-- 
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] avcodec/decode: Reset MMX state for receive_frame decoders, too

[Anton Khirnov]

Quoting Andreas Rheinhardt (2023-03-14 03:53:40)
> FFmpeg's assembly code currently does not abide by the
> plattform-specific ABIs wrt its handling of the X86 MMX flag:
> Resetting the MMX state is deferred to avoid doing it multiple times
> instead of ensuring that the CPU is in floating point state
> upon return from any function.
> 
> Furthermore, resetting said state is sometimes done generically,
> namely for all the decoders using the ordinary decode callback;
> yet this is not done for the decoders using the receive_frame API.
> 
> This led to problems when MJPEG (and the MJPEG-based decoders)
> were switched to the receive_frame API in commit
> e9a2a8777317d91af658f774c68442ac4aa726ec, because ff_mjpeg_decode_sos()
> only resets the MMX state on success, not on failure.
> Such issues are probably still possible with SMVJPEG, which still
> uses the receive_frame API. See issue #10210.
> 
> This commit therefore also resets the MMX state for
> the receive_frame API to avoid any more surprises of this sort.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Of course, I am in favour of actually abiding by the ABI,
> even if it would cost performance; notice that there are
> currently scenarios where emms_c() is called unnecessarily
> (it is called generically and therefore even for codecs
> that don't need it; furthermore, for some codecs it depends upon
> the CPU flags used whether MMX code is in use, so it is unnecessary
> for newer CPUs).

Ideally we'd kill off all MMX code and forget about such issues, but
that's presumably way easier said than done.

> 
>  libavcodec/decode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index d1ba7f167f..22a0f8eb25 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -547,6 +547,7 @@ static int decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>  
>      if (codec->cb_type == FF_CODEC_CB_TYPE_RECEIVE_FRAME) {
>          ret = codec->cb.receive_frame(avctx, frame);
> +        emms_c();

Should be ok.

-- 
Anton Khirnov
_______________________________________________
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".