Hi, On Mon, Oct 24, 2016 at 8:47 AM, wm4 <nfx...@googlemail.com> wrote:
> On Mon, 24 Oct 2016 07:54:47 -0400 > "Ronald S. Bultje" <rsbul...@gmail.com> wrote: > > > Hi, > > > > On Mon, Oct 24, 2016 at 3:36 AM, wm4 <nfx...@googlemail.com> wrote: > > > > > On Sun, 23 Oct 2016 13:02:01 -0400 > > > "Ronald S. Bultje" <rsbul...@gmail.com> wrote: > > > > > > > Hi, > > > > > > > > On Sat, Oct 22, 2016 at 11:36 PM, Michael Niedermayer < > > > > mich...@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 > > > > > <mich...@niedermayer.cc > > > > > > > wrote: > > > > > > > > > > > > > Signed-off-by: Michael Niedermayer <mich...@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(AVCodecConte > xt > > > > > *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.) > > > > > > I'm under the impression that it's part of the calling convention. That > is, > > any code anywhere (including mmx intrinsics, indeed) can - when called - > > expect the state to be cleared by the caller, just like you'd expect > > eax/edx to be caller-save (whereas esi/edi are callee-save). > > > > However, if you never call external code (including intrinsics), you can > > ignore this, just as you can ignore / create your own calling > > convention (remember fastcall etc.?). However, when calling any external > > code, this could (in theory) crash; it's just that right now it only > > crashes with musl when calling malloc/free. So basically, ffmpeg has its > > own calling convention, and manually calling emms_c() fixes "ffmpeg" > > calling convention to be compatible with "standard" calling > convention...? > > It can't really be a calling convention unless the compiler is aware of > it? > > We're doing things behind the compiler's back here. The safest > assumption would be that leaving the FPU state invalid while in C is > not allowed, period. > > The next safest assumption is that it's fine as long as we explicitly > don't use floating point or call external functions that aren't > MMX-aware. This would mean calling any libc functions or user callbacks > (including indirectly through e.g. av_log) is not fine. > Yes. And while I don't agree with it, I feel we should also mention Carl Eugen's alternative proposal, which is to document that ffmpeg doesn't formally adhere to this calling convention requirement [1]. Of course this is not as easy to verify as adding a hack-assert to > av_malloc/av_free. I agree. (Which is why I insist that the design of this particular "fix" is so important.) Ronald [1] https://patchwork.ffmpeg.org/patch/841/ _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
