mmitch / gbsplay

gameboy sound player
https://mmitch.github.io/gbsplay/
Other
98 stars 20 forks source link

-1 -2 -3 and -4 arguments don't mute channels on startup #14

Closed TheBlackParrot closed 7 years ago

TheBlackParrot commented 7 years ago

Using version 0.0.93-102-g33c5d71, the arguments specified in the help don't mute channels on startup. Muting channels manually still works, it's just the arguments that don't.

gbsplay -1 Jimmy\ White\'s\ Cueball.gbs

mmitch commented 7 years ago

I did a quick test with 0.0.93-102-g33c5d71 here and everything worked as expected:

gbsplay -4 examples/nightmode.gbs
gbsplay -1 -2 examples/nightmode.gbs
gbsplay -1 -2 -3 -4 examples/nightmode.gbs

So propably it's something about libraries, environment, compiler etc.

What system do you run?

I don't know exactly what we would need to track this down, could you please attach the output of ./configure as well as the generated config.mk?

Argument parsing is done here via getopt(). Which libc version are you using? (libc6-dev:amd64 2.23-0ubuntu4 on my system)

TheBlackParrot commented 7 years ago

Arch Linux x86_64 kernel 4.8.8, gcc 6.2.1 20160830, glibc 2.24-2

./configure output:

checking for working compiler:  ok
checking if OS is Cygwin:  no
checking for inttypes.h:  ok
checking for sys/soundcard.h:  ok
checking for alsa/asoundlib.h:  ok
checking for ESTRPIPE support:  ok
checking for pulse/simple.h:  ok
checking for audio/audiolib.h:  ok
checking if we need additional libs for -laudio:  no
checking for locale.h:  ok
checking for libintl.h:  ok
checking for gettext tools:  ok
checking if we need additional libs for i18n:  no
checking if cc supports -Wdeclaration-after-statement:  yes
checking if cc supports -Wl,-z,relro:  yes
checking if cc supports -Wl,-z,now:  yes
checking if cc supports -Wl,-pie:  no
checking if cc supports -pie:  yes
checking if cc supports -fstack-protector-strong:  yes
checking for regparm support:  ok
optional modules: +contrib +test -xmmsplugin
optional features: +alsa -debug +devdsp +hardening +i18n +iodumper +midi +nas +pulse +regparm -sharedlibgbs +stdout
EXTRA_CFLAGS=-Wdeclaration-after-statement -fstack-protector-strong -D_FORTIFY_SOURCE=2 -Wall -fsigned-char -Os -pipe -fomit-frame-pointer
EXTRA_LDFLAGS=-Wl,-z,relro -Wl,-z,now -pie -fstack-protector-strong

config.mk:

EXTRA_ALL := 
EXTRA_CFLAGS := -Wdeclaration-after-statement -fstack-protector-strong -D_FORTIFY_SOURCE=2 -Wall -fsigned-char -Os -pipe -fomit-frame-pointer
EXTRA_INSTALL := 
EXTRA_LDFLAGS := -Wl,-z,relro -Wl,-z,now -pie -fstack-protector-strong
EXTRA_SRCS := 
EXTRA_UNINSTALL := 
PTHREAD := 
XMMS_INPUT_PLUGIN_DIR := 
VERSION := 0.0.93-102-g33c5d71
prefix := /usr/local
exec_prefix := /usr/local
bindir := /usr/local/bin
libdir := /usr/local/lib
mandir := /usr/local/man
docdir := /usr/local/share/doc/gbsplay
localedir := /usr/local/share/locale
sysconfdir := /etc
CC := gcc
BUILDCC := gcc
HOSTCC := gcc
build_contrib := yes
build_test := yes
build_xmmsplugin := no
have_xgettext := yes
use_i18n := yes
use_sharedlibgbs := no
cygwin_build := no
libaudio_flags := 
plugout_alsa := yes
plugout_devdsp := yes
plugout_dsound :=
plugout_iodumper := yes
plugout_midi := yes
plugout_nas := yes
plugout_pulse := yes
plugout_stdout := yes
mmitch commented 7 years ago

I found your .gbs file here and with it I can reproduce the problem. I think the .gbs itself includes some code to un-mute all channels which is executed at the start of the play routine. I'm no expert on the inner workings of the hardware emulation, but it looks like gbsplay currently has no 'external' mute settings but rather changes the corresponding channel mute bits in the emulated hardware by accessing gbhw_ch[channel].mute. The same bits seem to be accessible from the running .gbs code and can be overwritten there.

If this is the case, then the manpage is technically correct :-) The channels are muted on startup, after that the .gbs code takes over and unmutes them, but that happens after startup…

@ranma, what do you make of this?

A possible solution could be to track commandline and keyboard mute status separately from the emulated hardware mute status and check both values in the sound output routine. A grep for gbhw.*mute shows only 7 lines so the changes should not be too complicated.

mmitch commented 7 years ago

Looks like the only thing that writes to the gbhw_ch[].mute flag is the memset() in apu_reset() in gbhw.c. For the initial player startup, the mute status from the commandline parameters are saved, the APU is reset and then the mute settings are restored. Later APU resets (via IO access to 0xff26 by the emulated player routine) don't restore the mute settings but reset all mute flags to off.

I moved the save & restore of the mute flags into the apu_reset() method so the flags should now survive all APU resets and not only the initial player setup.

Please check out the startup-mute-bug branch, my fix is applied there. If everything is ok, we can merge it into master.

TheBlackParrot commented 7 years ago

The fix works, although with all channels muted, using the same .gbs file, it skips songs after 2 seconds. It sounds like a rather unrealistic scenario to have everything muted from the start, but I figured I would mention it in case it's related to this fix.

mmitch commented 7 years ago

(sorry for the late reply) Skipping songs after 2 seconds with all channels muted from the start is normal behaviour, the same happens when you mute all channels interactively. The -T parameter (silence timeout) has a default value of 2 seconds, so that the next subsong starts, when there is silence for 2 seconds.

A GBS file contains no song length information so the player can never know if a subsong has a length of 1 second or will repeat forever. That's why we have the -T and -t (next subsong after a given time) options, defaulting to 2 respectively 120 seconds.

Use -T 0 to disable the silence timeout altogether.

mmitch commented 7 years ago

merged to master and deleted startup-mute-bug branch