GNOME Bugzilla – Bug 669279
sdl: SDL.AudioSpec class cannot be instantiated
Last modified: 2012-02-06 21:00:26 UTC
Created attachment 206660 [details] [review] Patch If you want to play audio using SDL[0], you first have to create an SDL_AudioSpec[1] (vala: SDL.AudioSpec) which you can then pass on to SDL_OpenAudio (vala: SDL.Audio.open). The problem is that you cannot create such an object; using "new AudioSpec()" results in valac complaining with: error: `SDL.AudioSpec' does not have a default constructor The only solution I've found for this problem, is to make SDL.AudioSpec a struct instead of a class. This change requires changing some function signatures since the SDL_* functions don't take/return a plain SDL_AudioSpec but a pointer to it[2][3]. The attached patch enables me to create an AudioSpec just like any other struct and to pass it to SDL.Audio.open() via the "ref" and "out" directives. If there is no better solution, please consider merging this patch. [0] http://www.libsdl.org/intro.en/usingsound.html [1] http://wiki.libsdl.org/moin.cgi/SDL_AudioSpec [2] http://wiki.libsdl.org/moin.cgi/SDL_OpenAudio [3] http://wiki.libsdl.org/moin.cgi/SDL_LoadWAV_RW
Review of attachment 206660 [details] [review]: Thanks for the patch. I'm adding some notes. ::: vapi/sdl.vapi @@ +984,2 @@ [CCode (cname="SDL_AudioSpec")] [Compact] Remove [Compact], it has no meaning for structs. @@ +1027,2 @@ [CCode (cname="SDL_OpenAudio")] + public static int open(ref AudioSpec desired, out AudioSpec obtained); Shouldn't be ref. From the docs, I don't see "desired" being changed. @@ +1036,2 @@ [CCode (cname="SDL_LoadWAV_RW")] + public static AudioSpec* load(RWops src, int freesrc=0, ref AudioSpec spec, uchar** audio_buf, ref uint32 audio_len); 1) Same here, "spec" shouldn't be ref. I don't see anything in the docs saying it being changed. 2) Return AudioSpec? (or unowned AudioSpec?, depends on whether sdl allocates it), instead of AudioSpec* 3) You can improve the bindings by replacing audio_buf and audio_len with out uint8[] audio_buf.
So, there is really no better way to solve this than making AudioSpec a struct, is there? (In reply to comment #1) > Remove [Compact], it has no meaning for structs. Agreed. > @@ +1027,2 @@ > [CCode (cname="SDL_OpenAudio")] > + public static int open(ref AudioSpec desired, out AudioSpec obtained); > > Shouldn't be ref. From the docs, I don't see "desired" being changed. Correct. But if I remove the "ref", how can Vala know that it should pass a pointer here instead of the struct itself? > @@ +1036,2 @@ > [CCode (cname="SDL_LoadWAV_RW")] > + public static AudioSpec* load(RWops src, int freesrc=0, ref AudioSpec > spec, uchar** audio_buf, ref uint32 audio_len); > > 1) Same here, "spec" shouldn't be ref. I don't see anything in the docs saying > it being changed. Same problem as above. > 2) Return AudioSpec? (or unowned AudioSpec?, depends on whether sdl allocates > it), instead of AudioSpec* Agreed. > 3) You can improve the bindings by replacing audio_buf and audio_len with out > uint8[] audio_buf. Will do.
Created attachment 206722 [details] [review] Patch
I've attached an improved and extended version of the original patch which also adds the previously missing callback field and fixes the arguments of Audio.mix()[0]. I'm pretty sure that the delegate part can be improved via something like [CCode (delegate_target=...)], any ideas? [0] For the upstream documentation, see <http://www.libsdl.org/cgi/docwiki.cgi/SDL_MixAudio>. If you're wondering whether we can also drop the "len" argument in Audio.mix(): No, we can't because people might pass an int16[] or similar as "dst" or "src" which works if you are using an explicit cast but Vala would still use the original length of the array which will lead to errors.
Review of attachment 206722 [details] [review]: Thanks for the update. For attributes reference: https://live.gnome.org/Vala/Manual/Attributes ::: vapi/sdl.vapi @@ +982,3 @@ }// AudioStatus + + [CCode (has_target = false)] Use [CCode (instance_pos = 0.1)] @@ +983,3 @@ + + [CCode (has_target = false)] + public delegate void AudioCallback(void* userdata, uint8[] stream); Drop the userdata parameter. @@ +994,3 @@ public uint16 padding; public uint32 size; + public AudioCallback callback; 1) Use the unowned keyword, because there's no DestroyNotify 2) Add [CCode (delegate_target = "userdata")] @@ +997,1 @@ public void* userdata; Drop this field since we refer it automatically
Created attachment 206751 [details] WhiteNoiseGenerator.vala I've added a simple white noise generator which serves as a testcase for creating an SDL.AudioSpec and using SDL.Audio.open() and SDL.Audio.mix(). This program works with the last patch I submitted.
Created attachment 206752 [details] [review] Patch I've attached an updated version of the patch which should implement all improvements suggested by Luca Bruno.
Created attachment 206753 [details] WhiteNoiseGenerator.vala I've attached an updated version of the testcase. It compiles fine, but when I run it I get this: $ ./WhiteNoiseGenerator ** (process:28482): CRITICAL **: white_noise_generator_write_cb: assertion `self != NULL' failed ** (process:28482): CRITICAL **: white_noise_generator_write_cb: assertion `self != NULL' failed ** (process:28482): CRITICAL **: white_noise_generator_write_cb: assertion `self != NULL' failed ... Any ideas?
Review of attachment 206752 [details] [review]: Perfect, except a slight mistake. Can you test with this last change? ::: vapi/sdl.vapi @@ +994,3 @@ public uint16 padding; public uint32 size; + [CCode (delegate_target = "userdata")] Sorry my fault, it must be delegate_target_cname = "userdata".
(In reply to comment #8) > $ ./WhiteNoiseGenerator > > ** (process:28482): CRITICAL **: white_noise_generator_write_cb: assertion > `self != NULL' failed You must keep WhiteNoiseGenerator alive. Store it in a local variable.
Created attachment 206758 [details] [review] Patch s/delegate_target/delegate_target_cname/ and add Audio.MIX_MAXVOLUME.
Created attachment 206759 [details] WhiteNoiseGenerator.vala Store the new WhiteNoiseGenerator in in a local variable and use the new SDL.Audio.MIX_MAXVOLUME. Unfortunately: $ valac-0.14 --pkg=sdl --vapidir=. WhiteNoiseGenerator.vala WhiteNoiseGenerator.vala:7.23-7.59: warning: local variable `generator' declared but never used WhiteNoiseGenerator generator = new WhiteNoiseGenerator(); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ /home/alexander/Desktop/vala-sdl/WhiteNoiseGenerator.vala.c: In function ‘white_noise_generator_construct’: /home/alexander/Desktop/vala-sdl/WhiteNoiseGenerator.vala.c:89:8: error: ‘SDL_AudioSpec’ has no member named ‘callback_target’ error: cc exited with status 256 Compilation failed: 1 error(s), 1 warning(s) $
Review of attachment 206758 [details] [review]: ::: vapi/sdl.vapi @@ +1041,2 @@ [CCode (cname="SDL_LoadWAV_RW")] + public static AudioSpec? load(RWops src, int freesrc=0, AudioSpec spec, out uint8[] audio_buf); Are you sure this is correct? From a very quick look at the documentation, it seems to me that the function modifies the AudioSpec value passed and returns a pointer to the struct without doing any allocation in regards to AudioSpec. I.e., shouldn't it be something like the following? public static unowned AudioSpec? load (RWops src, int freesrc, ref AudioSpec spec, out uint8[] audio_buf);
(In reply to comment #12) > /home/alexander/Desktop/vala-sdl/WhiteNoiseGenerator.vala.c: In function > ‘white_noise_generator_construct’: > /home/alexander/Desktop/vala-sdl/WhiteNoiseGenerator.vala.c:89:8: error: > ‘SDL_AudioSpec’ has no member named ‘callback_target’ > error: cc exited with status 256 > Compilation failed: 1 error(s), 1 warning(s) > $ You might need valac 0.15.
(In reply to comment #13) > Are you sure this is correct? From a very quick look at the documentation, it > seems to me that the function modifies the AudioSpec value passed and returns a > pointer to the struct without doing any allocation in regards to AudioSpec. > I.e., shouldn't it be something like the following? > > public static unowned AudioSpec? load (RWops src, int freesrc, ref AudioSpec > spec, out uint8[] audio_buf); You are absolutely correct. Looking at the source[0] shows multiple places[1][2] where spec is modified, so we definitely have to use "ref" here. And after re-reading the Vala documentation[3], I also agree that we have to use "unowned" here because the "master reference" is whatever we passed as the "spec" argument. I will update my patch shortly. [0] http://hg.libsdl.org/SDL/file/60a0e02107f9/src/audio/SDL_wave.c#l409 [1] http://hg.libsdl.org/SDL/file/60a0e02107f9/src/audio/SDL_wave.c#l511 [2] http://hg.libsdl.org/SDL/file/60a0e02107f9/src/audio/SDL_wave.c#l517 [3] http://live.gnome.org/Vala/ReferenceHandling#Unowned_References
Created attachment 206771 [details] [review] Patch Implement Jürg Billeter's suggestions and mention the bug in the commit message.
(In reply to comment #14) > You might need valac 0.15. IOW, you can successfully compile and run the testcase, right?
(In reply to comment #17) > (In reply to comment #14) > > You might need valac 0.15. > > IOW, you can successfully compile and run the testcase, right? No sorry, I was sure I compiled it but indeed raises the CC error. That's a valac bug.
(In reply to comment #18) > No sorry, I was sure I compiled it but indeed raises the CC error. That's a > valac bug. This may sound like a dumb question, but are you sure the attribute is called "delegate_target_cname"? I ask because a) The documentation[0] doesn't mention it b) I can't find any reference to it in codegen/valaccodeattribute.vala[1] c) Running grep -r '"delegate_target_cname"' . in the source tree returns nothing at all. Aren't there any other libraries which have to solve a similar task? [0] http://live.gnome.org/action/login/Vala/Manual/Attributes [1] http://git.gnome.org/browse/vala/tree/codegen/valaccodeattribute.vala
commit b467622bf89b6992eea7c6d15715525c95772727 Author: Alexander Kurtz <kurtz.alex@googlemail.com> Date: Sat Feb 4 17:31:12 2012 +0100 sdl: Make AudioSpec a struct, then fix Audio.open(), load() and mix() Fixes bug 669279. commit 383c309685ca888edb7dcfe7774f7ed108e77ad8 Author: Luca Bruno <lucabru@src.gnome.org> Date: Mon Feb 6 21:19:36 2012 +0100 codegen: Add support for delegate_target_cname in CCode This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.