-
Notifications
You must be signed in to change notification settings - Fork 276
Fix for Microphone Streaming Inconsistencies (Booleans/Error Codes for Initialisation, Starting Streams and Recording) #325
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
base: main
Are you sure you want to change the base?
Conversation
NOTES: - Added lines to set the state of the various Booleans used to keep track of the initialisation, streaming and recording of the microphone stream. The way it was set up before, in most instances these Booleans would not change, and portions of the code would never fire off as they relied on them to do so.
NOTES: - Changed the `WindowsMicrophoneStreamErrorCode` return types from `Success` to `AlreadyRunning` for `Initialize()` and `AlreadyRecording` for `StartRecording()` if the Booleans tracking those states are already true. Thought it to be more descript than setting everything to `Success` which is technically true as it is working, but it doesn't let the developer know that these states were already running and may cause them to add code where not needed.
NOTES: - Added new stream error codes to better indicate to the developer that a microphone stream state has or has not already started. Similar reasoning to the previous push, setting everything as `Success` may put the developer in a false sense of security that their code is doing what they are thinking, and causes them to add extra lines where it may not be needed in some instances. This makes it more clear as to what is happening and doesn't necessarily stop their code from running, but allows them to better debug and bug fix. - Added the new enums in at lower values than what was already there. Wanted to have them in order of how they appear in `WindowsMicrophoneStream.cs` but didn't want to change said order and end up breaking people's code that may be relying on the actual integer values rather than just the enums. It may not be that likely, but this at least keeps compatibility for those people in those niche cases - Added new enums to `ErrorCodes` in `MicStreamSelector.cpp` and Booleans to allow for this new error code data to be properly captured by Unity. Different functions will output these new integer values given by the enums and set Booleans to keep track of these states. - Added XML comments for documentation
NOTES: - Error codes were using old names, or were incorrect. Have been corrected to reflect their current names and to use the error code appropriate the state it is reporting on
@microsoft-github-policy-service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Though, I'm not sure anybody is monitoring this repo at Microsoft anymore 😢 I can ping some folks and see
{ | ||
if (initialized) | ||
{ | ||
// The microphone stream is already initialized, no need to alarm the calling code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we edit these comments throughout, now that we aren't returning Success
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem! Appreciate you taking a look, and no stress if no-one see's it, I know it's been out of active development for a while, but I was fixing some things on my end for a project and thought I may PR it just in case, haha. Still on MRTK2 for the time being.
Can definitely chuck those comments out though, wasn't sure whether to keep them or toss them as I didn't want to muck around with the codebase too much. Will make another edit in just a moe.
NOTES: - Removed the old comments from the if-statement checks in each of the methods that had them as they are no longer just returning `Success` error codes - Updated the XML comments for the methods with the if-statement checks that had their comments removed. Noted that the `WindowsMicrophoneStreamErrorCode` can either indicate success, failure, or a reason why it did not progress past the if-statement check
Overview
When using the MRTK2 Examples, specifically the Audio Demo related to the Windows Microphone Stream system, there would be issues when trying to switch the state of the microphone. A modification of the demo was made to initialise the stream using the Room stream type, record thirty seconds of audio, uninitialise it and then reinitialise it with the High Quality Voice stream type. However, the state of the microphone, whether it was initialised, recording or streaming, would never change and would just skip over the checks to see if they were still running or not. This would cause some errors where it would believe it hadn't started streaming or been initialised, and made it so attempting to switch between stream types would fail as it had never been uninitialised, or recordings would never start.
This addresses those issues and adds more error codes to both the scripts and plugin files to make it easier to spot issues. The reasoning being that the
SUCCESS
code, where correct as it is working, isn't too descriptive and could possibly lead toChanges