Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configure: Set the correct x32 machine option for yasm #200

Merged
merged 1 commit into from
Oct 10, 2015

Conversation

luke-jr
Copy link
Contributor

@luke-jr luke-jr commented Oct 10, 2015

No description provided.

@astiob
Copy link
Member

astiob commented Oct 10, 2015

Thanks! I’m afraid none of us have a x32 setup to test this (anyone correct me if I’m wrong), but if your patch does genuinely fix the build for you, I’ll accept it as is.

I have to ask though: have you tested the resulting binary to make sure it not only compiles but also actually works? I have some doubts regarding the interaction between the C pointer and integer types and the assembler code.

@luke-jr
Copy link
Contributor Author

luke-jr commented Oct 10, 2015

Best I was able to do so far was use the included test program with an image and ASS file. That did not crash, and reported something like 2 overlays merged (indicating it used assembly code, I think?). Anything else I could test with easily (ie, without needing a bunch of other codecs that don't work on x32 yet..)?

@astiob
Copy link
Member

astiob commented Oct 10, 2015

I’ll whip up a test ASS file and a comparison image in a minute.

@astiob
Copy link
Member

astiob commented Oct 10, 2015

Here, try this ASS file: https://gist.github.com/astiob/b4da312a8ec4d66d66fd
Specify 0 (or any number below 10) for the timestamp.
The output PNG should look like this: http://i.minus.com/ik71bos8rTP3.png

Wow, sorry this took so long.

@astiob astiob added the build label Oct 10, 2015
@luke-jr
Copy link
Contributor Author

luke-jr commented Oct 10, 2015

Not quite. I ended up with http://imgur.com/8QRprDw

@luke-jr
Copy link
Contributor Author

luke-jr commented Oct 10, 2015

Note: I get that same result on my regular 32-bit system too...

@astiob
Copy link
Member

astiob commented Oct 10, 2015

Oh, I messed up the resolution. I’m fixing the gist, but it looks like it’s probably working.

@astiob
Copy link
Member

astiob commented Oct 10, 2015

Gist updated, new expected image: http://i.minus.com/ibeU8qGj4JtMn3.png

@astiob
Copy link
Member

astiob commented Oct 10, 2015

Sorry for the trouble, and thanks again! One last thing: would you mind rebasing your patch on top of our master branch? (This way we could have it marked as “merged” on GitHub without an actual merge commit, which IMO is rather pointless for small changes like this.)

@luke-jr
Copy link
Contributor Author

luke-jr commented Oct 10, 2015

Perfect match.

Rebased.

@astiob astiob merged commit 4778001 into libass:master Oct 10, 2015
@astiob
Copy link
Member

astiob commented Oct 10, 2015

Thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants