[FFmpeg-devel] [PATCH] avutil/x86/emms: Document the emms_c() vs alloc/free relation.

[wm4]

On Mon, 24 Oct 2016 18:08:11 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Mon, Oct 24, 2016 at 02:36:23PM +0200, wm4 wrote:
> > On Mon, 24 Oct 2016 13:24:38 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Mon, Oct 24, 2016 at 09:38:00AM +0200, wm4 wrote:  
> > > > On Sun, 23 Oct 2016 05:37:25 +0200
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libavutil/x86/emms.h | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/libavutil/x86/emms.h b/libavutil/x86/emms.h
> > > > > index 6fda6e2..42c18e2 100644
> > > > > --- a/libavutil/x86/emms.h
> > > > > +++ b/libavutil/x86/emms.h
> > > > > @@ -31,6 +31,8 @@ void avpriv_emms_yasm(void);
> > > > >   * Empty mmx state.
> > > > >   * this must be called between any dsp function and float/double code.
> > > > >   * for example sin(); dsp->idct_put(); emms_c(); cos()
> > > > > + * Note, *alloc() and *free() also use float code in some libc implementations
> > > > > + * thus this also applies to them or any function using them.
> > > > >   */
> > > > >  static av_always_inline void emms_c(void)
> > > > >  {    
> > > > 
> > > > Overly specific and useless information. It's an implementation detail    
> > > 
> > > If we place emms_c() so as to ensure that the FPU state is clean
> > > before all calls to *alloc() and *free() (as is done in the posted
> > > patchset) then we need to document this
> > > so others working on the code are aware of it and wont mistakly break
> > > it.
> > > 
> > > If/when a different or more complete solution is implemented then this
> > > note needs to be adjusted accordingly.
> > > 
> > > [...]  
> > 
> > That fixes only the musl-specific issue. It could happen any time again
> > with a set of different callers.  
> 
> yes
> 
> 
> > Unless you want to put special effort
> > into fixing operation with musl (and keeping it working) it won't help
> > much to fix the correctness issues.  
> 
> i did want to do that, but ive a headache atm and the discussion
> about emms yesterday makes me not want to touch emms anymore
> 
> 
> > 
> > If anything, it should probably say "all external" calls or such.  
> 
> I see this as the long term goal, yes i agree it should be
> be documented if theres consensus about it
> 
> The way i imagined it was to fix the practically relevant issues and
> anything else thats easy or attached (like x86-64 for what is the same
> on x86-32) before 3.2 and have 3.2 work with musl on all archs
> then aim at fixing the rest over the following months unless some
> roadblocks like performance issues are hit.
> 
> This incremental approuch has been hit with quite some opposition so
> ill leave this to whoever likes to work on this

Nobody said it's going to be simple. Though personally I wouldn't mind
spamming emms() everywhere to make it somewhat more correct - just
don't pretend it actually fixes the problem.

There are some other "brutal" solutions:
A) require musl users to build with MMX disabled or have them set an
   environment variable that disables MMX at runtime as part of the CPU
   detection
B) add a compile time option that runs emms() unconditionally at the end
   of each mmx asm block

Since musl intentionally evades detection, neither can be enabled
automatically, probably. It would be interesting to see what the speed
impact of B) actually is.