Updated SDL Sound code #336

angstsmurf · 2018-10-21T17:45:19Z

This implements the extended GLK sound functions:

glk_schannel_create_ext()
glk_schannel_play_multi()
glk_schannel_set_volume_ext()
glk_schannel_pause()
glk_schannel_unpause()

What is still missing is working sound and volume notifications. They won't fire until some other event, such as the player pressing a key or moving the mouse, comes along, as reported in #204. This is implemented in #337, if only for Windows and macOS at present.

These changes only touch the SDL sound backend, and I have not been able to test whether FMOD sound still works.

I've made a test program here.

cspiegel · 2019-11-11T15:13:01Z

garglk/sndsdl.c

+    if (chan == NULL)
+        gli_strict_warning("MOD player called with an invalid channel!");


Since chan is dereferenced below, I presume the function should return here instead of continuing.

cspiegel · 2019-11-11T15:18:41Z

garglk/sndsdl.c

+    if (chan->timer)
+        SDL_RemoveTimer((SDL_TimerID)chan->timer);
+
+	chan->timer = (int)SDL_AddTimer((Uint32)(duration / FADE_GRANULARITY), volume_timer_callback, (void *)chan);


This is indented with a tab instead of 4 spaces.

In addition, chan->timer is an int and SDL_AddTimer returns a pointer. On many 64-bit systems this could result in the truncation of the pointer. timer should probably just be made an SDL_TimerID instead of int. I think you'd have to add a macro, e.g. -DGARGLK_USESDL when building with SDL, so you can conditionally include SDL/SDL_timer.h in garglk/garglk.h and conditionally define timer. In fact, you could probably conditionally include most/all of struct glk_schannel_struct so the members don't exist unless SDL is being used for sound.

Thanks for the review! I actually think SDL_TimerID changes from a pointer to an int between SDL versions, which is why the code ended up this way, but not really an excuse. I'll see if I can work out how to fix it properly.

cspiegel · 2019-11-11T15:31:31Z

garglk/sndsdl.c

-    {
-        gli_strict_warning("sound callback failed");
+
+        if (!sound_channel)


This is indented too far.

cspiegel · 2019-11-11T15:38:03Z

garglk/sndsdl.c

+
+        if (!sound_channel)
+        {
+        fprintf(stderr, "sound_completion_callback called with invalid sound_channel %d\n", chan);


This ought to be done with gli_strict_warning, building up a message first with snprintf. Ideally gli_strict_warning would take a format string and variable number of arguments. I've added #355 to do this.

cspiegel · 2019-11-11T15:39:48Z

garglk/sndsdl.c

+    if (result && paused)
+    {
+        if (paused)
+            fprintf(stderr, "glk_schannel_play_ext: pausing channel again\n");


Is this message necessary? If so, presumably it should be done via gli_strict_warning.

angstsmurf · 2019-11-11T20:28:50Z

Right, I think I've done all the requested changes, and it seems to build fine on my Mac at least. Someone should test if it still works with FMOD sound.

angstsmurf · 2019-11-11T20:51:41Z

A weird thing is that if I try to make the other SDL-specific members of glk_schannel_struct (sdl_rwops, sdl_memory and sdl_channel) also conditional on SDL sound, most games will break and only show a blank screen.

angstsmurf · 2019-11-14T21:25:15Z

Squashed all commits into one. Hopefully this makes the changes more readable.

cspiegel · 2019-11-15T19:40:03Z

garglk/garglk.h

@@ -32,6 +32,10 @@

 #include "gi_dispa.h"

+#ifdef GARGLK_USESDL
+#include "SDL/SDL_timer.h"


One last thing here: I just tried a Windows build, and it fails because the include path points directly to the SDL directory, meaning that SDL_timer.h has to be included directly, not using the SDL/ prefix. This seems to be the proper way to do it on Unix systems too, as pkg-config sdl --cflags spits out -I/usr/include/SDL.

I'd also request using angle brackets instead of quotes since SDL is nominally a system library, even though on Windows it's being included directly, e.g.:

#include <SDL_timer.h>

The test program looked good, so with this one change, I'll merge it in.

Fixed and squashed again.

This implements the extended GLK sound functions: glk_schannel_create_ext() glk_schannel_play_multi() glk_schannel_set_volume_ext() glk_schannel_pause() glk_schannel_unpause() Sound and volume notifications are still not working properly. See bug report garglk#204 and pull request garglk#337. These changes only touch the SDL sound backend. I've made a test program here: https://github.com/angstsmurf/soundtest/releases

angstsmurf · 2019-11-16T09:50:48Z

Rewrote the PR description and commit message.

cspiegel · 2019-11-17T23:24:49Z

Thanks for implementing this!

angstsmurf mentioned this pull request Oct 22, 2018

React to sound and volume notifications immediately #337

Merged

angstsmurf force-pushed the new-sound branch from 032702c to 56da595 Compare July 22, 2019 23:52

cspiegel reviewed Nov 11, 2019

View reviewed changes

garglk/sndsdl.c Outdated

{

gli_strict_warning("sound callback failed");

if (!sound_channel)

Copy link

Contributor

cspiegel Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indented too far.

cspiegel reviewed Nov 11, 2019

View reviewed changes

angstsmurf force-pushed the new-sound branch 2 times, most recently from 0676261 to 91604ef Compare November 11, 2019 20:26

angstsmurf force-pushed the new-sound branch from 90dd81a to 8ca9730 Compare November 11, 2019 20:32

angstsmurf force-pushed the new-sound branch 2 times, most recently from f9435a2 to 144a5fa Compare November 14, 2019 21:24

cspiegel reviewed Nov 15, 2019

View reviewed changes

angstsmurf force-pushed the new-sound branch from d851d41 to fcd52cf Compare November 15, 2019 21:51

angstsmurf force-pushed the new-sound branch from fcd52cf to b6f71d5 Compare November 16, 2019 09:48

cspiegel merged commit 2d098b0 into garglk:master Nov 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated SDL Sound code #336

Updated SDL Sound code #336

angstsmurf commented Oct 21, 2018 •

edited

cspiegel Nov 11, 2019

cspiegel Nov 11, 2019

cspiegel Nov 11, 2019

angstsmurf Nov 11, 2019 •

edited

cspiegel Nov 11, 2019

cspiegel Nov 11, 2019

cspiegel Nov 11, 2019

angstsmurf commented Nov 11, 2019

angstsmurf commented Nov 11, 2019 •

edited

angstsmurf commented Nov 14, 2019

cspiegel Nov 15, 2019

angstsmurf Nov 15, 2019

angstsmurf commented Nov 16, 2019

cspiegel commented Nov 17, 2019

		if (chan == NULL)
		gli_strict_warning("MOD player called with an invalid channel!");

Updated SDL Sound code #336

Updated SDL Sound code #336

Conversation

angstsmurf commented Oct 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angstsmurf Nov 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angstsmurf commented Nov 11, 2019

angstsmurf commented Nov 11, 2019 • edited

angstsmurf commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angstsmurf commented Nov 16, 2019

cspiegel commented Nov 17, 2019

angstsmurf commented Oct 21, 2018 •

edited

angstsmurf Nov 11, 2019 •

edited

angstsmurf commented Nov 11, 2019 •

edited