[FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop

[wm4]

On Sun, 23 Oct 2016 13:02:01 -0400
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> Hi,
> 
> On Sat, Oct 22, 2016 at 11:36 PM, Michael Niedermayer <
> michael at niedermayer.cc> wrote:  
> 
> > On Sat, Oct 22, 2016 at 10:10:01PM -0400, Ronald S. Bultje wrote:  
> > > Hi,
> > >
> > > general comment about all other dec patches.
> > >
> > > On Sat, Oct 22, 2016 at 3:02 PM, Michael Niedermayer  
> > <michael at niedermayer.cc  
> > > > wrote:  
> > >  
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > >  libavcodec/svq1dec.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/libavcodec/svq1dec.c b/libavcodec/svq1dec.c
> > > > index 2b72e08..0fe222e 100644
> > > > --- a/libavcodec/svq1dec.c
> > > > +++ b/libavcodec/svq1dec.c
> > > > @@ -744,6 +744,7 @@ static int svq1_decode_frame(AVCodecContext  
> > *avctx,  
> > > > void *data,
> > > >              }
> > > >          }
> > > >      }
> > > > +    emms_c();  
> > >
> > >
> > > This is hideous, you're sprinkling emms_c in various places to make some
> > > stupid test pass. The test is morbidly stupid and there is no general
> > > consensus on patterns to be followed as for where to place emms_c.  
> > Someone  
> > > who doesn't know any better will litter each new decoder with 10-20 calls
> > > to emms_c just because he found that other decoders do it in  
> > undocumented,  
> > > unexplained and unclear locations also.
> > >
> > > If you want this to be a "thing", you need to design and document  
> > carefully  
> > > where emms_c is necessary. Then come up with some system that makes this
> > > work by itself.  
> >
> > Your mail sounds quite aggressive to me, did i say something to anger
> > you ? It was certainly not intended
> >
> > About this patchset
> > FFmpeg is broken ATM (both in theory and practice with some libcs),
> > the way MMX code is used is not correct, emms_c()
> > calls are missing and required. The obvious way to fix that
> > (in practice) is adding emms_c() calls where they are missing.
> > I can document why each call is needed, if thats wanted?  
> 
> 
> Your representation of facts is strange, to say the least. Let's explore
> two related claims:
> 
> - (A) ffmpeg is broken in practice when calling musl malloc/free functions
> after calling MMX SIMD functions on x86-32 (which crashes).
> - (B) ffmpeg is broken in theory because we don't clear FPU state (as
> required) at the end of MMX SIMD functions.

I was under the impression that it is UB to have the FPU in MMX state
at any time while in C, not just while e.g. calling the stdlib. Maybe I
got that wrong (how would MMX intrinsics even work?) - can anyone shed
light on the exact requirements? (Possibly again, sorry.)