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.

Sign up for GitHub

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

speechManager: Ensure handling of skipped indexes #10730

Merged
merged 4 commits into from
Jan 31, 2020
Merged

speechManager: Ensure handling of skipped indexes #10730

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96f71f1
speechManager._handleIndex: make sure to handle any skipped indexes a…
michaelDCurran Jan 29, 2020
fe5498a
Fix linting issues.
michaelDCurran Jan 30, 2020
4d897cd
Fix import error.
michaelDCurran Jan 30, 2020
cbd8f24
Make suggested refactor from @feerrenrut.
michaelDCurran Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion source/speech/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import queueHandler
import synthDriverHandler
from .commands import *
from .commands import IndexCommand
from .priorities import Spri, SPEECH_PRIORITIES

class ParamChangeTracker(object):
Expand Down Expand Up @@ -147,6 +148,8 @@ def _reset(self):
self._curPriQueue = None
#: Maps indexes to BaseCallbackCommands.
self._indexesToCallbacks = {}
#: a list of indexes currently being spoken by the synthesizer
self._indexesSpeaking = []
#: Whether to push more speech when the synth reports it is done speaking.
self._shouldPushWhenDoneSpeaking = False

Expand Down Expand Up @@ -287,6 +290,11 @@ def _pushNextSpeech(self, doneSpeaking):
return self._pushNextSpeech(True)
seq = self._buildNextUtterance()
if seq:
# Record all indexes that will be sent to the synthesizer
# So that we can handle any accidentally skipped indexes.
for item in seq:
if isinstance(item, IndexCommand):
self._indexesSpeaking.append(item.index)
getSynth().speak(seq)

def _getNextPriority(self):
Expand Down Expand Up @@ -365,7 +373,20 @@ def _removeCompletedFromQueue(self, index):
del self._curPriQueue.pendingSequences[:seqIndex + 1]
return True, endOfUtterance

def _handleIndex(self, index):
def _handleIndex(self, index, handleSkippedIndexes=True):
try:
self._indexesSpeaking.remove(index)
except ValueError:
log.debugWarning("Unknown index %s" % index)
return
# A synth (such as OneCore) may skip indexes
# If before another index, with no text content in between.
# Therefore, detect this and ensure we handle all skipped indexes.
if handleSkippedIndexes:
for oldIndex in list(self._indexesSpeaking):
if oldIndex < index:
log.debugWarning("Handling skipped index %s" % oldIndex)
self._handleIndex(oldIndex, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if this was not recursive, I think it only adds extra complexity, because this function now does several things and recursing the skipped indexes is just one part. I think that _pushNextSpeech should only be called once. Despite my initial feeling that _removeCompletedFomQueue should only be called once and it being less efficient than handling this in bulk, it looks like it necessary. This should be well documented. It seems a shame that we are searching through the sequence for these indexes already (in _removeCompletedFomQueue). Any way, removing the recursive approach you could achieve the same with:

	def _handleIndex(self, index):
		# A synth (such as OneCore) may skip indexes
		# If before another index, with no text content in between.
		# Therefore, collect all skipped indexes.
		handleIndexes = []
		for oldIndex in self._indexesSpeaking:  # no need to take a copy anymore
			if oldIndex < index:
				log.debugWarning("Handling skipped index %s" % oldIndex)
				handleIndexes.append(oldIndex)
		handleIndexes.append(index)

		# a more concise approach to create handleIndexes:
		self._indexesSpeaking.sort()  # Note: if we wish to maintain out of order indexes (which I don't think we should allow) use s = sorted()
		indexOfIndex = self._indexesSpeaking.index(index)
		handleIndexes = self._indexesSpeaking[:indexOfIndex]

		valid, endOfUtterance = False, False
		for i in handleIndexes:
			try:
				self._indexesSpeaking.remove(i)
			except ValueError:
				log.error("Unknown index %s" % i)
				break  # try the rest, this is a very unexpected path.
			if i != index:
				log.debugWarning("Handling skipped index %s" % i)

			# we must do the following for each index, any/all of them may be end of utterance, which must
			# trigger _pushNextSpeech
			_valid, _endOfUtterance = self._removeCompletedFromQueue(i)
			valid = valid or _valid
			endOfUtterance = endOfUtterance or _endOfUtterance
			
			if _valid:
				callbackCommand = self._indexesToCallbacks.pop(index, None)
				if callbackCommand:
					try:
						callbackCommand.run()
					except:
						log.exception("Error running speech callback")
		if endOfUtterance:
			# Even if we have many indexes, we should only push next speech once.
			self._pushNextSpeech(False)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about the effect this will have on the timing of the callbacks. Or resulting callbacks being called that were intended to be associated with text that the synth decided not to speak. Though, thinking about this from oncore's side. If there are several indexes in sequence, with no speakable text in between, it would seem like a fairly logical optimization to only call the last one.

valid, endOfUtterance = self._removeCompletedFromQueue(index)
if not valid:
return
Expand Down