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

[wm4]

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. 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.

If anything, it should probably say "all external" calls or such.