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

Improve PlayHandle removal in Mixer #4871

Open
M374LX opened this issue Mar 3, 2019 · 2 comments
Open

Improve PlayHandle removal in Mixer #4871

M374LX opened this issue Mar 3, 2019 · 2 comments
Labels

Comments

@M374LX
Copy link
Contributor

M374LX commented Mar 3, 2019

Problem

In Mixer::renderNextBuffer(), when a clear signal is received (by calling Mixer::clear(), which sets m_clearSignal to true), Mixer::clearInternal() gets called and it simply adds every PlayHandle except InstrumentPlayHandles from m_playHandles to m_playHandlesToRemove. However, right after Mixer::clearInternal() is called in Mixer::renderNextBuffer(), a loop iterates through m_playHandlesToRemove and removes the same PlayHandles from m_playHandles. In other words, it unnecessarily stores a list of PlayHandles to be removed only to actually remove then right afterwards. Mixer::clear() seems to be called only within Song::stop().

Other than Mixer::clear(), the only method that adds a play handle to m_playHandlesToRemove is Mixer::removePlayHandle(), which is only called by the FileBrowser class to stop a preset preview PlayHandle. Also, only one preset preview PlayHandle seems to be active at a time.

Solution

  1. Add the members PresetPreviewPlayHandle m_presetPreviewPlayHandle and bool m_presetPreviewStopped to Mixer.

  2. Create the method Mixer::addPresetPreviewPlayHandle():

bool Mixer::addPresetPreviewPlayHandle( PresetPreviewPlayHandle *handle )
{
	m_presetPreviewStopped = false;
	m_presetPreviewPlayHandle = handle;
	return addPlayHandle( handle );
}
  1. Create the method Mixer::stopPresetPreview():
void Mixer::stopPreview()
{
	m_presetPreviewStopped = true;
}
  1. Modify Mixer::renderNextBuffer() so that:
if( m_clearSignal )
{
	m_clearSignal = false;
	clearInternal();
}

ConstPlayHandleList::Iterator it_rem = m_playHandlesToRemove.begin();
while( it_rem != m_playHandlesToRemove.end() )
{
	// [...]
}

becomes something like:

if( m_presetPreviewStopped && m_presetPreviewPlayHandle != NULL ) {
	//Actually stop m_presetPreviewPlayHandle [...]
	m_presetPreviewPlayHandle = NULL;
}

if( m_clearSignal )
{
	ConstPlayHandleList::Iterator it = m_playHandles.begin();
	while( it != m_playHandles.end() )
	{
		// [...]
	}

	m_clearSignal = false;
}
  1. Update FileBrowser.cpp with the new methods.
@PhysSong
Copy link
Member

PhysSong commented Mar 8, 2019

Mixer::clearInternal was added in #2879 by @jasp00 to fix a deadlock. I think we can remove it once we find a solution without deadlocks.
For preset previewing, I'm not sure why that can be an issue. I also think Mixer is not a place for handling preset previews.

@tresf tresf added the core label Mar 9, 2019
@Rossmaxx
Copy link
Contributor

Mixer has been rewritten recently. Does this issue still occur?

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

No branches or pull requests

4 participants