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

Osc lua error #1729

Closed
gipawu opened this issue Mar 27, 2015 · 9 comments
Closed

Osc lua error #1729

gipawu opened this issue Mar 27, 2015 · 9 comments

Comments

@gipawu
Copy link

gipawu commented Mar 27, 2015

I compiled MPV (tried both 0.8.2 and 0.8.3 version) in Fedora 22 but OSD is not displayed. I get this error:

[osc] stack traceback:
[osc]   [C]: in function 'mp.set_osd_ass'
[osc]   @osc.lua:1779: in function 'render'
[osc]   @osc.lua:1903: in function 'tick'
[osc]   @osc.lua:1946: in local 'prop'
[osc]   mp.defaults:342: in local 'handler'
[osc]   mp.defaults:418: in upvalue 'call_event_handlers'
[osc]   mp.defaults:454: in function 'mp.dispatch_events'
[osc]   mp.defaults:411: in function 'mp_event_loop'
[osc]   [C]: in ?
[osc]   [C]: in ?
[osc] Lua error: @osc.lua:1779: bad argument #1 to 'set_osd_ass' (number has no integer representation)

Edit: I think it's some incompatibility with Lua 5.3. With Lua 5.2 it's working fine.

@ghost
Copy link

ghost commented Apr 1, 2015

Yep, this is due to Lua 5.3. Maybe we should add a configure check to disable Lua if the version is 5.3. It's a different language, and supporting both 5.3 and 5.2/5.1 would be hard. Something below 5.3 is actually desirable, because then we can use luajit.

@ghost ghost closed this as completed in 9b59c17 Apr 1, 2015
@ghost
Copy link

ghost commented Apr 1, 2015

It's rejected now. Either at compile time or runtime, depending on luck.

@hroncok
Copy link
Contributor

hroncok commented May 3, 2015

Will add this as a patch to Fedora package.

@radistmorse
Copy link

Hm, and what are your plans for this issue? You got to migrate to 5.3 eventually.

Also, is it really that hard to make the code 5.3-friendly? I didn't dive deep, but it looks like lua will convert floats to ints automatically, provided that the floats have no fractional part. So using math.floor() wherever applicable should theoretically solve the problem.

UPD: ok, now I dived deep and I managed to fix it for lua 5.3, which shouldn't affect 5.2/5.1. Although I didn't check.

Are you sure you do not want to support 5.3? I think I can come up with the proper patch that will stricten the type usage in lua. It won't hurt even for 5.1/5.2.

@hroncok
Copy link
Contributor

hroncok commented May 4, 2015

What will actually get disabled by this. I've noticed missing OSD, and it is also in the error message, so I assume disabling Lua will disable that as well?

@radistmorse
Copy link

hroncok, I can give you a patch for 0.8.3 that is in rpmfusion right now that will fix the issue. I just submitted the pull request against trunk, but we can fix mpv in rpmfusion right now :)

@hroncok
Copy link
Contributor

hroncok commented May 4, 2015

Well, I'l update it to 0.9 anyway. Will try the patch from #1914

@ghost
Copy link

ghost commented May 4, 2015

Also, is it really that hard to make the code 5.3-friendly? I didn't dive deep, but it looks like lua will convert floats to ints automatically, provided that the floats have no fractional part. So using math.floor() wherever applicable should theoretically solve the problem.

There was an attempt, and it looked to me like making the code compatible for Lua 5.1/2 and 5.3 would result in a big mess.

Plus, there's actually no reason why we should support 5.3. It's a different language, not a newer release of the 5.1 language. Staying compatible to Luajit (which is basically the 5.1. language + some compatible 5.2 extensions) is a good idea.

Are you sure you do not want to support 5.3? I think I can come up with the proper patch that will stricten the type usage in lua. It won't hurt even for 5.1/5.2.

Staying compatible to both 5.1 and 5.2 is already a big mess, and I'm not sure if 5.3 can be additionally supported without making even a bigger mess. If you manage it without adding tons of hacks, fine, otherwise no thanks.

@hroncok
Copy link
Contributor

hroncok commented May 4, 2015

Well, we have compat-lua (5.1) in Fedora. Wouldn't using this fix the issue for us?

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

No branches or pull requests

3 participants