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

Fixes #4

Merged
merged 17 commits into from Aug 6, 2017
Merged

Fixes #4

merged 17 commits into from Aug 6, 2017

Conversation

daurnimator
Copy link
Collaborator

@daurnimator daurnimator commented Aug 4, 2017

@daurnimator
Copy link
Collaborator Author

For @peterbillam

@peterbillam
Copy link
Collaborator

Looks good :-) but

Compiling "Don't bother closing when NULL. Don't push just to throw away" I get:
src/lao.c: In function ‘l_close_device’:
src/lao.c:193:6: error: dereferencing pointer to incomplete type ‘ao_device {aka struct ao_device}’
if (*dev != NULL)
^~~~
Makefile:22: recipe for target 'ao.so' failed

Apart from that, there's just the usual warnings in l_initialize l_shutdown and l___gc
about
warning: unused parameter ‘L’ [-Wunused-parameter]
static int l_shutdown(lua_State* L)
and I guess there's a case for leaving those L in, just like you can put a trailing comma
after the last item in a table ...

@daurnimator
Copy link
Collaborator Author

@peterbillam fixed.

@TheLinx
Copy link
Owner

TheLinx commented Aug 4, 2017

Hello,
Thank you for the contributions. I myself haven't touched Lua in years now so I'm not the best maintainer for this.
I'm not sure on the list of things I'd have to do to shift responsibility but if you want it, I can go ahead and give it to you instead.

@peterbillam
Copy link
Collaborator

Yes, nice :-) And good to hear from TheLinx.

I had to add to example* the compatibility-lines
local numeric_version = string.gsub(_VERSION, "^%D+", "")
if tonumber(numeric_version) < 5.2 then
bit = require 'bit' -- LuaBitOp http://bitop.luajit.org/api.html
elseif _G.bit32 then
bit = _G.bit32
else
local f = load([[
bit.bor = function (a,b) return a|b end
bit.band = function (a,b) return a&b end
bit.rshift = function (a,n) return a>>n end
]])
f()
end

And also I notice that in play() the length of the byte-string is greater than buf_size eg:
buf_size = 176400 #buffer = 176404 str.len = 176404
so that if I use either
local rv = device:play(str)
or
local rv = device:play(str, string.len(str))
I get the message
lua: pjb_ao_test.lua:106: bad argument #2 to 'play' (too many bytes)
stack traceback:
[C]: in function 'play'
pjb_ao_test.lua:106: in main chunk

but it beeps sweetlly sinusoidally :-)

@daurnimator
Copy link
Collaborator Author

And also I notice that in play() the length of the byte-string is greater than buf_size eg:

Oops; off by one error. fixed.

@daurnimator
Copy link
Collaborator Author

Thank you for the contributions. I myself haven't touched Lua in years now so I'm not the best maintainer for this.
I'm not sure on the list of things I'd have to do to shift responsibility but if you want it, I can go ahead and give it to you instead.

I don't particularly want to take over the library: I have plenty to maintain already, and I don't use lao myself.

If you just want to add me as a collaborator here on github that's fine: that way I can merge things myself if I want to.

@peterbillam
Copy link
Collaborator

Well, I do plan to use lao a bit, but my big disadvantage is that I'm 69, and last year had a very dangerous medical emergency and am not feeling nearly as immortal as I used to, so you'd probably be looking for a replacement quite soon.. I guess taking over mainly involves tagging it when it's stable, and being in charge of the rockspec. So I'd like it if another volunteer could be found, and another may have to be found soon anyway.
If daurnimator could be made a collaborator that would be great, it's wonderful how he's got ao working.

@peterbillam
Copy link
Collaborator

Whenever I require "ao" I get fifteen lines of debug messages like
debug: Loaded driver null (built-in)
debug: Loaded driver wav (built-in)
and so on. It comes from libao.so
Is there a way of turning debug off by default ? I couldn't see it in https://xiph.org/ao/doc/ ...

@peterbillam
Copy link
Collaborator

debug off is easy :-) See https://xiph.org/ao/doc/config.html
In /etc/libao.conf or ~/.libao, set quiet=yes
Though the debug and verbose options didn't seem to have any effect.

@daurnimator
Copy link
Collaborator Author

so you'd probably be looking for a replacement quite soon.. I guess taking over mainly involves tagging it when it's stable, and being in charge of the rockspec. So I'd like it if another volunteer could be found, and another may have to be found soon anyway.

There can be more than one :)

If daurnimator could be made a collaborator that would be great

I have been made a collaborator now.

@peterbillam
Copy link
Collaborator

There can be more than one :)

Er, perhaps I should be a collaborator too, then ?

Back to quiet=yes : perhaps we could add a function corresponding to
https://xiph.org/ao/doc/ao_append_global_option.html
so that the programmers could do global_option('quiet', 'yes') or append_global_option('quiet', 'yes')
The libao function just has (key, value) arguments, so maybe we should copy that, or maybe we should expect a table, for compatibility with the option table of openLive()...
But I haven't tried it, and I'm a bit nervous about the word 'append' I mean will it also do 'overrule' ?

@daurnimator daurnimator merged commit 0921eb4 into TheLinx:master Aug 6, 2017
@daurnimator
Copy link
Collaborator Author

Back to quiet=yes : perhaps we could add a function corresponding to
https://xiph.org/ao/doc/ao_append_global_option.html
so that the programmers could do global_option('quiet', 'yes') or append_global_option('quiet', 'yes')
The libao function just has (key, value) arguments, so maybe we should copy that, or maybe we should expect a table, for compatibility with the option table of openLive()...
But I haven't tried it, and I'm a bit nervous about the word 'append' I mean will it also do 'overrule' ?

Looks like lao already supports passing options (in table form) as third argument to openLive.
e.g. device = ao.openLive(ao.defaultDriverId(), format, {quiet="any string"})

@daurnimator daurnimator deleted the fixes branch August 6, 2017 03:57
@peterbillam
Copy link
Collaborator

Looks like lao already supports passing options (in table form) as third argument to openLive
e.g. device = ao.openLive(ao.defaultDriverId(), format, {quiet="any string"})

Yes, but they're not the global options, they're just the driver options, and setting quiet='yes'
there has no effect, because the debug messages have already been printed. Like in
https://xiph.org/ao/doc/config.html
it distinguishes between AO Options and Driver Options. I'm hoping AO Options means the same as global_options, and I'm also hoping that calling ao_append_global_option after requiring the module, but before calling openLive or openFile will set quiet='yes' soon enough to stop those messages...
It's a separate issue; we should get a new lao version onto luarocks.org first, meanwhile I'll do some experimenting. We do already have the config files as a mechanism.
( The config files can be used to set both AO Options, and Driver Options )

@daurnimator
Copy link
Collaborator Author

Looks like the initial print comes from the automatic internal ao_initialize call.
Could you make a new issues requesting its removal? (perhaps we can e.g. auto-initialize on the first call to a function?)

@peterbillam
Copy link
Collaborator

Could you make a new issues requesting its removal?
You mean here in https://github.com/TheLinx/lao/ ?
I've just registered myself at xiph.org (which was fraught), but now I realise I was probably on the wrong track ...

(perhaps we can e.g. auto-initialize on the first call to a function?)
Yes! or if that function is append_global_option then we call that first and ao_initializesecond (I hope it works that way round),
whereas if it's some other function then we call ao_initialize first and the function second.

@daurnimator
Copy link
Collaborator Author

Could you make a new issues requesting its removal?
You mean here in https://github.com/TheLinx/lao/ ?

Yep :) I already wrote a PR. see #6.

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

Successfully merging this pull request may close these issues.

strndup in l_play() ? No luaL_register in 5.2
3 participants