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

[Ronald S. Bultje]

Hi,

On Sun, Oct 23, 2016 at 4:33 PM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> On Sun, Oct 23, 2016 at 01:02:01PM -0400, Ronald S. Bultje 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.
> >
> > Which are you trying to fix? You make it sound like you're fixing (B)
> (you
> > use the plural "libcs" and often use the word "required" or "standard"),
>
> All the crashes with musl are to the best of my knowledge FFmpeg
> violating the C standard.
> Iam interrested in fixing the cases where we violate the C standard,
> with a priority toward the cases that cause practical issues.
>
> musl really isnt involved in this, any C library can trigger these bugs
> in FFmpeg hence plural.


You're not answering the question. Are you fixing A or B? If you're trying
to say that your patch is aiming to fix B, then your patch is utterly and
totally inadequate and you know that.

Any SIMD function calling memcpy (a libc function) needs to have its FPU
state cleared when called, if you're claiming to fix B.

As such, your patch should be rejected on the same basis why the cabac
encoder should never have been accepted.

Ronald