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

SDL2: fix mixer errors, mixer.init(..., allowchanges=0) #861

Merged
merged 9 commits into from Mar 4, 2019
Merged

Conversation

dlon
Copy link
Member

@dlon dlon commented Feb 28, 2019

Close #860

@dlon dlon added mixer pygame.mixer 2.0 labels Feb 28, 2019
@illume illume changed the title SDL2: fix mixer errors SDL2: fix mixer errors, mixer.init(..., allowchanges=0) Feb 28, 2019
@illume
Copy link
Member

illume commented Mar 1, 2019

Ah. SDL 2 is required for the allowchanges stuff to work.
https://wiki.libsdl.org/SDL_OpenAudioDevice#allowed

  • SDL_AUDIO_ALLOW_FREQUENCY_CHANGE
  • SDL_AUDIO_ALLOW_FORMAT_CHANGE
  • SDL_AUDIO_ALLOW_CHANNELS_CHANGE
  • SDL_AUDIO_ALLOW_ANY_CHANGE

With SDL1 (and pygame), I think it defaulted to 0 behaviour.

@illume illume requested a review from charlesej March 1, 2019 07:52
src_c/mixer.c Outdated
}
channels = (channels >= 2) ? 2 : 1;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should let channels to go through as a number more than 2? I guess none of the code is prepared for this really (sound array etc), and it would require testing. But SDL2 accepts more than 2.

https://wiki.libsdl.org/SDL_AudioSpec#Remarks

channels specifies the number of output channels. As of SDL 2.0, supported values are 1 (mono), 2 (stereo), 4 (quad), and 6 (5.1). 

Copy link
Member

Choose a reason for hiding this comment

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

So, I guess this channels >= 2 test should only be for SDL1.
For SDL2 it should check for 1, 2, 4, and 6.

@dlon
Copy link
Member Author

dlon commented Mar 1, 2019 via email

src_c/mixer.c Outdated Show resolved Hide resolved
@dlon
Copy link
Member Author

dlon commented Mar 1, 2019

This also solved the issues in #850.

Copy link
Contributor

@charlesej charlesej left a comment

Choose a reason for hiding this comment

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

Good changes and good idea providing the allowedchanges argument.

src_c/mixer.c Outdated Show resolved Hide resolved
docs/reST/ref/mixer.rst Outdated Show resolved Hide resolved
src_c/constants.c Show resolved Hide resolved
src_c/mixer.c Outdated Show resolved Hide resolved
@dlon dlon mentioned this pull request Mar 2, 2019
@dlon
Copy link
Member Author

dlon commented Mar 2, 2019

From the SDL 1.2.15 docs:

int SDL_OpenAudio(SDL_AudioSpec *desired, SDL_AudioSpec *obtained);

This function opens the audio device with the desired parameters, and returns 0 if successful, placing the actual hardware parameters in the structure pointed to by obtained. If obtained is NULL, the audio data passed to the callback function will be guaranteed to be in the requested format, and will be automatically converted to the hardware audio format if necessary.

From SDL_mixer (1.*):

/* Open the mixer with a certain desired audio format */
int Mix_OpenAudio(int frequency, Uint16 format, int nchannels, int chunksize)
{
	int i;
	SDL_AudioSpec desired;

...

	/* Set the desired format and frequency */
	desired.freq = frequency;
	desired.format = format;
	desired.channels = nchannels;
	desired.samples = chunksize;
	desired.callback = mix_channels;
	desired.userdata = NULL;

	/* Accept nearly any audio format */
	if ( SDL_OpenAudio(&desired, &mixer) < 0 ) {
		return(-1);
	}
...
}

obtained is not NULL (it's &mixer), so I guess allowedchanges=0 is the wrong default. But changing it causes a bunch of test failures, so I guess we update the tests instead.

I set the default to AUDIO_ALLOW_FREQUENCY_CHANGE | AUDIO_ALLOW_CHANNELS_CHANGE, which is the same default that SDL2_mixer uses.

@illume illume merged commit 2272d02 into master Mar 4, 2019
@illume illume deleted the sdl2-tests branch March 4, 2019 07:05
case 6:
break;
default:
PyErr_SetString(PyExc_ValueError, "'channels' must be 1, 2, 4, or 6");
Copy link
Contributor

Choose a reason for hiding this comment

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

#113 can be closed since channels > 2 are supported in SDL2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mixer pygame.mixer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants