[FFmpeg-devel] Patch: Inline asm fixes for Intel compiler on Windows

Reimar Döffinger Reimar.Doeffinger at gmx.de
Sun Feb 9 23:21:22 CET 2014


On Thu, Feb 06, 2014 at 12:35:13PM +1100, Matt Oliver wrote:
> On 6 February 2014 05:45, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> > On Wed, Feb 05, 2014 at 11:41:38AM +1100, Matt Oliver wrote:
> > > Cheers for the feedback. Ill split up the patches to make things easier.
> > To
> > > start off I have split the libmpcodec patches into some smaller easily
> > > digestible chunks.
> > >
> > > Patch 1: Changes a couple of inline asm labels for using named labels to
> > > standard numbered labels. This has no affect but to allow for compilation
> > > with compilers that dont support named labels.
> >
> > If you use numbered labels I believe you need to append the jump
> > direction.
> > I.e.
> > jnz 1b
> > instead of
> > jnz 1
> > if you mean the 1: label that comes before.
> > At least that is what I see in e.g. libavcodec/x86/snowdsp.c and many
> > more files.
> >
> > > > Well, having the DECLARE_ALIGNED changes separately would be nice
> > since I
> > > think that part can be applied without further review.
> > >
> > > Patch 2: Is just the declare aligned fixes. This uses the existing cross
> > > platform specific declare aligned macro instead of the gcc only
> > attribute.
> > > Seperated as Reimar suggested.
> >
> > You seem to have lost the very first "static". Otherwise it looks ok to
> > me.
> >
> > > Patch 3: Removes unnecessary commas from some inline asm. Leaving the
> > > commas in with nothing following them causes icl compilation errors.
> > > removing them has no impact on anything else as ive seen this used else
> > > where in ffmpeg so this should have no impact.
> >
> > Looks good to me as well.
> > If nobody has any other comments I will try to get it applied to MPlayer
> > soon to try to keep the codebases in sync (or if I'm too slow
> > anyone with commit access there is of course free to apply them).
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> > If you use numbered labels I believe you need to append the jump
> > direction.
> 
> You only have to do this if you have multiple labels with the same name. In
> this case the forward/backward identifiers are so the assembler knows which
> label you are referring to (closest in front or closest behind). Since the
> labels in the patch are all unique then there is only one so we dont need
> to specify which direction to find it.

I don't trust that this is always true, and I see no reason to do things
different than all other places I could find.
So I changed that.

> > You seem to have lost the very first "static". Otherwise it looks ok to
> > me.
> 
> That I did, fixed in updated patch attached. I changed to using the
> predefined ASM_CONSTANT macro that auto includes static const.

You changed one uint8_t to int8_t, I fixed that before applying.
Otherwise all patches have been applied to MPlayer's libmpcodecs.
I have _not_ synced the FFmpeg code, if someone can easily do that
please go ahead.


More information about the ffmpeg-devel mailing list